Merge pull request #2209 from dinhxuanvu/invalid-label-selector

[release-4.6] Bug 1975456: Handle invalid label during resource cleanup

Author: openshift-merge-robot

pkg/controller/install/webhook.go

@@ -78,7 +78,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
 func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
 	webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
 	webhookLabels[WebhookDescKey] = desc.GenerateName
-	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+	webhookSelector := labels.SelectorFromValidatedSet(webhookLabels).String()
 
 	existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 	if err != nil {
@@ -124,7 +124,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
 func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
 	webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
 	webhookLabels[WebhookDescKey] = desc.GenerateName
-	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+	webhookSelector := labels.SelectorFromValidatedSet(webhookLabels).String()
 
 	existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 	if err != nil {

pkg/controller/operators/olm/apiservices.go

@@ -264,7 +264,7 @@ func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
 	for _, desc := range csv.Spec.WebhookDefinitions {
 		webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
 		webhookLabels[install.WebhookDescKey] = desc.GenerateName
-		webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+		webhookSelector := labels.SelectorFromValidatedSet(webhookLabels).String()
 
 		switch desc.Type {
 		case v1alpha1.MutatingAdmissionWebhook:
@@ -493,7 +493,7 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
 
 func (a *Operator) cleanUpRemovedWebhooks(csv *v1alpha1.ClusterServiceVersion) error {
 	webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
-	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+	webhookSelector := labels.SelectorFromValidatedSet(webhookLabels).String()
 
 	csvWebhookGenerateNames := make(map[string]struct{}, len(csv.Spec.WebhookDefinitions))
 	for _, webhook := range csv.Spec.WebhookDefinitions {
@@ -549,7 +549,7 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo
 		webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
 		webhookLabels[install.WebhookDescKey] = desc.GenerateName
 		webhookLabels[install.WebhookHashKey] = install.HashWebhookDesc(desc)
-		webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+		webhookSelector := labels.SelectorFromValidatedSet(webhookLabels).String()
 
 		webhookCount := 0
 		switch desc.Type {

pkg/controller/operators/olm/operator.go

@@ -968,7 +968,7 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 		}
 	}
 
-	webhookSelector := labels.SelectorFromSet(ownerutil.OwnerLabel(clusterServiceVersion, v1alpha1.ClusterServiceVersionKind)).String()
+	webhookSelector := labels.SelectorFromValidatedSet(ownerutil.OwnerLabel(clusterServiceVersion, v1alpha1.ClusterServiceVersionKind)).String()
 	mWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 	if err != nil {
 		logger.WithError(err).Warn("cannot list MutatingWebhookConfigurations")

pkg/controller/operators/olm/operatorgroup.go

@@ -185,7 +185,7 @@ func (a *Operator) operatorGroupDeleted(obj interface{}) {
 		"namespace":     op.GetNamespace(),
 	})
 
-	clusterRoles, err := a.lister.RbacV1().ClusterRoleLister().List(labels.SelectorFromSet(ownerutil.OwnerLabel(op, "OperatorGroup")))
+	clusterRoles, err := a.lister.RbacV1().ClusterRoleLister().List(labels.SelectorFromValidatedSet(ownerutil.OwnerLabel(op, "OperatorGroup")))
 	if err != nil {
 		logger.WithError(err).Error("failed to list ClusterRoles for garbage collection")
 		return

pkg/controller/registry/reconciler/configmap.go

@@ -208,7 +208,7 @@ func (c *ConfigMapRegistryReconciler) currentRoleBinding(source configMapCatalog
 
 func (c *ConfigMapRegistryReconciler) currentPods(source configMapCatalogSourceDecorator, image string) []*v1.Pod {
 	podName := source.Pod(image).GetName()
-	pods, err := c.Lister.CoreV1().PodLister().Pods(source.GetNamespace()).List(labels.SelectorFromSet(source.Selector()))
+	pods, err := c.Lister.CoreV1().PodLister().Pods(source.GetNamespace()).List(labels.SelectorFromValidatedSet(source.Selector()))
 	if err != nil {
 		logrus.WithField("pod", podName).WithError(err).Debug("couldn't find pod in cache")
 		return nil

pkg/controller/registry/reconciler/configmap_test.go

@@ -459,7 +459,7 @@ func TestConfigMapRegistryReconciler(t *testing.T) {
 			decorated := configMapCatalogSourceDecorator{tt.in.catsrc}
 
 			pod := decorated.Pod(registryImageName)
-			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
+			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromValidatedSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
 			outPods, err := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(context.TODO(), listOptions)
 			require.NoError(t, err)
 			require.Len(t, outPods.Items, 1)

pkg/controller/registry/reconciler/grpc_test.go

@@ -208,7 +208,7 @@ func TestGrpcRegistryReconciler(t *testing.T) {
 			decorated := grpcCatalogSourceDecorator{tt.in.catsrc}
 			pod := decorated.Pod()
 			service := decorated.Service()
-			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
+			listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromValidatedSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()}
 			outPods, podErr := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(context.TODO(), listOptions)
 			outService, serviceErr := client.KubernetesInterface().CoreV1().Services(service.GetNamespace()).Get(context.TODO(), service.GetName(), metav1.GetOptions{})
 

pkg/lib/ownerutil/util.go

@@ -268,7 +268,7 @@ func AdoptableLabels(labels map[string]string, checkName bool, targets ...Owner)
 
 // CSVOwnerSelector returns a label selector to find generated objects owned by owner
 func CSVOwnerSelector(owner *operatorsv1alpha1.ClusterServiceVersion) labels.Selector {
-	return labels.SelectorFromSet(OwnerLabel(owner, operatorsv1alpha1.ClusterServiceVersionKind))
+	return labels.SelectorFromValidatedSet(OwnerLabel(owner, operatorsv1alpha1.ClusterServiceVersionKind))
 }
 
 // AddOwner adds an owner to the ownerref list.

pkg/lib/ownerutil/util_test.go

@@ -1,7 +1,66 @@
 package ownerutil
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
+)
 
 func TestIsOwnedBy(t *testing.T) {
 	return
 }
+
+func TestCSVOwnerSelector(t *testing.T) {
+	csvType := metav1.TypeMeta{
+		Kind:       operatorsv1alpha1.ClusterServiceVersionKind,
+		APIVersion: operatorsv1alpha1.GroupVersion,
+	}
+
+	tests := []struct {
+		name string
+		csv  *operatorsv1alpha1.ClusterServiceVersion
+	}{
+		{
+			name: "CSV with name longer than 63 characters",
+			csv: &operatorsv1alpha1.ClusterServiceVersion{
+				TypeMeta: csvType,
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "clusterkubedescheduleroperator.4.6.0-202106010807.p0.git.5db84c5",
+					Namespace: "test-namespace",
+				},
+			},
+		},
+		{
+			name: "CSV with invalid name",
+			csv: &operatorsv1alpha1.ClusterServiceVersion{
+				TypeMeta: csvType,
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "something@somewhere",
+					Namespace: "test-namespace",
+				},
+			},
+		},
+		{
+			name: "CSV with empty string name",
+			csv: &operatorsv1alpha1.ClusterServiceVersion{
+				TypeMeta: csvType,
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "",
+					Namespace: "test-namespace",
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			selector := CSVOwnerSelector(tt.csv)
+
+			assert.NotNil(t, selector)
+			assert.False(t, selector.Empty())
+		})
+	}
+}

test/e2e/catalog_e2e_test.go

@@ -272,13 +272,13 @@ var _ = Describe("Catalog", func() {
 
 		// Await 1 CatalogSource registry pod matching the updated labels
 		singlePod := podCount(1)
-		selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": mainCatalogName, "olm.configMapResourceVersion": updatedConfigMap.GetResourceVersion()})
+		selector := labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": mainCatalogName, "olm.configMapResourceVersion": updatedConfigMap.GetResourceVersion()})
 		podList, err := awaitPods(GinkgoT(), c, testNamespace, selector.String(), singlePod)
 		require.NoError(GinkgoT(), err)
 		require.Equal(GinkgoT(), 1, len(podList.Items), "expected pod list not of length 1")
 
 		// Await 1 CatalogSource registry pod matching the updated labels
-		selector = labels.SelectorFromSet(map[string]string{"olm.catalogSource": mainCatalogName})
+		selector = labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": mainCatalogName})
 		podList, err = awaitPods(GinkgoT(), c, testNamespace, selector.String(), singlePod)
 		require.NoError(GinkgoT(), err)
 		require.Equal(GinkgoT(), 1, len(podList.Items), "expected pod list not of length 1")
@@ -538,7 +538,7 @@ var _ = Describe("Catalog", func() {
 		defer cleanupSource()
 
 		// Wait for a new registry pod to be created
-		selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": sourceName})
+		selector := labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": sourceName})
 		singlePod := podCount(1)
 		registryPods, err := awaitPods(GinkgoT(), c, testNamespace, selector.String(), singlePod)
 		require.NoError(GinkgoT(), err, "error awaiting registry pod")
@@ -602,7 +602,7 @@ var _ = Describe("Catalog", func() {
 
 		// Wait for a new registry pod to be created
 		c := newKubeClient()
-		selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()})
+		selector := labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": source.GetName()})
 		singlePod := podCount(1)
 		registryPods, err := awaitPods(GinkgoT(), c, source.GetNamespace(), selector.String(), singlePod)
 		require.NoError(GinkgoT(), err, "error awaiting registry pod")
@@ -753,7 +753,7 @@ var _ = Describe("Catalog", func() {
 
 		// wait for new catalog source pod to be created
 		// Wait for a new registry pod to be created
-		selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()})
+		selector := labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": source.GetName()})
 		singlePod := podCount(1)
 		registryPods, err := awaitPods(GinkgoT(), c, source.GetNamespace(), selector.String(), singlePod)
 		require.NoError(GinkgoT(), err, "error awaiting registry pod")
@@ -1020,7 +1020,7 @@ var _ = Describe("Catalog", func() {
 		Expect(err).ToNot(HaveOccurred())
 
 		// wait for new catalog source pod to be created and report ready
-		selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()})
+		selector := labels.SelectorFromValidatedSet(map[string]string{"olm.catalogSource": source.GetName()})
 		singlePod := podCount(1)
 		catalogPods, err := awaitPods(GinkgoT(), c, source.GetNamespace(), selector.String(), singlePod)
 		Expect(err).ToNot(HaveOccurred())
@@ -1043,7 +1043,7 @@ var _ = Describe("Catalog", func() {
 		}).Should(BeTrue())
 
 		// Wait roughly the polling interval for update pod to show up
-		updateSelector := labels.SelectorFromSet(map[string]string{"catalogsource.operators.coreos.com/update": source.GetName()})
+		updateSelector := labels.SelectorFromValidatedSet(map[string]string{"catalogsource.operators.coreos.com/update": source.GetName()})
 		updatePods, err := awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), updateSelector.String(), 5*time.Second, 2*time.Minute, singlePod)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(updatePods).ToNot(BeNil())

test/e2e/webhook_e2e_test.go

@@ -900,7 +900,7 @@ var _ = Describe("CSVs with a Webhook", func() {
 })
 
 func getWebhookWithGenerateName(c operatorclient.ClientInterface, generateName string) (*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
-	webhookSelector := labels.SelectorFromSet(map[string]string{install.WebhookDescKey: generateName}).String()
+	webhookSelector := labels.SelectorFromValidatedSet(map[string]string{install.WebhookDescKey: generateName}).String()
 	existingWebhooks, err := c.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 	if err != nil {
 		return nil, err