Bug 1935909: Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements.

pkg/controller/operators/olm/requirements.go

@@ -271,18 +271,9 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *v1alpha1.Strategy
 			}
 			// Check SA's ownership
 			if ownerutil.IsOwnedByKind(sa, v1alpha1.ClusterServiceVersionKind) && !ownerutil.IsOwnedBy(sa, csv) {
-				met = false
-				status.Status = v1alpha1.RequirementStatusReasonNotPresent
-				status.Message = "Service account is stale"
-				statusesSet[saName] = status
-				continue
-			}
-
-			// Check if the ServiceAccount is owned by CSV
-			if len(sa.GetOwnerReferences()) != 0 && !ownerutil.IsOwnedBy(sa, csv) {
 				met = false
 				status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
-				status.Message = "Service account is not owned by this ClusterServiceVersion"
+				status.Message = "Service account is owned by another ClusterServiceVersion"
 				statusesSet[saName] = status
 				continue
 			}

pkg/controller/operators/olm/requirements_test.go

@@ -14,6 +14,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestRequirementAndPermissionStatus(t *testing.T) {
@@ -297,7 +298,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 					Version: "v1",
 					Kind:    "ServiceAccount",
 					Name:    "sa",
-					Status:  v1alpha1.RequirementStatusReasonPresent,
+					Status:  v1alpha1.RequirementStatusReasonPresentNotSatisfied,
 					Dependents: []v1alpha1.DependentStatus{
 						{
 							Group:   "rbac.authorization.k8s.io",
@@ -641,7 +642,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 					Version: "v1",
 					Kind:    "CustomResourceDefinition",
 					Name:    "c1.g1",
-					Status:  v1alpha1.RequirementStatusReasonNotAvailable,
+					Status:  v1alpha1.RequirementStatusReasonNotPresent,
 				},
 				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
 					Group:   "operators.coreos.com",
@@ -773,55 +774,124 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 						},
 					},
 				},
-				&rbacv1.Role{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "role",
-						Namespace: namespace,
-					},
-					Rules: []rbacv1.PolicyRule{
+			},
+			existingExtObjs: nil,
+			met:             false,
+			expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
+				{"", "v1", "ServiceAccount", "sa"}: {
+					Version:    "v1",
+					Kind:       "ServiceAccount",
+					Name:       "sa",
+					Status:     v1alpha1.RequirementStatusReasonPresentNotSatisfied,
+					Dependents: []v1alpha1.DependentStatus{},
+				},
+				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
+					Group:   "operators.coreos.com",
+					Version: "v1alpha1",
+					Kind:    "ClusterServiceVersion",
+					Name:    "csv1",
+					Status:  v1alpha1.RequirementStatusReasonPresent,
+				},
+			},
+			expectedError: nil,
+		},
+		{
+			description: "RequirementMet/ServiceAccountOwnedByNonCSV",
+			csv: csvWithUID(csv("csv",
+				namespace,
+				"0.0.0",
+				"",
+				installStrategy(
+					"csv-dep",
+					[]v1alpha1.StrategyDeploymentPermissions{
 						{
-							APIGroups: []string{""},
-							Verbs:     []string{"*"},
-							Resources: []string{"donuts"},
+							ServiceAccountName: "sa",
 						},
 					},
-				},
-				&rbacv1.RoleBinding{
+					nil,
+				),
+				nil,
+				nil,
+				v1alpha1.CSVPhasePending,
+			), types.UID("csv-uid")),
+			existingObjs: []runtime.Object{
+				&corev1.ServiceAccount{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "roleBinding",
+						Name:      "sa",
 						Namespace: namespace,
+						UID:       types.UID("sa"),
+						OwnerReferences: []metav1.OwnerReference{
+							{
+								Kind: v1alpha1.SubscriptionKind, // arbitrary non-CSV kind
+								UID:  "non-csv",
+							},
+						},
 					},
-					Subjects: []rbacv1.Subject{
+				},
+			},
+			existingExtObjs: nil,
+			met:             true,
+			expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
+				{"", "v1", "ServiceAccount", "sa"}: {
+					Version:    "v1",
+					Kind:       "ServiceAccount",
+					Name:       "sa",
+					Status:     v1alpha1.RequirementStatusReasonPresent,
+					Dependents: []v1alpha1.DependentStatus{},
+				},
+				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
+					Group:   "operators.coreos.com",
+					Version: "v1alpha1",
+					Kind:    "ClusterServiceVersion",
+					Name:    "csv",
+					Status:  v1alpha1.RequirementStatusReasonPresent,
+				},
+			},
+			expectedError: nil,
+		},
+		{
+			description: "RequirementMet/ServiceAccountHasNoOwner",
+			csv: csvWithUID(csv("csv",
+				namespace,
+				"0.0.0",
+				"",
+				installStrategy(
+					"csv-dep",
+					[]v1alpha1.StrategyDeploymentPermissions{
 						{
-							Kind:      "ServiceAccount",
-							APIGroup:  "",
-							Name:      "sa",
-							Namespace: namespace,
+							ServiceAccountName: "sa",
 						},
 					},
-					RoleRef: rbacv1.RoleRef{
-						APIGroup: "rbac.authorization.k8s.io",
-						Kind:     "Role",
-						Name:     "role",
+					nil,
+				),
+				nil,
+				nil,
+				v1alpha1.CSVPhasePending,
+			), types.UID("csv-uid")),
+			existingObjs: []runtime.Object{
+				&corev1.ServiceAccount{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sa",
+						Namespace: namespace,
+						UID:       types.UID("sa"),
 					},
 				},
 			},
 			existingExtObjs: nil,
-			met:             false,
+			met:             true,
 			expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
 				{"", "v1", "ServiceAccount", "sa"}: {
-					Group:      "",
 					Version:    "v1",
 					Kind:       "ServiceAccount",
 					Name:       "sa",
-					Status:     v1alpha1.RequirementStatusReasonNotPresent,
+					Status:     v1alpha1.RequirementStatusReasonPresent,
 					Dependents: []v1alpha1.DependentStatus{},
 				},
-				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
+				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv"}: {
 					Group:   "operators.coreos.com",
 					Version: "v1alpha1",
 					Kind:    "ClusterServiceVersion",
-					Name:    "csv1",
+					Name:    "csv",
 					Status:  v1alpha1.RequirementStatusReasonPresent,
 				},
 			},
