feat(og): Project OperatorGroup errors to status conditions

Currently, errors associated with ServiceAccount not found or multiple
OperatorGroup in the same namespace are not recorded in OperatorGroup's
status. This PR will project those errors to OG's status conditions to
improve the UX.

Signed-off-by: Vu Dinh <[email protected]>

Author: dinhxuanvu

pkg/controller/operators/olm/operator.go

@@ -15,6 +15,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	meta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -827,6 +828,37 @@ func (a *Operator) syncNamespace(obj interface{}) error {
 		return err
 	}
 
+	// Query OG in this namespace
+	groups, err := a.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(namespace.GetName()).List(labels.Everything())
+
+	// Check if there is a stale multiple OG condition and clear it if existed.
+	if len(groups) == 1 {
+		og := groups[0]
+		if c := meta.FindStatusCondition(og.Status.Conditions, v1.MutlipleOperatorGroupCondition); c != nil {
+			meta.RemoveStatusCondition(&og.Status.Conditions, v1.MutlipleOperatorGroupCondition)
+			_, err = a.client.OperatorsV1().OperatorGroups(namespace.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
+			if err != nil {
+				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), c, err.Error())
+			}
+		}
+	} else if len(groups) > 1 {
+		// Add to all OG's status conditions to indicate they're multiple OGs in the
+		// same namespace which is not allowed.
+		cond := metav1.Condition{
+			Type:    v1.MutlipleOperatorGroupCondition,
+			Status:  metav1.ConditionTrue,
+			Reason:  v1.MultipleOperatorGroupsReason,
+			Message: "Multiple OperatorGroup found in the same namespace",
+		}
+		for _, og := range groups {
+			meta.SetStatusCondition(&og.Status.Conditions, cond)
+			_, err = a.client.OperatorsV1().OperatorGroups(namespace.GetName()).UpdateStatus(context.TODO(), og, metav1.UpdateOptions{})
+			if err != nil {
+				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", og.GetName(), cond, err.Error())
+			}
+		}
+	}
+
 	for _, group := range operatorGroupList {
 		namespaceSet := resolver.NewNamespaceSet(group.Status.Namespaces)
 

pkg/controller/operators/olm/operator_test.go

@@ -28,6 +28,7 @@ import (
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	meta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -4537,6 +4538,217 @@ func TestSyncOperatorGroups(t *testing.T) {
 	}
 }
 
+func TestOperatorGroupConditions(t *testing.T) {
+	logrus.SetLevel(logrus.DebugLevel)
+	clockFake := utilclock.NewFakeClock(time.Date(2006, time.January, 2, 15, 4, 5, 0, time.FixedZone("MST", -7*3600)))
+
+	operatorNamespace := "operator-ns"
+	opNamespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: operatorNamespace,
+		},
+	}
+	serviceAccount := serviceAccount("sa", operatorNamespace)
+
+	type initial struct {
+		operatorGroup *v1.OperatorGroup
+		clientObjs    []runtime.Object
+		k8sObjs       []runtime.Object
+	}
+
+	tests := []struct {
+		initial            initial
+		name               string
+		expectedConditions []metav1.Condition
+		expectError        bool
+	}{
+		{
+			name: "ValidOperatorGroup/NoServiceAccount",
+			initial: initial{
+				operatorGroup: &v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "operator-group-1",
+						Namespace: operatorNamespace,
+						UID:       "135e02a5-a7e2-44e7-abaa-88c63838993c",
+					},
+					Spec: v1.OperatorGroupSpec{
+						TargetNamespaces: []string{operatorNamespace},
+					},
+				},
+				k8sObjs: []runtime.Object{
+					&corev1.Namespace{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: operatorNamespace,
+						},
+					},
+				},
+			},
+			expectError:        false,
+			expectedConditions: []metav1.Condition{},
+		},
+		{
+			name: "ValidOperatorGroup/ValidServiceAccount",
+			initial: initial{
+				operatorGroup: &v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "operator-group-1",
+						Namespace: operatorNamespace,
+						UID:       "135e02a5-a7e2-44e7-abaa-88c63838993c",
+					},
+					Spec: v1.OperatorGroupSpec{
+						ServiceAccountName: "sa",
+						TargetNamespaces:   []string{operatorNamespace},
+					},
+				},
+				k8sObjs: []runtime.Object{
+					&corev1.Namespace{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: operatorNamespace,
+						},
+					},
+					serviceAccount,
+				},
+			},
+			expectError:        false,
+			expectedConditions: []metav1.Condition{},
+		},
+		{
+			name: "BadOperatorGroup/MissingServiceAccount",
+			initial: initial{
+				operatorGroup: &v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "operator-group-1",
+						Namespace: operatorNamespace,
+						UID:       "135e02a5-a7e2-44e7-abaa-88c63838993c",
+					},
+					Spec: v1.OperatorGroupSpec{
+						ServiceAccountName: "nonexistingSA",
+						TargetNamespaces:   []string{operatorNamespace},
+					},
+				},
+				k8sObjs: []runtime.Object{
+					&corev1.Namespace{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: operatorNamespace,
+						},
+					},
+				},
+			},
+			expectError: true,
+			expectedConditions: []metav1.Condition{
+				metav1.Condition{
+					Type:    v1.OperatorGroupServiceAccountCondition,
+					Status:  metav1.ConditionTrue,
+					Reason:  v1.OperatorGroupServiceAccountReason,
+					Message: "ServiceAccount nonexistingSA not found",
+				},
+			},
+		},
+		{
+			name: "BadOperatorGroup/MultipleOperatorGroups",
+			initial: initial{
+				operatorGroup: &v1.OperatorGroup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "operator-group-1",
+						Namespace: operatorNamespace,
+						UID:       "135e02a5-a7e2-44e7-abaa-88c63838993c",
+					},
+					Spec: v1.OperatorGroupSpec{
+						TargetNamespaces: []string{operatorNamespace},
+					},
+				},
+				clientObjs: []runtime.Object{
+					&v1.OperatorGroup{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "operator-group-2",
+							Namespace: operatorNamespace,
+							UID:       "cdc9643e-7c52-4f7c-ae75-28ccb6aec97d",
+						},
+						Spec: v1.OperatorGroupSpec{
+							TargetNamespaces: []string{operatorNamespace},
+						},
+					},
+				},
+				k8sObjs: []runtime.Object{
+					&corev1.Namespace{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: operatorNamespace,
+						},
+					},
+				},
+			},
+			expectError: true,
+			expectedConditions: []metav1.Condition{
+				metav1.Condition{
+					Type:    v1.MutlipleOperatorGroupCondition,
+					Status:  metav1.ConditionTrue,
+					Reason:  v1.MultipleOperatorGroupsReason,
+					Message: "Multiple OperatorGroup found in the same namespace",
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			namespaces := []string{}
+			// Pick out Namespaces
+			for _, obj := range tt.initial.k8sObjs {
+				if ns, ok := obj.(*corev1.Namespace); ok {
+					namespaces = append(namespaces, ns.GetName())
+				}
+			}
+
+			// Append operatorGroup to initialObjs
+			tt.initial.clientObjs = append(tt.initial.clientObjs, tt.initial.operatorGroup)
+
+			// Create test operator
+			ctx, cancel := context.WithCancel(context.TODO())
+			defer cancel()
+			op, err := NewFakeOperator(
+				ctx,
+				withClock(clockFake),
+				withNamespaces(namespaces...),
+				withOperatorNamespace(operatorNamespace),
+				withClientObjs(tt.initial.clientObjs...),
+				withK8sObjs(tt.initial.k8sObjs...),
+			)
+			require.NoError(t, err)
+
+			err = op.syncOperatorGroups(tt.initial.operatorGroup)
+			if !tt.expectError {
+				require.NoError(t, err)
+			}
+
+			// wait on operator group updated status to be in the cache
+			err = wait.PollImmediate(1*time.Millisecond, 5*time.Second, func() (bool, error) {
+				og, err := op.lister.OperatorsV1().OperatorGroupLister().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(tt.initial.operatorGroup.GetName())
+				if err != nil || og == nil {
+					return false, err
+				}
+				return true, nil
+			})
+			require.NoError(t, err)
+
+			// sync namespace
+			err = op.syncNamespace(opNamespace)
+			require.NoError(t, err)
+
+			operatorGroup, err := op.client.OperatorsV1().OperatorGroups(tt.initial.operatorGroup.GetNamespace()).Get(context.TODO(), tt.initial.operatorGroup.GetName(), metav1.GetOptions{})
+			require.NoError(t, err)
+			assert.Equal(t, len(tt.expectedConditions), len(operatorGroup.Status.Conditions))
+			if len(tt.expectedConditions) > 0 {
+				for _, cond := range tt.expectedConditions {
+					c := meta.FindStatusCondition(operatorGroup.Status.Conditions, cond.Type)
+					assert.Equal(t, cond.Status, c.Status)
+					assert.Equal(t, cond.Reason, c.Reason)
+					assert.Equal(t, cond.Message, c.Message)
+				}
+			}
+		})
+	}
+}
+
 func RequireObjectsInCache(t *testing.T, lister operatorlister.OperatorLister, namespace string, objects []runtime.Object, doCompare bool) error {
 	for _, object := range objects {
 		var err error

pkg/lib/scoped/syncer.go

@@ -7,6 +7,8 @@ import (
 
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	meta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/clock"
@@ -62,6 +64,9 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup
 			return
 		}
 
+		// Remove ServiceAccount condition if existed
+		meta.RemoveStatusCondition(&in.Status.Conditions, v1.OperatorGroupServiceAccountCondition)
+
 		// User must have removed ServiceAccount from the spec. We need to
 		// rest Status to a nil reference.
 		out, err = s.update(in, nil)
@@ -74,6 +79,21 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup
 	// A service account has been specified, we need to update the status.
 	sa, err := s.client.KubernetesInterface().CoreV1().ServiceAccounts(namespace).Get(context.TODO(), serviceAccountName, metav1.GetOptions{})
 	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			// Set OG's status condition to indicate SA is not found
+			cond := metav1.Condition{
+				Type:    v1.OperatorGroupServiceAccountCondition,
+				Status:  metav1.ConditionTrue,
+				Reason:  v1.OperatorGroupServiceAccountReason,
+				Message: fmt.Sprintf("ServiceAccount %s not found", serviceAccountName),
+			}
+
+			meta.SetStatusCondition(&in.Status.Conditions, cond)
+			_, uerr := s.update(in, nil)
+			if uerr != nil {
+				logger.Warnf("fail to upgrade operator group status og=%s with condition %+v: %s", in.GetName(), cond, uerr.Error())
+			}
+		}
 		err = fmt.Errorf("failed to get service account, sa=%s %v", serviceAccountName, err)
 		return
 	}
@@ -88,6 +108,11 @@ func (s *UserDefinedServiceAccountSyncer) SyncOperatorGroup(in *v1.OperatorGroup
 		return
 	}
 
+	// Remove SA not found condition if found
+	if c := meta.FindStatusCondition(in.Status.Conditions, v1.OperatorGroupServiceAccountCondition); c != nil {
+		meta.RemoveStatusCondition(&in.Status.Conditions, v1.OperatorGroupServiceAccountCondition)
+	}
+
 	out, err = s.update(in, ref)
 	if err != nil {
 		err = fmt.Errorf("failed to set status.serviceAccount, sa=%s %v", serviceAccountName, err)