-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support Class Field in VS/VSR #994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm I added a few suggestions, mostly consistency/comments.
docs-web/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
docs-web/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucacome please see my comments and suggestions.
Additionally,
There a number of cases that must be implemented. Note that when (1) endpoints, (2) configmap, (3)secret, (4) external services are updated, the configuration for the affected resources is regenerated. This means that though you filter resources with non-matched class in the handlers, since you don't filter them in other places, such resources will still appear in the configuration.
Also, make sure when processing a VS (like in syncVirtualServer
), filter out VSRs that have a different class.
Also, make sure to update the docs for ingress-class
and use-ingress-class-only
cli arguments to mention VirtualServer and VirtualServerRoute resources. That needs to be done in main.go
, cli args doc, helm chart doc (2 places)
docs-web/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
docs-web/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Raúl <Rulox@users.noreply.github.com>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments and suggestions.
I also replied here #994 (comment)
The new changes look good and cover the missing cases! However, we still need to cover the following remaining things:
(1) Also, make sure when processing a VS (like in syncVirtualServer), filter out VSRs that have a different class.
Here in (1) make sure that createVirtualServer
we don't start using VSRs of a different class.
(2) Also, make sure to update the docs for ingress-class and use-ingress-class-only cli arguments to mention VirtualServer and VirtualServerRoute resources. That needs to be done in main.go, cli args doc, helm chart doc (2 places)
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucacome thanks for the changes
The bug in createVirtualServer
is still there :( I provided more details
Also, since #996 was merged and you rebased against it, could you make sure that the fix in #996 also respect Ingress classes? It looks like that updateVirtualServersStatusFromEvents
and updateVirtualServerRoutesStatusFromEvents
are just needed to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Adding support for Class Field in VirtualServer and VirtualServerRoute resources.