Merge pull request #1267 from ecordell/fix-bad-opgroup-annotations

Bug 1789920: Fix bad opgroup annotations

Author: openshift-merge-robot

pkg/controller/operators/olm/operator.go

@@ -910,6 +910,11 @@ func (a *Operator) removeDanglingChildCSVs(csv *v1alpha1.ClusterServiceVersion)
 		}
 	}
 
+	if parent.GetNamespace() == csv.GetNamespace() {
+		logger.Debug("deleting copied CSV since it has incorrect parent annotations")
+		return a.deleteChild(csv, logger)
+	}
+
 	return nil
 }
 

pkg/controller/operators/olm/operatorgroup.go

@@ -183,6 +183,9 @@ func (a *Operator) annotateCSVs(group *v1.OperatorGroup, targetNamespaces []stri
 	targetNamespaceSet := resolver.NewNamespaceSet(targetNamespaces)
 
 	for _, csv := range a.csvSet(group.GetNamespace(), v1alpha1.CSVPhaseAny) {
+		if csv.IsCopied() {
+			continue
+		}
 		logger := logger.WithField("csv", csv.GetName())
 
 		originalNamespacesAnnotation, _ := a.copyOperatorGroupAnnotations(&csv.ObjectMeta)[v1.OperatorGroupTargetsAnnotationKey]

test/e2e/operator_groups_e2e_test.go

@@ -884,7 +884,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
 	// Generate operatorGroupD in namespaceD that selects namespace D and E
 	// Generate csvD in namespaceD
 	// Wait for csvD to be successful
-	// Wait for csvD to have a CSV with copied status in namespace D
+	// Wait for csvD to have a CSV with copied status in namespace E
 	// Wait for operatorGroupD to have providedAPI annotation with crdD's Kind.version.group
 	// Generate operatorGroupA in namespaceA that selects AllNamespaces
 	// Generate csvD in namespaceA
@@ -895,6 +895,8 @@ func TestOperatorGroupIntersection(t *testing.T) {
 	// Wait for csvA to be successful
 	// Ensure clusterroles created and aggregated for accessing provided APIs
 	// Wait for operatorGroupA to have providedAPI annotation with crdA's Kind.version.group in its providedAPIs annotation
+	// Wait for csvA to have a CSV with copied status in namespace D
+	// Ensure csvA retains the operatorgroup annotations for operatorgroupA
 	// Wait for csvA to have a CSV with copied status in namespace C
 	// Generate operatorGroupB in namespaceB that selects namespace C
 	// Generate csvB in namespaceB that owns crdA
@@ -1091,6 +1093,26 @@ func TestOperatorGroupIntersection(t *testing.T) {
 	}
 	require.NoError(t, awaitAnnotations(t, q, map[string]string{v1.OperatorGroupProvidedAPIsAnnotationKey: kvgA}))
 
+	// Wait for csvA to have a CSV with copied status in namespace D
+	csvAinNsD, err := awaitCSV(t, crc, nsD, csvA.GetName(), csvCopiedChecker)
+	require.NoError(t, err)
+
+	// trigger a resync of operatorgropuD
+	fetchedGroupD, err := crc.OperatorsV1().OperatorGroups(nsD).Get(groupD.GetName(), metav1.GetOptions{})
+	require.NoError(t, err)
+
+	fetchedGroupD.Annotations["bump"] = "update"
+	_, err = crc.OperatorsV1().OperatorGroups(nsD).Update(fetchedGroupD)
+	require.NoError(t, err)
+
+	// Ensure csvA retains the operatorgroup annotations for operatorgroupA
+	csvAinNsD, err = awaitCSV(t, crc, nsD, csvA.GetName(), csvCopiedChecker)
+	require.NoError(t, err)
+
+	require.Equal(t, groupA.GetName(), csvAinNsD.Annotations[v1.OperatorGroupAnnotationKey])
+	require.Equal(t, nsA, csvAinNsD.Annotations[v1.OperatorGroupNamespaceAnnotationKey])
+	require.Equal(t, nsA, csvAinNsD.Labels[v1alpha1.CopiedLabelKey])
+
 	// Await csvA's copy in namespaceC
 	_, err = awaitCSV(t, crc, nsC, csvA.GetName(), csvCopiedChecker)
 	require.NoError(t, err)
@@ -1849,3 +1871,253 @@ func TestOperatorGroupInsufficientPermissionsResolveViaServiceAccountRemoval(t *
 	})
 	require.NoError(t, err)
 }
+
+// Versions of OLM at 0.14.1 and older had a bug that would place the wrong namespace annotation on copied CSVs,
+// preventing them from being GCd. This ensures that any leftover CSVs in that state are properly cleared up.
+func TestCleanupCsvsWithBadOwnerOperatorGroups(t *testing.T) {
+	defer cleaner.NotifyTestComplete(t, true)
+	c := newKubeClient(t)
+	crc := newCRClient(t)
+	csvName := genName("another-csv-") // must be lowercase for DNS-1123 validation
+
+	opGroupNamespace := testNamespace
+	matchingLabel := map[string]string{"inGroup": opGroupNamespace}
+	otherNamespaceName := genName(opGroupNamespace + "-")
+
+	t.Log("Creating CRD")
+	mainCRDPlural := genName("opgroup-")
+	mainCRD := newCRD(mainCRDPlural)
+	cleanupCRD, err := createCRD(c, mainCRD)
+	require.NoError(t, err)
+	defer cleanupCRD()
+
+	t.Logf("Getting default operator group 'global-operators' installed via operatorgroup-default.yaml %v", opGroupNamespace)
+	operatorGroup, err := crc.OperatorsV1().OperatorGroups(opGroupNamespace).Get("global-operators", metav1.GetOptions{})
+	require.NoError(t, err)
+
+	expectedOperatorGroupStatus := v1.OperatorGroupStatus{
+		Namespaces: []string{metav1.NamespaceAll},
+	}
+
+	t.Log("Waiting on operator group to have correct status")
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetched, fetchErr := crc.OperatorsV1().OperatorGroups(opGroupNamespace).Get(operatorGroup.Name, metav1.GetOptions{})
+		if fetchErr != nil {
+			return false, fetchErr
+		}
+		if len(fetched.Status.Namespaces) > 0 {
+			require.ElementsMatch(t, expectedOperatorGroupStatus.Namespaces, fetched.Status.Namespaces)
+			fmt.Println(fetched.Status.Namespaces)
+			return true, nil
+		}
+		return false, nil
+	})
+	require.NoError(t, err)
+
+	t.Log("Creating CSV")
+	// Generate permissions
+	serviceAccountName := genName("nginx-sa")
+	permissions := []v1alpha1.StrategyDeploymentPermissions{
+		{
+			ServiceAccountName: serviceAccountName,
+			Rules: []rbacv1.PolicyRule{
+				{
+					Verbs:     []string{rbacv1.VerbAll},
+					APIGroups: []string{mainCRD.Spec.Group},
+					Resources: []string{mainCRDPlural},
+				},
+			},
+		},
+	}
+
+	serviceAccount := &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: opGroupNamespace,
+			Name:      serviceAccountName,
+		},
+	}
+	role := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: opGroupNamespace,
+			Name:      serviceAccountName + "-role",
+		},
+		Rules: permissions[0].Rules,
+	}
+	roleBinding := &rbacv1.RoleBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: opGroupNamespace,
+			Name:      serviceAccountName + "-rb",
+		},
+		Subjects: []rbacv1.Subject{
+			{
+				Kind:      "ServiceAccount",
+				Name:      serviceAccountName,
+				Namespace: opGroupNamespace,
+			},
+		},
+		RoleRef: rbacv1.RoleRef{
+			Kind: "Role",
+			Name: role.GetName(),
+		},
+	}
+	_, err = c.CreateServiceAccount(serviceAccount)
+	require.NoError(t, err)
+	defer func() {
+		c.DeleteServiceAccount(serviceAccount.GetNamespace(), serviceAccount.GetName(), metav1.NewDeleteOptions(0))
+	}()
+	createdRole, err := c.CreateRole(role)
+	require.NoError(t, err)
+	defer func() {
+		c.DeleteRole(role.GetNamespace(), role.GetName(), metav1.NewDeleteOptions(0))
+	}()
+	createdRoleBinding, err := c.CreateRoleBinding(roleBinding)
+	require.NoError(t, err)
+	defer func() {
+		c.DeleteRoleBinding(roleBinding.GetNamespace(), roleBinding.GetName(), metav1.NewDeleteOptions(0))
+	}()
+	// Create a new NamedInstallStrategy
+	deploymentName := genName("operator-deployment")
+	namedStrategy := newNginxInstallStrategy(deploymentName, permissions, nil)
+
+	aCSV := newCSV(csvName, opGroupNamespace, "", semver.MustParse("0.0.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, namedStrategy)
+	aCSV.Labels = map[string]string{"label": t.Name()}
+	createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Create(&aCSV)
+	require.NoError(t, err)
+
+	err = ownerutil.AddOwnerLabels(createdRole, createdCSV)
+	require.NoError(t, err)
+	_, err = c.UpdateRole(createdRole)
+	require.NoError(t, err)
+
+	err = ownerutil.AddOwnerLabels(createdRoleBinding, createdCSV)
+	require.NoError(t, err)
+	_, err = c.UpdateRoleBinding(createdRoleBinding)
+	require.NoError(t, err)
+
+	t.Log("wait for CSV to succeed")
+	_, err = fetchCSV(t, crc, createdCSV.GetName(), opGroupNamespace, csvSucceededChecker)
+	require.NoError(t, err)
+
+	t.Log("wait for roles to be promoted to clusterroles")
+	var fetchedRole *rbacv1.ClusterRole
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedRole, err = c.GetClusterRole(role.GetName())
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return false, nil
+			}
+			return false, err
+		}
+		return true, nil
+	})
+	require.EqualValues(t, append(role.Rules, rbacv1.PolicyRule{
+		Verbs:     []string{"get", "list", "watch"},
+		APIGroups: []string{""},
+		Resources: []string{"namespaces"},
+	}), fetchedRole.Rules)
+	var fetchedRoleBinding *rbacv1.ClusterRoleBinding
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedRoleBinding, err = c.GetClusterRoleBinding(roleBinding.GetName())
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				return false, nil
+			}
+			return false, err
+		}
+		return true, nil
+	})
+	require.EqualValues(t, roleBinding.Subjects, fetchedRoleBinding.Subjects)
+	require.EqualValues(t, roleBinding.RoleRef.Name, fetchedRoleBinding.RoleRef.Name)
+	require.EqualValues(t, "rbac.authorization.k8s.io", fetchedRoleBinding.RoleRef.APIGroup)
+	require.EqualValues(t, "ClusterRole", fetchedRoleBinding.RoleRef.Kind)
+
+	t.Log("ensure operator was granted namespace list permission")
+	res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{
+		Spec: authorizationv1.SubjectAccessReviewSpec{
+			User: "system:serviceaccount:" + opGroupNamespace + ":" + serviceAccountName,
+			ResourceAttributes: &authorizationv1.ResourceAttributes{
+				Group:    corev1.GroupName,
+				Version:  "v1",
+				Resource: "namespaces",
+				Verb:     "list",
+			},
+		},
+	})
+	require.NoError(t, err)
+	require.True(t, res.Status.Allowed, "got %#v", res.Status)
+
+	t.Log("Waiting for operator namespace csv to have annotations")
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedCSV, fetchErr := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Get(csvName, metav1.GetOptions{})
+		if fetchErr != nil {
+			if errors.IsNotFound(fetchErr) {
+				return false, nil
+			}
+			t.Logf("Error (in %v): %v", testNamespace, fetchErr.Error())
+			return false, fetchErr
+		}
+		if checkOperatorGroupAnnotations(fetchedCSV, operatorGroup, true, corev1.NamespaceAll) == nil {
+			return true, nil
+		}
+		return false, nil
+	})
+	require.NoError(t, err)
+
+	csvList, err := crc.OperatorsV1alpha1().ClusterServiceVersions(corev1.NamespaceAll).List(metav1.ListOptions{LabelSelector: fmt.Sprintf("label=%s", t.Name())})
+	require.NoError(t, err)
+	t.Logf("Found CSV count of %v", len(csvList.Items))
+
+	t.Logf("Create other namespace %s", otherNamespaceName)
+	otherNamespace := corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   otherNamespaceName,
+			Labels: matchingLabel,
+		},
+	}
+	_, err = c.KubernetesInterface().CoreV1().Namespaces().Create(&otherNamespace)
+	require.NoError(t, err)
+	defer func() {
+		err = c.KubernetesInterface().CoreV1().Namespaces().Delete(otherNamespaceName, &metav1.DeleteOptions{})
+		require.NoError(t, err)
+	}()
+
+	t.Log("Waiting to ensure copied CSV shows up in other namespace")
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetchedCSV, fetchErr := crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Get(csvName, metav1.GetOptions{})
+		if fetchErr != nil {
+			if errors.IsNotFound(fetchErr) {
+				return false, nil
+			}
+			t.Logf("Error (in %v): %v", otherNamespaceName, fetchErr.Error())
+			return false, fetchErr
+		}
+		if checkOperatorGroupAnnotations(fetchedCSV, operatorGroup, false, "") == nil {
+			return true, nil
+		}
+		return false, nil
+	})
+	require.NoError(t, err)
+
+	// Give copied CSV a bad operatorgroup annotation
+	fetchedCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Get(csvName, metav1.GetOptions{})
+	require.NoError(t, err)
+	fetchedCSV.Annotations[v1.OperatorGroupNamespaceAnnotationKey] = fetchedCSV.GetNamespace()
+	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Update(fetchedCSV)
+	require.NoError(t, err)
+
+	// wait for CSV to be gc'd
+	err = wait.Poll(pollInterval, 2*pollDuration, func() (bool, error) {
+		csv, fetchErr := crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Get(csvName, metav1.GetOptions{})
+		if fetchErr != nil {
+			if errors.IsNotFound(fetchErr) {
+				return true, nil
+			}
+			t.Logf("Error (in %v): %v", opGroupNamespace, fetchErr.Error())
+			return false, fetchErr
+		}
+		t.Logf("%#v", csv.Annotations)
+		t.Logf(csv.GetNamespace())
+		return false, nil
+	})
+	require.NoError(t, err)
+}