Merge branch 'release-4.11' into cherry-pick-opm-caching

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

@@ -2,6 +2,7 @@ package decorators
 
 import (
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/itchyny/gojq"
@@ -331,6 +332,23 @@ func (o *Operator) AddComponents(components ...runtime.Object) error {
 
 	o.Status.Components.Refs = append(o.Status.Components.Refs, refs...)
 
+	// Sort the component refs to so subsequent reconciles of the object do not change
+	// the status and result in an update to the object.
+	sort.SliceStable(o.Status.Components.Refs, func(i, j int) bool {
+		if o.Status.Components.Refs[i].Kind != o.Status.Components.Refs[j].Kind {
+			return o.Status.Components.Refs[i].Kind < o.Status.Components.Refs[j].Kind
+		}
+
+		if o.Status.Components.Refs[i].APIVersion != o.Status.Components.Refs[j].APIVersion {
+			return o.Status.Components.Refs[i].APIVersion < o.Status.Components.Refs[j].APIVersion
+		}
+
+		if o.Status.Components.Refs[i].Namespace != o.Status.Components.Refs[j].Namespace {
+			return o.Status.Components.Refs[i].Namespace < o.Status.Components.Refs[j].Namespace
+		}
+		return o.Status.Components.Refs[i].Name < o.Status.Components.Refs[j].Name
+	})
+
 	return nil
 }
 

staging/operator-lifecycle-manager/pkg/controller/operators/decorators/operator_test.go

@@ -174,6 +174,22 @@ func TestAddComponents(t *testing.T) {
 						},
 					}
 					operator.Status.Components.Refs = []operatorsv1.RichReference{
+						{
+							ObjectReference: &corev1.ObjectReference{
+								APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
+								Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+								Namespace:  "atlantic",
+								Name:       "puffin",
+							},
+							Conditions: []operatorsv1.Condition{
+								{
+									Type:    operatorsv1.ConditionType(operatorsv1alpha1.CSVPhaseSucceeded),
+									Status:  corev1.ConditionTrue,
+									Reason:  string(operatorsv1alpha1.CSVReasonInstallSuccessful),
+									Message: "this puffin is happy",
+								},
+							},
+						},
 						{
 							ObjectReference: &corev1.ObjectReference{
 								APIVersion: "v1",
@@ -195,22 +211,6 @@ func TestAddComponents(t *testing.T) {
 								},
 							},
 						},
-						{
-							ObjectReference: &corev1.ObjectReference{
-								APIVersion: operatorsv1alpha1.SchemeGroupVersion.String(),
-								Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
-								Namespace:  "atlantic",
-								Name:       "puffin",
-							},
-							Conditions: []operatorsv1.Condition{
-								{
-									Type:    operatorsv1.ConditionType(operatorsv1alpha1.CSVPhaseSucceeded),
-									Status:  corev1.ConditionTrue,
-									Reason:  string(operatorsv1alpha1.CSVReasonInstallSuccessful),
-									Message: "this puffin is happy",
-								},
-							},
-						},
 					}
 
 					return operator

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"
@@ -1119,6 +1120,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/pkg/controller/operators/operator_controller.go

@@ -9,6 +9,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/labels"
@@ -152,9 +153,11 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			return ctrl.Result{Requeue: true}, nil
 		}
 	} else {
-		if err := r.Status().Update(ctx, operator.Operator); err != nil {
-			log.Error(err, "Could not update Operator status")
-			return ctrl.Result{Requeue: true}, nil
+		if !equality.Semantic.DeepEqual(in.Status, operator.Operator.Status) {
+			if err := r.Status().Update(ctx, operator.Operator); err != nil {
+				log.Error(err, "Could not update Operator status")
+				return ctrl.Result{Requeue: true}, nil
+			}
 		}
 	}
 

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

@@ -2,6 +2,7 @@ package operators
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -204,6 +205,44 @@ var _ = Describe("Operator Controller", func() {
 				})
 			})
 
+			Context("when multiple types of a gvk are labeled", func() {
+				BeforeEach(func() {
+					newObjs := make([]runtime.Object, 9)
+
+					// Create objects in reverse order to ensure they are eventually ordered alphabetically in the status of the operator.
+					for i := 8; i >= 0; i-- {
+						newObjs[8-i] = testobj.WithLabels(map[string]string{expectedKey: ""}, testobj.WithNamespacedName(
+							&types.NamespacedName{Namespace: namespace, Name: fmt.Sprintf("sa-%d", i)}, &corev1.ServiceAccount{},
+						))
+					}
+
+					for _, obj := range newObjs {
+						Expect(k8sClient.Create(ctx, obj.(client.Object))).To(Succeed())
+					}
+
+					objs = append(objs, newObjs...)
+					expectedRefs = append(expectedRefs, toRefs(scheme, newObjs...)...)
+				})
+
+				It("should list each of the component references in alphabetical order by namespace and name", func() {
+					Eventually(func() ([]operatorsv1.RichReference, error) {
+						err := k8sClient.Get(ctx, name, operator)
+						return operator.Status.Components.Refs, err
+					}, timeout, interval).Should(ConsistOf(expectedRefs))
+
+					serviceAccountCount := 0
+					for _, ref := range operator.Status.Components.Refs {
+						if ref.Kind != rbacv1.ServiceAccountKind {
+							continue
+						}
+
+						Expect(ref.Name).Should(Equal(fmt.Sprintf("sa-%d", serviceAccountCount)))
+						serviceAccountCount++
+					}
+					Expect(serviceAccountCount).To(Equal(9))
+				})
+			})
+
 			Context("when component labels are removed", func() {
 
 				BeforeEach(func() {

staging/operator-lifecycle-manager/pkg/lib/scoped/token_retriever.go

@@ -82,6 +82,12 @@ func getAPISecret(logger logrus.FieldLogger, kubeclient operatorclient.ClientInt
 func filterSecretsBySAName(saName string, secrets *corev1.SecretList) map[string]*corev1.Secret {
 	secretMap := make(map[string]*corev1.Secret)
 	for _, ref := range secrets.Items {
+		// Avoid referencing the "ref" created by the range-for loop as
+		// the secret stored in the map will change if there are more
+		// entries in the list of secrets that the range-for loop is
+		// iterating over.
+		ref := ref
+
 		annotations := ref.GetAnnotations()
 		value := annotations[corev1.ServiceAccountNameKey]
 		if value == saName {

staging/operator-lifecycle-manager/pkg/lib/scoped/token_retriever_test.go

@@ -0,0 +1,117 @@
+package scoped
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const serviceAccountName = "foo"
+
+func TestFilterSecretsBySAName(t *testing.T) {
+	tests := []struct {
+		name              string
+		secrets           *corev1.SecretList
+		wantedSecretNames []string
+	}{
+		{
+			name: "NoSecretFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret"),
+					*newSecret("someSecret"),
+					*newSecret("zSecret"),
+				},
+			},
+			wantedSecretNames: []string{},
+		},
+		{
+			name: "FirstSecretFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+					*newSecret("someSecret"),
+					*newSecret("zSecret"),
+				},
+			},
+			wantedSecretNames: []string{"aSecret"},
+		},
+		{
+			name: "SecondSecretFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret"),
+					*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+					*newSecret("zSecret"),
+				},
+			},
+			wantedSecretNames: []string{"someSecret"},
+		},
+		{
+			name: "ThirdSecretFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret"),
+					*newSecret("someSecret"),
+					*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+				},
+			},
+			wantedSecretNames: []string{"zSecret"},
+		},
+		{
+			name: "TwoSecretsFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret"),
+					*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+					*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+				},
+			},
+			wantedSecretNames: []string{"someSecret", "zSecret"},
+		},
+		{
+			name: "AllSecretsFound",
+			secrets: &corev1.SecretList{
+				Items: []corev1.Secret{
+					*newSecret("aSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+					*newSecret("someSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+					*newSecret("zSecret", withAnnotations(map[string]string{corev1.ServiceAccountNameKey: serviceAccountName})),
+				},
+			},
+			wantedSecretNames: []string{"aSecret", "someSecret", "zSecret"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := filterSecretsBySAName(serviceAccountName, tt.secrets)
+			require.Equal(t, len(tt.wantedSecretNames), len(got))
+			for _, wantedSecretName := range tt.wantedSecretNames {
+				require.NotNil(t, got[wantedSecretName])
+				require.Equal(t, wantedSecretName, got[wantedSecretName].GetName())
+			}
+		})
+	}
+}
+
+type secretOption func(*corev1.Secret)
+
+func withAnnotations(annotations map[string]string) secretOption {
+	return func(s *corev1.Secret) {
+		s.SetAnnotations(annotations)
+	}
+}
+
+func newSecret(name string, opts ...secretOption) *corev1.Secret {
+	s := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: name,
+		},
+	}
+	for _, opt := range opts {
+		opt(s)
+	}
+	return s
+}

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() {