From deeaca0a790cad8ccebe7f215ec72b57b920428f Mon Sep 17 00:00:00 2001 From: Veronika Fisarova Date: Mon, 23 Feb 2026 14:52:04 +0100 Subject: [PATCH] AppCred fixes Fix propagation of ApplicatioNCredentialSecret into correct OCtavia Auth spec field. Fix reconcile on AC config changes (such as roles, expiry...). Fix deleting AC CRs when app creds are disabled (globally and for the service). Enhance kuttl test scenario to check the AC CR deletion. Signed-off-by: Veronika Fisarova Co-authored-by: Milana Levy --- internal/openstack/applicationcredential.go | 5 ++ internal/openstack/barbican.go | 4 +- internal/openstack/cinder.go | 4 +- internal/openstack/designate.go | 4 +- internal/openstack/glance.go | 5 +- internal/openstack/heat.go | 4 +- internal/openstack/ironic.go | 8 ++- internal/openstack/manila.go | 4 +- internal/openstack/neutron.go | 4 +- internal/openstack/nova.go | 4 +- internal/openstack/octavia.go | 8 +-- internal/openstack/placement.go | 4 +- internal/openstack/swift.go | 4 +- internal/openstack/telemetry.go | 22 +++----- internal/openstack/watcher.go | 4 +- .../03-assert-appcred-cleanup.yaml | 51 +++++++++++++++++++ .../03-disable-appcred-config.yaml | 5 ++ .../{03-cleanup.yaml => 04-cleanup.yaml} | 0 ...rs-cleanup.yaml => 04-errors-cleanup.yaml} | 0 19 files changed, 96 insertions(+), 48 deletions(-) create mode 100644 test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-assert-appcred-cleanup.yaml create mode 100644 test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-disable-appcred-config.yaml rename test/kuttl/tests/ctlplane-basic-deployment-with-appcred/{03-cleanup.yaml => 04-cleanup.yaml} (100%) rename test/kuttl/tests/ctlplane-basic-deployment-with-appcred/{03-errors-cleanup.yaml => 04-errors-cleanup.yaml} (100%) diff --git a/internal/openstack/applicationcredential.go b/internal/openstack/applicationcredential.go index 02d402ba9..87129be80 100644 --- a/internal/openstack/applicationcredential.go +++ b/internal/openstack/applicationcredential.go @@ -125,6 +125,11 @@ func EnsureApplicationCredentialForService( // Check if AC CR exists and is ready if acExists { + // We want to run reconcileApplicationCredential to update the AC CR if it exists and is ready and AC config fields changed + err = reconcileApplicationCredential(ctx, helper, instance, acName, serviceUser, secretName, passwordSelector, merged) + if err != nil { + return "", ctrl.Result{}, err + } if acCR.IsReady() { Log.Info("Application Credential is ready", "service", serviceName, "acName", acName, "secretName", acCR.Status.SecretName) return acCR.Status.SecretName, ctrl.Result{}, nil diff --git a/internal/openstack/barbican.go b/internal/openstack/barbican.go index 17aaa88b0..ab5d20b3b 100644 --- a/internal/openstack/barbican.go +++ b/internal/openstack/barbican.go @@ -73,8 +73,8 @@ func ReconcileBarbican(ctx context.Context, instance *corev1beta1.OpenStackContr barbicanSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Barbican.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Barbican.ApplicationCredential != nil || instance.Spec.Barbican.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/cinder.go b/internal/openstack/cinder.go index e68c58ce4..f514a47ad 100644 --- a/internal/openstack/cinder.go +++ b/internal/openstack/cinder.go @@ -96,8 +96,8 @@ func ReconcileCinder(ctx context.Context, instance *corev1beta1.OpenStackControl cinderSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Cinder.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Cinder.ApplicationCredential != nil || instance.Spec.Cinder.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/designate.go b/internal/openstack/designate.go index eb10935db..955bed177 100644 --- a/internal/openstack/designate.go +++ b/internal/openstack/designate.go @@ -85,8 +85,8 @@ func ReconcileDesignate(ctx context.Context, instance *corev1beta1.OpenStackCont designateSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Designate.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Designate.ApplicationCredential != nil || instance.Spec.Designate.Template.DesignateAPI.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/glance.go b/internal/openstack/glance.go index 594f9a722..39f11861b 100644 --- a/internal/openstack/glance.go +++ b/internal/openstack/glance.go @@ -128,9 +128,8 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl } } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Glance.ApplicationCredential) || hasACConfigured { - + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Glance.ApplicationCredential != nil || hasACConfigured { acSecretName, acResult, err := EnsureApplicationCredentialForService( ctx, helper, diff --git a/internal/openstack/heat.go b/internal/openstack/heat.go index 19c0cb7a1..46e1a10b1 100644 --- a/internal/openstack/heat.go +++ b/internal/openstack/heat.go @@ -120,8 +120,8 @@ func ReconcileHeat(ctx context.Context, instance *corev1beta1.OpenStackControlPl heatSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Heat.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Heat.ApplicationCredential != nil || instance.Spec.Heat.Template.Auth.ApplicationCredentialSecret != "" { heatACSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/ironic.go b/internal/openstack/ironic.go index 2f76e9e8b..c9bdc89fd 100644 --- a/internal/openstack/ironic.go +++ b/internal/openstack/ironic.go @@ -124,8 +124,8 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl ironicSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Ironic.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Ironic.ApplicationCredential != nil || instance.Spec.Ironic.Template.Auth.ApplicationCredentialSecret != "" || instance.Spec.Ironic.Template.IronicInspector.Auth.ApplicationCredentialSecret != "" { @@ -145,7 +145,6 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl return ctrl.Result{}, err } - // If AC is not ready, return immediately without updating the service CR if (acResult != ctrl.Result{}) { return acResult, nil } @@ -162,7 +161,7 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl instance, "ironic-inspector", ironicReady, - ironicSecret, // Inspector shares the same secret as ironic + ironicSecret, instance.Spec.Ironic.Template.IronicInspector.PasswordSelectors.Service, instance.Spec.Ironic.Template.IronicInspector.ServiceUser, instance.Spec.Ironic.ApplicationCredential, @@ -171,7 +170,6 @@ func ReconcileIronic(ctx context.Context, instance *corev1beta1.OpenStackControl return ctrl.Result{}, err } - // If AC is not ready, return immediately without updating the service CR if (inspectorACResult != ctrl.Result{}) { return inspectorACResult, nil } diff --git a/internal/openstack/manila.go b/internal/openstack/manila.go index 8c176b747..d461dfe03 100644 --- a/internal/openstack/manila.go +++ b/internal/openstack/manila.go @@ -75,8 +75,8 @@ func ReconcileManila(ctx context.Context, instance *corev1beta1.OpenStackControl manilaSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Manila.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Manila.ApplicationCredential != nil || instance.Spec.Manila.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/neutron.go b/internal/openstack/neutron.go index f36cca3a4..cb835e857 100644 --- a/internal/openstack/neutron.go +++ b/internal/openstack/neutron.go @@ -119,8 +119,8 @@ func ReconcileNeutron(ctx context.Context, instance *corev1beta1.OpenStackContro neutronSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Neutron.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Neutron.ApplicationCredential != nil || instance.Spec.Neutron.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/nova.go b/internal/openstack/nova.go index aa27677a8..7cffeafe1 100644 --- a/internal/openstack/nova.go +++ b/internal/openstack/nova.go @@ -191,8 +191,8 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl novaSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Nova.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Nova.ApplicationCredential != nil || instance.Spec.Nova.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/octavia.go b/internal/openstack/octavia.go index 5765b0152..c702893ef 100644 --- a/internal/openstack/octavia.go +++ b/internal/openstack/octavia.go @@ -167,9 +167,9 @@ func ReconcileOctavia(ctx context.Context, instance *corev1beta1.OpenStackContro octaviaSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Octavia.ApplicationCredential) || - instance.Spec.Octavia.Template.OctaviaAPI.Auth.ApplicationCredentialSecret != "" { + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Octavia.ApplicationCredential != nil || + instance.Spec.Octavia.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( ctx, @@ -194,7 +194,7 @@ func ReconcileOctavia(ctx context.Context, instance *corev1beta1.OpenStackContro // Set ApplicationCredentialSecret based on what the helper returned: // - If AC disabled: returns "" // - If AC enabled and ready: returns the AC secret name - instance.Spec.Octavia.Template.OctaviaAPI.Auth.ApplicationCredentialSecret = acSecretName + instance.Spec.Octavia.Template.Auth.ApplicationCredentialSecret = acSecretName } svcs, err := service.GetServicesListWithLabel( diff --git a/internal/openstack/placement.go b/internal/openstack/placement.go index 3658b1263..0c704a35c 100644 --- a/internal/openstack/placement.go +++ b/internal/openstack/placement.go @@ -79,8 +79,8 @@ func ReconcilePlacementAPI(ctx context.Context, instance *corev1beta1.OpenStackC placementSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Placement.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Placement.ApplicationCredential != nil || instance.Spec.Placement.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/swift.go b/internal/openstack/swift.go index 54c718614..33797bf0a 100644 --- a/internal/openstack/swift.go +++ b/internal/openstack/swift.go @@ -109,8 +109,8 @@ func ReconcileSwift(ctx context.Context, instance *corev1beta1.OpenStackControlP swiftSecret = instance.Spec.Secret } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Swift.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Swift.ApplicationCredential != nil || instance.Spec.Swift.Template.SwiftProxy.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/internal/openstack/telemetry.go b/internal/openstack/telemetry.go index c2009bc8c..fd1c6c61b 100644 --- a/internal/openstack/telemetry.go +++ b/internal/openstack/telemetry.go @@ -127,8 +127,8 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont // AC for Aodh (if service enabled) if instance.Spec.Telemetry.Template.Autoscaling.Enabled != nil && *instance.Spec.Telemetry.Template.Autoscaling.Enabled { - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Telemetry.ApplicationCredentialAodh) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Telemetry.ApplicationCredentialAodh != nil || instance.Spec.Telemetry.Template.Autoscaling.Aodh.Auth.ApplicationCredentialSecret != "" { // Apply same fallback logic as in CreateOrPatch to avoid passing empty values to AC @@ -162,15 +162,12 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont // - If AC enabled and ready: returns the AC secret name instance.Spec.Telemetry.Template.Autoscaling.Aodh.Auth.ApplicationCredentialSecret = aodhACSecretName } - } else { - // Aodh service disabled, clear the field - instance.Spec.Telemetry.Template.Autoscaling.Aodh.Auth.ApplicationCredentialSecret = "" } // AC for Ceilometer (if service enabled) if instance.Spec.Telemetry.Template.Ceilometer.Enabled != nil && *instance.Spec.Telemetry.Template.Ceilometer.Enabled { - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Telemetry.ApplicationCredentialCeilometer) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Telemetry.ApplicationCredentialCeilometer != nil || instance.Spec.Telemetry.Template.Ceilometer.Auth.ApplicationCredentialSecret != "" { // Apply same fallback logic as in CreateOrPatch to avoid passing empty values to AC @@ -204,15 +201,11 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont // - If AC enabled and ready: returns the AC secret name instance.Spec.Telemetry.Template.Ceilometer.Auth.ApplicationCredentialSecret = ceilometerACSecretName } - } else { - // Ceilometer service disabled, clear the field - instance.Spec.Telemetry.Template.Ceilometer.Auth.ApplicationCredentialSecret = "" } - // AC for CloudKitty (if service enabled) if instance.Spec.Telemetry.Template.CloudKitty.Enabled != nil && *instance.Spec.Telemetry.Template.CloudKitty.Enabled { - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Telemetry.ApplicationCredentialCloudKitty) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Telemetry.ApplicationCredentialCloudKitty != nil || instance.Spec.Telemetry.Template.CloudKitty.Auth.ApplicationCredentialSecret != "" { // Apply same fallback logic as in CreateOrPatch to avoid passing empty values to AC @@ -246,9 +239,6 @@ func ReconcileTelemetry(ctx context.Context, instance *corev1beta1.OpenStackCont // - If AC enabled and ready: returns the AC secret name instance.Spec.Telemetry.Template.CloudKitty.Auth.ApplicationCredentialSecret = cloudkittyACSecretName } - } else { - // CloudKitty service disabled, clear the field - instance.Spec.Telemetry.Template.CloudKitty.Auth.ApplicationCredentialSecret = "" } // preserve any previously set TLS certs, set CA cert diff --git a/internal/openstack/watcher.go b/internal/openstack/watcher.go index daf716232..d8295ee68 100644 --- a/internal/openstack/watcher.go +++ b/internal/openstack/watcher.go @@ -88,8 +88,8 @@ func ReconcileWatcher(ctx context.Context, instance *corev1beta1.OpenStackContro return "" } - // Only call if AC enabled or currently configured - if isACEnabled(instance.Spec.ApplicationCredential, instance.Spec.Watcher.ApplicationCredential) || + // Reconcile AC if configured (enabled or disabled) or secret previously set + if instance.Spec.Watcher.ApplicationCredential != nil || instance.Spec.Watcher.Template.Auth.ApplicationCredentialSecret != "" { acSecretName, acResult, err := EnsureApplicationCredentialForService( diff --git a/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-assert-appcred-cleanup.yaml b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-assert-appcred-cleanup.yaml new file mode 100644 index 000000000..0027dc0fa --- /dev/null +++ b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-assert-appcred-cleanup.yaml @@ -0,0 +1,51 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +commands: + - script: |- + set -euo pipefail + NS="${NAMESPACE}" + + SERVICES=(barbican cinder glance swift neutron placement nova ceilometer) + + wait_deleted() { + local kind=$1 name=$2 timeout=${3:-180} + echo "Waiting for $kind/$name to be deleted..." + local end=$((SECONDS + timeout)) + while [ $SECONDS -lt $end ]; do + if ! oc get "$kind" "$name" -n "$NS" &>/dev/null; then + echo "✓ $kind/$name deleted" + return 0 + fi + sleep 5 + done + echo "ERROR: $kind/$name still exists after ${timeout}s" + exit 1 + } + + echo "=========================================" + echo "Testing Application Credential Cleanup" + echo "=========================================" + echo + + echo "=== Verifying global ApplicationCredential is disabled ===" + global_enabled=$(oc get openstackcontrolplane openstack -n "$NS" -o jsonpath='{.spec.applicationCredential.enabled}') + if [ "$global_enabled" != "false" ]; then + echo "ERROR: OpenStackControlPlane.spec.applicationCredential.enabled expected 'false', got '$global_enabled'" + exit 1 + fi + echo "✓ OpenStackControlPlane.spec.applicationCredential.enabled = false" + echo + + echo "=== Verifying AC CRs are deleted ===" + for svc in "${SERVICES[@]}"; do + wait_deleted appcred "ac-$svc" 180 + done + echo + + echo "=== Verifying AC Secrets are cleaned up ===" + for svc in "${SERVICES[@]}"; do + wait_deleted secret "ac-$svc-secret" 120 + done + echo + + echo "All ApplicationCredential CRs and Secrets cleaned up successfully" diff --git a/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-disable-appcred-config.yaml b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-disable-appcred-config.yaml new file mode 100644 index 000000000..f468cb921 --- /dev/null +++ b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-disable-appcred-config.yaml @@ -0,0 +1,5 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: | + oc patch openstackcontrolplane openstack -n $NAMESPACE --type merge -p '{"spec":{"applicationCredential":{"enabled":false}}}' diff --git a/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-cleanup.yaml b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/04-cleanup.yaml similarity index 100% rename from test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-cleanup.yaml rename to test/kuttl/tests/ctlplane-basic-deployment-with-appcred/04-cleanup.yaml diff --git a/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-errors-cleanup.yaml b/test/kuttl/tests/ctlplane-basic-deployment-with-appcred/04-errors-cleanup.yaml similarity index 100% rename from test/kuttl/tests/ctlplane-basic-deployment-with-appcred/03-errors-cleanup.yaml rename to test/kuttl/tests/ctlplane-basic-deployment-with-appcred/04-errors-cleanup.yaml