diff --git a/internal/controller/core/openstackcontrolplane_controller.go b/internal/controller/core/openstackcontrolplane_controller.go index 919f40ea8..1f6ed9b49 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" @@ -125,7 +127,7 @@ func (r *OpenStackControlPlaneReconciler) GetLogger(ctx context.Context) logr.Lo // +kubebuilder:rbac:groups=redis.openstack.org,resources=redises,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch;create;update;patch;delete; // +kubebuilder:rbac:groups=route.openshift.io,resources=routes/custom-host,verbs=create;update;patch -// +kubebuilder:rbac:groups=core,resources=services,verbs=get;list; +// +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete; // +kubebuilder:rbac:groups=cert-manager.io,resources=issuers,verbs=get;list;watch;create;update;patch;delete; // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete; @@ -228,6 +230,17 @@ func (r *OpenStackControlPlaneReconciler) Reconcile(ctx context.Context, req ctr instance.Status.ObservedGeneration = instance.Generation // If we're not deleting this and the service object doesn't have our finalizer, add it. + // + // This must stay the only metadata write that is followed by more spec mutation, and + // it returns before any is made. The reconcilers mutate instance.Spec in memory (issued + // cert secret names, overrides, CA bundle names), and the deferred PatchInstance builds + // its MergeFrom patch on the main resource whenever the metadata changed - that patch + // would then persist those spec changes. The persisted spec is the user input the + // reconcilers snapshot as the bring-your-own cert signal and the secret field indexes read; + // persisted operator-issued names would be mistaken for BYO secrets (see resolveServiceCert). + // The finalizer removal in reconcileDelete is safe for the same reason: no spec mutation + // has happened on that path. Do not add annotation/label/finalizer writes after the spec + // mutations without going through a separate patch. if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) || isNewInstance { return ctrl.Result{}, nil } @@ -793,6 +806,7 @@ const ( tlsPodLevelInternalCACustomIssuer = ".spec.tls.podLevel.internal.ca.customIssuer" tlsPodLevelOvnCACustomIssuer = ".spec.tls.podLevel.ovn.ca.customIssuer" apiOverrideTLSSecretField = ".spec.apiOverride.tls.secretName" + tlsServiceSecretField = ".spec.template.tls.serviceSecretName" ) // fieldIndexers maps each indexed field to the func extracting its value(s) @@ -836,6 +850,8 @@ var fieldIndexers = map[string]client.IndexerFunc{ }, // the custom public endpoint cert secrets of all services apiOverrideTLSSecretField: indexAPIOverrideTLSSecret, + // the bring-your-own pod level service cert secrets of all services + tlsServiceSecretField: indexTLSServiceSecret, } // SetupWithManager sets up the controller with the Manager. @@ -900,6 +916,13 @@ func (r *OpenStackControlPlaneReconciler) SetupWithManager( handler.EnqueueRequestsFromMapFunc(r.findControlPlaneForSrc), builder.WithPredicates(backup.AnnotationChangedPredicate(openstack.ServiceCertSelector)), ). + // the service operators rename the Service ports and set its type; the legacy + // front port is injected only after that, see EnsureEndpointConfig + Watches( + &corev1.Service{}, + handler.EnqueueRequestsFromMapFunc(r.findControlPlaneForSrc), + builder.WithPredicates(openstack.ServicePortOrTypeChangedPredicate()), + ). Complete(r) } @@ -948,6 +971,83 @@ func apiOverrideTLSSecretNames(spec *corev1beta1.OpenStackControlPlaneSpec) []st return names } +// indexTLSServiceSecret is the field index func for tlsServiceSecretField +func indexTLSServiceSecret(rawObj client.Object) []string { + return tlsServiceSecretNames(&rawObj.(*corev1beta1.OpenStackControlPlane).Spec) +} + +// tlsServiceSecretNames returns the bring-your-own pod level service cert secret +// names, i.e. the user input the operator reads in EnsureEndpointConfig: +// tls.api.{public,internal} of all services (found by walking the spec, so new +// services are covered) and the simple TLS sections of horizon, novncproxy, +// prometheus and alertmanager. Operator owned secrets (ovn, vencrypt, metadata, ...) +// are deliberately not indexed. The indexed spec comes from the informer cache, +// i.e. the persisted user input - never the operator-issued names written back in +// memory. +func tlsServiceSecretNames(spec *corev1beta1.OpenStackControlPlaneSpec) []string { + names := tlsAPISecretNames(reflect.ValueOf(spec)) + + secrets := []*string{} + if t := spec.Horizon.Template; t != nil { + secrets = append(secrets, t.TLS.SecretName) + } + if t := spec.Nova.Template; t != nil { + for _, cell := range t.CellTemplates { + secrets = append(secrets, cell.NoVNCProxyServiceTemplate.TLS.Service.SecretName) + } + } + if t := spec.Telemetry.Template; t != nil { + secrets = append(secrets, t.MetricStorage.PrometheusTLS.SecretName, t.MetricStorage.AlertmanagerTLS.SecretName) + } + for _, sn := range secrets { + if sn != nil && *sn != "" { + names = append(names, *sn) + } + } + return names +} + +var tlsAPIServiceType = reflect.TypeFor[tls.APIService]() + +// tlsAPISecretNames walks v for tls.APIService and returns the secret names of +// its public and internal endpoint. +func tlsAPISecretNames(v reflect.Value) []string { + var names []string + switch v.Kind() { + case reflect.Pointer: + if !v.IsNil() { + names = tlsAPISecretNames(v.Elem()) + } + case reflect.Struct: + if v.Type() == tlsAPIServiceType { + api := v.Interface().(tls.APIService) + for _, sn := range []*string{api.Public.SecretName, api.Internal.SecretName} { + if sn != nil && *sn != "" { + names = append(names, *sn) + } + } + break + } + for i := range v.NumField() { + if v.Type().Field(i).IsExported() { + names = append(names, tlsAPISecretNames(v.Field(i))...) + } + } + case reflect.Slice, reflect.Array: + // skip slices of scalars (e.g. []byte) + if k := v.Type().Elem().Kind(); k == reflect.Struct || k == reflect.Pointer || k == reflect.Map { + for i := range v.Len() { + names = append(names, tlsAPISecretNames(v.Index(i))...) + } + } + case reflect.Map: + for _, k := range v.MapKeys() { + names = append(names, tlsAPISecretNames(v.MapIndex(k))...) + } + } + return names +} + func (r *OpenStackControlPlaneReconciler) findObjectsForSrc(ctx context.Context, src client.Object) []reconcile.Request { requests := []reconcile.Request{} diff --git a/internal/controller/core/openstackcontrolplane_controller_test.go b/internal/controller/core/openstackcontrolplane_controller_test.go index 43d6015e3..816ff8832 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) { diff --git a/internal/openstack/barbican.go b/internal/openstack/barbican.go index 50c6a6ce7..718076e72 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 3c391878c..b4d956b3c 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 721cf38b3..11dd626a7 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,110 @@ 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, so the two case branches below can + // never observe a stale mismatched number. + // + // 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 +434,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 +463,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 +486,103 @@ 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: the webhook validates endpointURL at +// admission, so this is defense in depth only - but the missing custom SAN is logged. +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 +638,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 +711,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 21b68748f..c98443fa6 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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_RefreshesStaleName for that). +func TestEnsureEndpointConfig_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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_LegacyPortInjection_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 b6e4269af..3dfc93652 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 34207515d..d8b5f7afa 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 24168ea16..02bbdbac1 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 a0571dad1..7cee17131 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 7b43fd7e6..6ef2af327 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 a7a7d8a77..025332760 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 956e68481..2c6c713dd 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 251a4d519..6b8d96f2e 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 c6c6b0062..97faa453a 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 ed5665f77..8708afec5 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 0dd842a4d..21cdef043 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 92ed4082f..0f2b2fde6 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 9033d0418..b641732a7 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 dace2e589..0cb9bc96c 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 60b2b9db3..dc0bd501a 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 1d5555f20..ea8929561 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 +}