Merge pull request #2198 from stuggi/fix-route-cert-secret-watch

Reconcile when custom route cert secret content changes

Author: openshift-merge-bot[bot]

internal/controller/core/openstackcontrolplane_controller.go

@@ -20,6 +20,8 @@ package core
 import (
 	"context"
 	"fmt"
+	"maps"
+	"slices"
 	"strings"
 	"time"
 
@@ -790,77 +792,59 @@ const (
 	tlsIngressCACustomIssuer          = ".spec.tls.ingress.ca.customIssuer"
 	tlsPodLevelInternalCACustomIssuer = ".spec.tls.podLevel.internal.ca.customIssuer"
 	tlsPodLevelOvnCACustomIssuer      = ".spec.tls.podLevel.ovn.ca.customIssuer"
+	apiOverrideTLSSecretField         = ".spec.apiOverride.tls.secretName"
 )
 
-var allWatchFields = []string{
-	passwordSecretField,
-	tlsCABundleSecretNameField,
-	tlsIngressCACustomIssuer,
-	tlsPodLevelInternalCACustomIssuer,
-	tlsPodLevelOvnCACustomIssuer,
-}
-
-// SetupWithManager sets up the controller with the Manager.
-func (r *OpenStackControlPlaneReconciler) SetupWithManager(
-	ctx context.Context, mgr ctrl.Manager) error {
-	// index passwordSecretField
-	if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, passwordSecretField, func(rawObj client.Object) []string {
-		// Extract the secret name from the spec, if one is provided
+// fieldIndexers maps each indexed field to the func extracting its value(s)
+// from the OpenStackControlPlane. The fields are used to reconcile when a
+// referenced secret/issuer changes, see findObjectsForSrc.
+var fieldIndexers = map[string]client.IndexerFunc{
+	passwordSecretField: func(rawObj client.Object) []string {
 		cr := rawObj.(*corev1beta1.OpenStackControlPlane)
 		if cr.Spec.Secret == "" {
 			return nil
 		}
 		return []string{cr.Spec.Secret}
-	}); err != nil {
-		return err
-	}
-
-	// index caBundleSecretNameField
-	if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, tlsCABundleSecretNameField, func(rawObj client.Object) []string {
-		// Extract the secret name from the spec, if one is provided
+	},
+	tlsCABundleSecretNameField: func(rawObj client.Object) []string {
 		cr := rawObj.(*corev1beta1.OpenStackControlPlane)
 		if cr.Spec.TLS.CaBundleSecretName == "" {
 			return nil
 		}
 		return []string{cr.Spec.TLS.CaBundleSecretName}
-	}); err != nil {
-		return err
-	}
-
-	// index tlsIngressCACustomIssuer
-	if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, tlsIngressCACustomIssuer, func(rawObj client.Object) []string {
-		// Extract the secret name from the spec, if one is provided
+	},
+	tlsIngressCACustomIssuer: func(rawObj client.Object) []string {
 		cr := rawObj.(*corev1beta1.OpenStackControlPlane)
 		if cr.Spec.TLS.Ingress.Ca.CustomIssuer == nil {
 			return nil
 		}
 		return []string{*cr.Spec.TLS.Ingress.Ca.CustomIssuer}
-	}); err != nil {
-		return err
-	}
-
-	// index tlsPodLevelInternalCACustomIssuer
-	if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, tlsPodLevelInternalCACustomIssuer, func(rawObj client.Object) []string {
-		// Extract the secret name from the spec, if one is provided
+	},
+	tlsPodLevelInternalCACustomIssuer: func(rawObj client.Object) []string {
 		cr := rawObj.(*corev1beta1.OpenStackControlPlane)
 		if cr.Spec.TLS.PodLevel.Internal.Ca.CustomIssuer == nil {
 			return nil
 		}
 		return []string{*cr.Spec.TLS.PodLevel.Internal.Ca.CustomIssuer}
-	}); err != nil {
-		return err
-	}
-
-	// index tlsPodLevelOvnCACustomIssuer
-	if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, tlsPodLevelOvnCACustomIssuer, func(rawObj client.Object) []string {
-		// Extract the secret name from the spec, if one is provided
+	},
+	tlsPodLevelOvnCACustomIssuer: func(rawObj client.Object) []string {
 		cr := rawObj.(*corev1beta1.OpenStackControlPlane)
 		if cr.Spec.TLS.PodLevel.Ovn.Ca.CustomIssuer == nil {
 			return nil
 		}
 		return []string{*cr.Spec.TLS.PodLevel.Ovn.Ca.CustomIssuer}
-	}); err != nil {
-		return err
+	},
+	// the custom public endpoint cert secrets of all services
+	apiOverrideTLSSecretField: indexAPIOverrideTLSSecret,
+}
+
+// SetupWithManager sets up the controller with the Manager.
+func (r *OpenStackControlPlaneReconciler) SetupWithManager(
+	ctx context.Context, mgr ctrl.Manager) error {
+	for field, indexer := range fieldIndexers {
+		if err := mgr.GetFieldIndexer().IndexField(ctx, &corev1beta1.OpenStackControlPlane{}, field, indexer); err != nil {
+			return err
+		}
 	}
 
 	return ctrl.NewControllerManagedBy(mgr).
@@ -919,12 +903,57 @@ func (r *OpenStackControlPlaneReconciler) SetupWithManager(
 		Complete(r)
 }
 
+// indexAPIOverrideTLSSecret is the field index func for apiOverrideTLSSecretField
+func indexAPIOverrideTLSSecret(rawObj client.Object) []string {
+	return apiOverrideTLSSecretNames(&rawObj.(*corev1beta1.OpenStackControlPlane).Spec)
+}
+
+// apiOverrideTLSSecretNames returns the custom cert secret names set via
+// apiOverride.tls.secretName (or the service specific override variants) of
+// all services.
+func apiOverrideTLSSecretNames(spec *corev1beta1.OpenStackControlPlaneSpec) []string {
+	overrides := []corev1beta1.Override{
+		spec.Keystone.APIOverride,
+		spec.Placement.APIOverride,
+		spec.Cinder.APIOverride,
+		spec.Neutron.APIOverride,
+		spec.Nova.APIOverride,
+		spec.Heat.APIOverride,
+		spec.Heat.CnfAPIOverride,
+		spec.Ironic.APIOverride,
+		spec.Ironic.InspectorOverride,
+		spec.Manila.APIOverride,
+		spec.Horizon.APIOverride,
+		spec.Telemetry.AodhAPIOverride,
+		spec.Telemetry.CloudKittyAPIOverride,
+		spec.Telemetry.PrometheusOverride,
+		spec.Telemetry.AlertmanagerOverride,
+		spec.Swift.ProxyOverride,
+		spec.Octavia.APIOverride,
+		spec.Designate.APIOverride,
+		spec.Barbican.APIOverride,
+		spec.Watcher.APIOverride,
+	}
+	overrides = slices.AppendSeq(overrides, maps.Values(spec.Glance.APIOverride))
+	for _, c := range spec.Nova.CellOverride {
+		overrides = append(overrides, c.NoVNCProxy)
+	}
+
+	names := []string{}
+	for _, o := range overrides {
+		if o.TLS != nil && o.TLS.SecretName != "" {
+			names = append(names, o.TLS.SecretName)
+		}
+	}
+	return names
+}
+
 func (r *OpenStackControlPlaneReconciler) findObjectsForSrc(ctx context.Context, src client.Object) []reconcile.Request {
 	requests := []reconcile.Request{}
 
 	Log := r.GetLogger(ctx)
 
-	for _, field := range allWatchFields {
+	for field := range fieldIndexers {
 		crList := &corev1beta1.OpenStackControlPlaneList{}
 		listOps := &client.ListOptions{
 			FieldSelector: fields.OneTermEqualSelector(field, src.GetName()),

internal/controller/core/openstackcontrolplane_controller_test.go

@@ -0,0 +1,127 @@
+package core
+
+import (
+	"context"
+	"fmt"
+	"reflect"
+	"slices"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	corev1beta1 "github.com/openstack-k8s-operators/openstack-operator/api/core/v1beta1"
+)
+
+var overrideType = reflect.TypeFor[corev1beta1.Override]()
+
+// containsOverride returns true if t is, or holds, a corev1beta1.Override.
+func containsOverride(t reflect.Type) bool {
+	switch t.Kind() {
+	case reflect.Struct:
+		if t == overrideType {
+			return true
+		}
+		if t.PkgPath() != overrideType.PkgPath() {
+			return false
+		}
+		for f := range t.Fields() {
+			if f.IsExported() && containsOverride(f.Type) {
+				return true
+			}
+		}
+	case reflect.Pointer, reflect.Slice, reflect.Array, reflect.Map:
+		return containsOverride(t.Elem())
+	}
+	return false
+}
+
+// setOverrideSecrets walks v and sets a unique TLS secretName on every
+// corev1beta1.Override found, including map values, and collects the names.
+// It fails on shapes holding an Override that it can't handle, so a new shape
+// in the spec can't be silently skipped.
+func setOverrideSecrets(t *testing.T, v reflect.Value, names *[]string) {
+	t.Helper()
+	switch {
+	case v.Type() == overrideType:
+		name := fmt.Sprintf("secret-%d", len(*names))
+		v.Set(reflect.ValueOf(corev1beta1.Override{TLS: &corev1beta1.TLSServiceOverride{SecretName: name}}))
+		*names = append(*names, name)
+	case v.Kind() == reflect.Struct && v.Type().PkgPath() == overrideType.PkgPath():
+		for i := range v.NumField() {
+			if v.Type().Field(i).IsExported() {
+				setOverrideSecrets(t, v.Field(i), names)
+			}
+		}
+	case v.Kind() == reflect.Map && v.Type().Elem().Kind() == reflect.Struct && containsOverride(v.Type().Elem()):
+		v.Set(reflect.MakeMap(v.Type()))
+		elem := reflect.New(v.Type().Elem()).Elem()
+		setOverrideSecrets(t, elem, names)
+		v.SetMapIndex(reflect.ValueOf("key").Convert(v.Type().Key()), elem)
+	case containsOverride(v.Type()):
+		t.Fatalf("unsupported shape %s holding an Override, extend setOverrideSecrets and apiOverrideTLSSecretNames", v.Type())
+	}
+}
+
+// TestAPIOverrideTLSSecretNames guards against Override fields being added to
+// the spec without being added to apiOverrideTLSSecretNames.
+func TestAPIOverrideTLSSecretNames(t *testing.T) {
+	spec := &corev1beta1.OpenStackControlPlaneSpec{}
+	if got := apiOverrideTLSSecretNames(spec); len(got) != 0 {
+		t.Fatalf("expected no secrets for empty spec, got %v", got)
+	}
+
+	var want []string
+	setOverrideSecrets(t, reflect.ValueOf(spec).Elem(), &want)
+	if len(want) == 0 {
+		t.Fatal("found no Override fields in the spec")
+	}
+	got := apiOverrideTLSSecretNames(spec)
+	slices.Sort(got)
+	slices.Sort(want)
+	if !slices.Equal(got, want) {
+		t.Errorf("secret names not collected for all Override fields\nwant: %v\ngot:  %v", want, got)
+	}
+
+	// empty secretName and nil TLS are ignored
+	spec = &corev1beta1.OpenStackControlPlaneSpec{}
+	spec.Keystone.APIOverride.TLS = &corev1beta1.TLSServiceOverride{}
+	if got := apiOverrideTLSSecretNames(spec); len(got) != 0 {
+		t.Errorf("expected empty secretName to be ignored, got %v", got)
+	}
+}
+
+// TestFindObjectsForSrcAPIOverrideTLSSecret verifies a change of a custom
+// route cert secret enqueues the control plane referencing it.
+func TestFindObjectsForSrcAPIOverrideTLSSecret(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := corev1beta1.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+
+	cp := &corev1beta1.OpenStackControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "cp", Namespace: "ns"}}
+	cp.Spec.Keystone.APIOverride.TLS = &corev1beta1.TLSServiceOverride{SecretName: "custom-cert"}
+
+	builder := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cp)
+	for field, indexer := range fieldIndexers {
+		builder = builder.WithIndex(&corev1beta1.OpenStackControlPlane{}, field, indexer)
+	}
+	r := &OpenStackControlPlaneReconciler{Client: builder.Build()}
+
+	secret := func(name, ns string) client.Object {
+		return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}}
+	}
+
+	if got := r.findObjectsForSrc(context.Background(), secret("custom-cert", "ns")); len(got) != 1 || got[0].Name != "cp" {
+		t.Errorf("expected control plane to be enqueued, got %v", got)
+	}
+	if got := r.findObjectsForSrc(context.Background(), secret("other", "ns")); len(got) != 0 {
+		t.Errorf("expected no requests for unrelated secret, got %v", got)
+	}
+	if got := r.findObjectsForSrc(context.Background(), secret("custom-cert", "other-ns")); len(got) != 0 {
+		t.Errorf("expected no requests for secret in another namespace, got %v", got)
+	}
+}

