Merge pull request #2207 from stuggi/lb-pub-endpt-reuse

[lb pub endpt] MetalLB public endpoint reuse: service cert SANs, BYO cert, front port

Author: openshift-merge-bot[bot]

internal/controller/core/openstackcontrolplane_controller.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"maps"
+	"reflect"
 	"slices"
 	"strings"
 	"time"
@@ -41,6 +42,7 @@ import (
 	"github.com/openstack-k8s-operators/lib-common/modules/common/backup"
 	condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	common_helper "github.com/openstack-k8s-operators/lib-common/modules/common/helper"
+	"github.com/openstack-k8s-operators/lib-common/modules/common/tls"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/webhook"
 	corev1 "k8s.io/api/core/v1"
 
@@ -125,7 +127,7 @@ func (r *OpenStackControlPlaneReconciler) GetLogger(ctx context.Context) logr.Lo
 // +kubebuilder:rbac:groups=redis.openstack.org,resources=redises,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;create;update;patch;delete;
 // +kubebuilder:rbac:groups=route.openshift.io,resources=routes/custom-host,verbs=create;update;patch
-// +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;
+// +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete;
 // +kubebuilder:rbac:groups=cert-manager.io,resources=issuers,verbs=get;list;watch;create;update;patch;delete;
 // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete;
@@ -228,6 +230,17 @@ func (r *OpenStackControlPlaneReconciler) Reconcile(ctx context.Context, req ctr
 	instance.Status.ObservedGeneration = instance.Generation
 
 	// If we're not deleting this and the service object doesn't have our finalizer, add it.
+	//
+	// This must stay the only metadata write that is followed by more spec mutation, and
+	// it returns before any is made. The reconcilers mutate instance.Spec in memory (issued
+	// cert secret names, overrides, CA bundle names), and the deferred PatchInstance builds
+	// its MergeFrom patch on the main resource whenever the metadata changed - that patch
+	// would then persist those spec changes. The persisted spec is the user input the
+	// reconcilers snapshot as the bring-your-own cert signal and the secret field indexes read;
+	// persisted operator-issued names would be mistaken for BYO secrets (see resolveServiceCert).
+	// The finalizer removal in reconcileDelete is safe for the same reason: no spec mutation
+	// has happened on that path. Do not add annotation/label/finalizer writes after the spec
+	// mutations without going through a separate patch.
 	if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) || isNewInstance {
 		return ctrl.Result{}, nil
 	}
@@ -793,6 +806,7 @@ const (
 	tlsPodLevelInternalCACustomIssuer = ".spec.tls.podLevel.internal.ca.customIssuer"
 	tlsPodLevelOvnCACustomIssuer      = ".spec.tls.podLevel.ovn.ca.customIssuer"
 	apiOverrideTLSSecretField         = ".spec.apiOverride.tls.secretName"
+	tlsServiceSecretField             = ".spec.template.tls.serviceSecretName"
 )
 
 // fieldIndexers maps each indexed field to the func extracting its value(s)
@@ -836,6 +850,8 @@ var fieldIndexers = map[string]client.IndexerFunc{
 	},
 	// the custom public endpoint cert secrets of all services
 	apiOverrideTLSSecretField: indexAPIOverrideTLSSecret,
+	// the bring-your-own pod level service cert secrets of all services
+	tlsServiceSecretField: indexTLSServiceSecret,
 }
 
 // SetupWithManager sets up the controller with the Manager.
@@ -900,6 +916,13 @@ func (r *OpenStackControlPlaneReconciler) SetupWithManager(
 			handler.EnqueueRequestsFromMapFunc(r.findControlPlaneForSrc),
 			builder.WithPredicates(backup.AnnotationChangedPredicate(openstack.ServiceCertSelector)),
 		).
+		// the service operators rename the Service ports and set its type; the
+		// endpointURL front port is injected only after that, see EnsureEndpointConfig
+		Watches(
+			&corev1.Service{},
+			handler.EnqueueRequestsFromMapFunc(r.findControlPlaneForSrc),
+			builder.WithPredicates(openstack.ServicePortOrTypeChangedPredicate()),
+		).
 		Complete(r)
 }
 
