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.

Sign up for GitHub

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

Jump to bottom

[maistra-2.6] Rebase gateway controller #1002

Merged
merged 5 commits into from
Jun 4, 2024
Merged

[maistra-2.6] Rebase gateway controller #1002

merged 5 commits into from
Jun 4, 2024

Conversation

eoinfennessy
Copy link
Member

No description provided.

@eoinfennessy eoinfennessy reopened this May 21, 2024
@eoinfennessy eoinfennessy changed the base branch from maistra-2.5 to maistra-2.6 May 21, 2024 10:38
@eoinfennessy eoinfennessy changed the title [WIP] Rebase gateway controller WIP: [maistra-2.6] Rebase gateway controller May 21, 2024
@jewertow jewertow added the tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges. label May 22, 2024
@openshift-ci openshift-ci bot added size/M and removed size/XXL labels May 22, 2024
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

3 similar comments
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

@openshift-ci openshift-ci bot added size/L and removed size/M labels May 23, 2024
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

@eoinfennessy eoinfennessy force-pushed the rebase-gateway-controller branch 2 times, most recently from e881c39 to 2074e01 Compare May 23, 2024 14:57
@eoinfennessy eoinfennessy changed the title WIP: [maistra-2.6] Rebase gateway controller [maistra-2.6] Rebase gateway controller May 23, 2024
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

eoinfennessy
eoinfennessy commented May 23, 2024
View reviewed changes
pilot/pkg/config/kube/crdclient/client.go
Comment on lines +356 to +359
// if in gw controller mode, don't filter gw-api resources
if features.EnableGatewayControllerMode && s.Group() == gvk.KubernetesGateway.Group {
return true
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from crdclient/cache_handler.go

@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

jewertow
jewertow reviewed May 27, 2024
View reviewed changes
Copy link
Member

@jewertow jewertow left a comment

Choose a reason for hiding this comment

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

Could you add an integration test for custom class and controller names?

pilot/pkg/config/kube/gateway/deploymentcontroller.go Outdated Show resolved Hide resolved
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

eoinfennessy
eoinfennessy commented May 30, 2024
View reviewed changes
tests/integration/servicemesh/maistra/maistra.go
Comment on lines +186 to +201
func PatchIstiodAndRestart(istioNs namespace.Getter, patch string) resource.SetupFn {
return func(ctx resource.Context) error {
kubeClient := ctx.Clusters().Default().Kube()
var lastSeenGeneration int64
if err := waitForIstiod(kubeClient, istioNs.Get(), &lastSeenGeneration); err != nil {
return err
}
if err := patchIstiodArgs(kubeClient, istioNs.Get(), patch); err != nil {
return err
}
if err := waitForIstiod(kubeClient, istioNs.Get(), &lastSeenGeneration); err != nil {
return err
}
return nil
}
}
Copy link
Member Author

@eoinfennessy eoinfennessy May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It might be worth cleaning up this file after #983 merges. There are a number of functions such as DisableWebhooksAndRestart, EnableIOR, DisableIOR that duplicate the code in this function. These could be removed and the tests consuming them modified to use this function. Patch constants could also be moved to their consuming packages (unless used in multiple packages?).

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the IOR rebase. It will be merged very soon.

@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

1 similar comment
@eoinfennessy
Copy link
Member Author

/test maistra-istio-unit-2-6

jewertow
jewertow reviewed May 30, 2024
View reviewed changes
Copy link
Member

@jewertow jewertow left a comment

Choose a reason for hiding this comment

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

Great job! Let's wait for IOR and then we will merge it.

@yannuil yannuil force-pushed the rebase-gateway-controller branch from 079a429 to 12d2feb Compare May 31, 2024 13:54
dgn and others added 5 commits June 4, 2024 11:07
@dgn @eoinfennessy
Introduce GatewayControllerMode
d2ba12a 
Co-authored-by: Daniel Grimm <[email protected]>
Signed-off-by: Yann Liu <[email protected]>

Move GatewayControllerMode filtering
@dgn @eoinfennessy
Make default gateway class and controller names configurable
b158bb8 
* gw-api: make default class name and controller name configurable

This is really useful in multi-tenant/multi-revision scenarios

Co-authored-by: Daniel Grimm <[email protected]>
Signed-off-by: Yann Liu <[email protected]>

* gw-api: use consistent controller name

Co-authored-by: Daniel Grimm <[email protected]>
Signed-off-by: Yann Liu <[email protected]>
@dgn @eoinfennessy
Allow to set ConfigMap name for istio-ca-root-cert
e0afaac 
Co-authored-by: Daniel Grimm <[email protected]>
Signed-off-by: Yann Liu <[email protected]>
@dgn @eoinfennessy
Custom gateway injection annotation
ec35ad0 
Co-authored-by: Daniel Grimm <[email protected]>
Signed-off-by: Yann Liu <[email protected]>
@eoinfennessy
Add integration tests for custom gatewayClass and controller names
3ae0bde
@eoinfennessy
Copy link
Member Author

/retest-required

@eoinfennessy eoinfennessy added okay to merge and removed tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges. labels Jun 4, 2024
@eoinfennessy eoinfennessy added tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges. and removed do-not-merge/hold labels Jun 4, 2024
@eoinfennessy
Copy link
Member Author

/retest-required

1 similar comment
@eoinfennessy
Copy link
Member Author

/retest-required

@openshift-ci OpenShift CI

This comment was marked as outdated.

Sign in to view
@eoinfennessy
Copy link
Member Author

/retest-required

@openshift-merge-bot openshift-merge-bot bot merged commit f1c1c5d into maistra:maistra-2.6 Jun 4, 2024
10 checks passed
@yannuil yannuil mentioned this pull request Jun 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jewertow jewertow jewertow approved these changes

Assignees
No one assigned
Labels
okay to merge size/L tide/merge-method-rebase Denotes a PR that should be rebased by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eoinfennessy @jewertow @dgn