Merge pull request #1549 from ecordell/rbac-fix

Bug 1838054: fix(catalog): no operatorgroups in a namespace should be an error when resolving

pkg/controller/operators/catalog/operator.go

@@ -1170,6 +1170,28 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 		return
 	}
 
+	querier := o.serviceAccountQuerier.NamespaceQuerier(plan.GetNamespace())
+	ref, err := querier()
+	if err != nil {
+		syncError = fmt.Errorf("attenuated service account query failed - %v", err)
+		return
+	}
+
+	if ref != nil {
+		out := plan.DeepCopy()
+		out.Status.AttenuatedServiceAccountRef = ref
+
+		if !reflect.DeepEqual(plan, out) {
+			if _, updateErr := o.client.OperatorsV1alpha1().InstallPlans(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); updateErr != nil {
+				syncError = fmt.Errorf("failed to attach attenuated ServiceAccount to status - %v", updateErr)
+				return
+			}
+
+			logger.WithField("attenuated-sa", ref.Name).Info("successfully attached attenuated ServiceAccount to status")
+			return
+		}
+	}
+
 	// Attempt to unpack bundles before installing
 	// Note: This should probably use the attenuated client to prevent users from resolving resources they otherwise don't have access to.
 	if len(plan.Status.BundleLookups) > 0 {
@@ -1201,28 +1223,6 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 		}
 	}
 
-	querier := o.serviceAccountQuerier.NamespaceQuerier(plan.GetNamespace())
-	ref, err := querier()
-	if err != nil {
-		syncError = fmt.Errorf("attenuated service account query failed - %v", err)
-		return
-	}
-
-	if ref != nil {
-		out := plan.DeepCopy()
-		out.Status.AttenuatedServiceAccountRef = ref
-
-		if !reflect.DeepEqual(plan, out) {
-			if _, updateErr := o.client.OperatorsV1alpha1().InstallPlans(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); updateErr != nil {
-				syncError = fmt.Errorf("failed to attach attenuated ServiceAccount to status - %v", updateErr)
-				return
-			}
-
-			logger.WithField("attenuated-sa", ref.Name).Info("successfully attached attenuated ServiceAccount to status")
-			return
-		}
-	}
-
 	outInstallPlan, syncError := transitionInstallPlanState(logger.Logger, o, *plan, o.now())
 
 	if syncError != nil {

pkg/controller/operators/catalog/operator_test.go

@@ -135,6 +135,57 @@ func TestTransitionInstallPlan(t *testing.T) {
 	}
 }
 
+func TestSyncInstallPlanUnhappy(t *testing.T) {
+	namespace := "ns"
+
+	tests := []struct {
+		testName string
+		in       *v1alpha1.InstallPlan
+		err      error
+	}{
+		{
+			testName: "NoStatus",
+			in:       installPlan("p", namespace, v1alpha1.InstallPlanPhaseNone),
+			err:      nil,
+		},
+		{
+			// This checks that installplans are not applied when no operatorgroup is present
+			testName: "HasSteps/NoOperatorGroup",
+			in: withSteps(installPlan("p", namespace, v1alpha1.InstallPlanPhaseInstalling, "csv"),
+				[]*v1alpha1.Step{
+					{
+						Resource: v1alpha1.StepResource{
+							CatalogSource:          "catalog",
+							CatalogSourceNamespace: namespace,
+							Group:                  "",
+							Version:                "v1",
+							Kind:                   "ServiceAccount",
+							Name:                   "sa",
+							Manifest: toManifest(t, serviceAccount("sa", namespace, "",
+								objectReference("init secret"))),
+						},
+						Status: v1alpha1.StepStatusUnknown,
+					},
+				},
+			),
+			err:  fmt.Errorf("attenuated service account query failed - no operator group found that is managing this namespace"),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.testName, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+
+			op, err := NewFakeOperator(ctx, namespace, []string{namespace}, withClientObjs(tt.in))
+			require.NoError(t, err)
+
+			err = op.syncInstallPlans(tt.in)
+			require.Equal(t, tt.err, err)
+		})
+	}
+}
+
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"
 

pkg/lib/scoped/querier.go

@@ -35,27 +35,27 @@ func (f *UserDefinedServiceAccountQuerier) NamespaceQuerier(namespace string) Se
 			logFieldName: logFieldValue,
 		})
 
-		return QueryServiceAccountFromNamespace(logger, f.crclient, namespace)
+		return queryServiceAccountFromNamespace(logger, f.crclient, namespace)
 	}
 
 	return querierFunc
 }
 
 // QueryServiceAccountFromNamespace will return the reference to a service account
 // associated with the operator group for the given namespace.
-// - If no operator group is found in the namespace, both reference and err are set to nil.
+// - If no operator group is found in the namespace, an error is returned.
 // - If an operator group found is not managing the namespace then it is ignored.
 // - If no operator group is managing this namespace then both reference and err are set to nil.
 // - If more than one operator group are managing this namespace then an error is thrown.
