-
Notifications
You must be signed in to change notification settings - Fork 28
make Gateway controllers watch changes on Secrets referenced by spec.listeners.tls.certificateRef #2661
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
base: main
Are you sure you want to change the base?
Conversation
eff2299 to
b643fee
Compare
b643fee to
6282675
Compare
ingress-controller/internal/controllers/gateway/gateway_controller.go
Outdated
Show resolved
Hide resolved
3234f73 to
d071cba
Compare
pmalek
left a comment
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.
Minor nits, overall this is a solid fix 👍
Can we add a changelog entry for this?
ingress-controller/internal/controllers/gateway/gateway_controller.go
Outdated
Show resolved
Hide resolved
28da430 to
7eedeff
Compare
|
please, backport this in 2.0.x so we can release it with #2707 |
…listeners.tls.certificateRef Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]> Co-authored-by: Patryk Małek <[email protected]>
Co-authored-by: Patryk Małek <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
… not GatewayTLSConfig Signed-off-by: Jintao Zhang <[email protected]>
ff4647d to
50837e5
Compare
| require.Eventually(t, func() bool { | ||
| var cur gatewayv1.GatewayClass | ||
| if err := c.Get(ctx, types.NamespacedName{Name: gc.Name}, &cur); err != nil { | ||
| return false | ||
| } | ||
| cond := metav1.Condition{ | ||
| Type: string(gatewayv1.GatewayClassConditionStatusAccepted), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(gatewayv1.GatewayClassReasonAccepted), | ||
| ObservedGeneration: cur.Generation, | ||
| LastTransitionTime: metav1.Now(), | ||
| } | ||
| // Use shared helper to set/merge the condition. | ||
| gwcd := gwcdecor.DecorateGatewayClass(&cur) | ||
| k8sutils.SetCondition(cond, gwcd) | ||
| if err := c.Status().Update(ctx, &cur); err != nil { | ||
| return false | ||
| } | ||
| return true |
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.
This looks like something that we could reuse. We already have a bunch of similar helpers in https://github.com/Kong/kong-operator/blob/e90fd984547cd60e3ce8c481f368514dd1936411/pkg/utils/test/predicates.go, e.g.
kong-operator/pkg/utils/test/predicates.go
Lines 463 to 484 in e90fd98
| // DataPlaneUpdateEventually is a helper function for tests that returns a function | |
| // that can be used to update the DataPlane. | |
| // Should be used in conjunction with require.Eventually or assert.Eventually. | |
| func DataPlaneUpdateEventually(t *testing.T, ctx context.Context, dataplaneNN types.NamespacedName, clients K8sClients, updateFunc func(*operatorv1beta1.DataPlane)) func() bool { | |
| return func() bool { | |
| cl := clients.OperatorClient.GatewayOperatorV1beta1().DataPlanes(dataplaneNN.Namespace) | |
| dp, err := cl.Get(ctx, dataplaneNN.Name, metav1.GetOptions{}) | |
| if err != nil { | |
| t.Logf("error getting dataplane: %v", err) | |
| return false | |
| } | |
| updateFunc(dp) | |
| _, err = cl.Update(ctx, dp, metav1.UpdateOptions{}) | |
| if err != nil { | |
| t.Logf("error updating dataplane: %v", err) | |
| return false | |
| } | |
| return true | |
| } | |
| } |
I think it would make sense to put it there and use it here. WDYT?
| // waitForGatewayResolvedRefsStatus waits until the Gateway's listener ResolvedRefs condition | ||
| // matches the expected status, or fails after the default timeout. | ||
| func waitForGatewayResolvedRefsStatus( | ||
| t *testing.T, | ||
| ctx context.Context, | ||
| c client.Client, | ||
| namespace, name string, | ||
| status metav1.ConditionStatus, | ||
| ) { | ||
| t.Helper() | ||
| require.Eventually(t, func() bool { | ||
| var cur gatewayv1.Gateway | ||
| if err := c.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, &cur); err != nil { | ||
| return false | ||
| } | ||
| if len(cur.Status.Listeners) == 0 { | ||
| return false | ||
| } | ||
| for _, ls := range cur.Status.Listeners { | ||
| for _, cond := range ls.Conditions { | ||
| if cond.Type == string(gatewayv1.ListenerConditionResolvedRefs) && cond.Status == status { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| }, waitTime, tickTime) | ||
| } |
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.
Ditto about defining a reusable test assertion helper.
| handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, s *corev1.Secret) []reconcile.Request { | ||
| // Use field index to list only Gateways that reference this Secret. | ||
| var gwList gwtypes.GatewayList | ||
| nn := client.ObjectKeyFromObject(s) | ||
| if err := r.List(ctx, &gwList, client.MatchingFields{idx.TLSCertificateSecretsOnGatewayIndex: nn.String()}); err != nil { | ||
| ctrllog.FromContext(ctx).Error(err, "failed to list indexed gateways for Secret watch", "secret", nn) | ||
| return nil | ||
| } | ||
| recs := make([]reconcile.Request, 0, len(gwList.Items)) | ||
| for i := range gwList.Items { | ||
| gw := gwList.Items[i] | ||
| // Optional pre-filter: only enqueue Gateways managed by this controller. | ||
| if !r.gatewayHasMatchingGatewayClass(&gw) { | ||
| continue | ||
| } | ||
| recs = append(recs, reconcile.Request{NamespacedName: client.ObjectKeyFromObject(&gw)}) | ||
| } | ||
| return recs | ||
| }), | ||
| ), |
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.
WDYT about extracting this into a function and adding a test for it?
What this PR does / why we need it:
Which issue this PR fixes
Fixes #2646
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect significant changes