test/functional/ctlplane/openstackoperator_controller_test.go

@@ -1592,6 +1592,69 @@ var _ = Describe("OpenStackOperator controller", func() {
 
 			})
 
+			// End-to-end smoke test. It does not isolate the secret watch, a
+			// periodic requeue also picks up the change; the registered watch is
+			// asserted by the "should enqueue the control plane" test below.
+			It("should update the route when the custom cert secret content changes", func() {
+				certSecret := CreateCertSecret(names.CustomServiceCertSecretName)
+				DeferCleanup(k8sClient.Delete, ctx, certSecret)
+
+				keystoneRouteName := types.NamespacedName{Name: "keystone-public", Namespace: namespace}
+
+				Eventually(func(g Gomega) {
+					g.Expect(keystone.GetKeystoneAPI(names.KeystoneAPIName)).Should(Not(BeNil()))
+				}, timeout, interval).Should(Succeed())
+				keystone.SimulateKeystoneAPIReady(names.KeystoneAPIName)
+
+				Eventually(func(g Gomega) {
+					route := &routev1.Route{}
+					g.Expect(th.K8sClient.Get(th.Ctx, keystoneRouteName, route)).Should(Succeed())
+					g.Expect(route.Spec.TLS).Should(Not(BeNil()))
+					g.Expect(route.Spec.TLS.Certificate).Should(Equal(string(certSecret.Data["tls.crt"])))
+				}, timeout, interval).Should(Succeed())
+
+				// update the secret content, name stays the same
+				Eventually(func(g Gomega) {
+					s := &k8s_corev1.Secret{}
+					g.Expect(th.K8sClient.Get(th.Ctx, names.CustomServiceCertSecretName, s)).Should(Succeed())
+					s.Data["tls.crt"] = []byte("updated-cert")
+					g.Expect(th.K8sClient.Update(th.Ctx, s)).Should(Succeed())
+				}, timeout, interval).Should(Succeed())
+
+				Eventually(func(g Gomega) {
+					route := &routev1.Route{}
+					g.Expect(th.K8sClient.Get(th.Ctx, keystoneRouteName, route)).Should(Succeed())
+					g.Expect(route.Spec.TLS).Should(Not(BeNil()))
+					g.Expect(route.Spec.TLS.Certificate).Should(Equal("updated-cert"))
+				}, timeout, interval).Should(Succeed())
+			})
+
+			// Asserts the Secret watch registered by SetupWithManager enqueues the
+			// control plane when the custom cert secret changes. A second manager
+			// runs the real controller setup with a recording no-op reconcile, so
+			// after the control plane events settled only the watch can enqueue.
+			It("should enqueue the control plane when the custom cert secret content changes", func() {
+				DeferCleanup(k8sClient.Delete, ctx, CreateCertSecret(names.CustomServiceCertSecretName))
+				rec := startReconcileRecorder()
+
+				// the control plane is enqueued initially, wait for its events to settle
+				Eventually(func(g Gomega) {
+					count, last := rec.stats()
+					g.Expect(count).Should(BeNumerically(">", 0))
+					g.Expect(time.Since(last)).Should(BeNumerically(">", 3*time.Second))
+				}, timeout, interval).Should(Succeed())
+				before, _ := rec.stats()
+
+				s := &k8s_corev1.Secret{}
+				Expect(th.K8sClient.Get(th.Ctx, names.CustomServiceCertSecretName, s)).Should(Succeed())
+				s.Data["tls.crt"] = []byte("updated-cert")
+				Expect(th.K8sClient.Update(th.Ctx, s)).Should(Succeed())
+
+				Eventually(func(g Gomega) {
+					count, _ := rec.stats()
+					g.Expect(count).Should(BeNumerically(">", before))
+				}, 2*time.Second, interval).Should(Succeed())
+			})
 		})
 	})