-func QueryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.Interface, namespace string) (reference *corev1.ObjectReference, err error) {
+func queryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.Interface, namespace string) (reference *corev1.ObjectReference, err error) {
 	// TODO: use a lister instead of a noncached client here.
 	list, err := crclient.OperatorsV1().OperatorGroups(namespace).List(context.TODO(), metav1.ListOptions{})
 	if err != nil {
 		return
 	}
 
 	if len(list.Items) == 0 {
-		logger.Warnf("list query returned an empty list")
+		err = fmt.Errorf("no operator group found that is managing this namespace")
 		return
 	}
 
@@ -70,7 +70,7 @@ func QueryServiceAccountFromNamespace(logger *logrus.Entry, crclient versioned.I
 	}
 
 	if len(groups) == 0 {
-		logger.Warn("no operator group found that is managing this namespace")
+		err = fmt.Errorf("no operator group found that is managing this namespace")
 		return
 	}
 

pkg/lib/scoped/querier_test.go

@@ -0,0 +1,159 @@
+package scoped
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	v1 "github.com/operator-framework/api/pkg/operators/v1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
+)
+
+func TestUserDefinedServiceAccountQuerier(t *testing.T) {
+	tests := []struct {
+		name          string
+		crclient      versioned.Interface
+		namespace     string
+		wantReference *corev1.ObjectReference
+		wantErr       bool
+		err           error
+	}{
+		{
+			name:      "NoOperatorGroup",
+			crclient:  fake.NewSimpleClientset(),
+			namespace: "ns",
+			wantErr:   true,
+			err:       fmt.Errorf("no operator group found that is managing this namespace"),
+		},
+		{
+			name: "OperatorGroup/NamespaceNotInSpec",
+			crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: "ns",
+				},
+				Spec: v1.OperatorGroupSpec{
+					TargetNamespaces: []string{"other"},
+				},
+			}),
+			namespace: "ns",
+			wantErr:   true,
+			err:       fmt.Errorf("no operator group found that is managing this namespace"),
+		},
+		{
+			name: "OperatorGroup/NamespaceNotInStatus",
+			crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: "ns",
+				},
+				Spec: v1.OperatorGroupSpec{
+					TargetNamespaces: []string{"ns"},
+				},
+			}),
+			namespace: "ns",
+			wantErr:   true,
+			err:       fmt.Errorf("no operator group found that is managing this namespace"),
+		},
+		{
+			name: "OperatorGroup/Multiple",
+			crclient: fake.NewSimpleClientset(
+				&v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "og",
+						Namespace: "ns",
+					},
+					Spec: v1.OperatorGroupSpec{
+						TargetNamespaces: []string{"ns"},
+					},
+					Status: v1.OperatorGroupStatus{
+						Namespaces: []string{"ns"},
+					},
+				},
+				&v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "og2",
+						Namespace: "ns",
+					},
+					Spec: v1.OperatorGroupSpec{
+						TargetNamespaces: []string{"ns"},
+					},
+					Status: v1.OperatorGroupStatus{
+						Namespaces: []string{"ns"},
+					},
+				},
+			),
+			namespace: "ns",
+			wantErr:   true,
+			err:       fmt.Errorf("more than one operator group(s) are managing this namespace count=2"),
+		},
+		{
+			name: "OperatorGroup/NamespaceInStatus/NoSA",
+			crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: "ns",
+				},
+				Spec: v1.OperatorGroupSpec{
+					TargetNamespaces: []string{"ns"},
+				},
+				Status: v1.OperatorGroupStatus{
+					Namespaces: []string{"ns"},
+				},
+			}),
+			namespace: "ns",
+			wantErr:   false,
+			err:       nil,
+		},
+		{
+			name: "OperatorGroup/NamespaceInStatus/ServiceAccountRef",
+			crclient: fake.NewSimpleClientset(&v1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: "ns",
+				},
+				Spec: v1.OperatorGroupSpec{
+					TargetNamespaces:   []string{"ns"},
+					ServiceAccountName: "sa",
+				},
+				Status: v1.OperatorGroupStatus{
+					Namespaces: []string{"ns"},
+					ServiceAccountRef: &corev1.ObjectReference{
+						Kind:      "ServiceAccount",
+						Namespace: "ns",
+						Name:      "sa",
+					},
+				},
+			}),
+			namespace: "ns",
+			wantErr:   false,
+			err:       nil,
+			wantReference: &corev1.ObjectReference{
+				Kind:      "ServiceAccount",
+				Namespace: "ns",
+				Name:      "sa",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			logger, _ := test.NewNullLogger()
+			f := &UserDefinedServiceAccountQuerier{
+				crclient: tt.crclient,
+				logger:   logger,
+			}
+			gotReference, err := f.NamespaceQuerier(tt.namespace)()
+			if tt.wantErr {
+				require.Equal(t, tt.err, err)
+			} else {
+				require.Nil(t, tt.err)
+			}
+			require.Equal(t, tt.wantReference, gotReference)
+		})
+	}
+}