@@ -843,7 +913,8 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 				require.Error(t, err)
 				require.EqualError(t, test.expectedError, err.Error())
 			}
-			require.Equal(t, test.met, met)
+			assert := assert.New(t)
+			assert.Equal(test.met, met)
 
 			for _, status := range statuses {
 				key := gvkn{
@@ -854,14 +925,15 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 				}
 
 				expected, ok := test.expectedRequirementStatuses[key]
-				require.True(t, ok, fmt.Sprintf("permission requirement status %+v found but not expected", key))
-				require.Len(t, status.Dependents, len(expected.Dependents), "number of dependents is not what was expected")
+				assert.True(ok, fmt.Sprintf("permission requirement status %+v found but not expected", key))
+				assert.Equal(expected.Status, status.Status)
+				assert.Len(status.Dependents, len(expected.Dependents), "number of dependents is not what was expected")
 
 				// Delete the requirement status to mark as found
 				delete(test.expectedRequirementStatuses, key)
 			}
 
-			require.Len(t, test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
+			assert.Len(test.expectedRequirementStatuses, 0, "not all expected permission requirement statuses were found")
 		})
 	}
 }

test/e2e/csv_e2e_test.go

@@ -127,6 +127,113 @@ var _ = Describe("ClusterServiceVersion", func() {
 		})
 	})
 
+	When("a csv requires a serviceaccount solely owned by a non-csv", func() {
+		var (
+			cm  corev1.ConfigMap
+			sa  corev1.ServiceAccount
+			csv v1alpha1.ClusterServiceVersion
+		)
+
+		BeforeEach(func() {
+			cm = corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "cm-",
+					Namespace:    testNamespace,
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), &cm)).To(Succeed())
+
+			sa = corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "sa-",
+					Namespace:    testNamespace,
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							Name:       cm.GetName(),
+							APIVersion: corev1.SchemeGroupVersion.String(),
+							Kind:       "ConfigMap",
+							UID:        cm.GetUID(),
+						},
+					},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), &sa)).To(Succeed())
+
+			csv = v1alpha1.ClusterServiceVersion{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       v1alpha1.ClusterServiceVersionKind,
+					APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "csv-",
+					Namespace:    testNamespace,
+				},
+				Spec: v1alpha1.ClusterServiceVersionSpec{
+					InstallStrategy: v1alpha1.NamedInstallStrategy{
+						StrategyName: v1alpha1.InstallStrategyNameDeployment,
+						StrategySpec: v1alpha1.StrategyDetailsDeployment{
+							DeploymentSpecs: []v1alpha1.StrategyDeploymentSpec{
+								{
+									Name: "foo",
+									Spec: appsv1.DeploymentSpec{
+										Selector: &metav1.LabelSelector{
+											MatchLabels: map[string]string{"app": "foo"},
+										},
+										Template: corev1.PodTemplateSpec{
+											ObjectMeta: metav1.ObjectMeta{
+												Labels: map[string]string{"app": "foo"},
+											},
+											Spec: corev1.PodSpec{Containers: []corev1.Container{
+												{
+													Name:  genName("foo"),
+													Image: *dummyImage,
+												},
+											}},
+										},
+									},
+								},
+							},
+							Permissions: []v1alpha1.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: sa.GetName(),
+									Rules:              []rbacv1.PolicyRule{},
+								},
+							},
+						},
+					},
+					InstallModes: []v1alpha1.InstallMode{
+						{
+							Type:      v1alpha1.InstallModeTypeAllNamespaces,
+							Supported: true,
+						},
+					},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), &csv)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			if cm.GetName() != "" {
+				Expect(ctx.Ctx().Client().Delete(context.Background(), &cm)).To(Succeed())
+			}
+		})
+
+		It("considers the serviceaccount requirement satisfied", func() {
+			Eventually(func() (v1alpha1.StatusReason, error) {
+				if err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(&csv), &csv); err != nil {
+					return "", err
+				}
+				for _, requirement := range csv.Status.RequirementStatus {
+					if requirement.Name != sa.GetName() {
+						continue
+					}
+					return requirement.Status, nil
+				}
+				return "", fmt.Errorf("missing expected requirement %q", sa.GetName())
+			}).Should(Equal(v1alpha1.RequirementStatusReasonPresent))
+		})
+	})
+
 	It("create with unmet requirements min kube version", func() {
 
 		depName := genName("dep-")