Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Add annotations to combine ApisixPluginConfig with k8s ingress resource #799

Closed
lxm opened this issue Dec 15, 2021 · 25 comments · Fixed by #1139
Closed

proposal: Add annotations to combine ApisixPluginConfig with k8s ingress resource #799

lxm opened this issue Dec 15, 2021 · 25 comments · Fixed by #1139
Assignees
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@lxm
Copy link
Contributor

lxm commented Dec 15, 2021

Since we have implemented ApisixPluginConfig with apisix-ingress-controller. How about add support to combine k8s ingress resource with ApisixPluginConfig by ingress annotations

@tokers
Copy link
Contributor

tokers commented Dec 16, 2021

@lxm That's a good idea to extend the standard ingress resource.

@lxm
Copy link
Contributor Author

lxm commented Dec 16, 2021

@lxm That's a good idea to extend the standard ingress resource.

I'm working on this feature, assign to me?

@tokers
Copy link
Contributor

tokers commented Dec 16, 2021

@lxm Assigned!

@tao12345666333 tao12345666333 added the enhancement New feature or request label Dec 20, 2021
@tao12345666333
Copy link
Member

xref: #638

@tao12345666333
Copy link
Member

Hi, ApisixPluginConfig has been implemented. See #638 for details.

@lxm
Copy link
Contributor Author

lxm commented Jan 4, 2022

Hi, ApisixPluginConfig has been implemented. See #638 for details.

got it

@lxm
Copy link
Contributor Author

lxm commented Jan 4, 2022

Hi, ApisixPluginConfig has been implemented. See #638 for details.

no update for crds in charts?

@tao12345666333
Copy link
Member

no update for crds in charts?

@lxm do you mean Helm chart? v1.4 helm chart has not be merged. apache/apisix-helm-chart#208

you can using https://github.com/apache/apisix-ingress-controller/tree/master/samples/deploy/crd/v1

@tao12345666333 tao12345666333 added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 22, 2022
@tao12345666333
Copy link
Member

ref #1086

@dickens7
Copy link
Contributor

How is this feature going? Is there a release plan?

@tao12345666333
Copy link
Member

@dickens7 We are currently completing the remaining work in v1.5 and are ready to go to the release process.

This feature, I want to put it in v1.6 version.

Of course this is just a plan, if anyone is interested in this feature, please submit a PR, let's develop this project better

@tao12345666333
Copy link
Member

@lxm do you still working on this feature?

@dickens7
Copy link
Contributor

dickens7 commented Jul 5, 2022

The current ingress annotation Plugin implementation, PluginConfigId is already used

if len(plugins) > 0 {
route.Plugins = *(plugins.DeepCopy())
pluginConfig = apisixv1.NewDefaultPluginConfig()
pluginConfig.Name = composeIngressPluginName(ing.Namespace, pathRule.Backend.Service.Name)
pluginConfig.ID = id.GenID(route.Name)
pluginConfig.Plugins = *(plugins.DeepCopy())
ctx.AddPluginConfig(pluginConfig)
route.PluginConfigId = pluginConfig.ID
}

@dickens7
Copy link
Contributor

dickens7 commented Jul 5, 2022

route.Plugins and route.pluginConfigId are duplicate. I wrote a sample version to see if this works. I don't know if there will be compatibility issues.

https://github.com/dickens7/apisix-ingress-controller/commit/76fd06887313fe0c23a54955f72074c19bb5e5ec

@AlinsRan
Copy link
Contributor

AlinsRan commented Jul 6, 2022

route.Plugins and route.pluginConfigId are duplicate. I wrote a sample version to see if this works. I don't know if there will be compatibility issues.

dickens7@76fd068

Yeah. I'm implementing this function. Have you finished it?

@dickens7
Copy link
Contributor

dickens7 commented Jul 6, 2022

Yeah. I'm implementing this function. Have you finished it?

Not finished, just wrote a simple example that can achieve the function

@tao12345666333
Copy link
Member

@dickens7 @AlinsRan Thanks for your contribution!!!

If you want to get this done, you can reply first so I'll assign it to you. The community will know you're working on it.

@tao12345666333
Copy link
Member

This allows us to collaborate better and avoid duplication of work

@AlinsRan
Copy link
Contributor

AlinsRan commented Jul 6, 2022

Yeah. I'm implementing this function. Have you finished it?

Not finished, just wrote a simple example that can achieve the function

Do you want to try?

@tao12345666333
Copy link
Member

@dickens7 let me assign this to you?

@dickens7
Copy link
Contributor

dickens7 commented Jul 6, 2022

@tao12345666333 All right, assign it to me

Need to help me confirm this problem, it should be no problem from the current code.

route.Plugins and route.pluginConfigId are duplicate. I wrote a sample version to see if this works. I don't know if there will be compatibility issues.

dickens7@76fd068

@tao12345666333 tao12345666333 assigned dickens7 and unassigned lxm Jul 6, 2022
@tao12345666333
Copy link
Member

Assigned

@AlinsRan Can you explain to @dickens7 if you have time

@AlinsRan
Copy link
Contributor

AlinsRan commented Jul 6, 2022

@tao12345666333 All right, assign it to me

Need to help me confirm this problem, it should be no problem from the current code.

route.Plugins and route.pluginConfigId are duplicate. I wrote a sample version to see if this works. I don't know if there will be compatibility issues.
dickens7@76fd068

I think there's no problem. You can submit a PR, including the complete e2e and test cases.

@dickens7
Copy link
Contributor

dickens7 commented Jul 6, 2022

okay.

@tao12345666333
Copy link
Member

#1139 has been merged.
This feature will be introduced in v1.5 release.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants