refactor(operatorgroup): account for TargetNamespace field in

updateNamespaceList(...)

Author: njhale

deploy/chart/templates/0000_30_13-operatorgroup.crd.yaml

@@ -63,6 +63,7 @@ spec:
               type: array
               items:
                 type: string
+                pattern: ^\S+$
             serviceAccountName:
               type: string
           required:

pkg/api/apis/operators/v1alpha1/clusterserviceversion_types.go

@@ -136,7 +136,8 @@ type ClusterServiceVersionSpec struct {
 	Links                     []AppLink                 `json:"links,omitempty"`
 	Icon                      []Icon                    `json:"icon,omitempty"`
 
-	// InstallModes
+	// InstallModes specify supported installation types
+	// +optional
 	InstallModes []InstallMode `json:"installModes,omitempty"`
 
 	// The name of a CSV this one replaces. Should match the `metadata.Name` field of the old CSV.

pkg/controller/operators/olm/operator.go

@@ -477,6 +477,8 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
 func (a *Operator) operatorGroupForActiveCSV(logger *logrus.Entry, csv *v1alpha1.ClusterServiceVersion) *v1alpha2.OperatorGroup {
 	annotations := csv.GetAnnotations()
 
+	// TODO: Only return OperatorGroup if InstallModes allow the CSV to be part of the group or no installmodes are declared
+
 	// not part of a group yet
 	if annotations == nil {
 		logger.Info("not part of any operatorgroup, no annotations")

pkg/controller/operators/olm/operator_test.go

@@ -2246,7 +2246,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 	ownerutil.AddNonBlockingOwner(ownedDeployment, operatorCSV)
 
 	annotatedDeployment := ownedDeployment.DeepCopy()
-	annotatedDeployment.Spec.Template.SetAnnotations(map[string]string{v1alpha2.OperatorGroupTargetsAnnotationKey: targetNamespace + "," + operatorNamespace, v1alpha2.OperatorGroupAnnotationKey: "operator-group-1", v1alpha2.OperatorGroupNamespaceAnnotationKey: operatorNamespace})
+	annotatedDeployment.Spec.Template.SetAnnotations(map[string]string{v1alpha2.OperatorGroupTargetsAnnotationKey: operatorNamespace + "," + targetNamespace, v1alpha2.OperatorGroupAnnotationKey: "operator-group-1", v1alpha2.OperatorGroupNamespaceAnnotationKey: operatorNamespace})
 	annotatedDeployment.SetLabels(map[string]string{
 		"olm.owner":           "csv1",
 		"olm.owner.namespace": "operator-ns",
@@ -2365,7 +2365,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 				k8sObjs: namespaces,
 			},
 			expectedStatus: v1alpha2.OperatorGroupStatus{
-				Namespaces:  []string{targetNamespace, operatorNamespace},
+				Namespaces:  []string{operatorNamespace, targetNamespace},
 				LastUpdated: timeNow(),
 			},
 		},
@@ -2390,12 +2390,12 @@ func TestSyncOperatorGroups(t *testing.T) {
 				crds:       []runtime.Object{crd},
 			},
 			expectedStatus: v1alpha2.OperatorGroupStatus{
-				Namespaces:  []string{targetNamespace, operatorNamespace},
+				Namespaces:  []string{operatorNamespace, targetNamespace},
 				LastUpdated: timeNow(),
 			},
 			final: final{objects: map[string][]runtime.Object{
 				operatorNamespace: {
-					withAnnotations(operatorCSVFinal.DeepCopy(), map[string]string{v1alpha2.OperatorGroupTargetsAnnotationKey: targetNamespace + "," + operatorNamespace, v1alpha2.OperatorGroupAnnotationKey: "operator-group-1", v1alpha2.OperatorGroupNamespaceAnnotationKey: operatorNamespace}),
+					withAnnotations(operatorCSVFinal.DeepCopy(), map[string]string{v1alpha2.OperatorGroupTargetsAnnotationKey: operatorNamespace + "," + targetNamespace, v1alpha2.OperatorGroupAnnotationKey: "operator-group-1", v1alpha2.OperatorGroupNamespaceAnnotationKey: operatorNamespace}),
 					annotatedDeployment,
 				},
 				targetNamespace: {

pkg/controller/operators/olm/operatorgroup.go

@@ -3,6 +3,7 @@ package olm
 import (
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
@@ -400,45 +401,56 @@ func (a *Operator) updateNamespaceList(op *v1alpha2.OperatorGroup) ([]string, er
 		return nil, err
 	}
 
-	namespaceList := []string{}
+	namespaceSet := make(map[string]struct{})
+	namespaceAll := false
 	if op.Spec.TargetNamespaces != nil && len(op.Spec.TargetNamespaces) > 0 {
-		// TODO: Ensure namespaceList is a set
-		namespaceList = append(namespaceList, op.Spec.TargetNamespaces...)
-	} else if selector.Empty() || selector == nil {
-		namespaceList = append(namespaceList, corev1.NamespaceAll)
+		for _, ns := range op.Spec.TargetNamespaces {
+			if ns == corev1.NamespaceAll {
+				return nil, fmt.Errorf("TargetNamespaces cannot contain NamespaceAll: %v", op.Spec.TargetNamespaces)
+			}
+			namespaceSet[ns] = struct{}{}
+		}
+	} else if selector == nil || selector.Empty() {
+		namespaceSet[corev1.NamespaceAll] = struct{}{}
+		namespaceAll = true
 	} else {
 		matchedNamespaces, err := a.lister.CoreV1().NamespaceLister().List(selector)
 		if err != nil {
 			return nil, err
 		}
 
-		operatorGroupNamespaceSelected := false
 		for _, ns := range matchedNamespaces {
-			namespaceList = append(namespaceList, ns.GetName())
-			if ns.GetName() == op.GetNamespace() {
-				operatorGroupNamespaceSelected = true
-			}
+			namespaceSet[ns.GetName()] = struct{}{}
 		}
+	}
 
-		// always include the operatorgroup namespace as a target namespace
-		if !operatorGroupNamespaceSelected {
-			namespaceList = append(namespaceList, op.GetNamespace())
-		}
+	if !namespaceAll {
+		// Add operator namespace to the set
+		namespaceSet[op.GetNamespace()] = struct{}{}
 	}
 
+	namespaceList := []string{}
+	for ns := range namespaceSet {
+		namespaceList = append(namespaceList, ns)
+	}
+	sort.StringSlice(namespaceList).Sort()
+
 	if !namespacesChanged(namespaceList, op.Status.Namespaces) {
 		// status is current with correct namespaces, so no further updates required
 		return namespaceList, nil
 	}
+
 	a.Log.Debugf("Namespace change detected, found: %v", namespaceList)
 	op.Status = v1alpha2.OperatorGroupStatus{
 		Namespaces:  namespaceList,
 		LastUpdated: timeNow(),
 	}
+
 	_, err = a.client.OperatorsV1alpha2().OperatorGroups(op.GetNamespace()).UpdateStatus(op)
 	if err != nil {
 		return namespaceList, err
 	}
+
 	return namespaceList, nil
 }