Skip to content

[release-4.8] Bug 1989779: installplans: retry crd updates on conflicts #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 8, 2021
Merged

[release-4.8] Bug 1989779: installplans: retry crd updates on conflicts #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 64 additions & 58 deletions staging/operator-lifecycle-manager/pkg/controller/install/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}

Expand Down
88 changes: 50 additions & 38 deletions staging/operator-lifecycle-manager/pkg/controller/operators/catalog/step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ 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"
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/util/retry"

"github.com/operator-framework/api/pkg/operators/v1alpha1"
listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading