From d8557ae1d37625880d0ec409b58717bff43cc7b4 Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Thu, 13 Aug 2026 18:49:54 +0200 Subject: [PATCH 1/2] OCPBUGS-35210: prevent KCM from deleting SA-token secrets created before their SA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SA-token Secrets included in an operator bundle are placed earlier in the InstallPlan step list than the synthesized ServiceAccount step. OLM creates the Secret before the SA exists; the Kubernetes token controller (KCM) immediately deletes any token secret whose referenced ServiceAccount is absent, and OLM then marks the step as Created permanently — preventing any future retry and leaving the operator without its token secret. The issue is not always reproduceable (see test document) because a rescheduled loop run is able to get the stale RV, which still does not see the Secret as created, but the prior loop created the SA so this time the Secret will not be deleted. Fix: add a StepperFunc for BundleSecretKind (mirroring the existing CRD StepperFunc pattern). The new NewBundleSecretStep checks whether the SA referenced by the secret already exists before attempting creation: - SA absent → return WaitingForAPI; NeedsRequeue() returns true, keeping phase=Installing and triggering a 5-second requeue. - SA present → create the secret with correct owner refs (live API UID lookup, matching getUpdatedOwnerReferences behaviour) and return Created/Present. Because the StepperFunc handles WaitingForAPI internally it never reaches the main ExecutePlan switch case that would otherwise skip the step, so no changes to the switch statement or to NeedsRequeue() are required. Uses the attenuated (OperatorGroup-scoped) client for SA check and Secret creation, matching existing EnsureBundleSecret behaviour. Uses a live olmClient API call for CSV UID lookup in owner references — the informer lister can return empty UIDs due to cache timing. Also adds structured debug logging to syncInstallPlans (plan resourceVersion, per-step BS/SA status at reconcile start, UpdateStatus call/result) to make the race observable in OLM pod logs during investigation. Most likely the additional logging will be removed by further force pushed commits. Tested: 30-iteration statistical reproducer against OCP 4.17 — 0/30 bug fires with the fix (without the fix we are looking at roughly 60% failure/40% Success). This PR is still a draft and work in progress. --- cmd/catalog/start.go | 10 ++ pkg/controller/operators/catalog/operator.go | 131 ++++++++++++----- pkg/controller/operators/catalog/step.go | 104 +++++++++++++- .../operators/catalog/step_ensurer.go | 29 ++-- pkg/controller/operators/catalog/step_test.go | 135 ++++++++++++++++++ pkg/controller/registry/resolver/steps.go | 18 ++- 6 files changed, 367 insertions(+), 60 deletions(-) diff --git a/cmd/catalog/start.go b/cmd/catalog/start.go index 7161131880..21202891eb 100644 --- a/cmd/catalog/start.go +++ b/cmd/catalog/start.go @@ -49,6 +49,16 @@ func newRootCmd() *cobra.Command { if o.debug { logger.SetLevel(logrus.DebugLevel) } + // OCPBUGS-35210: use millisecond timestamps so OLM and audit log + // entries can be correlated at sub-second precision. + // Set on both the local logger AND the global package logger so that + // code using logrus.WithFields() directly also emits milliseconds. + msFormatter := &logrus.TextFormatter{ + TimestampFormat: "2006-01-02T15:04:05.000Z07:00", + FullTimestamp: true, + } + logger.SetFormatter(msFormatter) + logrus.SetFormatter(msFormatter) logger.Infof("log level %s", logger.Level) ctx, cancel := context.WithCancel(signals.Context()) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index a8e3677446..7be00cb5fd 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -2100,6 +2100,23 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) { logger.Info("syncing") + // OCPBUGS-35210: log the step statuses this reconcile sees at start. + // Proves whether this loop received a stale cached plan (NotPresent) or + // the post-UpdateStatus version (Created) for the BundleSecret step. + if len(plan.Status.Plan) > 0 { + for i, step := range plan.Status.Plan { + if step.Resource.Kind == "BundleSecret" || step.Resource.Kind == "ServiceAccount" { + logger.WithFields(logrus.Fields{ + "resourceVersion": plan.ResourceVersion, + "stepIndex": i, + "kind": step.Resource.Kind, + "name": step.Resource.Name, + "status": step.Status, + }).Debug("installplan step status at reconcile start") + } + } + } + if len(plan.Status.Plan) == 0 && len(plan.Status.BundleLookups) == 0 { logger.Info("skip processing installplan without status - subscription sync responsible for initial status") return @@ -2107,6 +2124,9 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) { // Complete and Failed are terminal phases if plan.Status.Phase == v1alpha1.InstallPlanPhaseFailed || plan.Status.Phase == v1alpha1.InstallPlanPhaseComplete { + // OCPBUGS-35210: log so we can confirm terminal-phase early exit in the timeline. + // Loops that see phase=Complete exit here without executing any steps. + logger.WithField("phase", plan.Status.Phase).Debug("phase is terminal, skipping execution") return } @@ -2169,8 +2189,26 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) { defer o.requeueSubscriptionForInstallPlan(plan, logger) + // OCPBUGS-35210: log what step statuses are being persisted and the + // resourceVersion. A concurrent reconcile that reads before this write + // will have an older resourceVersion and see different step statuses. + { + fields := logrus.Fields{ + "resourceVersion": outInstallPlan.ResourceVersion, + "phase": outInstallPlan.Status.Phase, + } + for i, step := range outInstallPlan.Status.Plan { + if step.Resource.Kind == "BundleSecret" || step.Resource.Kind == "ServiceAccount" { + fields[fmt.Sprintf("step[%d].%s", i, step.Resource.Kind)] = string(step.Status) + } + } + logger.WithFields(fields).Debug("calling UpdateStatus") + } + // Update InstallPlan with status of transition. Log errors if we can't write them to the status. - if _, err := o.client.OperatorsV1alpha1().InstallPlans(plan.GetNamespace()).UpdateStatus(context.TODO(), outInstallPlan, metav1.UpdateOptions{}); err != nil { + if updatedPlan, err := o.client.OperatorsV1alpha1().InstallPlans(plan.GetNamespace()).UpdateStatus(context.TODO(), outInstallPlan, metav1.UpdateOptions{}); err != nil { + // OCPBUGS-35210: a 409 here means step statuses were NOT persisted. + // A concurrent reconcile that already read NotPresent will re-execute the BundleSecret step. logger = logger.WithField("updateError", err.Error()) updateErr := errors.New("error updating InstallPlan status: " + err.Error()) if syncError == nil { @@ -2179,6 +2217,13 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) { } logger.Info("error transitioning InstallPlan") syncError = fmt.Errorf("error transitioning InstallPlan: %s and error updating InstallPlan status: %s", syncError, updateErr) + } else { + // OCPBUGS-35210: log the new resourceVersion after a successful write. + // Any reconcile loop that read a lower resourceVersion saw stale data. + logger.WithFields(logrus.Fields{ + "newResourceVersion": updatedPlan.ResourceVersion, + "phase": updatedPlan.Status.Phase, + }).Debug("UpdateStatus succeeded") } return @@ -2474,9 +2519,10 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { o.logger.Errorf("failed to get a client for plan execution- %v", err) return err } - b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, builderDynamicClient, r, o.logger, o.recorder) + b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, kubeclient, o.client, builderDynamicClient, r, o.logger, o.recorder) for i, step := range plan.Status.Plan { + beforeStatus := plan.Status.Plan[i].Status if err := func(i int, step *v1alpha1.Step) error { wr.PopWarnings() defer func() { @@ -2521,14 +2567,25 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { } switch step.Status { - case v1alpha1.StepStatusPresent, v1alpha1.StepStatusCreated, v1alpha1.StepStatusWaitingForAPI: + case v1alpha1.StepStatusPresent, v1alpha1.StepStatusCreated: + // OCPBUGS-35210: log skipped steps so we can confirm which reconcile + // loop sees Created (and skips) vs NotPresent (and re-executes). + if step.Resource.Kind == "BundleSecret" || step.Resource.Kind == "ServiceAccount" { + o.logger.WithFields(logrus.Fields{ + "kind": step.Resource.Kind, + "name": step.Resource.Name, + "status": step.Status, + }).Debug("skipping step — already Created/Present") + } + return nil + case v1alpha1.StepStatusWaitingForAPI: return nil case v1alpha1.StepStatusUnknown, v1alpha1.StepStatusNotPresent: manifest, err := r.ManifestForStep(step) if err != nil { return err } - o.logger.WithFields(logrus.Fields{"kind": step.Resource.Kind, "name": step.Resource.Name}).Debug("execute resource") + o.logger.WithFields(logrus.Fields{"kind": step.Resource.Kind, "name": step.Resource.Name, "stepIndex": i}).Debug("execute resource") switch step.Resource.Kind { case v1alpha1.ClusterServiceVersionKind: // Marshal the manifest into a CSV instance. @@ -2596,40 +2653,6 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { plan.Status.Plan[i].Status = status - case resolver.BundleSecretKind: - var s corev1.Secret - err := json.Unmarshal([]byte(manifest), &s) - if err != nil { - return errorwrap.Wrapf(err, "error parsing step manifest: %s", step.Resource.Name) - } - - // add ownerrefs on the secret that point to the CSV in the bundle - if step.Resolving != "" { - owner := &v1alpha1.ClusterServiceVersion{} - owner.SetNamespace(plan.GetNamespace()) - owner.SetName(step.Resolving) - ownerutil.AddNonBlockingOwner(&s, owner) - } - - // Update UIDs on all CSV OwnerReferences - updated, err := o.getUpdatedOwnerReferences(s.OwnerReferences, plan.Namespace) - if err != nil { - return errorwrap.Wrapf(err, "error generating ownerrefs for secret %s", s.GetName()) - } - s.SetOwnerReferences(updated) - s.SetNamespace(namespace) - if s.Labels == nil { - s.Labels = map[string]string{} - } - s.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue - - status, err := ensurer.EnsureBundleSecret(plan.Namespace, &s) - if err != nil { - return err - } - - plan.Status.Plan[i].Status = status - case secretKind: status, err := ensurer.EnsureSecret(o.namespace, plan.GetNamespace(), step.Resource.Name) if err != nil { @@ -2915,8 +2938,40 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { return notFoundErr } } + // OCPBUGS-35210: log the step that caused ExecutePlan to fail. + // This error becomes syncError in syncInstallPlans and appears in the + // UpdateStatus WRITE log — it is NOT a UpdateStatus error itself. + o.logger.WithFields(logrus.Fields{ + "kind": step.Resource.Kind, + "name": step.Resource.Name, + "stepIndex": i, + "error": err.Error(), + }).Debug("step execution failed — ExecutePlan returning error") return err } + // OCPBUGS-35210: log each step's outcome. Distinguish between steps that + // were actually executed (status changed) and steps that were skipped + // because they were already in a terminal state (status unchanged). + afterStatus := plan.Status.Plan[i].Status + if afterStatus == beforeStatus { + msg := "step execution made no progress" + if afterStatus == v1alpha1.StepStatusCreated || afterStatus == v1alpha1.StepStatusPresent { + msg = "step skipped — already in terminal state" + } + o.logger.WithFields(logrus.Fields{ + "kind": step.Resource.Kind, + "name": step.Resource.Name, + "stepIndex": i, + "status": afterStatus, + }).Debug(msg) + } else { + o.logger.WithFields(logrus.Fields{ + "kind": step.Resource.Kind, + "name": step.Resource.Name, + "stepIndex": i, + "result": afterStatus, + }).Debug("step execution result") + } } // Loop over one final time to check and see if everything is good. diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index e2afbde78c..bc413534ca 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -2,9 +2,11 @@ package catalog import ( "context" + "encoding/json" "fmt" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/pkg/errors" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" @@ -19,10 +21,12 @@ import ( "k8s.io/client-go/util/retry" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/alongside" crdlib "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/crd" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" ) // Stepper manages cluster interactions based on the step. @@ -43,6 +47,8 @@ type builder struct { plan *v1alpha1.InstallPlan csvLister listersv1alpha1.ClusterServiceVersionLister opclient operatorclient.ClientInterface + attenuatedClient operatorclient.ClientInterface + olmClient versioned.Interface dynamicClient dynamic.Interface manifestResolver ManifestResolver logger logrus.FieldLogger @@ -51,11 +57,13 @@ type builder struct { annotator alongside.Annotator } -func newBuilder(plan *v1alpha1.InstallPlan, csvLister listersv1alpha1.ClusterServiceVersionLister, opclient operatorclient.ClientInterface, dynamicClient dynamic.Interface, manifestResolver ManifestResolver, logger logrus.FieldLogger, er record.EventRecorder) *builder { +func newBuilder(plan *v1alpha1.InstallPlan, csvLister listersv1alpha1.ClusterServiceVersionLister, opclient operatorclient.ClientInterface, attenuatedClient operatorclient.ClientInterface, olmClient versioned.Interface, dynamicClient dynamic.Interface, manifestResolver ManifestResolver, logger logrus.FieldLogger, er record.EventRecorder) *builder { return &builder{ plan: plan, csvLister: csvLister, opclient: opclient, + attenuatedClient: attenuatedClient, + olmClient: olmClient, dynamicClient: dynamicClient, manifestResolver: manifestResolver, logger: logger, @@ -91,6 +99,8 @@ func (b *builder) create(step v1alpha1.Step) (Stepper, error) { case crdlib.V1Beta1Version: return b.NewCRDV1Beta1Step(b.opclient.ApiextensionsInterface().ApiextensionsV1beta1(), &step, manifest), nil } + case resolver.BundleSecretKind: + return b.NewBundleSecretStep(&step, manifest), nil } return nil, notSupportedStepperErr{fmt.Sprintf("stepper interface does not support %s", step.Resource.Kind)} } @@ -318,3 +328,95 @@ func setInstalledAlongsideAnnotation(a alongside.Annotator, dst metav1.Object, n a.ToObject(dst, nns) } + +// NewBundleSecretStep returns a StepperFunc for BundleSecret steps (OCPBUGS-35210 Fix 2). +// +// SA-token Secrets must not be created before their owning ServiceAccount exists — the +// Kubernetes token controller (KCM) immediately deletes orphaned token secrets, and +// EnsureBundleSecret would mark the step Created permanently, preventing any retry. +// +// This StepperFunc returns WaitingForAPI when the SA is absent so that NeedsRequeue() +// keeps phase=Installing and OLM retries after 5 s. On the retry the SA has been +// created (it appears later in the plan), and the secret is created successfully. +// WaitingForAPI in the StepperFunc path is handled here directly — it never reaches +// the main ExecutePlan switch that would otherwise skip the step. +func (b *builder) NewBundleSecretStep(step *v1alpha1.Step, manifest string) StepperFunc { + return func() (v1alpha1.StepStatus, error) { + switch step.Status { + case v1alpha1.StepStatusPresent, v1alpha1.StepStatusCreated: + return step.Status, nil + } + + namespace := b.plan.GetNamespace() + + var s corev1.Secret + if err := json.Unmarshal([]byte(manifest), &s); err != nil { + return v1alpha1.StepStatusUnknown, err + } + + saName := s.Annotations[corev1.ServiceAccountNameKey] + if s.Type == corev1.SecretTypeServiceAccountToken && saName != "" { + _, saErr := b.attenuatedClient.KubernetesInterface().CoreV1(). + ServiceAccounts(namespace).Get(context.TODO(), saName, metav1.GetOptions{}) + if apierrors.IsNotFound(saErr) { + logrus.WithFields(logrus.Fields{ + "secret": s.Name, + "sa": saName, + }).Info("BundleSecretStep: SA not yet created — returning WaitingForAPI (OCPBUGS-35210)") + return v1alpha1.StepStatusWaitingForAPI, nil + } + // Forbidden means the scoped client lacks get on serviceaccounts; proceed + // and attempt secret creation — KCM will gate on SA existence regardless. + if saErr != nil && !apierrors.IsForbidden(saErr) { + return v1alpha1.StepStatusUnknown, saErr + } + } + + s.SetNamespace(namespace) + if s.Labels == nil { + s.Labels = map[string]string{} + } + s.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue + + // Add the resolving CSV as a non-blocking owner so the secret is GC'd on + // uninstall. Use the lister (catalog-operator credentials, avoids extra API + // call) — UID is stable once set so a briefly-stale lister entry is safe. + if step.Resolving != "" { + csv, err := b.csvLister.ClusterServiceVersions(namespace).Get(step.Resolving) + if err != nil { + return v1alpha1.StepStatusUnknown, fmt.Errorf("error getting csv %s for secret owner ref: %w", step.Resolving, err) + } + ownerutil.AddNonBlockingOwner(&s, csv) + } + + // Refresh UIDs on any pre-existing CSV owner refs shipped in the bundle + // manifest so Kubernetes GC can match them on uninstall. + updated, err := refreshCSVOwnerRefUIDs(s.OwnerReferences, b.olmClient, namespace) + if err != nil { + return v1alpha1.StepStatusUnknown, fmt.Errorf("error refreshing owner references for secret %s: %w", s.GetName(), err) + } + s.SetOwnerReferences(updated) + + return createOrUpdateSecret(b.attenuatedClient, namespace, &s) + } +} + +// refreshCSVOwnerRefUIDs populates the UID field on any CSV-kind owner references +// using a live API call, matching the behaviour of getUpdatedOwnerReferences used +// by the old BundleSecret handler. A live call (not the lister) is used so that +// freshly-created CSVs whose UIDs have not yet synced to the informer cache are +// handled correctly. +func refreshCSVOwnerRefUIDs(refs []metav1.OwnerReference, olmClient versioned.Interface, namespace string) ([]metav1.OwnerReference, error) { + updated := append([]metav1.OwnerReference(nil), refs...) + for i, owner := range refs { + if owner.Kind == v1alpha1.ClusterServiceVersionKind { + csv, err := olmClient.OperatorsV1alpha1().ClusterServiceVersions(namespace).Get(context.TODO(), owner.Name, metav1.GetOptions{}) + if err != nil { + return nil, err + } + owner.UID = csv.GetUID() + updated[i] = owner + } + } + return updated, nil +} diff --git a/pkg/controller/operators/catalog/step_ensurer.go b/pkg/controller/operators/catalog/step_ensurer.go index 046f04b209..5b344884b7 100644 --- a/pkg/controller/operators/catalog/step_ensurer.go +++ b/pkg/controller/operators/catalog/step_ensurer.go @@ -114,28 +114,27 @@ func (o *StepEnsurer) EnsureSecret(operatorNamespace, planNamespace, name string return } -// EnsureBundleSecret creates user-specified secrets from the bundle. Called when StepResource.Secret is true -func (o *StepEnsurer) EnsureBundleSecret(namespace string, secret *corev1.Secret) (status v1alpha1.StepStatus, err error) { - _, createErr := o.kubeClient.KubernetesInterface().CoreV1().Secrets(namespace).Create(context.TODO(), secret, metav1.CreateOptions{}) +// createOrUpdateSecret creates or updates a Secret using the given client. +// Returns Created on fresh create, Present on update (AlreadyExists), or an error. +func createOrUpdateSecret(client operatorclient.ClientInterface, namespace string, secret *corev1.Secret) (v1alpha1.StepStatus, error) { + _, createErr := client.KubernetesInterface().CoreV1().Secrets(namespace).Create(context.TODO(), secret, metav1.CreateOptions{}) if createErr == nil { - status = v1alpha1.StepStatusCreated - return + return v1alpha1.StepStatusCreated, nil } - if !apierrors.IsAlreadyExists(createErr) { - err = errorwrap.Wrapf(createErr, "error updating secret: %s", secret.GetName()) - return + return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(createErr, "error creating secret: %s", secret.GetName()) } - - secret.SetNamespace(namespace) // NOTE: any annotations/changes applied to the secret are lost - if _, updateErr := o.kubeClient.UpdateSecret(secret); updateErr != nil { - err = errorwrap.Wrapf(updateErr, "error updating secret: %s", secret.GetName()) - return + if _, updateErr := client.UpdateSecret(secret); updateErr != nil { + return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(updateErr, "error updating secret: %s", secret.GetName()) } + return v1alpha1.StepStatusPresent, nil +} - status = v1alpha1.StepStatusPresent - return +// EnsureBundleSecret creates user-specified secrets from the bundle. Called when StepResource.Secret is true +func (o *StepEnsurer) EnsureBundleSecret(namespace string, secret *corev1.Secret) (v1alpha1.StepStatus, error) { + secret.SetNamespace(namespace) + return createOrUpdateSecret(o.kubeClient, namespace, secret) } // EnsureServiceAccount writes the specified ServiceAccount object to the cluster. diff --git a/pkg/controller/operators/catalog/step_test.go b/pkg/controller/operators/catalog/step_test.go index 250d225930..ddbd432dc6 100644 --- a/pkg/controller/operators/catalog/step_test.go +++ b/pkg/controller/operators/catalog/step_test.go @@ -1,17 +1,26 @@ package catalog import ( + "context" + "encoding/json" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" "github.com/operator-framework/api/pkg/operators/v1alpha1" + olmfake "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" v1alpha1listers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/alongside" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/operatorclientmocks" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes" ) @@ -170,3 +179,129 @@ func TestSetInstalledAlongsideAnnotation(t *testing.T) { }) } } + +// TestNewBundleSecretStep is a regression test for OCPBUGS-35210. +// +// SA-token Secrets appear earlier in the InstallPlan step list than their +// owning ServiceAccount. Without the fix, OLM creates the Secret before the +// SA exists; KCM's TokensController immediately deletes orphaned token +// secrets, and OLM marks the step Created permanently with no retry path. +// +// The fix: NewBundleSecretStep returns WaitingForAPI when the SA is absent +// (so NeedsRequeue keeps phase=Installing) and creates the Secret once the +// SA exists on the next reconcile. +func TestNewBundleSecretStep(t *testing.T) { + const ( + namespace = "test-ns" + saName = "test-operator-sa" + secName = "test-operator-metrics-token" + csvName = "test-operator.v1.0.0" + ) + + // Build the SA-token Secret manifest that would appear in the bundle. + tokenSecret := corev1.Secret{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, + ObjectMeta: metav1.ObjectMeta{ + Name: secName, + Namespace: namespace, + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: saName, + }, + }, + Type: corev1.SecretTypeServiceAccountToken, + } + manifest, err := json.Marshal(&tokenSecret) + require.NoError(t, err) + + step := &v1alpha1.Step{ + Resolving: csvName, + Status: v1alpha1.StepStatusUnknown, + Resource: v1alpha1.StepResource{ + Name: secName, + Kind: "BundleSecret", + Manifest: string(manifest), + }, + } + + csv := v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: csvName, + Namespace: namespace, + UID: "test-csv-uid", + }, + } + plan := &v1alpha1.InstallPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "test-plan", Namespace: namespace}, + } + + t.Run("SA absent: returns WaitingForAPI without creating Secret", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + // No SA in the fake client — SA is absent. + fakeK8s := k8sfake.NewSimpleClientset() + mockClient := operatorclientmocks.NewMockClientInterface(ctrl) + mockClient.EXPECT().KubernetesInterface().Return(fakeK8s).AnyTimes() + + csvIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + _ = csvIndexer.Add(&csv) + csvLister := v1alpha1listers.NewClusterServiceVersionLister(csvIndexer) + + b := &builder{ + plan: plan, + attenuatedClient: mockClient, + csvLister: csvLister, + olmClient: olmfake.NewSimpleClientset(&csv), + } + + stepCopy := *step + stepCopy.Status = v1alpha1.StepStatusUnknown + status, err := b.NewBundleSecretStep(&stepCopy, string(manifest))() + + assert.NoError(t, err) + assert.Equal(t, v1alpha1.StepStatusWaitingForAPI, status, + "should return WaitingForAPI when SA is absent so NeedsRequeue keeps phase=Installing") + + // Secret must NOT have been created. + _, getErr := fakeK8s.CoreV1().Secrets(namespace).Get(context.TODO(), secName, metav1.GetOptions{}) + assert.True(t, errors.IsNotFound(getErr), + "Secret must not be created before its ServiceAccount exists (OCPBUGS-35210)") + }) + + t.Run("SA present: creates Secret and returns Created", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: saName, Namespace: namespace}, + } + fakeK8s := k8sfake.NewSimpleClientset(sa) + mockClient := operatorclientmocks.NewMockClientInterface(ctrl) + mockClient.EXPECT().KubernetesInterface().Return(fakeK8s).AnyTimes() + + csvIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + _ = csvIndexer.Add(&csv) + csvLister := v1alpha1listers.NewClusterServiceVersionLister(csvIndexer) + + b := &builder{ + plan: plan, + attenuatedClient: mockClient, + csvLister: csvLister, + olmClient: olmfake.NewSimpleClientset(&csv), + } + + stepCopy := *step + stepCopy.Status = v1alpha1.StepStatusWaitingForAPI // simulates the retry reconcile + status, err := b.NewBundleSecretStep(&stepCopy, string(manifest))() + + assert.NoError(t, err) + assert.Equal(t, v1alpha1.StepStatusCreated, status, + "should create the Secret and return Created once the SA exists") + + // Secret must exist. + secret, getErr := fakeK8s.CoreV1().Secrets(namespace).Get(context.TODO(), secName, metav1.GetOptions{}) + require.NoError(t, getErr, "Secret should have been created (OCPBUGS-35210)") + assert.Equal(t, corev1.SecretTypeServiceAccountToken, secret.Type) + assert.Equal(t, saName, secret.Annotations[corev1.ServiceAccountNameKey]) + }) +} diff --git a/pkg/controller/registry/resolver/steps.go b/pkg/controller/registry/resolver/steps.go index dbe3be8534..e31e914936 100644 --- a/pkg/controller/registry/resolver/steps.go +++ b/pkg/controller/registry/resolver/steps.go @@ -146,7 +146,18 @@ func NewStepResourceFromBundle(bundle *api.Bundle, namespace, replaces, catalogS if err != nil { return nil, err } - steps := []v1alpha1.StepResource{step} + // Synthesized SA/RBAC steps come before bundle objects so that any + // BundleSecret in the bundle always has its owning ServiceAccount available + // when its step executes (fixes the common-path case of OCPBUGS-35210). + // The runtime WaitingForAPI check in NewBundleSecretStep is still required + // for the stale-cache race and for SAs that are themselves in bundle.Object. + operatorServiceAccountSteps, err := NewServiceAccountStepResources(csv, catalogSourceName, catalogSourceNamespace) + if err != nil { + return nil, err + } + steps := make([]v1alpha1.StepResource, 0, 1+len(operatorServiceAccountSteps)+len(bundle.Object)) + steps = append(steps, step) // CSV first + steps = append(steps, operatorServiceAccountSteps...) for _, object := range bundle.Object { dec := yaml.NewYAMLOrJSONDecoder(strings.NewReader(object), 10) @@ -166,11 +177,6 @@ func NewStepResourceFromBundle(bundle *api.Bundle, namespace, replaces, catalogS steps = append(steps, step) } - operatorServiceAccountSteps, err := NewServiceAccountStepResources(csv, catalogSourceName, catalogSourceNamespace) - if err != nil { - return nil, err - } - steps = append(steps, operatorServiceAccountSteps...) return steps, nil } From 039659691f46352a153e12a46a4d399e09dc6e2e Mon Sep 17 00:00:00 2001 From: Raimund Sacherer Date: Fri, 21 Aug 2026 17:17:50 +0200 Subject: [PATCH 2/2] pass attenuated crclient as olmClient in newBuilder Use the attenuated versioned.Interface (crclient) instead of the unattenuated catalog-operator o.client for the builder's olmClient. This scopes refreshCSVOwnerRefUIDs CSV lookups to the InstallPlan SA credentials, consistent with how attenuatedClient is used for Secret creation in NewBundleSecretStep. Also include syncError in InstallPlan diagnostic log fields when reconciliation returns an error. --- pkg/controller/operators/catalog/operator.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 7be00cb5fd..4158c04ae8 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -2197,6 +2197,9 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) { "resourceVersion": outInstallPlan.ResourceVersion, "phase": outInstallPlan.Status.Phase, } + if syncError != nil { + fields["syncError"] = syncError.Error() + } for i, step := range outInstallPlan.Status.Plan { if step.Resource.Kind == "BundleSecret" || step.Resource.Kind == "ServiceAccount" { fields[fmt.Sprintf("step[%d].%s", i, step.Resource.Kind)] = string(step.Status) @@ -2519,7 +2522,7 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { o.logger.Errorf("failed to get a client for plan execution- %v", err) return err } - b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, kubeclient, o.client, builderDynamicClient, r, o.logger, o.recorder) + b := newBuilder(plan, o.lister.OperatorsV1alpha1().ClusterServiceVersionLister(), builderKubeClient, kubeclient, crclient, builderDynamicClient, r, o.logger, o.recorder) for i, step := range plan.Status.Plan { beforeStatus := plan.Status.Plan[i].Status