Allow non-CSV-owned ServiceAccounts to satisfy CSV requirements.

The existing check, intended to prevent a race condition that could
occur during CSV upgrades, marks a ServiceAccount as NotPresent when
it has at least one owner but is not owned by the current CSV. It's
expected for the ServiceAccount "default" not to be have an
ownerreference to a CSV, so the check can be relaxed to only consider
owners of kind ClusterServiceVersion.

This also combines two ServiceAccount checks that were independently
added to address the same race condition into a single check.

Signed-off-by: Ben Luddy <[email protected]>

Author: benluddy

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",
@@ -814,7 +815,7 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 					Version:    "v1",
 					Kind:       "ServiceAccount",
 					Name:       "sa",
-					Status:     v1alpha1.RequirementStatusReasonNotPresent,
+					Status:     v1alpha1.RequirementStatusReasonPresentNotSatisfied,
 					Dependents: []v1alpha1.DependentStatus{},
 				},
 				{"operators.coreos.com", "v1alpha1", "ClusterServiceVersion", "csv1"}: {
@@ -827,6 +828,85 @@ func TestRequirementAndPermissionStatus(t *testing.T) {
 			},
 			expectedError: nil,
 		},
+		{
+			description: "RequirementMet/ServiceAccountOwnedByNonCSV",
+			csv: csvWithUID(csv("csv",
+				namespace,
+				"0.0.0",
+				"",
+				installStrategy(
+					"csv-dep",
+					[]v1alpha1.StrategyDeploymentPermissions{
+						{
+							ServiceAccountName: "sa",
+						},
+					},
+					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"),
+						OwnerReferences: []metav1.OwnerReference{
+							{
+								Kind: v1alpha1.SubscriptionKind, // arbitrary non-CSV kind
+								UID:  "non-csv",
+							},
+						},
+					},
+				},
+				&rbacv1.Role{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "role",
+						Namespace: namespace,
+					},
+				},
+				&rbacv1.RoleBinding{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "roleBinding",
+						Namespace: namespace,
+					},
+					Subjects: []rbacv1.Subject{
+						{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: namespace,
+						},
+					},
+					RoleRef: rbacv1.RoleRef{
+						APIGroup: "rbac.authorization.k8s.io",
+						Kind:     "Role",
+						Name:     "role",
+					},
+				},
+			},
+			existingExtObjs: nil,
+			met:             true,
+			expectedRequirementStatuses: map[gvkn]v1alpha1.RequirementStatus{
+				{"", "v1", "ServiceAccount", "sa"}: {
+					Group:      "",
+					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,
+		},
 	}
 
 	for _, test := range tests {
@@ -843,7 +923,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 +935,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-")