From 4a4385e8e81c212538e972f4cf2fe8572e033845 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Fri, 14 Aug 2026 09:33:44 +0200 Subject: [PATCH 1/4] fix(olm): fix ConversionWebhook spec.conversion lifecycle during CSV upgrades Two bugs fixed: 1. Upgrade race: spec.conversion was written to CRDs during Install() before any pod from the new deployment was ready. Once set, the apiserver routes conversion calls to the new service endpoint, but since no pod is serving /convert yet, those calls return HTTP 404. Fix: skip ConversionWebhook descriptors in createOrUpdateCertResourcesForDeployment() so spec.conversion is never written during Install(). Add EnsureConversionWebhooks() on *StrategyDeploymentInstaller and call it from areWebhooksAvailable(), which is only reached after CheckInstalled() confirms pods are ready. Gate areWebhooksAvailable() behind strategyInstalled && strategyErr == nil to ensure EnsureConversionWebhooks() is never called prematurely. 2. Missing cleanup when replacement CSV drops the ConversionWebhook: handleClusterServiceVersionDeletion returned unconditionally when any replacement CSV was found, assuming the replacement would manage spec.conversion. If the replacement dropped the ConversionWebhook entirely, spec.conversion stayed pointing at the now-deleted service, causing all CR conversion requests to fail. Fix: build the set of CRDs still covered by a ConversionWebhook in the replacement CSV. Only reset spec.conversion to NoneConverter for CRDs the new CSV dropped. CRDs the replacement still covers are left intact. Return early if the CSV list fails to avoid incorrectly clearing spec.conversion with an incomplete picture. Also adds a nil guard on crd.Spec.Conversion before writing to it, fixing a latent panic in the no-replacement path. Co-Authored-By: Claude --- pkg/controller/install/deployment.go | 24 +++++++++++ pkg/controller/operators/olm/apiservices.go | 14 ++++++- pkg/controller/operators/olm/operator.go | 46 +++++++++++++++++---- 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/pkg/controller/install/deployment.go b/pkg/controller/install/deployment.go index 714badb2ea..7f21a933a0 100644 --- a/pkg/controller/install/deployment.go +++ b/pkg/controller/install/deployment.go @@ -123,6 +123,14 @@ func (i *StrategyDeploymentInstaller) createOrUpdateCertResourcesForDeployment() return err } case *webhookDescriptionWithCAPEM: + // ConversionWebhook CRD patching is deferred to the post-Install readiness + // check (areWebhooksAvailable) so that spec.conversion is only written once + // the new deployment's pods are actually serving /convert. Writing it here + // during Install() — before any pod is ready — causes a window where the + // apiserver routes conversion calls to pods that return HTTP 404. + if d.webhookDescription.Type == v1alpha1.ConversionWebhook { + continue + } err := i.createOrUpdateWebhook(d.caPEM, d.webhookDescription) if err != nil { return err @@ -134,6 +142,22 @@ func (i *StrategyDeploymentInstaller) createOrUpdateCertResourcesForDeployment() return nil } +// EnsureConversionWebhooks writes spec.conversion on CRDs for any ConversionWebhook +// entries in the cert resources. Called after the deployment is confirmed ready so that +// the conversion endpoint is only activated when the new pods are serving /convert. +func (i *StrategyDeploymentInstaller) EnsureConversionWebhooks() error { + for _, desc := range i.getCertResources() { + d, ok := desc.(*webhookDescriptionWithCAPEM) + if !ok || d.webhookDescription.Type != v1alpha1.ConversionWebhook { + continue + } + if err := i.createOrUpdateConversionWebhook(d.caPEM, d.webhookDescription); err != nil { + return err + } + } + return nil +} + func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec, specLabels k8slabels.Set) (deployment *appsv1.Deployment, hash string, err error) { dep := &appsv1.Deployment{Spec: spec} dep.SetName(name) diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go index f0deae1706..8ea29af618 100644 --- a/pkg/controller/operators/olm/apiservices.go +++ b/pkg/controller/operators/olm/apiservices.go @@ -562,7 +562,11 @@ func (a *Operator) cleanUpRemovedWebhooks(csv *v1alpha1.ClusterServiceVersion) e return nil } -func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bool, error) { +// areWebhooksAvailable checks that all webhook resources declared in the CSV exist and +// are correctly configured. For ConversionWebhook entries it also writes spec.conversion +// on the target CRDs using the provided installer, ensuring conversion is only activated +// once the new deployment's pods are ready to serve /convert. +func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion, installer install.StrategyInstaller) (bool, error) { err := a.cleanUpRemovedWebhooks(csv) if err != nil { return false, err @@ -593,6 +597,14 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo } webhookCount = len(webhookList.Items) case v1alpha1.ConversionWebhook: + // Write spec.conversion on each target CRD now that the deployment is confirmed + // ready. This is deferred from Install() to prevent routing conversion calls to + // pods that are not yet serving /convert. + if sdi, ok := installer.(*install.StrategyDeploymentInstaller); ok { + if err := sdi.EnsureConversionWebhooks(); err != nil { + return false, fmt.Errorf("conversionWebhook not ready: %w", err) + } + } for _, conversionCRD := range desc.ConversionCRDs { // check if CRD exists on cluster crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{}) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 7aaa75eb5d..c125eab5e1 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -1302,11 +1302,27 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { // webhook from the CRD definition. csvs, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(clusterServiceVersion.GetNamespace()).List(labels.Everything()) if err != nil { - logger.Errorf("error listing csvs: %v\n", err) + // Without a complete CSV list we cannot safely determine which CRDs are still + // covered by a replacement CSV. Bail out to avoid incorrectly clearing + // spec.conversion on CRDs that a replacement still owns. + logger.Errorf("error listing csvs, skipping conversion webhook cleanup: %v\n", err) + return } + + // Build the set of CRDs whose ConversionWebhook is still covered by the replacement CSV. + // If the replacement dropped the ConversionWebhook for a given CRD, spec.conversion on + // that CRD must be reset — otherwise it keeps pointing at the now-deleted service and + // all CR requests against that CRD will fail. + coveredCRDs := map[string]bool{} for _, csv := range csvs { if csv.Spec.Replaces == clusterServiceVersion.GetName() { - return + for _, desc := range csv.Spec.WebhookDefinitions { + if desc.Type == v1alpha1.ConversionWebhook { + for _, crdName := range desc.ConversionCRDs { + coveredCRDs[crdName] = true + } + } + } } } @@ -1316,6 +1332,12 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { } for i, crdName := range desc.ConversionCRDs { + if coveredCRDs[crdName] { + // Replacement CSV still has a ConversionWebhook for this CRD; leave + // spec.conversion intact so in-flight conversion calls keep working. + continue + } + crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) if err != nil { logger.Errorf("error getting CRD %v which was defined in CSVs spec.WebhookDefinition[%d]: %v\n", crdName, i, err) @@ -1323,11 +1345,13 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { } copy := crd.DeepCopy() - copy.Spec.Conversion.Strategy = apiextensionsv1.NoneConverter - copy.Spec.Conversion.Webhook = nil + if copy.Spec.Conversion != nil { + copy.Spec.Conversion.Strategy = apiextensionsv1.NoneConverter + copy.Spec.Conversion.Webhook = nil - if _, err = a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), copy, metav1.UpdateOptions{}); err != nil { - logger.Errorf("error updating conversion strategy for CRD %v: %v\n", crdName, err) + if _, err = a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), copy, metav1.UpdateOptions{}); err != nil { + logger.Errorf("error updating conversion strategy for CRD %v: %v\n", crdName, err) + } } } } @@ -2614,7 +2638,14 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst } apiServicesInstalled, apiServiceErr := a.areAPIServicesAvailable(csv) - webhooksInstalled, webhookErr := a.areWebhooksAvailable(csv) + // Only attempt to write spec.conversion once the deployment is confirmed ready. + // areWebhooksAvailable calls EnsureConversionWebhooks, so calling it when + // strategyInstalled is false would recreate the upgrade race we are fixing. + webhooksInstalled := false + var webhookErr error + if strategyInstalled && strategyErr == nil { + webhooksInstalled, webhookErr = a.areWebhooksAvailable(csv, installer) + } if strategyInstalled && apiServicesInstalled && webhooksInstalled { // if there's no error, we're successfully running @@ -2981,3 +3012,4 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets .. out, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(context.TODO(), out, metav1.UpdateOptions{}) return out, err } + From d4fee98bf9d9a34ed3828dcf88b376624e09b669 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Fri, 14 Aug 2026 16:07:46 +0200 Subject: [PATCH 2/4] fix(olm): gofmt operator.go Co-Authored-By: Claude --- pkg/controller/operators/olm/operator.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index c125eab5e1..ed82f05192 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -3012,4 +3012,3 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets .. out, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(context.TODO(), out, metav1.UpdateOptions{}) return out, err } - From 9cf4d974f8b9f5e1efa378411ab449259db93026 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Tue, 18 Aug 2026 09:03:48 +0200 Subject: [PATCH 3/4] fix(olm): return explicit error on type assertion failure in areWebhooksAvailable Co-Authored-By: Claude --- pkg/controller/operators/olm/apiservices.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go index 8ea29af618..70e15e07ee 100644 --- a/pkg/controller/operators/olm/apiservices.go +++ b/pkg/controller/operators/olm/apiservices.go @@ -600,10 +600,12 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion, ins // Write spec.conversion on each target CRD now that the deployment is confirmed // ready. This is deferred from Install() to prevent routing conversion calls to // pods that are not yet serving /convert. - if sdi, ok := installer.(*install.StrategyDeploymentInstaller); ok { - if err := sdi.EnsureConversionWebhooks(); err != nil { - return false, fmt.Errorf("conversionWebhook not ready: %w", err) - } + sdi, ok := installer.(*install.StrategyDeploymentInstaller) + if !ok { + return false, fmt.Errorf("conversionWebhook requires a StrategyDeploymentInstaller, got %T", installer) + } + if err := sdi.EnsureConversionWebhooks(); err != nil { + return false, fmt.Errorf("conversionWebhook not ready: %w", err) } for _, conversionCRD := range desc.ConversionCRDs { // check if CRD exists on cluster From 759758e8c0414809b79c96a269eb18401f457571 Mon Sep 17 00:00:00 2001 From: Ugo Giordano Date: Tue, 18 Aug 2026 09:03:50 +0200 Subject: [PATCH 4/4] fix(olm): drop redundant strategyErr == nil guard in updateInstallStatus Co-Authored-By: Claude --- pkg/controller/operators/olm/operator.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index ed82f05192..315e84abe5 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -2641,9 +2641,11 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst // Only attempt to write spec.conversion once the deployment is confirmed ready. // areWebhooksAvailable calls EnsureConversionWebhooks, so calling it when // strategyInstalled is false would recreate the upgrade race we are fixing. + // Note: CheckInstalled never returns (true, non-nil error), so strategyInstalled + // is sufficient — no need to also gate on strategyErr. webhooksInstalled := false var webhookErr error - if strategyInstalled && strategyErr == nil { + if strategyInstalled { webhooksInstalled, webhookErr = a.areWebhooksAvailable(csv, installer) }