From 8b80f2605bf8936fbffc6a955f36ac70d7d155fe Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Thu, 29 Jul 2021 15:24:21 -0400 Subject: [PATCH] installplans: retry crd updates on conflicts (#2293) Signed-off-by: Joe Lanford --- pkg/controller/install/webhook.go | 122 ++++++++++++----------- pkg/controller/operators/catalog/step.go | 88 +++++++++------- 2 files changed, 114 insertions(+), 96 deletions(-) diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go index 8b608754cd..c6fbb731fc 100644 --- a/pkg/controller/install/webhook.go +++ b/pkg/controller/install/webhook.go @@ -5,16 +5,17 @@ import ( "fmt" "hash/fnv" - "github.com/operator-framework/api/pkg/operators/v1alpha1" - hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" - log "github.com/sirupsen/logrus" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/client-go/util/retry" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" ) func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error { @@ -200,69 +201,74 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by // iterate over all the ConversionCRDs for _, conversionCRD := range desc.ConversionCRDs { - // Get existing CRD on cluster - crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err) - } + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + // Get existing CRD on cluster + crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err) + } - // check if this CRD is an owned CRD - foundCRD := false - for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned { - if ownedCRD.Name == conversionCRD { - foundCRD = true - break + // check if this CRD is an owned CRD + foundCRD := false + for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned { + if ownedCRD.Name == conversionCRD { + foundCRD = true + break + } + } + if !foundCRD { + return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD) } - } - if !foundCRD { - return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD) - } - // crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions. - // Allowed values are: - // - None: The converter only change the apiVersion and would not touch any other field in the custom resource. - // - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false. - // References: - // - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html - // - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields - // By default the strategy is none - // Reference: - // - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions - if crd.Spec.PreserveUnknownFields != false { - return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion") - } + // crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions. + // Allowed values are: + // - None: The converter only change the apiVersion and would not touch any other field in the custom resource. + // - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false. + // References: + // - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html + // - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields + // By default the strategy is none + // Reference: + // - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions + if crd.Spec.PreserveUnknownFields != false { + return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion") + } - // Conversion WebhookClientConfig should not be set when Strategy is None - // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions - // Conversion WebhookClientConfig needs to be set when Strategy is None - // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks + // Conversion WebhookClientConfig should not be set when Strategy is None + // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions + // Conversion WebhookClientConfig needs to be set when Strategy is None + // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks - // use user defined path for CRD conversion webhook, else set default value - conversionWebhookPath := "/" - if desc.WebhookPath != nil { - conversionWebhookPath = *desc.WebhookPath - } + // use user defined path for CRD conversion webhook, else set default value + conversionWebhookPath := "/" + if desc.WebhookPath != nil { + conversionWebhookPath = *desc.WebhookPath + } - // Override Name, Namespace, and CABundle - crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{ - Strategy: "Webhook", - Webhook: &apiextensionsv1.WebhookConversion{ - ClientConfig: &apiextensionsv1.WebhookClientConfig{ - Service: &apiextensionsv1.ServiceReference{ - Namespace: i.owner.GetNamespace(), - Name: desc.DomainName() + "-service", - Path: &conversionWebhookPath, - Port: &desc.ContainerPort, + // Override Name, Namespace, and CABundle + crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{ + Strategy: "Webhook", + Webhook: &apiextensionsv1.WebhookConversion{ + ClientConfig: &apiextensionsv1.WebhookClientConfig{ + Service: &apiextensionsv1.ServiceReference{ + Namespace: i.owner.GetNamespace(), + Name: desc.DomainName() + "-service", + Path: &conversionWebhookPath, + Port: &desc.ContainerPort, + }, + CABundle: caPEM, }, - CABundle: caPEM, + ConversionReviewVersions: desc.AdmissionReviewVersions, }, - ConversionReviewVersions: desc.AdmissionReviewVersions, - }, - } + } - // update CRD conversion Specs - if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("Error updating CRD with Conversion info: %v", err) + // update CRD conversion Specs + if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("Error updating CRD with Conversion info: %w", err) + } + return nil + }); err != nil { + return err } } diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go index 31393e636e..1093c1ddd1 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -6,7 +6,6 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" @@ -14,6 +13,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/dynamic" + "k8s.io/client-go/util/retry" "github.com/operator-framework/api/pkg/operators/v1alpha1" listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1" @@ -132,27 +132,33 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter _, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{}) if k8serrors.IsAlreadyExists(createError) { - currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) - crd.SetResourceVersion(currentCRD.GetResourceVersion()) - if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name) - } + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) + crd.SetResourceVersion(currentCRD.GetResourceVersion()) + if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { + return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err) + } - // check to see if stored versions changed and whether the upgrade could cause potential data loss - safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd) - if !safe { - b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err) - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "risk of data loss updating %s", step.Resource.Name) - } - if err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name) - } + // check to see if stored versions changed and whether the upgrade could cause potential data loss + safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd) + if !safe { + b.logger.Errorf("risk of data loss updating %q: %s", step.Resource.Name, err) + return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err) + } + if err != nil { + return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err) + } - // Update CRD to new version - setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD) - _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + // Update CRD to new version + setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD) + _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err) + } + return nil + }) if err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error updating CRD: %s", step.Resource.Name) + return v1alpha1.StepStatusUnknown, err } // If it already existed, mark the step as Present. // they were equal - mark CRD as present @@ -181,7 +187,7 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi if k8serrors.IsNotFound(err) { return v1alpha1.StepStatusNotPresent, nil } else { - return v1alpha1.StepStatusNotPresent, errors.Wrapf(err, "error finding the %s CRD", crd.Name) + return v1alpha1.StepStatusNotPresent, fmt.Errorf("error finding the %q CRD: %w", crd.Name, err) } } established, namesAccepted := false, false @@ -210,28 +216,34 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi _, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{}) if k8serrors.IsAlreadyExists(createError) { - currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) - crd.SetResourceVersion(currentCRD.GetResourceVersion()) + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) + crd.SetResourceVersion(currentCRD.GetResourceVersion()) - if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name) - } + if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { + return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err) + } - // check to see if stored versions changed and whether the upgrade could cause potential data loss - safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd) - if !safe { - b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err) - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "risk of data loss updating %s", step.Resource.Name) - } - if err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name) - } + // check to see if stored versions changed and whether the upgrade could cause potential data loss + safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd) + if !safe { + b.logger.Errorf("risk of data loss updating %q: %s", step.Resource.Name, err) + return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err) + } + if err != nil { + return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err) + } - // Update CRD to new version - setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD) - _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + // Update CRD to new version + setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD) + _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err) + } + return nil + }) if err != nil { - return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error updating CRD: %s", step.Resource.Name) + return v1alpha1.StepStatusUnknown, err } // If it already existed, mark the step as Present. // they were equal - mark CRD as present