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..70e15e07ee 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,16 @@ 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. + 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 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..315e84abe5 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,16 @@ 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. + // 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 { + webhooksInstalled, webhookErr = a.areWebhooksAvailable(csv, installer) + } if strategyInstalled && apiServicesInstalled && webhooksInstalled { // if there's no error, we're successfully running