@@ -948,6 +971,83 @@ func apiOverrideTLSSecretNames(spec *corev1beta1.OpenStackControlPlaneSpec) []st
 	return names
 }
 
+// indexTLSServiceSecret is the field index func for tlsServiceSecretField
+func indexTLSServiceSecret(rawObj client.Object) []string {
+	return tlsServiceSecretNames(&rawObj.(*corev1beta1.OpenStackControlPlane).Spec)
+}
+
+// tlsServiceSecretNames returns the bring-your-own pod level service cert secret
+// names, i.e. the user input the operator reads in EnsureEndpointConfig:
+// tls.api.{public,internal} of all services (found by walking the spec, so new
+// services are covered) and the simple TLS sections of horizon, novncproxy,
+// prometheus and alertmanager. Operator owned secrets (ovn, vencrypt, metadata, ...)
+// are deliberately not indexed. The indexed spec comes from the informer cache,
+// i.e. the persisted user input - never the operator-issued names written back in
+// memory.
+func tlsServiceSecretNames(spec *corev1beta1.OpenStackControlPlaneSpec) []string {
+	names := tlsAPISecretNames(reflect.ValueOf(spec))
+
+	secrets := []*string{}
+	if t := spec.Horizon.Template; t != nil {
+		secrets = append(secrets, t.TLS.SecretName)
+	}
+	if t := spec.Nova.Template; t != nil {
+		for _, cell := range t.CellTemplates {
+			secrets = append(secrets, cell.NoVNCProxyServiceTemplate.TLS.Service.SecretName)
+		}
+	}
+	if t := spec.Telemetry.Template; t != nil {
+		secrets = append(secrets, t.MetricStorage.PrometheusTLS.SecretName, t.MetricStorage.AlertmanagerTLS.SecretName)
+	}
+	for _, sn := range secrets {
+		if sn != nil && *sn != "" {
+			names = append(names, *sn)
+		}
+	}
+	return names
+}
+
+var tlsAPIServiceType = reflect.TypeFor[tls.APIService]()
+
+// tlsAPISecretNames walks v for tls.APIService and returns the secret names of
+// its public and internal endpoint.
+func tlsAPISecretNames(v reflect.Value) []string {
+	var names []string
+	switch v.Kind() {
+	case reflect.Pointer:
+		if !v.IsNil() {
+			names = tlsAPISecretNames(v.Elem())
+		}
+	case reflect.Struct:
+		if v.Type() == tlsAPIServiceType {
+			api := v.Interface().(tls.APIService)
+			for _, sn := range []*string{api.Public.SecretName, api.Internal.SecretName} {
+				if sn != nil && *sn != "" {
+					names = append(names, *sn)
+				}
+			}
+			break
+		}
+		for i := range v.NumField() {
+			if v.Type().Field(i).IsExported() {
+				names = append(names, tlsAPISecretNames(v.Field(i))...)
+			}
+		}
+	case reflect.Slice, reflect.Array:
+		// skip slices of scalars (e.g. []byte)
+		if k := v.Type().Elem().Kind(); k == reflect.Struct || k == reflect.Pointer || k == reflect.Map {
+			for i := range v.Len() {
+				names = append(names, tlsAPISecretNames(v.Index(i))...)
+			}
+		}
+	case reflect.Map:
+		for _, k := range v.MapKeys() {
+			names = append(names, tlsAPISecretNames(v.MapIndex(k))...)
+		}
+	}
+	return names
+}
+
 func (r *OpenStackControlPlaneReconciler) findObjectsForSrc(ctx context.Context, src client.Object) []reconcile.Request {
 	requests := []reconcile.Request{}
 

internal/controller/core/openstackcontrolplane_controller_test.go

@@ -7,9 +7,16 @@ import (
 	"slices"
 	"testing"
 
+	horizonv1 "github.com/openstack-k8s-operators/horizon-operator/api/v1beta1"
+	keystonev1 "github.com/openstack-k8s-operators/keystone-operator/api/v1beta1"
+	"github.com/openstack-k8s-operators/lib-common/modules/common/tls"
+	neutronv1 "github.com/openstack-k8s-operators/neutron-operator/api/v1beta1"
+	novav1 "github.com/openstack-k8s-operators/nova-operator/api/nova/v1beta1"
+	telemetryv1 "github.com/openstack-k8s-operators/telemetry-operator/api/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -94,6 +101,125 @@ func TestAPIOverrideTLSSecretNames(t *testing.T) {
 	}
 }
 
+// setTLSAPISecrets walks v and sets a unique secretName on both endpoints of
+// every tls.APIService found, collecting the names.
+func setTLSAPISecrets(v reflect.Value, names *[]string) {
+	switch v.Kind() {
+	case reflect.Struct:
+		if v.Type() == tlsAPIServiceType {
+			api := v.Addr().Interface().(*tls.APIService)
+			for _, svc := range []*tls.GenericService{&api.Public, &api.Internal} {
+				name := fmt.Sprintf("tls-secret-%d", len(*names))
+				svc.SecretName = &name
+				*names = append(*names, name)
+			}
+			return
+		}
+		for i := range v.NumField() {
+			if v.Type().Field(i).IsExported() {
+				setTLSAPISecrets(v.Field(i), names)
+			}
+		}
+	case reflect.Pointer:
+		if v.IsNil() && v.Type().Elem().Kind() == reflect.Struct {
+			v.Set(reflect.New(v.Type().Elem()))
+		}
+		if !v.IsNil() {
+			setTLSAPISecrets(v.Elem(), names)
+		}
+	case reflect.Map:
+		if v.Type().Elem().Kind() == reflect.Struct {
+			v.Set(reflect.MakeMap(v.Type()))
+			elem := reflect.New(v.Type().Elem()).Elem()
+			setTLSAPISecrets(elem, names)
+			v.SetMapIndex(reflect.ValueOf("key").Convert(v.Type().Key()), elem)
+		}
+	}
+}
+
+// TestTLSServiceSecretNames verifies the tls.api secrets of every service in the
+// spec (incl. map values like glance) are collected.
+func TestTLSServiceSecretNames(t *testing.T) {
+	spec := &corev1beta1.OpenStackControlPlaneSpec{}
+	if got := tlsServiceSecretNames(spec); len(got) != 0 {
+		t.Fatalf("expected no secrets for empty spec, got %v", got)
+	}
+
+	var want []string
+	setTLSAPISecrets(reflect.ValueOf(spec).Elem(), &want)
+	if len(want) == 0 {
+		t.Fatal("found no tls.api fields in the spec")
+	}
+	got := tlsServiceSecretNames(spec)
+	slices.Sort(got)
+	slices.Sort(want)
+	if !slices.Equal(got, want) {
+		t.Errorf("tls.api secret names not collected\nwant: %v\ngot:  %v", want, got)
+	}
+
+	// empty secretName is ignored
+	spec = &corev1beta1.OpenStackControlPlaneSpec{}
+	spec.Keystone.Template = &keystonev1.KeystoneAPISpecCore{}
+	spec.Keystone.Template.TLS.API.Public.SecretName = ptr.To("")
+	if got := tlsServiceSecretNames(spec); len(got) != 0 {
+		t.Errorf("expected empty secretName to be ignored, got %v", got)
+	}
+}
+
+// TestTLSServiceSecretNamesSimpleTLS verifies the simple TLS sections the operator
+// reads as bring-your-own are indexed, and the operator owned ones are not.
+func TestTLSServiceSecretNamesSimpleTLS(t *testing.T) {
+	spec := &corev1beta1.OpenStackControlPlaneSpec{}
+	spec.Horizon.Template = &horizonv1.HorizonSpecCore{}
+	spec.Horizon.Template.TLS.SecretName = ptr.To("horizon-cert")
+	spec.Nova.Template = &novav1.NovaSpecCore{CellTemplates: map[string]novav1.NovaCellTemplate{"cell1": {}}}
+	cell := spec.Nova.Template.CellTemplates["cell1"]
+	cell.NoVNCProxyServiceTemplate.TLS.Service.SecretName = ptr.To("novnc-cert")
+	cell.NoVNCProxyServiceTemplate.TLS.Vencrypt.SecretName = ptr.To("vencrypt-operator-owned")
+	spec.Nova.Template.CellTemplates["cell1"] = cell
+	spec.Telemetry.Template = &telemetryv1.TelemetrySpecCore{}
+	spec.Telemetry.Template.MetricStorage.PrometheusTLS.SecretName = ptr.To("prometheus-cert")
+	spec.Telemetry.Template.MetricStorage.AlertmanagerTLS.SecretName = ptr.To("alertmanager-cert")
+	spec.Neutron.Template = &neutronv1.NeutronAPISpecCore{}
+	spec.Neutron.Template.TLS.Ovn.SecretName = ptr.To("ovn-operator-owned")
+
+	got := tlsServiceSecretNames(spec)
+	slices.Sort(got)
+	want := []string{"alertmanager-cert", "horizon-cert", "novnc-cert", "prometheus-cert"}
+	if !slices.Equal(got, want) {
+		t.Errorf("want %v, got %v", want, got)
+	}
+}
+
+// TestFindObjectsForSrcTLSServiceSecret verifies a change of a BYO service cert
+// secret enqueues the control plane referencing it.
+func TestFindObjectsForSrcTLSServiceSecret(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.Template = &keystonev1.KeystoneAPISpecCore{}
+	cp.Spec.Keystone.Template.TLS.API.Internal.SecretName = ptr.To("byo-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 string) client.Object {
+		return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}}
+	}
+	if got := r.findObjectsForSrc(context.Background(), secret("byo-cert")); 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")); len(got) != 0 {
+		t.Errorf("expected no requests for unrelated secret, got %v", got)
+	}
+}
+
 // TestFindObjectsForSrcAPIOverrideTLSSecret verifies a change of a custom
 // route cert secret enqueues the control plane referencing it.
 func TestFindObjectsForSrcAPIOverrideTLSSecret(t *testing.T) {

internal/openstack/barbican.go

@@ -108,6 +108,8 @@ func ReconcileBarbican(ctx context.Context, instance *corev1beta1.OpenStackContr
 		instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret = acSecretName
 	}
 
+	// snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert)
+	barbicanUserTLS := instance.Spec.Barbican.Template.BarbicanAPI.TLS
 	// preserve any previously set TLS certs, set CA cert
 	if instance.Spec.TLS.PodLevel.Enabled {
 		instance.Spec.Barbican.Template.BarbicanAPI.TLS = barbican.Spec.BarbicanAPI.TLS
@@ -136,7 +138,7 @@ func ReconcileBarbican(ctx context.Context, instance *corev1beta1.OpenStackContr
 			instance.Spec.Barbican.APIOverride,
 			corev1beta1.OpenStackControlPlaneExposeBarbicanReadyCondition,
 			false, // TODO: (mschuppert) could be removed when all integrated service support TLS
-			instance.Spec.Barbican.Template.BarbicanAPI.TLS,
+			barbicanUserTLS,
 		)
 		if err != nil {
 			return ctrlResult, err

internal/openstack/cinder.go

@@ -132,6 +132,8 @@ func ReconcileCinder(ctx context.Context, instance *corev1beta1.OpenStackControl
 		instance.Spec.Cinder.Template.Auth.ApplicationCredentialSecret = acSecretName
 	}
 
+	// snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert)
+	cinderUserTLS := instance.Spec.Cinder.Template.CinderAPI.TLS
 	// preserve any previously set TLS certs,set CA cert
 	if instance.Spec.TLS.PodLevel.Enabled {
 		instance.Spec.Cinder.Template.CinderAPI.TLS = cinder.Spec.CinderAPI.TLS
@@ -160,7 +162,7 @@ func ReconcileCinder(ctx context.Context, instance *corev1beta1.OpenStackControl
 			instance.Spec.Cinder.APIOverride,
 			corev1beta1.OpenStackControlPlaneExposeCinderReadyCondition,
 			false, // TODO (mschuppert) could be removed when all integrated service support TLS
-			instance.Spec.Cinder.Template.CinderAPI.TLS,
+			cinderUserTLS,
 		)
 		if err != nil {
 			return ctrlResult, err