From d3f9d600b5b8bd3e86bfcf33889b81ab60b66a87 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Tue, 18 Aug 2026 10:27:34 +0100 Subject: [PATCH] ROX-36296: update version selector during delete --- .../pkg/central/reconciler/argo_reconciler.go | 14 ++++ .../pkg/central/reconciler/reconciler.go | 62 +++++++++++++++ .../pkg/central/reconciler/reconciler_test.go | 76 +++++++++++++++++++ fleetshard/pkg/k8s/constants.go | 5 ++ fleetshard/pkg/testutils/k8s.go | 17 +++++ 5 files changed, 174 insertions(+) diff --git a/fleetshard/pkg/central/reconciler/argo_reconciler.go b/fleetshard/pkg/central/reconciler/argo_reconciler.go index 1964eb428d..40ab2a9432 100644 --- a/fleetshard/pkg/central/reconciler/argo_reconciler.go +++ b/fleetshard/pkg/central/reconciler/argo_reconciler.go @@ -59,6 +59,20 @@ func (r *argoReconciler) ensureApplicationExists(ctx context.Context, remoteCent return nil } +// applicationExists reports whether the tenant's ArgoCD Application currently exists. Used to +// avoid recreating it (via ensureApplicationExists) after it has already been deleted by an +// earlier pass of the (multi-step, async) deletion flow. +func (r *argoReconciler) applicationExists(ctx context.Context, tenantNamespace string) (bool, error) { + err := r.client.Get(ctx, r.getArgoCdAppObjectKey(tenantNamespace), &argocd.Application{}) + if err != nil { + if apiErrors.IsNotFound(err) { + return false, nil + } + return false, fmt.Errorf("getting ArgoCD application: %w", err) + } + return true, nil +} + func (r *argoReconciler) makeDesiredArgoCDApplication(remoteCentral private.ManagedCentral, centralDBConnectionString string) (*argocd.Application, error) { values := remoteCentral.Spec.TenantResourcesValues diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index b643051ceb..4c406430a8 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -76,6 +76,11 @@ const ( centralEncryptionKeySecretName = "central-encryption-key-chain" // pragma: allowlist secret authProviderClientCredentialsSecretName = "default-auth-provider-client-credentials" // pragma: allowlist secret tenantImagePullSecretName = "stackrox" // pragma: allowlist secret + + // rolloutGroupValuesKey is the tenant-resources helm value that controls which + // rhacs-operator version reconciles a tenant's Central CR (rendered into the + // rhacs.redhat.com/version-selector label by the tenant-resources chart). + rolloutGroupValuesKey = "rolloutGroup" ) type needsReconcileFunc func(changed bool, central private.ManagedCentral, storedSecrets []string) bool @@ -293,6 +298,43 @@ func (r *CentralReconciler) reconcileInstanceDeletion(ctx context.Context, remot remoteCentralName := remoteCentral.Metadata.Name remoteCentralNamespace := remoteCentral.Metadata.Namespace + // Refresh the tenant's rolloutGroup/version-selector before tearing anything down, but only if + // the ArgoCD Application still exists: deletion is async and reconcileInstanceDeletion is + // called repeatedly until it completes, so once a previous pass has already deleted the + // Application there's nothing left to sync -- and calling ensureApplicationExists again would + // wrongly recreate it. + // + // This step matters because, unlike a normal (non-deletion) reconcile, nothing else in this + // deletion path ever pushes an updated rolloutGroup to the ArgoCD Application. If a tenant is + // deleted while its Central CR is still pinned (via a stale rhacs.redhat.com/version-selector + // label) to an operator version that has since been undeployed as part of a canary upgrade, no + // operator will ever reconcile the CR again, and its uninstall finalizer (added by the + // underlying helm-operator framework) will never be removed -- leaving the Central/namespace + // stuck in Terminating indefinitely (ROX-36296). + appExists, err := r.argoReconciler.applicationExists(ctx, remoteCentralNamespace) + if err != nil { + return nil, errors.Wrapf(err, "checking ArgoCD application for central %s/%s", remoteCentralNamespace, remoteCentralName) + } + if appExists { + // The DB connection string is intentionally left empty here: the instance is being + // deleted, so there's no live traffic to break, and computing it for real could otherwise + // trigger (re-)provisioning a managed DB for an instance that never got that far before + // being deleted. + if err := r.argoReconciler.ensureApplicationExists(ctx, remoteCentral, ""); err != nil { + return nil, errors.Wrapf(err, "syncing rollout group for central %s/%s before deletion", remoteCentralNamespace, remoteCentralName) + } + + versionSelectorSynced, err := r.versionSelectorMatchesRolloutGroup(ctx, remoteCentral) + if err != nil { + return nil, errors.Wrapf(err, "checking version selector for central %s/%s", remoteCentralNamespace, remoteCentralName) + } + if !versionSelectorSynced { + // The Central CR's version-selector label hasn't caught up with the ArgoCD sync yet. + // Retry on the next reconcile instead of proceeding with deletion. + return nil, ErrDeletionInProgress + } + } + deleted, err := r.ensureCentralDeleted(ctx, remoteCentral) if err != nil { return nil, errors.Wrapf(err, "delete central %s/%s", remoteCentralNamespace, remoteCentralName) @@ -303,6 +345,26 @@ func (r *CentralReconciler) reconcileInstanceDeletion(ctx context.Context, remot return nil, ErrDeletionInProgress } +// versionSelectorMatchesRolloutGroup reports whether the live Central CR's +// rhacs.redhat.com/version-selector label matches the currently configured rolloutGroup value. If +// the Central CR no longer exists there is nothing left to sync, so this reports true. +func (r *CentralReconciler) versionSelectorMatchesRolloutGroup(ctx context.Context, remoteCentral private.ManagedCentral) (bool, error) { + centralCRList := &unstructured.UnstructuredList{} + centralCRList.SetGroupVersionKind(k8s.CentralGVK) + + if err := r.client.List(ctx, centralCRList, &ctrlClient.ListOptions{Namespace: remoteCentral.Metadata.Namespace}); err != nil { + return false, fmt.Errorf("getting current central CR from k8s: %w", err) + } + + if len(centralCRList.Items) == 0 { + return true, nil + } + + expected := getTenantResourcesValue(remoteCentral, rolloutGroupValuesKey, "") + actual := centralCRList.Items[0].GetLabels()[k8s.VersionSelectorLabelKey] + return actual == expected, nil +} + func (r *CentralReconciler) reconcileDeclarativeConfigurationData(ctx context.Context, remoteCentral private.ManagedCentral) error { if !r.argoReconciler.isArgoDeclarativeConfigReconciliationEnabled(remoteCentral) { diff --git a/fleetshard/pkg/central/reconciler/reconciler_test.go b/fleetshard/pkg/central/reconciler/reconciler_test.go index 619ce0b7af..4558465104 100644 --- a/fleetshard/pkg/central/reconciler/reconciler_test.go +++ b/fleetshard/pkg/central/reconciler/reconciler_test.go @@ -27,6 +27,7 @@ import ( "github.com/stackrox/acs-fleet-manager/pkg/client/fleetmanager" fmMocks "github.com/stackrox/acs-fleet-manager/pkg/client/fleetmanager/mocks" centralNotifierUtils "github.com/stackrox/rox/central/notifiers/utils" + platform "github.com/stackrox/rox/operator/api/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" @@ -384,6 +385,81 @@ func TestReconcileDelete(t *testing.T) { assert.True(t, k8sErrors.IsNotFound(err)) } +// TestReconcileDeleteWaitsForStaleVersionSelector reproduces ROX-36296: a Central CR whose +// rhacs.redhat.com/version-selector label is stale (still pointing at an operator version that +// may already be undeployed as part of a canary upgrade) must not be torn down until the label +// has been refreshed to match the currently configured rolloutGroup. Otherwise no operator would +// ever pick up the CR again to remove its uninstall finalizer, orphaning it. +// +// The fake test harness (testutils.ReconcileTracker) simulates ArgoCD by synchronously re-syncing +// the Central CR's labels the moment the ArgoCD Application is created/updated, whereas in a real +// cluster that sync happens asynchronously. To exercise the "still waiting for the label to catch +// up" branch of reconcileInstanceDeletion, this test sets the Application's rolloutGroup to its +// final value up front (so ensureApplicationExists has nothing left to update) and then manually +// drives the live Central CR's label out of sync, mimicking the real-world window where ArgoCD +// hasn't reconciled the new label yet. +func TestReconcileDeleteWaitsForStaleVersionSelector(t *testing.T) { + managedCentral := simpleManagedCentral + managedCentral.Spec.TenantResourcesValues = map[string]interface{}{ + rolloutGroupValuesKey: "v2", + } + + fakeClient, _, r := getClientTrackerAndReconciler(t, nil, defaultReconcilerOptions) + + _, err := r.Reconcile(context.TODO(), managedCentral) + require.NoError(t, err) + + // Simulate the real-world window where the tenant's ArgoCD Application already targets the + // new rollout group, but the live Central CR hasn't been synced to the new + // version-selector label yet (e.g. operator v1 was undeployed before ArgoCD got around to + // re-syncing this particular tenant). + centralCR := &platform.Central{} + require.NoError(t, fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralName, Namespace: centralNamespace}, centralCR)) + centralCR.Labels[k8s.VersionSelectorLabelKey] = "v1" + require.NoError(t, fakeClient.Update(context.TODO(), centralCR)) + + deletedCentral := managedCentral + deletedCentral.Metadata.DeletionTimestamp = "2006-01-02T15:04:05Z07:00" + + status, err := r.Reconcile(context.TODO(), deletedCentral) + require.ErrorIs(t, err, ErrDeletionInProgress) + require.Nil(t, status) + + // Nothing should have been torn down while the label is still stale. + namespace := &v1.Namespace{} + assert.NoError(t, fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralNamespace}, namespace)) + assert.NoError(t, fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralArgoCDAppName, Namespace: openshiftGitopsNamespace}, &argocd.Application{})) + + // Reconciling again without anything changing must keep waiting. + status, err = r.Reconcile(context.TODO(), deletedCentral) + require.ErrorIs(t, err, ErrDeletionInProgress) + require.Nil(t, status) + + // ArgoCD finally syncs the new label onto the live Central CR. + require.NoError(t, fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralName, Namespace: centralNamespace}, centralCR)) + centralCR.Labels[k8s.VersionSelectorLabelKey] = "v2" + require.NoError(t, fakeClient.Update(context.TODO(), centralCR)) + + // Deletion can now proceed (mirroring the two-call async deletion pattern used elsewhere). + status, err = r.Reconcile(context.TODO(), deletedCentral) + require.ErrorIs(t, err, ErrDeletionInProgress) + require.Nil(t, status) + + status, err = r.Reconcile(context.TODO(), deletedCentral) + require.NoError(t, err) + require.NotNil(t, status) + + readyCondition, ok := conditionForType(status.Conditions, conditionTypeReady) + require.True(t, ok, "Ready condition not found in conditions", status.Conditions) + assert.Equal(t, "False", readyCondition.Status) + assert.Equal(t, "Deleted", readyCondition.Reason) + + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralArgoCDAppName, Namespace: openshiftGitopsNamespace}, &argocd.Application{}) + assert.True(t, k8sErrors.IsNotFound(err)) + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: centralNamespace}, &v1.Namespace{}) + assert.True(t, k8sErrors.IsNotFound(err)) +} + func TestReconcileDeleteWithManagedDB(t *testing.T) { managedDBProvisioningClient := &cloudprovider.DBClientMock{} managedDBProvisioningClient.EnsureDBProvisionedFunc = func(_ context.Context, databaseID, acsInstanceID, _ string, _ bool) error { diff --git a/fleetshard/pkg/k8s/constants.go b/fleetshard/pkg/k8s/constants.go index 497c5d883c..fe11b90510 100644 --- a/fleetshard/pkg/k8s/constants.go +++ b/fleetshard/pkg/k8s/constants.go @@ -6,4 +6,9 @@ const ( ManagedByLabelKey = "app.kubernetes.io/managed-by" // ManagedByFleetshardValue used for indication that the resource is managed by the fleetshard sync ManagedByFleetshardValue = "rhacs-fleetshard" + // VersionSelectorLabelKey is the label the rhacs-operator uses (via its CENTRAL_LABEL_SELECTOR + // setting) to decide which Central CRs it reconciles. It is stamped onto the Central CR by the + // tenant-resources Helm chart from the tenant's rolloutGroup value, allowing multiple operator + // versions to coexist during a canary upgrade. + VersionSelectorLabelKey = "rhacs.redhat.com/version-selector" ) diff --git a/fleetshard/pkg/testutils/k8s.go b/fleetshard/pkg/testutils/k8s.go index 9ca2a0bac8..c3a52073d4 100644 --- a/fleetshard/pkg/testutils/k8s.go +++ b/fleetshard/pkg/testutils/k8s.go @@ -65,6 +65,12 @@ var centralLabels = map[string]string{ "app.kubernetes.io/component": "central", } +// versionSelectorLabelKey mirrors the rhacs.redhat.com/version-selector label the real +// tenant-resources Helm chart stamps onto the Central CR from the rolloutGroup helm value +// (see fleetshard/pkg/k8s.VersionSelectorLabelKey; duplicated here as a literal to avoid an +// import cycle between fleetshard/pkg/k8s tests and this package). +const versionSelectorLabelKey = "rhacs.redhat.com/version-selector" + var ( _ k8sTesting.ObjectTracker = (*ReconcileTracker)(nil) ) @@ -236,6 +242,9 @@ func centralCrFromArgoCdApp(app *argoCDApplication) *platform.Central { ObjectMeta: metav1.ObjectMeta{ Name: app.instanceName(), Namespace: app.destinationNamespace, + Labels: map[string]string{ + versionSelectorLabelKey: app.rolloutGroup(), + }, }, } } @@ -373,6 +382,14 @@ func (a *argoCDApplication) instanceName() string { return a.helmValues["instanceName"].(string) } +func (a *argoCDApplication) rolloutGroup() string { + value, ok := a.helmValues["rolloutGroup"] + if !ok { + return "" + } + return value.(string) +} + func newArgoCDApplicationFromCustomResource(app *argoCd.Application) (*argoCDApplication, error) { helmValues := map[string]interface{}{} if err := json.Unmarshal(app.Spec.Source.Helm.ValuesObject.Raw, &helmValues); err != nil {