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

Support Class Field in VS/VSR #994

Merged
merged 18 commits into from
Jun 26, 2020
Merged

Support Class Field in VS/VSR #994

merged 18 commits into from
Jun 26, 2020

Conversation

lucacome
Copy link
Member

Adding support for Class Field in VirtualServer and VirtualServerRoute resources.

Copy link
Contributor

@Rulox Rulox left a 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.

Copy link
Contributor

@pleshakov pleshakov left a 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)

internal/k8s/handlers.go Outdated Show resolved Hide resolved
internal/k8s/controller_test.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
lucacome and others added 4 commits June 22, 2020 16:27
Co-authored-by: Raúl <Rulox@users.noreply.github.com>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
@lucacome lucacome requested review from pleshakov and Rulox June 23, 2020 01:14
Copy link
Contributor

@pleshakov pleshakov left a 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.

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)

internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/handlers.go Outdated Show resolved Hide resolved
internal/k8s/controller_test.go Outdated Show resolved Hide resolved
lucacome and others added 3 commits June 23, 2020 15:22
@lucacome lucacome requested a review from pleshakov June 24, 2020 23:10
Copy link
Contributor

@pleshakov pleshakov left a 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.

internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/handlers.go Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@lucacome lucacome merged commit 3428be3 into master Jun 26, 2020
@lucacome lucacome deleted the class-field branch June 26, 2020 00:36
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants