From a434d2738044c9394c1361a44df8822b77b58a6f Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Thu, 1 Oct 2026 18:04:21 +0200 Subject: [PATCH 1/2] [lb pub endpt] Public endpoint reuse: service cert, front port, live port name Support a MetalLB LoadBalancer public endpoint with a custom hostname, certificate and port (RHOSP 17.1 -> RHOSO 18 adoption). - Service cert: SANs come from the canonical service.GetVHostServerName set ({custom host, short, FQDN}); the custom endpointURL host is added for public and internal endpoints, a bad endpointURL falls back to the in-cluster names and is logged. A bring-your-own pod-level cert is taken from tls.api..secretName (snapshotted pre-restore at every EnsureEndpointConfig call site), else one is issued. It applies to the LoadBalancer public endpoint and the internal endpoint; a Route-backed public endpoint keeps the operator-issued cert (a custom cert there goes on the Route via apiOverride.tls.secretName, which is validated only when a Route exists). Fix the cert cleanup delete target (Service cert, not Route cert). - Front port: for a TLS LoadBalancer public endpoint with an endpointURL, inject a ports[] override with the endpointURL port (the scheme default when the URL has none). There is no implicit director legacy port: without an endpointURL the Service keeps its native port, and adopters who keep a RHOSP 17.1 front port put it in the endpointURL (e.g. https://keystone.example.com:13000). Injection is skipped for non-TLS or routed endpoints, non-LoadBalancer Services, multi-port Services, and while the live port is not yet named after its endpoint type ("public"), since an override carrying the old -public name is rejected by the service operator's by-name port merge before the Service is patched. A watch on managed Services' port names and type wakes the control plane when the service operator renames the port. The override Spec is copied before it is mutated. - Routes: CreateRoute reads the target port name from the live Service instead of assuming it equals the object name. A user endpointURL is kept when a Route is replaced by a LoadBalancer. - Glance: return the requeue of the API that asked for it, and do not let a later endpoint's valid secret overwrite CustomTLSReady False set earlier in the same reconcile. Jira: OSPRH-39364 Jira: OSPRH-39366 Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Martin Schuppert --- .../core/openstackcontrolplane_controller.go | 20 +- internal/openstack/barbican.go | 4 +- internal/openstack/cinder.go | 4 +- internal/openstack/common.go | 341 +++- internal/openstack/common_test.go | 1619 +++++++++++++++++ internal/openstack/designate.go | 4 +- internal/openstack/glance.go | 13 +- internal/openstack/heat.go | 8 +- internal/openstack/horizon.go | 4 +- internal/openstack/ironic.go | 8 +- internal/openstack/keystone.go | 4 +- internal/openstack/manila.go | 4 +- internal/openstack/neutron.go | 4 +- internal/openstack/nova.go | 11 +- internal/openstack/octavia.go | 6 +- internal/openstack/placement.go | 4 +- internal/openstack/swift.go | 4 +- internal/openstack/telemetry.go | 13 +- internal/openstack/watcher.go | 4 +- .../openstackoperator_controller_test.go | 198 ++ test/functional/ctlplane/secret_watch_test.go | 12 + 21 files changed, 2179 insertions(+), 110 deletions(-) diff --git a/internal/controller/core/openstackcontrolplane_controller.go b/internal/controller/core/openstackcontrolplane_controller.go index 919f40ea8c..e180b50c9f 100644 --- a/internal/controller/core/openstackcontrolplane_controller.go +++ b/internal/controller/core/openstackcontrolplane_controller.go @@ -125,7 +125,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 +228,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 } @@ -900,6 +911,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) } diff --git a/internal/openstack/barbican.go b/internal/openstack/barbican.go index 50c6a6ce7a..718076e721 100644 --- a/internal/openstack/barbican.go +++ b/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 diff --git a/internal/openstack/cinder.go b/internal/openstack/cinder.go index 3c391878c9..b4d956b3cb 100644 --- a/internal/openstack/cinder.go +++ b/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 diff --git a/internal/openstack/common.go b/internal/openstack/common.go index 721cf38b3d..d75da7049a 100644 --- a/internal/openstack/common.go +++ b/internal/openstack/common.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "time" @@ -50,7 +51,9 @@ import ( "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -95,6 +98,32 @@ func EnsureDeleted(ctx context.Context, helper *helper.Helper, obj client.Object } +// ServicePortOrTypeChangedPredicate fires when a Service managed by the +// openstack-operator has its port names or its type changed by the service +// operator. The endpointURL front port is only injected for a LoadBalancer whose live +// port is named after its endpoint type, and nothing else wakes the control plane +// after that rename or type change. +func ServicePortOrTypeChangedPredicate() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return false }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldSvc, ok := e.ObjectOld.(*k8s_corev1.Service) + newSvc, ok2 := e.ObjectNew.(*k8s_corev1.Service) + if !ok || !ok2 { + return false + } + if _, managed := newSvc.Labels[ooSelector]; !managed { + return false + } + return oldSvc.Spec.Type != newSvc.Spec.Type || + !slices.EqualFunc(oldSvc.Spec.Ports, newSvc.Spec.Ports, + func(a, b k8s_corev1.ServicePort) bool { return a.Name == b.Name }) + }, + } +} + // GetServiceOpenStackOperatorLabel - returns the labels to be added to service override func GetServiceOpenStackOperatorLabel(value string) map[string]string { return map[string]string{ @@ -253,8 +282,9 @@ func EnsureEndpointConfig( // if a custom cert secret was provided we'll use this for // the route, otherwise the issuer is used to request one - // for the endpoint. - if ingressOverride.TLS != nil && ingressOverride.TLS.SecretName != "" { + // for the endpoint. The custom secret is the Route cert only, so it + // has no effect (is not even validated) when there is no Route. + if ed.Route.Create && ingressOverride.TLS != nil && ingressOverride.TLS.SecretName != "" { ed.Route.TLS.SecretName = ptr.To(ingressOverride.TLS.SecretName) validateSecret := &tls.GenericService{SecretName: ed.Route.TLS.SecretName} _, err := validateSecret.ValidateCertSecret(ctx, helper, instance.GetNamespace()) @@ -279,8 +309,7 @@ func EnsureEndpointConfig( err.Error())) return endpoints, ctrl.Result{}, err } - instance.Status.Conditions.MarkTrue(corev1.OpenStackControlPlaneCustomTLSReadyCondition, - corev1.OpenStackControlPlaneCustomTLSReadyMessage) + markCustomTLSReady(instance) } else { ed.Route.TLS.IssuerName = instance.GetPublicIssuer() } @@ -288,70 +317,112 @@ func EnsureEndpointConfig( if ed.Service.TLS.Enabled { ed.Service.TLS.CaBundleSecretName = tls.CABundleSecret - // if a custom cert secret was provided and ed.Route.Create == false - // we'll use this for the service, otherwise issue a cert. This is for - // use case where you deploy without ingress/routes and also use - // a LoadBalancer (MetalLB) for the public endpoints. - if !ed.Route.Create && (tlsConfig.API.Public.SecretName != nil && *tlsConfig.API.Public.SecretName != "") { - ed.Service.TLS.SecretName = tlsConfig.API.Public.SecretName - _, err := ed.Service.TLS.ValidateCertSecret(ctx, helper, instance.GetNamespace()) - if err != nil { - if k8s_errors.IsNotFound(err) { - return endpoints, ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil - } - return endpoints, ctrl.Result{}, err - } - // Delete the issued certificate if it exists - cert := certmanager.NewCertificate( - &certmgrv1.Certificate{ - ObjectMeta: metav1.ObjectMeta{ - Name: ed.Route.TLS.CertName, - Namespace: ed.Namespace, - }, - }, - 5*time.Second, - ) - err = cert.Delete(ctx, helper) - if err != nil { - return endpoints, ctrl.Result{}, err - } - } else { - // issue a certificate for public pod virthost - certRequest := certmanager.CertificateRequest{ - IssuerName: instance.GetPublicIssuer(), - CertName: ed.Service.TLS.CertName, - Hostnames: []string{ - fmt.Sprintf("%s.%s.svc", ed.Name, instance.Namespace), - fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, clusterDomain), - }, - Ips: nil, - Annotations: ed.Annotations, - Labels: util.MergeMaps(ed.Labels, map[string]string{ServiceCertSelector: ""}), - Usages: nil, - } + // Pod-level public Service cert: bring-your-own via tls.api.public.secretName + // (snapshotted pre-restore, passed as tlsConfig.API.Public.SecretName), else + // issue one. A Route-backed endpoint always uses the operator-issued cert (the + // router trusts it through the internal CA): a custom cert there belongs on the + // Route, via ingressOverride.TLS.SecretName (apiOverride.tls.secretName, handled + // above), which must never feed the service cert here. + // Hostnames come from the canonical set {custom, short, FQDN} (custom host + // only present when endpointURL is set) produced by GetVHostServerName - + // the same helper the public vhost ServerName/ServerAlias use - so the + // cert SAN never drifts from the vhost names. + certRequest := certmanager.CertificateRequest{ + IssuerName: instance.GetPublicIssuer(), + CertName: ed.Service.TLS.CertName, + Hostnames: ed.serviceCertHostnames(helper, clusterDomain), + Ips: nil, + Annotations: ed.Annotations, + Labels: util.MergeMaps(ed.Labels, map[string]string{ServiceCertSelector: ""}), + Usages: nil, + } - addSubjNames := util.GetStringListFromMap(svc.Annotations, tls.AdditionalSubjectNamesKey) - if len(addSubjNames) > 0 { - certRequest.Hostnames = append(certRequest.Hostnames, addSubjNames...) - } - if instance.Spec.TLS.Ingress.Cert.Duration != nil { - certRequest.Duration = &instance.Spec.TLS.Ingress.Cert.Duration.Duration - } - if instance.Spec.TLS.Ingress.Cert.RenewBefore != nil { - certRequest.RenewBefore = &instance.Spec.TLS.Ingress.Cert.RenewBefore.Duration + // additionalSubjectNames is a cert-only escape hatch - not part of the + // endpoint's vhost identity - so it's appended here only, never + // routed through GetVHostServerName. + addSubjNames := util.GetStringListFromMap(svc.Annotations, tls.AdditionalSubjectNamesKey) + if len(addSubjNames) > 0 { + certRequest.Hostnames = append(certRequest.Hostnames, addSubjNames...) + } + if instance.Spec.TLS.Ingress.Cert.Duration != nil { + certRequest.Duration = &instance.Spec.TLS.Ingress.Cert.Duration.Duration + } + if instance.Spec.TLS.Ingress.Cert.RenewBefore != nil { + certRequest.RenewBefore = &instance.Spec.TLS.Ingress.Cert.RenewBefore.Duration + } + + byoSecret := tlsConfig.API.Public.SecretName + if ed.Route.Create && byoSecret != nil && *byoSecret != "" { + helper.GetLogger().Info(fmt.Sprintf( + "Service %s: tls.api.public.secretName is ignored for a Route-backed endpoint, use apiOverride.tls.secretName for the Route cert", + ed.Name)) + byoSecret = nil + } + + ctrlResult, err := ed.resolveServiceCert(ctx, instance, helper, certRequest, byoSecret) + if err != nil { + return endpoints, ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return endpoints, ctrlResult, nil + } + } + + // For a TLS LoadBalancer public endpoint (no route) with an endpointURL, + // set the Service front port to the endpointURL's port so the catalog + // endpointURL and the live Service agree. Without an endpointURL the + // service keeps its native port; adopters who need a director-era + // front port (e.g. keystone 13000) put it in the endpointURL. targetPort is not touched - the port Name comes straight + // off the live Service, so glance's per-API "glance--public" + // naming needs no special-case. Skipped when: non-TLS, routed (ClusterIP), + // no endpointURL port is set, or the + // Service has more than one port (ambiguous which one is "the" public + // port - the admin assigns ports for that case via the same override). + // + // The injected entry's Name is re-synced to the live Service's port name + // on every reconcile (even once previously injected) so a later + // service-operator port-name rename doesn't leave it pinned to a stale + // value forever; only the Port number is preserved once set, since it may + // have been hand-tuned by the admin instead of left at the desired + // default. An override with more than one port entry is a deliberate + // admin config beyond what this default ever produces, so it's left + // untouched. A ports[].port that disagrees with an explicit endpointURL port + // is rejected at admission on the service operator's own CR, not in this repo + // (e.g. KeystoneAPI validates it via lib-common's + // webhook.ValidatePortNameOverrides), so the two case branches below can never + // observe a stale mismatched number once that CR update lands. + // + // Also skipped while the live port is not yet named after the endpoint + // type ("public"): the service operator hasn't renamed it, and an + // override carrying the old live name is rejected by its by-name merge + // ("unknown service port name") before the Service is ever patched, so + // the rename (and any type change) would never land. Skipping lets the + // operator patch the Service first; the Service watch + // (ServicePortOrTypeChangedPredicate) then wakes the control plane so the + // next reconcile injects. + if svc.Spec.Type == k8s_corev1.ServiceTypeLoadBalancer && !ed.Route.Create && + ed.Service.TLS.Enabled && len(svc.Spec.Ports) == 1 && + svc.Spec.Ports[0].Name == string(ed.Type) { + // desired front-port: the port implied by endpointURL (explicit, + // or its scheme's standard port) if one can be determined. + desiredPort, wantInject, err := service.GetEndpointURLPort(ed.Service.OverrideSpec.EndpointURL) + if err == nil && wantInject { + // Spec is shared with the in-memory parent spec: copy before mutating + if ed.Service.OverrideSpec.Spec == nil { + ed.Service.OverrideSpec.Spec = &service.OverrideServiceSpec{} + } else { + ed.Service.OverrideSpec.Spec = ed.Service.OverrideSpec.Spec.DeepCopy() } - certSecret, ctrlResult, err := certmanager.EnsureCert( - ctx, - helper, - certRequest, - nil) - if err != nil { - return endpoints, ctrlResult, err - } else if (ctrlResult != ctrl.Result{}) { - return endpoints, ctrlResult, nil + switch len(ed.Service.OverrideSpec.Spec.Ports) { + case 0: + ed.Service.OverrideSpec.Spec.Ports = []service.OverrideServicePort{ + {Name: svc.Spec.Ports[0].Name, Port: desiredPort}, + } + case 1: + // admin set a port explicitly - keep their number, only + // re-sync the name to the live Service (admission rejects + // a number that disagrees with endpointURL). + ed.Service.OverrideSpec.Spec.Ports[0].Name = svc.Spec.Ports[0].Name } - - ed.Service.TLS.SecretName = &certSecret.Name } } @@ -365,21 +436,25 @@ func EnsureEndpointConfig( case service.EndpointInternal: if ed.Service.TLS.Enabled { ed.Service.TLS.CaBundleSecretName = tls.CABundleSecret - // create certificate for internal pod virthost - // request certificate + // Pod-level internal Service cert: bring-your-own via tls.api.internal.secretName + // (snapshotted pre-restore, passed as tlsConfig.API.Internal.SecretName), else issue. + // Hostnames come from the canonical set {custom, short, FQDN} (custom host + // only present when endpointURL is set) produced by GetVHostServerName - + // same helper as the public branch - so a custom internal endpointURL + // host gets a matching cert SAN too (previously only the public branch + // did this). certRequest := certmanager.CertificateRequest{ - IssuerName: instance.GetInternalIssuer(), - CertName: ed.Service.TLS.CertName, - Hostnames: []string{ - fmt.Sprintf("%s.%s.svc", ed.Name, instance.Namespace), - fmt.Sprintf("%s.%s.svc.%s", ed.Name, instance.Namespace, clusterDomain), - }, + IssuerName: instance.GetInternalIssuer(), + CertName: ed.Service.TLS.CertName, + Hostnames: ed.serviceCertHostnames(helper, clusterDomain), Ips: nil, Annotations: ed.Annotations, Labels: util.MergeMaps(ed.Labels, map[string]string{ServiceCertSelector: ""}), Usages: nil, } + // additionalSubjectNames is a cert-only escape hatch - appended here only, + // never routed through GetVHostServerName (see the public branch above). addSubjNames := util.GetStringListFromMap(svc.Annotations, tls.AdditionalSubjectNamesKey) if len(addSubjNames) > 0 { certRequest.Hostnames = append(certRequest.Hostnames, addSubjNames...) @@ -390,18 +465,13 @@ func EnsureEndpointConfig( if instance.Spec.TLS.PodLevel.Internal.Cert.RenewBefore != nil { certRequest.RenewBefore = &instance.Spec.TLS.PodLevel.Internal.Cert.RenewBefore.Duration } - certSecret, ctrlResult, err := certmanager.EnsureCert( - ctx, - helper, - certRequest, - nil) + + ctrlResult, err := ed.resolveServiceCert(ctx, instance, helper, certRequest, tlsConfig.API.Internal.SecretName) if err != nil { return endpoints, ctrlResult, err } else if (ctrlResult != ctrl.Result{}) { return endpoints, ctrlResult, nil } - - ed.Service.TLS.SecretName = &certSecret.Name } } @@ -418,6 +488,104 @@ func EnsureEndpointConfig( return endpoints, ctrl.Result{}, nil } +// serviceCertHostnames returns the canonical {custom host, short, FQDN} names of the +// endpoint, the same set the vhost ServerName/ServerAlias use. On a bad endpointURL +// it falls back to the in-cluster names and logs the missing custom SAN: the service +// operator's own CR validates endpointURL at admission (e.g. KeystoneAPI via lib-common's +// webhook.ValidateEndpointURL), not this repo, so this is defense in depth only. +func (ed *EndpointDetail) serviceCertHostnames(helper *helper.Helper, clusterDomain string) []string { + base := fmt.Sprintf("%s.%s.svc", ed.Name, ed.Namespace) + sn, aliases, err := service.GetVHostServerName(ed.Service.OverrideSpec, base) + if err != nil { + helper.GetLogger().Info(fmt.Sprintf("Service %s: invalid endpointURL, cert has no custom SAN: %v", ed.Name, err)) + return []string{base, base + "." + clusterDomain} + } + return append([]string{sn}, aliases...) +} + +// markCustomTLSReady marks CustomTLSReady True unless an earlier endpoint in this +// reconcile pass already set it False (InitConditions resets all conditions at the start +// of every reconcile, so a False here is from this pass) - e.g. one glance API with a missing +// secret must not be hidden by the next one with a valid secret. +func markCustomTLSReady(instance *corev1.OpenStackControlPlane) { + if instance.Status.Conditions.IsFalse(corev1.OpenStackControlPlaneCustomTLSReadyCondition) { + return + } + instance.Status.Conditions.MarkTrue(corev1.OpenStackControlPlaneCustomTLSReadyCondition, + corev1.OpenStackControlPlaneCustomTLSReadyMessage) +} + +// resolveServiceCert resolves the pod-level Service TLS cert for an endpoint: +// bring-your-own via userSecret (deleting any previously operator-issued cert), or +// issue one via certmanager.EnsureCert using certRequest otherwise. Shared by the +// public and internal branches of EnsureEndpointConfig. +// +// userSecret MUST be the user's pre-restore BYO signal: the parent +// .Template.TLS.API..SecretName snapshotted before each reconciler's +// "preserve any previously set TLS certs" restore. The parent OpenStackControlPlane +// spec is never persisted by the operator (PatchInstance patches only metadata+status), +// so that snapshot is pure user input and can never echo an operator-issued name. +// Reading it post-restore instead would make the operator mistake its own issued cert +// for a user override and delete it on the next reconcile (the 2026-10-09 bug). +func (ed *EndpointDetail) resolveServiceCert( + ctx context.Context, + instance *corev1.OpenStackControlPlane, + helper *helper.Helper, + certRequest certmanager.CertificateRequest, + userSecret *string, +) (ctrl.Result, error) { + if userSecret != nil && *userSecret != "" { + ed.Service.TLS.SecretName = userSecret + validateSecret := &tls.GenericService{SecretName: ed.Service.TLS.SecretName} + _, err := validateSecret.ValidateCertSecret(ctx, helper, instance.GetNamespace()) + if err != nil { + if k8s_errors.IsNotFound(err) { + // Since the CA cert secret should have been manually created by the user when provided in the spec, + // we treat this as a warning because it means that reconciliation will not be able to continue. + instance.Status.Conditions.Set(condition.FalseCondition( + corev1.OpenStackControlPlaneCustomTLSReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + corev1.OpenStackControlPlaneCustomTLSReadyWaitingMessage, + *userSecret)) + return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + } + instance.Status.Conditions.Set(condition.FalseCondition( + corev1.OpenStackControlPlaneCustomTLSReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + corev1.OpenStackControlPlaneCustomTLSReadyErrorMessage, + *userSecret, + err.Error())) + return ctrl.Result{}, err + } + markCustomTLSReady(instance) + // Delete the operator-issued certificate if it exists + cert := certmanager.NewCertificate( + &certmgrv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: ed.Service.TLS.CertName, + Namespace: ed.Namespace, + }, + }, + 5*time.Second, + ) + if err := cert.Delete(ctx, helper); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + + certSecret, ctrlResult, err := certmanager.EnsureCert(ctx, helper, certRequest, nil) + if err != nil { + return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil + } + ed.Service.TLS.SecretName = &certSecret.Name + return ctrl.Result{}, nil +} + func (ed *EndpointDetail) ensureRoute( ctx context.Context, instance *corev1.OpenStackControlPlane, @@ -473,11 +641,6 @@ func (ed *EndpointDetail) ensureRoute( err = fmt.Errorf("error deleting route %s: %w", r.Name, err) return ctrl.Result{}, err } - - if ed.Service.OverrideSpec.EndpointURL != nil { - ed.Service.OverrideSpec.EndpointURL = nil - helper.GetLogger().Info(fmt.Sprintf("Service %s override endpointURL removed", svc.Name)) - } } } } @@ -551,8 +714,14 @@ func (ed *EndpointDetail) CreateRoute( owner metav1.Object, ) (ctrl.Result, error) { // initialize the route with any custom provided route override - // per default use the service name as targetPortName if we don't have the annotation. + // per default read the target port name straight off the live Service's single + // port, rather than assuming it matches the Service object's name - the two are + // not guaranteed to be identical. With several ports there is no way to tell + // which one to target, so keep the Service name (override via annotation). targetPortName := ed.Service.Spec.Name + if len(ed.Service.Spec.Spec.Ports) == 1 { + targetPortName = ed.Service.Spec.Spec.Ports[0].Name + } if name, ok := ed.Service.Spec.Annotations[service.AnnotationIngressTargetPortNameKey]; ok && name != "" { targetPortName = name } diff --git a/internal/openstack/common_test.go b/internal/openstack/common_test.go index 21b68748ff..040719d028 100644 --- a/internal/openstack/common_test.go +++ b/internal/openstack/common_test.go @@ -17,28 +17,41 @@ limitations under the License. package openstack import ( + "cmp" "context" + "fmt" "testing" + "time" . "github.com/onsi/gomega" //revive:disable:dot-imports + certmgrv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + certmgrmetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" routev1 "github.com/openshift/api/route/v1" + "github.com/openstack-k8s-operators/lib-common/modules/certmanager" + "github.com/openstack-k8s-operators/lib-common/modules/common/clusterdns" "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + "github.com/openstack-k8s-operators/lib-common/modules/common/service" + "github.com/openstack-k8s-operators/lib-common/modules/common/tls" corev1 "github.com/openstack-k8s-operators/openstack-operator/api/core/v1beta1" k8s_corev1 "k8s.io/api/core/v1" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/event" ) func init() { // Register OpenShift route scheme _ = routev1.AddToScheme(scheme.Scheme) _ = corev1.AddToScheme(scheme.Scheme) + _ = certmgrv1.AddToScheme(scheme.Scheme) } // setupTestHelper creates a fake client and helper for testing @@ -46,11 +59,13 @@ func setupTestHelper(objects ...client.Object) *helper.Helper { s := scheme.Scheme _ = routev1.AddToScheme(s) _ = corev1.AddToScheme(s) + _ = certmgrv1.AddToScheme(s) fakeClient := fakeclient.NewClientBuilder(). WithScheme(s). WithObjects(objects...). WithStatusSubresource(&routev1.Route{}). + WithStatusSubresource(&certmgrv1.Certificate{}). Build() // Create a fake kubernetes clientset @@ -641,3 +656,1607 @@ func TestValidateRouteCertSecret_NilSecret(t *testing.T) { // A nil secret is tolerated (the caller may not have fetched one). g.Expect(validateRouteCertSecret(nil, "custom-route-cert")).ToNot(HaveOccurred()) } + +// TestEnsureEndpointConfig_EndpointURL_CustomHostname tests that when endpointURL +// is set on a LoadBalancer public endpoint, the custom hostname flows through correctly. +// issuerFor creates a fake cert-manager Issuer so certmanager.EnsureCert can +// proceed far enough to create the Certificate object (the secret need not +// exist - EnsureCert requeues at that point, after the Certificate is created). +func issuerFor(name string) *certmgrv1.Issuer { + return &certmgrv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test-namespace"}, + } +} + +func TestEnsureEndpointConfig_EndpointURL_CustomHostname(t *testing.T) { + ctx := context.Background() + clusterDomain := clusterdns.GetDNSClusterDomain() + + tests := []struct { + name string + endpointURL *string + expectedSANs []string + }{ + { + name: "endpointURL with custom hostname", + endpointURL: ptr.To("https://keystone.example.com:5000"), + expectedSANs: []string{ + "keystone.example.com", + "keystone-public.test-namespace.svc", + "keystone-public.test-namespace.svc." + clusterDomain, + }, + }, + { + name: "no endpointURL", + endpointURL: nil, + expectedSANs: []string{ + "keystone-public.test-namespace.svc", + "keystone-public.test-namespace.svc." + clusterDomain, + }, + }, + { + name: "empty endpointURL", + endpointURL: ptr.To(""), + expectedSANs: []string{ + "keystone-public.test-namespace.svc", + "keystone-public.test-namespace.svc." + clusterDomain, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // Create a public service with endpoint annotation + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "false", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + {Name: "keystone-public", Port: 5000}, + }, + }, + } + + h := setupTestHelper(publicSvc, issuerFor("rootca-public")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + PodLevel: corev1.TLSPodLevelConfig{ + Enabled: true, + }, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{} + if tt.endpointURL != nil { + svcOverrides[service.EndpointPublic] = service.RoutedOverrideSpec{ + EndpointURL: tt.endpointURL, + } + } + + _, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + svcOverrides, + corev1.Override{}, + "TestCondition", + false, + tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + cert := &certmgrv1.Certificate{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{ + Name: "keystone-public-svc", Namespace: "test-namespace", + }, cert)).To(Succeed()) + g.Expect(cert.Spec.DNSNames).To(ConsistOf(tt.expectedSANs)) + }) + } +} + +// TestEnsureEndpointConfig_InternalEndpointURL_CustomHostname is the Gap 2 +// regression guard: a custom internal endpointURL must land in the internal +// cert SAN too, not just the public one. +func TestEnsureEndpointConfig_InternalEndpointURL_CustomHostname(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + clusterDomain := clusterdns.GetDNSClusterDomain() + + internalSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-internal", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "internal", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeClusterIP, + Ports: []k8s_corev1.ServicePort{{Name: "keystone-internal", Port: 5000}}, + }, + } + + h := setupTestHelper(internalSvc, issuerFor("rootca-internal")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{PodLevel: corev1.TLSPodLevelConfig{Enabled: true}}, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*internalSvc}} + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointInternal: {EndpointURL: ptr.To("https://keystone-internal.example.com")}, + } + + _, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, svcList, svcOverrides, corev1.Override{}, + "TestCondition", false, tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + cert := &certmgrv1.Certificate{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{ + Name: "keystone-internal-svc", Namespace: "test-namespace", + }, cert)).To(Succeed()) + g.Expect(cert.Spec.DNSNames).To(ConsistOf( + "keystone-internal.example.com", + "keystone-internal.test-namespace.svc", + "keystone-internal.test-namespace.svc."+clusterDomain, + )) +} + +// TestEnsureEndpointConfig_InternalBYOCert verifies tls.api.internal.secretName is +// used as the internal pod-level cert and the operator-issued Service cert deleted. +func TestEnsureEndpointConfig_InternalBYOCert(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + internalSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-internal", + Namespace: "test-namespace", + Annotations: map[string]string{service.AnnotationEndpointKey: "internal"}, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeClusterIP, + Ports: []k8s_corev1.ServicePort{{Name: "keystone-internal", Port: 5000}}, + }, + } + byoSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "byo-internal-cert", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + issuedCert := &certmgrv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "keystone-internal-svc", Namespace: "test-namespace"}, + } + + h := setupTestHelper(internalSvc, byoSecret, issuedCert, issuerFor("rootca-internal")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{PodLevel: corev1.TLSPodLevelConfig{Enabled: true}}, + }, + } + tlsConfig := tls.API{} + tlsConfig.API.Internal.SecretName = ptr.To("byo-internal-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*internalSvc}}, + map[service.Endpoint]service.RoutedOverrideSpec{}, corev1.Override{}, + "TestCondition", false, tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + ed := endpoints.EndpointDetails[service.EndpointInternal] + g.Expect(ed.Service.TLS.SecretName).ToNot(BeNil()) + g.Expect(*ed.Service.TLS.SecretName).To(Equal("byo-internal-cert")) + + err = h.GetClient().Get(ctx, client.ObjectKeyFromObject(issuedCert), &certmgrv1.Certificate{}) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "issued internal service cert must be deleted") +} + +// TestResolveServiceCert_BYO covers the BYO outcomes of resolveServiceCert: a missing +// secret requeues with CustomTLSReady False and must not continue, a present secret +// marks CustomTLSReady True and deletes the previously issued cert. +func TestResolveServiceCert_BYO(t *testing.T) { + for _, secretExists := range []bool{false, true} { + t.Run(fmt.Sprintf("secretExists=%t", secretExists), func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + issuedCert := &certmgrv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "keystone-public-svc", Namespace: "test-namespace"}, + } + objects := []client.Object{issuedCert} + if secretExists { + objects = append(objects, &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "byo-cert", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + }) + } + h := setupTestHelper(objects...) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + } + ed := &EndpointDetail{ + Name: "keystone-public", + Namespace: "test-namespace", + Service: ServiceDetails{TLS: ServiceTLSDetails{CertName: "keystone-public-svc"}}, + } + + res, err := ed.resolveServiceCert(ctx, instance, h, certmanager.CertificateRequest{}, ptr.To("byo-cert")) + g.Expect(err).ToNot(HaveOccurred()) + + cond := instance.Status.Conditions.Get(corev1.OpenStackControlPlaneCustomTLSReadyCondition) + g.Expect(cond).ToNot(BeNil()) + getErr := h.GetClient().Get(ctx, client.ObjectKeyFromObject(issuedCert), &certmgrv1.Certificate{}) + if secretExists { + g.Expect(res).To(Equal(ctrl.Result{})) + g.Expect(cond.Status).To(Equal(k8s_corev1.ConditionTrue)) + g.Expect(k8s_errors.IsNotFound(getErr)).To(BeTrue(), "issued cert must be deleted") + } else { + g.Expect(res.RequeueAfter).To(Equal(10 * time.Second)) + g.Expect(cond.Status).To(Equal(k8s_corev1.ConditionFalse)) + g.Expect(getErr).ToNot(HaveOccurred(), "issued cert must be kept until the BYO secret exists") + } + }) + } +} + +// TestEnsureEndpointConfig_AdditionalSubjectNames verifies additionalSubjectNames +// land on the cert SAN on top of the canonical {short, FQDN} set, since it is a +// cert-only escape hatch appended after the GetVHostServerName call, not a +// replacement for the canonical set. +func TestEnsureEndpointConfig_AdditionalSubjectNames(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + clusterDomain := clusterdns.GetDNSClusterDomain() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "false", + tls.AdditionalSubjectNamesKey: "extra1.example.com,extra2.example.com", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{{Name: "keystone-public", Port: 5000}}, + }, + } + + h := setupTestHelper(publicSvc, issuerFor("rootca-public")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{PodLevel: corev1.TLSPodLevelConfig{Enabled: true}}, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + + _, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, svcList, + map[service.Endpoint]service.RoutedOverrideSpec{}, corev1.Override{}, + "TestCondition", false, tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + cert := &certmgrv1.Certificate{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{ + Name: "keystone-public-svc", Namespace: "test-namespace", + }, cert)).To(Succeed()) + g.Expect(cert.Spec.DNSNames).To(ConsistOf( + "keystone-public.test-namespace.svc", + "keystone-public.test-namespace.svc."+clusterDomain, + "extra1.example.com", + "extra2.example.com", + )) +} + +// TestEnsureEndpointConfig_BadEndpointURL_FallsBackToBaseNames covers the +// common.go defense-in-depth fallback for a CR created before the admission +// webhook validated endpointURL (or any value that slips through): the +// reconcile must degrade to the base {short, FQDN} SANs, never error or panic. +func TestEnsureEndpointConfig_BadEndpointURL_FallsBackToBaseNames(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + clusterDomain := clusterdns.GetDNSClusterDomain() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "false", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{{Name: "keystone-public", Port: 5000}}, + }, + } + + h := setupTestHelper(publicSvc, issuerFor("rootca-public")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{PodLevel: corev1.TLSPodLevelConfig{Enabled: true}}, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + // no scheme/host - GetVHostServerName returns ErrEndpointURLNoHost + service.EndpointPublic: {EndpointURL: ptr.To("not-a-valid-url")}, + } + + g.Expect(func() { + _, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, svcList, svcOverrides, corev1.Override{}, + "TestCondition", false, tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + }).ToNot(Panic()) + + cert := &certmgrv1.Certificate{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{ + Name: "keystone-public-svc", Namespace: "test-namespace", + }, cert)).To(Succeed()) + g.Expect(cert.Spec.DNSNames).To(ConsistOf( + "keystone-public.test-namespace.svc", + "keystone-public.test-namespace.svc."+clusterDomain, + )) +} + +// TestEnsureEndpointConfig_LoadBalancerBYOCert tests that for LoadBalancer +// endpoints the pod-level service cert is brought-your-own via +// tls.api.public.secretName (passed as tlsConfig.API.Public.SecretName), while +// apiOverride.tls.secretName (ingressOverride) is the Route cert only and must NOT +// feed the service cert. +func TestEnsureEndpointConfig_LoadBalancerBYOCert(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + serviceType k8s_corev1.ServiceType + ingressOverrideTLS *string + tlsConfigPublicSecret *string + expectSecretUsed bool + expectedSecretName string + }{ + { + // apiOverride.tls is route-only: it must not become the service cert, so + // the service cert falls through to issuance instead of the override secret. + name: "LoadBalancer ingressOverride does not feed service cert", + serviceType: k8s_corev1.ServiceTypeLoadBalancer, + ingressOverrideTLS: ptr.To("custom-service-cert"), + expectSecretUsed: false, + }, + { + name: "LoadBalancer with tls.api.public.secretName", + serviceType: k8s_corev1.ServiceTypeLoadBalancer, + ingressOverrideTLS: nil, + tlsConfigPublicSecret: ptr.To("global-public-cert"), + expectSecretUsed: true, + expectedSecretName: "global-public-cert", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + // Create a public service + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "true", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: tt.serviceType, + Ports: []k8s_corev1.ServicePort{ + {Name: "keystone-public", Port: 5000}, + }, + }, + } + + // Create the BYO cert secret if expected + var objects []client.Object + // Create the issuers needed for cert issuance + publicIssuer := &certmgrv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rootca-public", + Namespace: "test-namespace", + }, + } + objects = append(objects, publicIssuer) + // A previously operator-issued service cert: must be deleted when the + // user brings their own, left alone otherwise. + issuedCert := &certmgrv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "keystone-public-svc", Namespace: "test-namespace"}, + Status: certmgrv1.CertificateStatus{Conditions: []certmgrv1.CertificateCondition{ + {Type: certmgrv1.CertificateConditionReady, Status: certmgrmetav1.ConditionTrue}, + }}, + } + issuedSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "cert-keystone-public-svc", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + objects = append(objects, issuedCert, issuedSecret) + if tt.expectSecretUsed { + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: tt.expectedSecretName, + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + objects = append(objects, certSecret) + } + objects = append(objects, publicSvc) + + h := setupTestHelper(objects...) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{ + Enabled: true, + }, + PodLevel: corev1.TLSPodLevelConfig{ + Enabled: true, + }, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + + ingressOverride := corev1.Override{} + if tt.ingressOverrideTLS != nil { + ingressOverride.TLS = &corev1.TLSServiceOverride{ + SecretName: *tt.ingressOverrideTLS, + } + } + + tlsConfig := tls.API{} + if tt.tlsConfigPublicSecret != nil { + tlsConfig.API.Public.SecretName = tt.tlsConfigPublicSecret + } + + // Call EnsureEndpointConfig + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + map[service.Endpoint]service.RoutedOverrideSpec{}, + ingressOverride, + "TestCondition", + false, + tlsConfig, + ) + + if tt.expectSecretUsed { + // Should succeed when BYO cert secret exists and is valid + g.Expect(err).ToNot(HaveOccurred()) + + // Verify the service cert uses the expected secret + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.TLS.SecretName).ToNot(BeNil()) + g.Expect(*publicEndpoint.Service.TLS.SecretName).To(Equal(tt.expectedSecretName)) + + // the operator-issued Service cert (not the Route cert) is deleted + err = h.GetClient().Get(ctx, client.ObjectKeyFromObject(issuedCert), &certmgrv1.Certificate{}) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "issued service cert must be deleted") + } else { + // For ClusterIP or when no BYO cert, cert issuance will be attempted + // (may fail in test env, but that's expected) + // apiOverride (ingress) secret is the Route cert only: with no Route it is + // not even validated (it does not exist here), and it must NOT leak into + // the service cert - the operator-issued one is used and kept. + g.Expect(err).ToNot(HaveOccurred()) + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.TLS.SecretName).ToNot(BeNil()) + g.Expect(*publicEndpoint.Service.TLS.SecretName).To(Equal("cert-keystone-public-svc")) + g.Expect(h.GetClient().Get(ctx, client.ObjectKeyFromObject(issuedCert), &certmgrv1.Certificate{})).To(Succeed()) + g.Expect(instance.Status.Conditions.Get(corev1.OpenStackControlPlaneCustomTLSReadyCondition)).To(BeNil()) + } + }) + } +} + +// TestEnsureEndpointConfig_FrontPortInjection_TLSLoadBalancer tests that a TLS +// LoadBalancer public endpoint without an endpointURL keeps its native port: no +// director/RHOSP legacy port is injected implicitly. +func TestEnsureEndpointConfig_FrontPortInjection_TLSLoadBalancer(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 5000}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + map[service.Endpoint]service.RoutedOverrideSpec{}, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) +} + +// TestEnsureEndpointConfig_FrontPortInjection_Skips tests the skip conditions: +// non-TLS, non-LoadBalancer, and a user-supplied ports[] +// override whose Port number must win over the default (its Name is still +// re-synced to the live Service port - see +// TestEnsureEndpointConfig_FrontPortInjection_RefreshesStaleName for that). +func TestEnsureEndpointConfig_FrontPortInjection_Skips(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + svcType k8s_corev1.ServiceType + podLevelTLS bool + userPorts []service.OverrideServicePort + expectedPort []service.OverrideServicePort + }{ + { + name: "non-TLS LoadBalancer public endpoint", + podLevelTLS: false, + }, + { + // no ingress-create annotation, so Route.Create is false: only the + // Service type keeps a non-LoadBalancer public Service out. + name: "ClusterIP public endpoint without ingress-create annotation", + svcType: k8s_corev1.ServiceTypeClusterIP, + podLevelTLS: true, + }, + { + name: "user-supplied port number wins, name re-synced", + podLevelTLS: true, + // Name deliberately diverges from the live Service port's name + // ("public") so the assertion below can't pass by + // coincidence - it proves Name gets re-synced while Port doesn't. + userPorts: []service.OverrideServicePort{{Name: "stale-name", Port: 9999}}, + expectedPort: []service.OverrideServicePort{{Name: "public", Port: 9999}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: cmp.Or(tt.svcType, k8s_corev1.ServiceTypeLoadBalancer), + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 5000}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: tt.podLevelTLS}, + }, + }, + } + + // always set an endpointURL so only the skip condition under test + // keeps the port from being injected + override := service.RoutedOverrideSpec{EndpointURL: ptr.To("https://keystone.foo.bar:13000")} + if tt.userPorts != nil { + override.OverrideSpec = service.OverrideSpec{ + Spec: &service.OverrideServiceSpec{Ports: tt.userPorts}, + } + } + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{service.EndpointPublic: override} + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + svcOverrides, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + if tt.userPorts != nil { + expected := tt.expectedPort + if expected == nil { + expected = tt.userPorts + } + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).ToNot(BeNil()) + g.Expect(publicEndpoint.Service.OverrideSpec.Spec.Ports).To(Equal(expected)) + } else { + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) + } + }) + } +} + +// TestEnsureEndpointConfig_FrontPortInjection_SkipsRoute tests that a routed +// (ClusterIP) public endpoint never gets the legacy port injected, even when pod +// level TLS is enabled for the service. +func TestEnsureEndpointConfig_FrontPortInjection_SkipsRoute(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "true", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeClusterIP, + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 5000}, + }, + }, + } + + // Pre-create the service cert secret so EnsureCert's issuance path (taken + // because this is a routed endpoint, so the BYO-secret shortcut does not + // apply) finds it immediately instead of requeuing. + svcCertSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-keystone-public-svc", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + }, + } + publicIssuer := &certmgrv1.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rootca-public", + Namespace: "test-namespace", + }, + } + + h := setupTestHelper(publicSvc, svcCertSecret, publicIssuer) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + // Route (ingress) TLS left disabled so route creation doesn't need + // its own cert plumbing - only pod-level (service) TLS matters here. + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: {EndpointURL: ptr.To("https://keystone.foo.bar:13000")}, + }, + corev1.Override{}, + "TestCondition", + false, + tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) +} + +// TestEnsureEndpointConfig_FrontPortInjection_SkipsMultiplePorts tests that a +// public Service with more than one port is left alone - it's ambiguous which +// port is "the" public one, and the admin assigns ports for that case via the +// existing ports[] override instead. +func TestEnsureEndpointConfig_FrontPortInjection_SkipsMultiplePorts(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 5000}, + {Name: "keystone-public-2", Port: 5001}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: {EndpointURL: ptr.To("https://keystone.foo.bar:13000")}, + }, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) +} + +// TestEnsureEndpointConfig_FrontPortInjection_FollowsEndpointURL tests that +// the Service front-port follows the port implied by endpointURL - explicit, +// or the scheme's standard port when omitted - instead of backing off to +// native, so the catalog URL and the live Service agree. +func TestEnsureEndpointConfig_FrontPortInjection_FollowsEndpointURL(t *testing.T) { + tests := []struct { + name string + endpointURL *string + userPorts []service.OverrideServicePort + expectedPort []service.OverrideServicePort + }{ + { + // Also covers clearing a previously-set ports[] override on a later + // reconcile: EnsureEndpointConfig is stateless per call, so that + // collapses to this same case - the Service falls back to the + // endpointURL port, not to native. + name: "endpointURL with explicit port, no ports[] override: Service follows the URL port", + endpointURL: ptr.To("https://placement.foo.bar:13000"), + expectedPort: []service.OverrideServicePort{{Name: "public", Port: 13000}}, + }, + { + name: "endpointURL with explicit port, matching ports[] override: name re-synced", + endpointURL: ptr.To("https://placement.foo.bar:13000"), + userPorts: []service.OverrideServicePort{{Name: "stale-name", Port: 13000}}, + expectedPort: []service.OverrideServicePort{{Name: "public", Port: 13000}}, + }, + { + name: "endpointURL without a port: falls back to the https scheme default", + endpointURL: ptr.To("https://placement.foo.bar"), + expectedPort: []service.OverrideServicePort{{Name: "public", Port: 443}}, + }, + { + name: "endpointURL with an unrecognized scheme and no port: no inject, stays native", + endpointURL: ptr.To("ftp://placement.foo.bar"), + expectedPort: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 8778}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + override := service.RoutedOverrideSpec{EndpointURL: tt.endpointURL} + if tt.userPorts != nil { + override.OverrideSpec = service.OverrideSpec{ + Spec: &service.OverrideServiceSpec{Ports: tt.userPorts}, + } + } + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: override, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + svcOverrides, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + if tt.expectedPort == nil { + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) + return + } + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).ToNot(BeNil()) + g.Expect(publicEndpoint.Service.OverrideSpec.Spec.Ports).To(Equal(tt.expectedPort)) + }) + } +} + +// TestEnsureEndpointConfig_FrontPortInjection_RefreshesStaleName tests that a +// previously-injected ports[] entry has its Name re-synced to the live +// Service's current port name on every reconcile - e.g. after a service +// operator renames the port - while the Port value (which may have been +// hand-tuned by the admin) is preserved as-is. +func TestEnsureEndpointConfig_FrontPortInjection_RefreshesStaleName(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + // Renamed port: object name stays "placement-public", but the + // live port is now bare "public". + {Name: "public", Port: 8778}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + const legacyPort int32 = 13778 + + // Simulate a stale entry injected by a previous reconcile, back when the + // live Service's port was still named "placement-public". + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: { + EndpointURL: ptr.To("https://placement.foo.bar:13778"), + OverrideSpec: service.OverrideSpec{ + Spec: &service.OverrideServiceSpec{ + Ports: []service.OverrideServicePort{{Name: "placement-public", Port: legacyPort}}, + }, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + svcOverrides, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).ToNot(BeNil()) + g.Expect(publicEndpoint.Service.OverrideSpec.Spec.Ports).To(Equal( + []service.OverrideServicePort{{Name: "public", Port: legacyPort}}, + )) + // the caller's (parent spec) override must not be mutated in place + g.Expect(svcOverrides[service.EndpointPublic].Spec.Ports[0].Name).To(Equal("placement-public")) +} + +// TestEnsureEndpointConfig_FrontPortInjection_SkipsUnrenamedPort tests that no +// ports[] entry is injected while the live Service port still carries the old +// "-public" name: the service operator's by-name merge would reject +// that name before it could patch (rename) the Service, deadlocking the rename. +func TestEnsureEndpointConfig_FrontPortInjection_SkipsUnrenamedPort(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement-public", + Namespace: "test-namespace", + Annotations: map[string]string{service.AnnotationEndpointKey: "public"}, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{{Name: "placement-public", Port: 8778}}, + }, + } + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "global-public-cert", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}}, + map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: {EndpointURL: ptr.To("https://keystone.foo.bar:13000")}, + }, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).To(BeNil()) +} + +// TestEnsureEndpointConfig_FrontPortInjection_PreservesMultiplePortsOverride +// tests that an override with more than one port entry (an admin config this +// default never produces on its own) is left completely untouched. +func TestEnsureEndpointConfig_FrontPortInjection_PreservesMultiplePortsOverride(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 5000}, + }, + }, + } + + certSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "global-public-cert", + Namespace: "test-namespace", + }, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + + h := setupTestHelper(publicSvc, certSecret) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + + userPorts := []service.OverrideServicePort{ + {Name: "public", Port: 9999}, + {Name: "keystone-debug", Port: 9998}, + } + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: { + EndpointURL: ptr.To("https://keystone.foo.bar:13000"), + OverrideSpec: service.OverrideSpec{ + Spec: &service.OverrideServiceSpec{Ports: userPorts}, + }, + }, + } + + svcList := &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}} + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("global-public-cert") + + endpoints, _, err := EnsureEndpointConfig( + ctx, + instance, + h, + instance, + svcList, + svcOverrides, + corev1.Override{}, + "TestCondition", + false, + tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + publicEndpoint := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(publicEndpoint.Service.OverrideSpec.Spec).ToNot(BeNil()) + g.Expect(publicEndpoint.Service.OverrideSpec.Spec.Ports).To(Equal(userPorts)) +} + +// TestCreateRoute_TargetPortName_ReadsLivePortName tests that CreateRoute reads +// the target port name straight off the live Service's port, not off the +// Service object's own name - the two are not guaranteed to be identical. +func TestCreateRoute_TargetPortName_ReadsLivePortName(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + h := setupTestHelper() + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + } + + ed := &EndpointDetail{ + Name: "placement-public", + Namespace: "test-namespace", + Service: ServiceDetails{ + Spec: &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement-public", + Namespace: "test-namespace", + }, + Spec: k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 8778}, + }, + }, + }, + }, + } + + _, err := ed.CreateRoute(ctx, instance, h, instance) + g.Expect(err).ToNot(HaveOccurred()) + + createdRoute := &routev1.Route{} + err = h.GetClient().Get(ctx, types.NamespacedName{Name: ed.Name, Namespace: ed.Namespace}, createdRoute) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(createdRoute.Spec.Port).ToNot(BeNil()) + g.Expect(createdRoute.Spec.Port.TargetPort.String()).To(Equal("public")) +} + +// TestCreateRoute_TargetPortName_MultiplePortsKeepsServiceName tests that with +// several live ports the first one is not picked arbitrarily: the Service name +// stays the default target. +func TestCreateRoute_TargetPortName_MultiplePortsKeepsServiceName(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + h := setupTestHelper() + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + } + + ed := &EndpointDetail{ + Name: "placement-public", + Namespace: "test-namespace", + Service: ServiceDetails{ + Spec: &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "placement-public", Namespace: "test-namespace"}, + Spec: k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 8778}, + {Name: "other", Port: 9999}, + }, + }, + }, + }, + } + + _, err := ed.CreateRoute(ctx, instance, h, instance) + g.Expect(err).ToNot(HaveOccurred()) + + createdRoute := &routev1.Route{} + g.Expect(h.GetClient().Get(ctx, types.NamespacedName{Name: ed.Name, Namespace: ed.Namespace}, createdRoute)).To(Succeed()) + g.Expect(createdRoute.Spec.Port.TargetPort.String()).To(Equal("placement-public")) +} + +// TestCreateRoute_TargetPortName_AnnotationOverrideWins tests that the +// AnnotationIngressTargetPortNameKey override still takes precedence over the +// live Service's port name. +func TestCreateRoute_TargetPortName_AnnotationOverrideWins(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + h := setupTestHelper() + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-controlplane", + Namespace: "test-namespace", + }, + } + + ed := &EndpointDetail{ + Name: "placement-public", + Namespace: "test-namespace", + Service: ServiceDetails{ + Spec: &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationIngressTargetPortNameKey: "custom-target-port", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{ + {Name: "public", Port: 8778}, + }, + }, + }, + }, + } + + _, err := ed.CreateRoute(ctx, instance, h, instance) + g.Expect(err).ToNot(HaveOccurred()) + + createdRoute := &routev1.Route{} + err = h.GetClient().Get(ctx, types.NamespacedName{Name: ed.Name, Namespace: ed.Namespace}, createdRoute) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(createdRoute.Spec.Port).ToNot(BeNil()) + g.Expect(createdRoute.Spec.Port.TargetPort.String()).To(Equal("custom-target-port")) +} + +func TestServicePortOrTypeChangedPredicate(t *testing.T) { + svc := func(label bool, ports ...string) *k8s_corev1.Service { + s := &k8s_corev1.Service{} + if label { + s.Labels = map[string]string{ooSelector: ""} + } + for _, p := range ports { + s.Spec.Ports = append(s.Spec.Ports, k8s_corev1.ServicePort{Name: p, Port: 8778}) + } + return s + } + tests := []struct { + name string + old, new *k8s_corev1.Service + want bool + }{ + {"managed service, port renamed", svc(true, "placement-public"), svc(true, "public"), true}, + {"managed service, port number only", svc(true, "public"), func() *k8s_corev1.Service { + s := svc(true, "public") + s.Spec.Ports[0].Port = 13778 + return s + }(), false}, + {"managed service, type changed", svc(true, "public"), func() *k8s_corev1.Service { + s := svc(true, "public") + s.Spec.Type = k8s_corev1.ServiceTypeLoadBalancer + return s + }(), true}, + {"managed service, unchanged", svc(true, "public"), svc(true, "public"), false}, + {"unmanaged service, port renamed", svc(false, "a"), svc(false, "b"), false}, + } + p := ServicePortOrTypeChangedPredicate() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(p.Update(event.UpdateEvent{ObjectOld: tt.old, ObjectNew: tt.new})).To(Equal(tt.want)) + }) + } + g := NewWithT(t) + g.Expect(p.Create(event.CreateEvent{Object: svc(true, "public")})).To(BeFalse()) +} + +// TestResolveServiceCert_BYOMissingNotHiddenByLaterValid mirrors the glance per-API +// loop: an endpoint with a missing BYO secret sets CustomTLSReady False, and a later +// endpoint in the same pass with a valid secret must not flip it back to True. +func TestResolveServiceCert_BYOMissingNotHiddenByLaterValid(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + h := setupTestHelper(&k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "valid-cert", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + }) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + } + newED := func(name string) *EndpointDetail { + return &EndpointDetail{ + Name: name, + Namespace: "test-namespace", + Service: ServiceDetails{TLS: ServiceTLSDetails{CertName: name + "-svc"}}, + } + } + + res, err := newED("glance-a-public").resolveServiceCert(ctx, instance, h, certmanager.CertificateRequest{}, ptr.To("missing-cert")) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(10 * time.Second)) + + _, err = newED("glance-b-public").resolveServiceCert(ctx, instance, h, certmanager.CertificateRequest{}, ptr.To("valid-cert")) + g.Expect(err).ToNot(HaveOccurred()) + + cond := instance.Status.Conditions.Get(corev1.OpenStackControlPlaneCustomTLSReadyCondition) + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status).To(Equal(k8s_corev1.ConditionFalse)) +} + +// TestEnsureEndpointConfig_RouteToLoadBalancer_KeepsEndpointURL tests that moving an +// endpoint from a Route to a LoadBalancer (ingress-create "false") deletes the owned +// Route but keeps the user's endpointURL, instead of clearing it for one reconcile. +func TestEnsureEndpointConfig_RouteToLoadBalancer_KeepsEndpointURL(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Labels: map[string]string{ooAppSelector: "keystone"}, + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "false", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeLoadBalancer, + Ports: []k8s_corev1.ServicePort{{Name: "public", Port: 5000}}, + }, + } + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace", UID: "cp-uid"}, + } + ownedRoute := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Labels: map[string]string{ooAppSelector: "keystone"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "core.openstack.org/v1beta1", Kind: "OpenStackControlPlane", + Name: "test-controlplane", UID: "cp-uid", Controller: ptr.To(true), + }}, + }, + Spec: routev1.RouteSpec{To: routev1.RouteTargetReference{Name: "keystone-public"}}, + } + + h := setupTestHelper(publicSvc, ownedRoute) + svcOverrides := map[service.Endpoint]service.RoutedOverrideSpec{ + service.EndpointPublic: {EndpointURL: ptr.To("https://keystone.example.com")}, + } + + endpoints, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}}, + svcOverrides, corev1.Override{}, "TestCondition", false, tls.API{}, + ) + g.Expect(err).ToNot(HaveOccurred()) + + err = h.GetClient().Get(ctx, client.ObjectKeyFromObject(ownedRoute), &routev1.Route{}) + g.Expect(k8s_errors.IsNotFound(err)).To(BeTrue(), "owned route must be deleted") + + ed := endpoints.EndpointDetails[service.EndpointPublic] + g.Expect(ed.Service.OverrideSpec.EndpointURL).To(Equal(ptr.To("https://keystone.example.com"))) +} + +// TestEnsureEndpointConfig_RouteBackedPublic_IgnoresBYOServiceCert tests that a +// Route-backed public endpoint keeps the operator-issued pod cert even when +// tls.api.public.secretName is set: a custom cert for a Route goes on the Route +// (apiOverride.tls.secretName). +func TestEnsureEndpointConfig_RouteBackedPublic_IgnoresBYOServiceCert(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + publicSvc := &k8s_corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "keystone-public", + Namespace: "test-namespace", + Annotations: map[string]string{ + service.AnnotationEndpointKey: "public", + service.AnnotationIngressCreateKey: "true", + }, + }, + Spec: k8s_corev1.ServiceSpec{ + Type: k8s_corev1.ServiceTypeClusterIP, + Ports: []k8s_corev1.ServicePort{{Name: "public", Port: 5000}}, + }, + } + byoSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "byo-public-cert", Namespace: "test-namespace"}, + Data: map[string][]byte{ + "tls.crt": []byte("cert-data"), + "tls.key": []byte("key-data"), + "ca.crt": []byte("ca-data"), + }, + } + issuedCert := &certmgrv1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "keystone-public-svc", Namespace: "test-namespace"}, + Status: certmgrv1.CertificateStatus{Conditions: []certmgrv1.CertificateCondition{ + {Type: certmgrv1.CertificateConditionReady, Status: certmgrmetav1.ConditionTrue}, + }}, + } + + issuedSecret := &k8s_corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "cert-keystone-public-svc", Namespace: "test-namespace"}, + Data: byoSecret.Data, + } + + h := setupTestHelper(publicSvc, byoSecret, issuedCert, issuedSecret, issuerFor("rootca-public")) + instance := &corev1.OpenStackControlPlane{ + ObjectMeta: metav1.ObjectMeta{Name: "test-controlplane", Namespace: "test-namespace"}, + Spec: corev1.OpenStackControlPlaneSpec{ + TLS: corev1.TLSSection{ + Ingress: corev1.TLSIngressConfig{Enabled: true}, + PodLevel: corev1.TLSPodLevelConfig{Enabled: true}, + }, + }, + } + tlsConfig := tls.API{} + tlsConfig.API.Public.SecretName = ptr.To("byo-public-cert") + + // the call may requeue on the (not yet ready) route cert; the service cert is + // resolved before that, which is what is asserted below + _, _, err := EnsureEndpointConfig( + ctx, instance, h, instance, &k8s_corev1.ServiceList{Items: []k8s_corev1.Service{*publicSvc}}, + map[service.Endpoint]service.RoutedOverrideSpec{}, corev1.Override{}, + "TestCondition", false, tlsConfig, + ) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(h.GetClient().Get(ctx, client.ObjectKeyFromObject(issuedCert), &certmgrv1.Certificate{})).To(Succeed(), + "operator-issued service cert must be kept") + g.Expect(instance.Status.Conditions.Get(corev1.OpenStackControlPlaneCustomTLSReadyCondition)).To(BeNil(), + "an ignored BYO secret must not set CustomTLSReady") +} diff --git a/internal/openstack/designate.go b/internal/openstack/designate.go index b6e4269af1..3dfc93652c 100644 --- a/internal/openstack/designate.go +++ b/internal/openstack/designate.go @@ -66,6 +66,8 @@ func ReconcileDesignate(ctx context.Context, instance *corev1beta1.OpenStackCont designate.Name) } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + designateUserTLS := instance.Spec.Designate.Template.DesignateAPI.TLS // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Designate.Template.DesignateAPI.TLS = designate.Spec.DesignateAPI.TLS @@ -142,7 +144,7 @@ func ReconcileDesignate(ctx context.Context, instance *corev1beta1.OpenStackCont instance.Spec.Designate.APIOverride, corev1beta1.OpenStackControlPlaneExposeDesignateReadyCondition, false, // TODO: (oschwart) could be removed when all integrated service support TLS - instance.Spec.Designate.Template.DesignateAPI.TLS, + designateUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/glance.go b/internal/openstack/glance.go index 34207515d1..d8b5f7afa6 100644 --- a/internal/openstack/glance.go +++ b/internal/openstack/glance.go @@ -10,6 +10,7 @@ import ( "github.com/openstack-k8s-operators/lib-common/modules/common/condition" "github.com/openstack-k8s-operators/lib-common/modules/common/helper" "github.com/openstack-k8s-operators/lib-common/modules/common/service" + "github.com/openstack-k8s-operators/lib-common/modules/common/tls" "github.com/openstack-k8s-operators/lib-common/modules/common/util" "github.com/openstack-k8s-operators/lib-common/modules/common/webhook" corev1beta1 "github.com/openstack-k8s-operators/openstack-operator/api/core/v1beta1" @@ -165,6 +166,10 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl } } + // snapshot pre-restore BYO service cert secrets per glanceAPI (pure user input, + // captured before the restore below overwrites the map entry; see resolveServiceCert) + glanceUserTLS := map[string]tls.API{} + // add selector to service overrides for name, glanceAPI := range instance.Spec.Glance.Template.GlanceAPIs { eps := []service.Endpoint{service.EndpointPublic, service.EndpointInternal} @@ -184,6 +189,7 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl glanceAPI.Override.Service[endpointType] = svcOverride } + glanceUserTLS[name] = glanceAPI.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { glanceAPI.TLS.API = glance.Spec.GlanceAPIs[name].TLS.API @@ -193,6 +199,7 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl } var changed = false + var pending ctrl.Result for name, glanceAPI := range instance.Spec.Glance.Template.GlanceAPIs { // Retrieve the services by Label and filter on glanceAPI: for // each instance we should get **only** the associated `SVCs` @@ -225,7 +232,7 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Glance.APIOverride[name], corev1beta1.OpenStackControlPlaneExposeGlanceReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - glanceAPI.TLS, + glanceUserTLS[name], ) if err != nil { return ctrlResult, err @@ -243,12 +250,14 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl // only when the iteration on the whole APIList is over if (ctrlResult != ctrl.Result{}) { changed = true + pending = ctrlResult } } instance.Spec.Glance.Template.GlanceAPIs[name] = glanceAPI } if changed { - return ctrl.Result{}, nil + // keep the requeue of the instance that asked for it + return pending, nil } Log.Info("Reconciling Glance", "Glance.Namespace", instance.Namespace, "Glance.Name", glanceName) diff --git a/internal/openstack/heat.go b/internal/openstack/heat.go index 24168ea164..02bbdbac19 100644 --- a/internal/openstack/heat.go +++ b/internal/openstack/heat.go @@ -107,6 +107,10 @@ func ReconcileHeat(ctx context.Context, instance *corev1beta1.OpenStackControlPl } } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + heatAPIUserTLS := instance.Spec.Heat.Template.HeatAPI.TLS + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + heatCfnUserTLS := instance.Spec.Heat.Template.HeatCfnAPI.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Heat.Template.HeatAPI.TLS = heat.Spec.HeatAPI.TLS @@ -173,7 +177,7 @@ func ReconcileHeat(ctx context.Context, instance *corev1beta1.OpenStackControlPl instance.Spec.Heat.APIOverride, corev1beta1.OpenStackControlPlaneExposeHeatReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Heat.Template.HeatAPI.TLS, + heatAPIUserTLS, ) if err != nil { return ctrlResult, err @@ -209,7 +213,7 @@ func ReconcileHeat(ctx context.Context, instance *corev1beta1.OpenStackControlPl instance.Spec.Heat.CnfAPIOverride, corev1beta1.OpenStackControlPlaneExposeHeatReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Heat.Template.HeatCfnAPI.TLS, + heatCfnUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/horizon.go b/internal/openstack/horizon.go index a0571dad1b..7cee171315 100644 --- a/internal/openstack/horizon.go +++ b/internal/openstack/horizon.go @@ -80,6 +80,8 @@ func ReconcileHorizon(ctx context.Context, instance *corev1beta1.OpenStackContro } } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + horizonUserTLS := instance.Spec.Horizon.Template.TLS // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Horizon.Template.TLS = horizon.Spec.TLS @@ -131,7 +133,7 @@ func ReconcileHorizon(ctx context.Context, instance *corev1beta1.OpenStackContro tls.API{ API: tls.APIService{ Public: tls.GenericService{ - SecretName: instance.Spec.Horizon.Template.TLS.SecretName, + SecretName: horizonUserTLS.SecretName, }, }, }, diff --git a/internal/openstack/ironic.go b/internal/openstack/ironic.go index 7b43fd7e6b..6ef2af327c 100644 --- a/internal/openstack/ironic.go +++ b/internal/openstack/ironic.go @@ -112,6 +112,10 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl } } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + ironicAPIUserTLS := instance.Spec.Ironic.Template.IronicAPI.TLS + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + ironicInspectorUserTLS := instance.Spec.Ironic.Template.IronicInspector.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Ironic.Template.IronicAPI.TLS = ironic.Spec.IronicAPI.TLS @@ -214,7 +218,7 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Ironic.APIOverride, corev1beta1.OpenStackControlPlaneExposeIronicReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Ironic.Template.IronicAPI.TLS, + ironicAPIUserTLS, ) if err != nil { return ctrlResult, err @@ -251,7 +255,7 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Ironic.InspectorOverride, corev1beta1.OpenStackControlPlaneExposeIronicReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Ironic.Template.IronicInspector.TLS, + ironicInspectorUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/keystone.go b/internal/openstack/keystone.go index a7a7d8a774..025332760e 100644 --- a/internal/openstack/keystone.go +++ b/internal/openstack/keystone.go @@ -64,6 +64,8 @@ func ReconcileKeystoneAPI(ctx context.Context, instance *corev1beta1.OpenStackCo } } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + keystoneUserTLS := instance.Spec.Keystone.Template.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Keystone.Template.TLS = keystoneAPI.Spec.TLS @@ -92,7 +94,7 @@ func ReconcileKeystoneAPI(ctx context.Context, instance *corev1beta1.OpenStackCo instance.Spec.Keystone.APIOverride, corev1beta1.OpenStackControlPlaneExposeKeystoneAPIReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Keystone.Template.TLS, + keystoneUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/manila.go b/internal/openstack/manila.go index 956e68481a..2c6c713ddf 100644 --- a/internal/openstack/manila.go +++ b/internal/openstack/manila.go @@ -110,6 +110,8 @@ func ReconcileManila(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Manila.Template.Auth.ApplicationCredentialSecret = acSecretName } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + manilaUserTLS := instance.Spec.Manila.Template.ManilaAPI.TLS // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Manila.Template.ManilaAPI.TLS = manila.Spec.ManilaAPI.TLS @@ -139,7 +141,7 @@ func ReconcileManila(ctx context.Context, instance *corev1beta1.OpenStackControl instance.Spec.Manila.APIOverride, corev1beta1.OpenStackControlPlaneExposeManilaReadyCondition, false, // TODO: (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Manila.Template.ManilaAPI.TLS, + manilaUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/neutron.go b/internal/openstack/neutron.go index 251a4d5195..6b8d96f2e1 100644 --- a/internal/openstack/neutron.go +++ b/internal/openstack/neutron.go @@ -71,6 +71,8 @@ func ReconcileNeutron(ctx context.Context, instance *corev1beta1.OpenStackContro } } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + neutronUserTLS := instance.Spec.Neutron.Template.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Neutron.Template.TLS = neutronAPI.Spec.TLS @@ -176,7 +178,7 @@ func ReconcileNeutron(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Neutron.APIOverride, corev1beta1.OpenStackControlPlaneExposeNeutronReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - tls.API{API: instance.Spec.Neutron.Template.TLS.API}, + tls.API{API: neutronUserTLS.API}, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/nova.go b/internal/openstack/nova.go index c6c6b00624..97faa453a1 100644 --- a/internal/openstack/nova.go +++ b/internal/openstack/nova.go @@ -135,6 +135,8 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl instance.Spec.Nova.Template.APIServiceTemplate.Override.Service[endpointType], nova.Name+"-api") } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + novaAPIUserTLS := instance.Spec.Nova.Template.APIServiceTemplate.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Nova.Template.APIServiceTemplate.TLS = nova.Spec.APIServiceTemplate.TLS @@ -154,6 +156,10 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl } instance.Spec.Nova.Template.MetadataServiceTemplate.TLS.CaBundleSecretName = instance.Status.TLS.CaBundleSecretName } + // snapshot pre-restore BYO novncproxy cert secret per cell (pure user input, captured + // before the restore below overwrites it; used in the later cell loop; see resolveServiceCert) + novncUserSecret := map[string]*string{} + // Cells for cellName, cellTemplate := range instance.Spec.Nova.Template.CellTemplates { // add override where novncproxy enabled is not specified or explicitely set to true @@ -163,6 +169,7 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl } cellTemplate.NoVNCProxyServiceTemplate.Override.Service.AddLabel(getNoVNCProxyLabelMap(nova.Name, cellName)) + novncUserSecret[cellName] = cellTemplate.NoVNCProxyServiceTemplate.TLS.Service.SecretName // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { cellTemplate.NoVNCProxyServiceTemplate.TLS = nova.Spec.CellTemplates[cellName].NoVNCProxyServiceTemplate.TLS @@ -249,7 +256,7 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl instance.Spec.Nova.APIOverride, corev1beta1.OpenStackControlPlaneExposeNovaReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Nova.Template.APIServiceTemplate.TLS, + novaAPIUserTLS, ) if err != nil { return ctrlResult, err @@ -340,7 +347,7 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl tls.API{ API: tls.APIService{ Public: tls.GenericService{ - SecretName: cellTemplate.NoVNCProxyServiceTemplate.TLS.Service.SecretName, + SecretName: novncUserSecret[cellName], }, }, }, diff --git a/internal/openstack/octavia.go b/internal/openstack/octavia.go index ed5665f776..8708afec56 100644 --- a/internal/openstack/octavia.go +++ b/internal/openstack/octavia.go @@ -112,6 +112,10 @@ func ReconcileOctavia(ctx context.Context, instance *corev1beta1.OpenStackContro octavia.Name) } + // snapshot the user's TLS before the restore below: it is the bring-your-own + // service cert signal for EnsureEndpointConfig + octaviaUserTLS := tls.API{API: instance.Spec.Octavia.Template.OctaviaAPI.TLS.API} + // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { clusterDomain := clusterdns.GetDNSClusterDomain() @@ -224,7 +228,7 @@ func ReconcileOctavia(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Octavia.APIOverride, corev1beta1.OpenStackControlPlaneExposeOctaviaReadyCondition, false, // TODO: (mschuppert) could be removed when all integrated service support TLS - tls.API{}, + octaviaUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/placement.go b/internal/openstack/placement.go index 0dd842a4db..21cdef0435 100644 --- a/internal/openstack/placement.go +++ b/internal/openstack/placement.go @@ -114,6 +114,8 @@ func ReconcilePlacementAPI(ctx context.Context, instance *corev1beta1.OpenStackC instance.Spec.Placement.Template.Auth.ApplicationCredentialSecret = acSecretName } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + placementUserTLS := instance.Spec.Placement.Template.TLS // set CA cert and preserve any previously set TLS certs if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Placement.Template.TLS = placementAPI.Spec.TLS @@ -142,7 +144,7 @@ func ReconcilePlacementAPI(ctx context.Context, instance *corev1beta1.OpenStackC instance.Spec.Placement.APIOverride, corev1beta1.OpenStackControlPlaneExposePlacementAPIReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Placement.Template.TLS, + placementUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/swift.go b/internal/openstack/swift.go index 92ed4082f5..0f2b2fde6a 100644 --- a/internal/openstack/swift.go +++ b/internal/openstack/swift.go @@ -144,6 +144,8 @@ func ReconcileSwift(ctx context.Context, instance *corev1beta1.OpenStackControlP instance.Spec.Swift.Template.SwiftProxy.Auth.ApplicationCredentialSecret = acSecretName } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + swiftUserTLS := instance.Spec.Swift.Template.SwiftProxy.TLS // preserve any previously set TLS certs,set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Swift.Template.SwiftProxy.TLS = swift.Spec.SwiftProxy.TLS @@ -172,7 +174,7 @@ func ReconcileSwift(ctx context.Context, instance *corev1beta1.OpenStackControlP instance.Spec.Swift.ProxyOverride, corev1beta1.OpenStackControlPlaneExposeSwiftReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Swift.Template.SwiftProxy.TLS, + swiftUserTLS, ) if err != nil { return ctrlResult, err diff --git a/internal/openstack/telemetry.go b/internal/openstack/telemetry.go index 9033d0418c..b641732a7c 100644 --- a/internal/openstack/telemetry.go +++ b/internal/openstack/telemetry.go @@ -272,6 +272,11 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont instance.Spec.Telemetry.Template.CloudKitty.Auth.ApplicationCredentialSecret = "" } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + aodhUserTLS := instance.Spec.Telemetry.Template.Autoscaling.Aodh.TLS + cloudKittyUserTLS := instance.Spec.Telemetry.Template.CloudKitty.CloudKittyAPI.TLS + prometheusUserTLS := instance.Spec.Telemetry.Template.MetricStorage.PrometheusTLS + alertmanagerUserTLS := instance.Spec.Telemetry.Template.MetricStorage.AlertmanagerTLS // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Telemetry.Template.Autoscaling.Aodh.TLS = telemetry.Spec.Autoscaling.Aodh.TLS @@ -378,7 +383,7 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont instance.Spec.Telemetry.CloudKittyAPIOverride, corev1beta1.OpenStackControlPlaneExposeTelemetryReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Telemetry.Template.CloudKitty.CloudKittyAPI.TLS, + cloudKittyUserTLS, ) if err != nil { return ctrlResult, err @@ -404,7 +409,7 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont instance.Spec.Telemetry.AodhAPIOverride, corev1beta1.OpenStackControlPlaneExposeTelemetryReadyCondition, false, // TODO (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Telemetry.Template.Autoscaling.Aodh.TLS, + aodhUserTLS, ) if err != nil { return ctrlResult, err @@ -434,7 +439,7 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont tls.API{ API: tls.APIService{ Public: tls.GenericService{ - SecretName: instance.Spec.Telemetry.Template.MetricStorage.PrometheusTLS.SecretName, + SecretName: prometheusUserTLS.SecretName, }, }, }, @@ -503,7 +508,7 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont tls.API{ API: tls.APIService{ Public: tls.GenericService{ - SecretName: instance.Spec.Telemetry.Template.MetricStorage.AlertmanagerTLS.SecretName, + SecretName: alertmanagerUserTLS.SecretName, }, }, }, diff --git a/internal/openstack/watcher.go b/internal/openstack/watcher.go index dace2e5898..0cb9bc96cb 100644 --- a/internal/openstack/watcher.go +++ b/internal/openstack/watcher.go @@ -123,6 +123,8 @@ func ReconcileWatcher(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Watcher.Template.Auth.ApplicationCredentialSecret = acSecretName } + // snapshot pre-restore BYO service cert secret (pure user input; see resolveServiceCert) + watcherUserTLS := instance.Spec.Watcher.Template.APIServiceTemplate.TLS // preserve any previously set TLS certs, set CA cert if instance.Spec.TLS.PodLevel.Enabled { instance.Spec.Watcher.Template.APIServiceTemplate.TLS = watcher.Spec.APIServiceTemplate.TLS @@ -151,7 +153,7 @@ func ReconcileWatcher(ctx context.Context, instance *corev1beta1.OpenStackContro instance.Spec.Watcher.APIOverride, corev1beta1.OpenStackControlPlaneExposeWatcherReadyCondition, false, // TODO: (mschuppert) could be removed when all integrated service support TLS - instance.Spec.Watcher.Template.APIServiceTemplate.TLS, + watcherUserTLS, ) if err != nil { return ctrlResult, err diff --git a/test/functional/ctlplane/openstackoperator_controller_test.go b/test/functional/ctlplane/openstackoperator_controller_test.go index 60b2b9db37..dc0bd501a7 100644 --- a/test/functional/ctlplane/openstackoperator_controller_test.go +++ b/test/functional/ctlplane/openstackoperator_controller_test.go @@ -40,6 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + certmgrv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" routev1 "github.com/openshift/api/route/v1" cinderv1 "github.com/openstack-k8s-operators/cinder-operator/api/v1beta1" rabbitmqv1 "github.com/openstack-k8s-operators/infra-operator/apis/rabbitmq/v1beta1" @@ -47,6 +48,7 @@ import ( "github.com/openstack-k8s-operators/lib-common/modules/certmanager" common_annotations "github.com/openstack-k8s-operators/lib-common/modules/common/annotations" + "github.com/openstack-k8s-operators/lib-common/modules/common/clusterdns" "github.com/openstack-k8s-operators/lib-common/modules/common/condition" "github.com/openstack-k8s-operators/lib-common/modules/common/service" "github.com/openstack-k8s-operators/lib-common/modules/common/tls" @@ -1558,6 +1560,30 @@ var _ = Describe("OpenStackOperator controller", func() { ) }) + // the condition is only set while a custom secret is configured: removing + // the (still missing) secret from the spec must not leave it False + It("should drop the OpenStackControlPlaneCustomTLSReadyCondition once the missing secret is removed from the spec", func() { + th.ExpectConditionWithDetails( + names.OpenStackControlplaneName, + ConditionGetterFunc(OpenStackControlPlaneConditionGetter), + corev1.OpenStackControlPlaneCustomTLSReadyCondition, + k8s_corev1.ConditionFalse, + condition.ErrorReason, + fmt.Sprintf(corev1.OpenStackControlPlaneCustomTLSReadyWaitingMessage, names.CustomServiceCertSecretName.Name), + ) + + Eventually(func(g Gomega) { + OSCtlplane := GetOpenStackControlPlane(names.OpenStackControlplaneName) + OSCtlplane.Spec.Keystone.APIOverride.TLS = nil + g.Expect(th.K8sClient.Update(th.Ctx, OSCtlplane)).To(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + OSCtlplane := GetOpenStackControlPlane(names.OpenStackControlplaneName) + g.Expect(OSCtlplane.Status.Conditions.Get(corev1.OpenStackControlPlaneCustomTLSReadyCondition)).To(BeNil()) + }, timeout, interval).Should(Succeed()) + }) + It("should have created keystone with route using custom cert secret", func() { DeferCleanup(k8sClient.Delete, ctx, CreateCertSecret(names.CustomServiceCertSecretName)) @@ -1655,6 +1681,178 @@ var _ = Describe("OpenStackOperator controller", func() { g.Expect(count).Should(BeNumerically(">", before)) }, 2*time.Second, interval).Should(Succeed()) }) + + // The Service watch re-triggers the legacy front port injection after the + // service operator renamed the port or changed the Service type. Same + // recording manager approach as above: after the initial events settled only + // the watch can enqueue. + It("should enqueue the control plane when a managed Service port is renamed", func() { + rec := startReconcileRecorder() + before := waitReconcileRecorderSettled(rec) + + svc := th.GetService(types.NamespacedName{Name: "keystone-public", Namespace: namespace}) + svc.Spec.Ports[0].Name = "public" + Expect(th.K8sClient.Update(th.Ctx, svc)).Should(Succeed()) + + Eventually(func(g Gomega) { + count, _ := rec.stats() + g.Expect(count).Should(BeNumerically(">", before)) + }, 2*time.Second, interval).Should(Succeed()) + }) + + It("should enqueue the control plane when a managed Service type changes", func() { + rec := startReconcileRecorder() + before := waitReconcileRecorderSettled(rec) + + svc := th.GetService(types.NamespacedName{Name: "keystone-public", Namespace: namespace}) + svc.Spec.Type = k8s_corev1.ServiceTypeLoadBalancer + Expect(th.K8sClient.Update(th.Ctx, svc)).Should(Succeed()) + + Eventually(func(g Gomega) { + count, _ := rec.stats() + g.Expect(count).Should(BeNumerically(">", before)) + }, 2*time.Second, interval).Should(Succeed()) + }) + + It("should not enqueue the control plane when an unmanaged Service port is renamed", func() { + rec := startReconcileRecorder() + before := waitReconcileRecorderSettled(rec) + + name := types.NamespacedName{Name: "not-managed", Namespace: namespace} + th.CreateService(name, nil, k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{{Name: "a", Port: int32(8080), Protocol: k8s_corev1.ProtocolTCP}}, + }) + svc := th.GetService(name) + svc.Spec.Ports[0].Name = "b" + Expect(th.K8sClient.Update(th.Ctx, svc)).Should(Succeed()) + + Consistently(func(g Gomega) { + count, _ := rec.stats() + g.Expect(count).Should(Equal(before)) + }, 3*time.Second, interval).Should(Succeed()) + }) + }) + }) + + // Regression: an operator-issued pod-level service cert must survive + // subsequent reconciles. The reconciler snapshots the user's pre-restore + // tls.api.*.secretName as the bring-your-own signal; if it instead read the + // post-restore value (which carries the operator's own issued secret name) + // it would mistake the issued cert for a user override and delete it on the + // next reconcile. See resolveServiceCert in internal/openstack/common.go. + When("A TLSe OpenStackControlplane issues its own keystone service certs", func() { + BeforeEach(func() { + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.RabbitMQCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.RabbitMQCell1CertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.RabbitMQNotificationsCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.MemcachedCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.OVNNorthdCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.OVNControllerCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.OVNMetricsCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.InstanceHAMetricsCertName)) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret(names.NeutronOVNCertName)) + + // TLSe (PodLevel enabled) with no BYO keystone service cert, so the + // operator issues the keystone-{public,internal}-svc certs itself. + DeferCleanup( + th.DeleteInstance, + CreateOpenStackControlPlane(names.OpenStackControlplaneName, GetDefaultOpenStackControlPlaneSpec()), + ) + }) + + It("keeps the operator-issued keystone service certs across reconciles", func() { + keystonePublicSvcName := types.NamespacedName{Name: "keystone-public", Namespace: namespace} + keystoneInternalSvcName := types.NamespacedName{Name: "keystone-internal", Namespace: namespace} + publicCert := types.NamespacedName{Name: "keystone-public-svc", Namespace: namespace} + internalCert := types.NamespacedName{Name: "keystone-internal-svc", Namespace: namespace} + + // Pre-create the secrets the operator-issued certs reference. cert-manager + // is not running in envtest, so without these the EnsureCert secret lookup + // never resolves. Their presence is also what makes this a real regression: + // the buggy path (reading the post-restore secretName) would validate these + // secrets, pass, and then delete the issued Certificate - so the certs only + // survive if the reconciler uses the pre-restore BYO snapshot. + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret( + types.NamespacedName{Name: "cert-" + publicCert.Name, Namespace: namespace})) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret( + types.NamespacedName{Name: "cert-" + internalCert.Name, Namespace: namespace})) + // the public route cert secret too, so ensureRoute does not requeue and + // the reconciler reaches the write-back that persists the service-cert + // secret names into keystoneAPI.Spec.TLS (what the next reconcile restores) + DeferCleanup(k8sClient.Delete, ctx, th.CreateCertSecret( + types.NamespacedName{Name: "cert-keystone-public-route", Namespace: namespace})) + + // wait for the KeystoneAPI CR before creating the k8s services, so the + // reconciler doesn't run endpoint config before the owner CR exists + Eventually(func(g Gomega) { + g.Expect(keystone.GetKeystoneAPI(names.KeystoneAPIName)).Should(Not(BeNil())) + }, timeout, interval).Should(Succeed()) + + th.CreateService( + keystonePublicSvcName, + map[string]string{"osctlplane-service": "keystone", "osctlplane": ""}, + k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{ + {Name: "keystone-public", Port: int32(5000), Protocol: k8s_corev1.ProtocolTCP}, + }, + }) + keystoneSvc := th.GetService(keystonePublicSvcName) + keystoneSvc.Annotations = map[string]string{ + service.AnnotationIngressCreateKey: "true", + service.AnnotationEndpointKey: "public", + } + Expect(th.K8sClient.Status().Update(th.Ctx, keystoneSvc)).To(Succeed()) + + th.CreateService( + keystoneInternalSvcName, + map[string]string{"osctlplane-service": "keystone", "osctlplane": ""}, + k8s_corev1.ServiceSpec{ + Ports: []k8s_corev1.ServicePort{ + {Name: "keystone-internal", Port: int32(5000), Protocol: k8s_corev1.ProtocolTCP}, + }, + }) + keystoneSvc = th.GetService(keystoneInternalSvcName) + keystoneSvc.Annotations = map[string]string{ + service.AnnotationIngressCreateKey: "false", + service.AnnotationEndpointKey: "internal", + } + Expect(th.K8sClient.Status().Update(th.Ctx, keystoneSvc)).To(Succeed()) + + keystone.SimulateKeystoneAPIReady(names.KeystoneAPIName) + + // operator issues both pod-level service certs + Eventually(func(g Gomega) { + g.Expect(crtmgr.GetCert(internalCert)).ShouldNot(BeNil()) + g.Expect(crtmgr.GetCert(publicCert)).ShouldNot(BeNil()) + }, timeout, interval).Should(Succeed()) + + // both certs carry the canonical {short, FQDN} SAN set - the same names + // GetVHostServerName hands to the vhost, so SNI and cert SAN agree + clusterDomain := clusterdns.GetDNSClusterDomain() + Expect(crtmgr.GetCert(publicCert).Spec.DNSNames).To(ConsistOf( + "keystone-public."+namespace+".svc", + "keystone-public."+namespace+".svc."+clusterDomain, + )) + Expect(crtmgr.GetCert(internalCert).Spec.DNSNames).To(ConsistOf( + "keystone-internal."+namespace+".svc", + "keystone-internal."+namespace+".svc."+clusterDomain, + )) + + // the operator persists the issued secret names into keystoneAPI.Spec.TLS, + // which the next reconcile restores - this is the value the reconciler must + // NOT mistake for a user-provided BYO cert + Eventually(func(g Gomega) { + ksAPI := keystone.GetKeystoneAPI(names.KeystoneAPIName) + g.Expect(ksAPI.Spec.TLS.API.Internal.SecretName).To(HaveValue(Equal("cert-" + internalCert.Name))) + }, timeout, interval).Should(Succeed()) + + // and they are not deleted by any later reconcile (the pre-restore + // snapshot must keep the issued certs from looking like a BYO override) + Consistently(func(g Gomega) { + cert := &certmgrv1.Certificate{} + g.Expect(k8sClient.Get(ctx, internalCert, cert)).Should(Succeed()) + g.Expect(k8sClient.Get(ctx, publicCert, cert)).Should(Succeed()) + }, "10s", interval).Should(Succeed()) }) }) diff --git a/test/functional/ctlplane/secret_watch_test.go b/test/functional/ctlplane/secret_watch_test.go index 1d5555f206..ea8929561f 100644 --- a/test/functional/ctlplane/secret_watch_test.go +++ b/test/functional/ctlplane/secret_watch_test.go @@ -80,3 +80,15 @@ func startReconcileRecorder() *reconcileRecorder { return rec } + +// waitReconcileRecorderSettled waits until the initial events of the control plane +// were recorded and went quiet, and returns the count at that point. +func waitReconcileRecorderSettled(rec *reconcileRecorder) int { + 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()) + count, _ := rec.stats() + return count +} From c0c83da5ee08e20ef147d8fb7ebdc968d02022f3 Mon Sep 17 00:00:00 2001 From: Martin Schuppert Date: Mon, 2 Nov 2026 15:08:31 +0100 Subject: [PATCH 2/2] [lb pub endpt] Reconcile when a bring-your-own service cert secret changes When a user updates the content of a secret referenced by a service template's bring-your-own pod level cert - tls.api.{public,internal} .secretName, or the simple TLS sections of horizon, novncproxy, prometheus and alertmanager - the control plane was not reconciled, so a missing or invalid secret was only noticed through the requeue. Index those secret names of all services so the existing Secret watch enqueues the control plane, like apiOverride.tls.secretName. The tls.api names are collected by walking the spec for tls.APIService, so new services are covered without a list to maintain; the simple TLS sections are read explicitly. Operator owned secrets (ovn, vencrypt, metadata) are 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. Add unit tests for the secret name collection and for findObjectsForSrc. Jira: OSPRH-39364 Jira: OSPRH-39366 Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Martin Schuppert --- .../core/openstackcontrolplane_controller.go | 82 ++++++++++++ .../openstackcontrolplane_controller_test.go | 126 ++++++++++++++++++ 2 files changed, 208 insertions(+) diff --git a/internal/controller/core/openstackcontrolplane_controller.go b/internal/controller/core/openstackcontrolplane_controller.go index e180b50c9f..5e1c958652 100644 --- a/internal/controller/core/openstackcontrolplane_controller.go +++ b/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" @@ -804,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) @@ -847,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. @@ -966,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{} diff --git a/internal/controller/core/openstackcontrolplane_controller_test.go b/internal/controller/core/openstackcontrolplane_controller_test.go index 43d6015e32..816ff88325 100644 --- a/internal/controller/core/openstackcontrolplane_controller_test.go +++ b/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) {