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 b6f56f08f9..bef14979f1 100644 --- a/pkg/controller/operators/catalog/step.go +++ b/pkg/controller/operators/catalog/step.go @@ -4,22 +4,21 @@ import ( "context" "fmt" - "k8s.io/client-go/dynamic" - "k8s.io/client-go/tools/cache" - - "github.com/operator-framework/api/pkg/operators/v1alpha1" - crdlib "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/crd" - index "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/index" - "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" - errorwrap "github.com/pkg/errors" logger "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" apiextensionsv1beta1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/retry" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + crdlib "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/crd" + index "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/index" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" ) // Stepper manages cluster interactions based on the step. @@ -99,7 +98,7 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter if k8serrors.IsNotFound(err) { return v1alpha1.StepStatusNotPresent, nil } else { - return v1alpha1.StepStatusNotPresent, errorwrap.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 @@ -126,39 +125,45 @@ 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{}) - // Verify CRD ownership, only attempt to update if - // CRD has only one owner - // Example: provided=database.coreos.com/v1alpha1/EtcdCluster - matchedCSV, err := index.V1CRDProviderNames(b.csvToProvidedAPIs, crd) - if err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error find matched CSV: %s", step.Resource.Name) - } - crd.SetResourceVersion(currentCRD.GetResourceVersion()) - if len(matchedCSV) == 1 { - logger.Debugf("Found one owner for CRD %v", crd) - } else if len(matchedCSV) > 1 { - logger.Debugf("Found multiple owners for CRD %v", crd) + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) + // Verify CRD ownership, only attempt to update if + // CRD has only one owner + // Example: provided=database.coreos.com/v1alpha1/EtcdCluster + matchedCSV, err := index.V1CRDProviderNames(b.csvToProvidedAPIs, crd) + if err != nil { + return fmt.Errorf("error finding matched CSV %q: %w", step.Resource.Name, err) + } + crd.SetResourceVersion(currentCRD.GetResourceVersion()) + if len(matchedCSV) == 1 { + logger.Debugf("Found one owner for CRD %v", crd) + } else if len(matchedCSV) > 1 { + logger.Debugf("Found multiple owners for CRD %v", crd) - if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name) + 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, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name) - } - if err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.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 %s: %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 - _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + // Update CRD to new version + _, 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, errorwrap.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 @@ -187,7 +192,7 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi if k8serrors.IsNotFound(err) { return v1alpha1.StepStatusNotPresent, nil } else { - return v1alpha1.StepStatusNotPresent, errorwrap.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 @@ -214,39 +219,45 @@ 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{}) - // Verify CRD ownership, only attempt to update if - // CRD has only one owner - // Example: provided=database.coreos.com/v1alpha1/EtcdCluster - matchedCSV, err := index.V1Beta1CRDProviderNames(b.csvToProvidedAPIs, crd) - if err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error find matched CSV: %s", step.Resource.Name) - } - crd.SetResourceVersion(currentCRD.GetResourceVersion()) - if len(matchedCSV) == 1 { - logger.Debugf("Found one owner for CRD %v", crd) - } else if len(matchedCSV) > 1 { - logger.Debugf("Found multiple owners for CRD %v", crd) + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{}) + // Verify CRD ownership, only attempt to update if + // CRD has only one owner + // Example: provided=database.coreos.com/v1alpha1/EtcdCluster + matchedCSV, err := index.V1Beta1CRDProviderNames(b.csvToProvidedAPIs, crd) + if err != nil { + return fmt.Errorf("error finding matched CSV %q: %w", step.Resource.Name, err) + } + crd.SetResourceVersion(currentCRD.GetResourceVersion()) + if len(matchedCSV) == 1 { + logger.Debugf("Found one owner for CRD %v", crd) + } else if len(matchedCSV) > 1 { + logger.Debugf("Found multiple owners for CRD %v", crd) - if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.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, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name) - } - if err != nil { - return v1alpha1.StepStatusUnknown, errorwrap.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 %s: %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 - _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}) + // Update CRD to new version + _, 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, errorwrap.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