Merge pull request #360 from perdasilva/webhook_clean_up

OCPBUGS-78: Cleanup conversion webhooks when an operator is uninstalled

Author: openshift-merge-robot

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator.go

@@ -11,6 +11,7 @@ import (
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -1121,6 +1122,46 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 			logger.WithError(err).Warnf("failed to requeue gc event: %v", webhook)
 		}
 	}
+
+	// Conversion webhooks are defined within a CRD.
+	// In an effort to prevent customer dataloss, OLM does not delete CRDs associated with a CSV when it is deleted.
+	// Deleting a CSV that introduced a conversion webhook removes the deployment that serviced the conversion webhook calls.
+	// If a conversion webhook is defined and the service isn't available, all requests against the CR associated with the CRD will fail.
+	// This ultimately breaks kubernetes garbage collection and prevents OLM from reinstalling the CSV as CR validation against the new CRD's
+	// openapiv3 schema fails.
+	// As such, when a CSV is deleted OLM will check if it is being replaced. If the CSV is not being replaced, OLM will remove the conversion
+	// 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)
+	}
+	for _, csv := range csvs {
+		if csv.Spec.Replaces == clusterServiceVersion.GetName() {
+			return
+		}
+	}
+
+	for _, desc := range clusterServiceVersion.Spec.WebhookDefinitions {
+		if desc.Type != v1alpha1.ConversionWebhook || len(desc.ConversionCRDs) == 0 {
+			continue
+		}
+
+		for i, crdName := range desc.ConversionCRDs {
+			crd, err := a.lister.APIExtensionsV1().CustomResourceDefinitionLister().Get(crdName)
+			if err != nil {
+				logger.Errorf("error getting CRD %v which was defined in CSVs spec.WebhookDefinition[%d]: %v\n", crdName, i, err)
+				continue
+			}
+
+			copy := crd.DeepCopy()
+			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)
+			}
+		}
+	}
 }
 
 func (a *Operator) removeDanglingChildCSVs(csv *v1alpha1.ClusterServiceVersion) error {

staging/operator-lifecycle-manager/test/e2e/csv_e2e_test.go

@@ -4206,6 +4206,9 @@ func findLastEvent(events *corev1.EventList) (event corev1.Event) {
 func buildCSVCleanupFunc(c operatorclient.ClientInterface, crc versioned.Interface, csv operatorsv1alpha1.ClusterServiceVersion, namespace string, deleteCRDs, deleteAPIServices bool) cleanupFunc {
 	return func() {
 		err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace).Delete(context.TODO(), csv.GetName(), metav1.DeleteOptions{})
+		if err != nil && apierrors.IsNotFound(err) {
+			err = nil
+		}
 		Expect(err).ShouldNot(HaveOccurred())
 
 		if deleteCRDs {

staging/operator-lifecycle-manager/test/e2e/webhook_e2e_test.go

@@ -639,6 +639,7 @@ var _ = Describe("CSVs with a Webhook", func() {
 		}).Should(Equal(2))
 	})
 	When("Installed from a catalog Source", func() {
+		const csvName = "webhook-operator.v0.0.1"
 		var cleanupCSV cleanupFunc
 		var cleanupCatSrc cleanupFunc
 		var cleanupSubscription cleanupFunc
@@ -687,7 +688,7 @@ var _ = Describe("CSVs with a Webhook", func() {
 			defer cleanupSubscription()
 
 			// Wait for webhook-operator v2 csv to succeed
-			csv, err := awaitCSV(crc, source.GetNamespace(), "webhook-operator.v0.0.1", csvSucceededChecker)
+			csv, err := awaitCSV(crc, source.GetNamespace(), csvName, csvSucceededChecker)
 			require.NoError(GinkgoT(), err)
 
 			cleanupCSV = buildCSVCleanupFunc(c, crc, *csv, source.GetNamespace(), true, true)
@@ -775,6 +776,25 @@ var _ = Describe("CSVs with a Webhook", func() {
 			require.True(GinkgoT(), ok, "Unable to get spec.conversion.valid of v2 object")
 			require.True(GinkgoT(), v2SpecConversionMutate)
 			require.True(GinkgoT(), v2SpecConversionValid)
+
+			// Check that conversion strategies are disabled after uninstalling the operator.
+			err = crc.OperatorsV1alpha1().ClusterServiceVersions(generatedNamespace.GetName()).Delete(context.TODO(), csvName, metav1.DeleteOptions{})
+			require.NoError(GinkgoT(), err)
+
+			Eventually(func() error {
+				crd, err := c.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), "webhooktests.webhook.operators.coreos.io", metav1.GetOptions{})
+				if err != nil {
+					return err
+				}
+
+				if crd.Spec.Conversion.Strategy != apiextensionsv1.NoneConverter {
+					return fmt.Errorf("conversion strategy is not NoneConverter")
+				}
+				if crd.Spec.Conversion.Webhook != nil {
+					return fmt.Errorf("webhook is not nil")
+				}
+				return nil
+			}).Should(Succeed())
 		})
 	})
 	When("WebhookDescription has conversionCRDs field", func() {