Merge pull request #568 from jpeeler/add-operator-group-2

Operator group follow ups

Author: openshift-merge-robot

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

@@ -25,6 +25,36 @@ spec:
           properties:
             selector:
               type: object
+              description: Label selector to find resources associated with or managed by the operator
+              properties:
+                matchLabels:
+                  type: object
+                  description: Label key:value pairs to match directly
+                matchExpressions:
+                  type: array
+                  description: A set of expressions to match against the resource.
+                  items:
+                    allOf:
+                      - type: object
+                        required:
+                        - key
+                        - operator
+                        - values
+                        properties:
+                          key:
+                            type: string
+                            description: the key to match
+                          operator:
+                            type: string
+                            description: the operator for the expression
+                            enum:
+                            - In
+                            - NotIn
+                            - Exists
+                            - DoesNotExist
+                          values:
+                            type: array
+                            description: set of values for the expression
             serviceAccountName:
               type: string
           required:

pkg/controller/operators/olm/operator_test.go

@@ -134,10 +134,10 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO
 	// Create the new operator
 	queueOperator, err := queueinformer.NewOperatorFromClient(opClientFake)
 	op := &Operator{
-		Operator: queueOperator,
-		client:   clientFake,
-		lister:   operatorlister.NewLister(),
-		resolver: resolver,
+		Operator:  queueOperator,
+		client:    clientFake,
+		lister:    operatorlister.NewLister(),
+		resolver:  resolver,
 		csvQueues: make(map[string]workqueue.RateLimitingInterface),
 		recorder:  eventRecorder,
 	}
@@ -2183,13 +2183,12 @@ func TestSyncOperatorGroups(t *testing.T) {
 						},
 					},
 				},
-
 				clientObjs: []runtime.Object{
 					csv("csv1",
 						testNS,
 						"",
 						installStrategy("csv1-dep1", nil, nil),
-						[]*v1beta1.CustomResourceDefinition{crd("c1", "v1")},
+						[]*v1beta1.CustomResourceDefinition{crd("c1.fake.api.group", "v1")},
 						[]*v1beta1.CustomResourceDefinition{},
 						v1alpha1.CSVPhaseSucceeded,
 					),
@@ -2240,7 +2239,7 @@ func TestSyncOperatorGroups(t *testing.T) {
 						testNS,
 						"",
 						installStrategy("csv1-dep1", nil, nil),
-						[]*v1beta1.CustomResourceDefinition{crd("c1", "v1")},
+						[]*v1beta1.CustomResourceDefinition{crd("c1.fake.api.group", "v1")},
 						[]*v1beta1.CustomResourceDefinition{},
 						v1alpha1.CSVPhaseSucceeded,
 					),

pkg/controller/operators/olm/operatorgroup.go

@@ -180,22 +180,18 @@ func (a *Operator) ensureClusterRoles(op *v1alpha2.OperatorGroup) error {
 		apiEditPolicyRules := []rbacv1.PolicyRule{}
 		apiViewPolicyRules := []rbacv1.PolicyRule{}
 		for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
-			resourceNames := []string{}
-			for _, resource := range owned.Resources {
-				resourceNames = append(resourceNames, resource.Name)
+			nameGroupPair := strings.SplitN(owned.Name, ".", 2) // -> etcdclusters etcd.database.coreos.com
+			if len(nameGroupPair) != 2 {
+				return fmt.Errorf("Invalid parsing of name '%v', got %v", owned.Name, nameGroupPair)
 			}
-			managerPolicyRules = append(managerPolicyRules, rbacv1.PolicyRule{Verbs: []string{"*"}, APIGroups: []string{owned.Name}, Resources: resourceNames})
-			apiEditPolicyRules = append(apiEditPolicyRules, rbacv1.PolicyRule{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{owned.Name}, Resources: []string{owned.Kind}})
-			apiViewPolicyRules = append(apiViewPolicyRules, rbacv1.PolicyRule{Verbs: []string{"get", "list", "watch"}, APIGroups: []string{owned.Name}, Resources: []string{owned.Kind}})
+			managerPolicyRules = append(managerPolicyRules, rbacv1.PolicyRule{Verbs: []string{"*"}, APIGroups: []string{nameGroupPair[1]}, Resources: []string{nameGroupPair[0]}})
+			apiEditPolicyRules = append(apiEditPolicyRules, rbacv1.PolicyRule{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{nameGroupPair[1]}, Resources: []string{nameGroupPair[0]}})
+			apiViewPolicyRules = append(apiViewPolicyRules, rbacv1.PolicyRule{Verbs: []string{"get", "list", "watch"}, APIGroups: []string{nameGroupPair[1]}, Resources: []string{nameGroupPair[0]}})
 		}
 		for _, owned := range csv.Spec.APIServiceDefinitions.Owned {
-			resourceNames := []string{}
-			for _, resource := range owned.Resources {
-				resourceNames = append(resourceNames, resource.Name)
-			}
-			managerPolicyRules = append(managerPolicyRules, rbacv1.PolicyRule{Verbs: []string{"*"}, APIGroups: []string{owned.Group}, Resources: resourceNames})
-			apiEditPolicyRules = append(apiEditPolicyRules, rbacv1.PolicyRule{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{owned.Group}, Resources: []string{owned.Kind}})
-			apiViewPolicyRules = append(apiViewPolicyRules, rbacv1.PolicyRule{Verbs: []string{"get", "list", "watch"}, APIGroups: []string{owned.Group}, Resources: []string{owned.Kind}})
+			managerPolicyRules = append(managerPolicyRules, rbacv1.PolicyRule{Verbs: []string{"*"}, APIGroups: []string{owned.Group}, Resources: []string{owned.Name}})
+			apiEditPolicyRules = append(apiEditPolicyRules, rbacv1.PolicyRule{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{owned.Group}, Resources: []string{owned.Name}})
+			apiViewPolicyRules = append(apiViewPolicyRules, rbacv1.PolicyRule{Verbs: []string{"get", "list", "watch"}, APIGroups: []string{owned.Group}, Resources: []string{owned.Name}})
 		}
 
 		clusterRole := &rbacv1.ClusterRole{

pkg/controller/operators/olm/requirements.go

@@ -28,6 +28,7 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
 		// check if CRD exists - this verifies group, version, and kind, so no need for GVK check via discovery
 		crd, err := a.lister.APIExtensionsV1beta1().CustomResourceDefinitionLister().Get(r.Name)
 		if err != nil {
+			log.Debugf("Setting 'met' to false, %v with err: %v", r.Name, err)
 			status.Status = v1alpha1.RequirementStatusReasonNotPresent
 			met = false
 			statuses = append(statuses, status)
@@ -277,6 +278,9 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe
 	// Aggregate requirement and permissions statuses
 	statuses := append(reqStatuses, permStatuses...)
 	met := reqMet && permMet
+	if !met {
+		log.Debugf("reqMet=%#v permMet=%v\n", reqMet, permMet)
+	}
 
 	return met, statuses, nil
 }

test/e2e/operator_groups_e2e_test.go

@@ -6,13 +6,15 @@ import (
 	"strings"
 	"testing"
 
-	appsv1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"github.com/coreos/go-semver/semver"
 	"github.com/stretchr/testify/require"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	extv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha2"
@@ -27,39 +29,49 @@ func DeploymentComplete(deployment *appsv1.Deployment, newStatus *appsv1.Deploym
 }
 
 // Currently this function only modifies the watchedNamespace in the container command
-func patchOlmDeployment(t *testing.T, c operatorclient.ClientInterface, newNamespace string) []string {
+func patchOlmDeployment(t *testing.T, c operatorclient.ClientInterface, newNamespace string) (cleanupFunc func() error) {
 	runningDeploy, err := c.GetDeployment(testNamespace, "olm-operator")
 	require.NoError(t, err)
 
-	command := runningDeploy.Spec.Template.Spec.Containers[0].Command
+	oldCommand := runningDeploy.Spec.Template.Spec.Containers[0].Command
 	re, err := regexp.Compile(`-watchedNamespaces\W(\S+)`)
 	require.NoError(t, err)
-	newCommand := re.ReplaceAllString(strings.Join(command, " "), "$0"+","+newNamespace)
-	t.Logf("original=%#v newCommand=%#v", command, newCommand)
+	newCommand := re.ReplaceAllString(strings.Join(oldCommand, " "), "$0"+","+newNamespace)
+	t.Logf("original=%#v newCommand=%#v", oldCommand, newCommand)
 	finalNewCommand := strings.Split(newCommand, " ")
 	runningDeploy.Spec.Template.Spec.Containers[0].Command = make([]string, len(finalNewCommand))
 	copy(runningDeploy.Spec.Template.Spec.Containers[0].Command, finalNewCommand)
 
-	newDeployment, updated, err := c.UpdateDeployment(runningDeploy)
+	olmDeployment, updated, err := c.UpdateDeployment(runningDeploy)
 	if err != nil || updated == false {
 		t.Fatalf("Deployment update failed: (updated %v) %v\n", updated, err)
 	}
 	require.NoError(t, err)
 
 	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
 		t.Log("Polling for OLM deployment update...")
-		fetchedDeployment, err := c.GetDeployment(newDeployment.Namespace, newDeployment.Name)
+		fetchedDeployment, err := c.GetDeployment(olmDeployment.Namespace, olmDeployment.Name)
 		if err != nil {
 			return false, err
 		}
-		if DeploymentComplete(newDeployment, &fetchedDeployment.Status) {
+		if DeploymentComplete(olmDeployment, &fetchedDeployment.Status) {
 			return true, nil
 		}
 		return false, nil
 	})
-
 	require.NoError(t, err)
-	return command
+
+	return func() error {
+		olmDeployment.Spec.Template.Spec.Containers[0].Command = oldCommand
+		_, updated, err := c.UpdateDeployment(olmDeployment)
+		if err != nil || updated == false {
+			t.Fatalf("Deployment update failed: (updated %v) %v\n", updated, err)
+		}
+		if err != nil {
+			return err
+		}
+		return nil
+	}
 }
 
 func checkOperatorGroupAnnotations(obj metav1.Object, op *v1alpha2.OperatorGroup, targetNamespaces string) error {
@@ -78,6 +90,7 @@ func checkOperatorGroupAnnotations(obj metav1.Object, op *v1alpha2.OperatorGroup
 
 func TestOperatorGroup(t *testing.T) {
 	// Create namespace with specific label
+	// Create CRD
 	// Create CSV in operator namespace
 	// Create operator group that watches namespace and uses specific label
 	// Verify operator group status contains correct status
@@ -101,10 +114,18 @@ func TestOperatorGroup(t *testing.T) {
 	createdOtherNamespace, err := c.KubernetesInterface().CoreV1().Namespaces().Create(&otherNamespace)
 	require.NoError(t, err)
 
-	oldCommand := patchOlmDeployment(t, c, otherNamespaceName)
+	cleanupOlmDeployment := patchOlmDeployment(t, c, otherNamespaceName)
+
+	t.Log("Creating CRD")
+	mainCRDPlural := genName("ins")
+	apiGroup := "cluster.com"
+	mainCRDName := mainCRDPlural + "." + apiGroup
+	mainCRD := newCRD(mainCRDName, testNamespace, mainCRDPlural)
+	cleanupCRD, err := createCRD(c, mainCRD)
+	require.NoError(t, err)
 
 	t.Log("Creating CSV")
-	aCSV := newCSV(csvName, testNamespace, "", *semver.New("0.0.0"), nil, nil, newNginxInstallStrategy("operator-deployment", nil, nil))
+	aCSV := newCSV(csvName, testNamespace, "", *semver.New("0.0.0"), []extv1beta1.CustomResourceDefinition{mainCRD}, nil, newNginxInstallStrategy("operator-deployment", nil, nil))
 	createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Create(&aCSV)
 	require.NoError(t, err)
 
@@ -119,6 +140,7 @@ func TestOperatorGroup(t *testing.T) {
 				MatchLabels: matchingLabel,
 			},
 		},
+		//ServiceAccountName: "default-sa",
 	}
 	_, err = crc.OperatorsV1alpha2().OperatorGroups(testNamespace).Create(&operatorGroup)
 	require.NoError(t, err)
@@ -133,12 +155,36 @@ func TestOperatorGroup(t *testing.T) {
 			return false, fetchErr
 		}
 		if len(fetched.Status.Namespaces) > 0 {
-			require.Equal(t, expectedOperatorGroupStatus.Namespaces[0].Name, fetched.Status.Namespaces[0].Name)
+			require.EqualValues(t, expectedOperatorGroupStatus.Namespaces[0].Name, fetched.Status.Namespaces[0].Name)
 			return true, nil
 		}
 		return false, nil
 	})
 
+	t.Log("Checking for proper RBAC permissions in target namespace")
+	roleList, err := c.KubernetesInterface().RbacV1().ClusterRoles().List(metav1.ListOptions{})
+	for _, item := range roleList.Items {
+		role, err := c.GetClusterRole(item.GetName())
+		require.NoError(t, err)
+		switch roleName := item.GetName(); roleName {
+		case "owned-crd-manager-another-csv":
+			managerPolicyRules := []rbacv1.PolicyRule{
+				rbacv1.PolicyRule{Verbs: []string{"*"}, APIGroups: []string{apiGroup}, Resources: []string{mainCRDPlural}},
+			}
+			require.Equal(t, managerPolicyRules, role.Rules)
+		case "e2e-operator-group-edit":
+			editPolicyRules := []rbacv1.PolicyRule{
+				rbacv1.PolicyRule{Verbs: []string{"create", "update", "patch", "delete"}, APIGroups: []string{apiGroup}, Resources: []string{mainCRDPlural}},
+			}
+			require.Equal(t, editPolicyRules, role.Rules)
+		case "e2e-operator-group-view":
+			viewPolicyRules := []rbacv1.PolicyRule{
+				rbacv1.PolicyRule{Verbs: []string{"get", "list", "watch"}, APIGroups: []string{apiGroup}, Resources: []string{mainCRDPlural}},
+			}
+			require.Equal(t, viewPolicyRules, role.Rules)
+		}
+	}
+
 	t.Log("Waiting for operator namespace csv to have annotations")
 	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
 		fetchedCSV, fetchErr := crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Get(csvName, metav1.GetOptions{})
@@ -171,8 +217,8 @@ func TestOperatorGroup(t *testing.T) {
 	require.NoError(t, err)
 	require.EqualValues(t, v1alpha1.CSVReasonCopied, fetchedCSV.Status.Reason)
 	// also check name and spec
-	require.Equal(t, createdCSV.Name, fetchedCSV.Name)
-	require.Equal(t, createdCSV.Spec, fetchedCSV.Spec)
+	require.EqualValues(t, createdCSV.Name, fetchedCSV.Name)
+	require.EqualValues(t, createdCSV.Spec, fetchedCSV.Spec)
 
 	t.Log("Waiting on deployment to have correct annotations")
 	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
@@ -195,29 +241,18 @@ func TestOperatorGroup(t *testing.T) {
 	require.NoError(t, err)
 
 	t.Log("Waiting for orphaned CSV to be deleted")
-	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+	err = waitForDelete(func() error {
 		_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Get(csvName, metav1.GetOptions{})
-		if err != nil {
-			if errors.IsNotFound(err) {
-				return true, nil
-			}
-			return false, err
-		}
-		return false, nil
+		return err
 	})
 	require.NoError(t, err)
 
 	// clean up
-	// TODO: unpatch function
-	runningDeploy, err := c.GetDeployment(testNamespace, "olm-operator")
-	require.NoError(t, err)
-	runningDeploy.Spec.Template.Spec.Containers[0].Command = oldCommand
-	_, updated, err := c.UpdateDeployment(runningDeploy)
-	if err != nil || updated == false {
-		t.Fatalf("Deployment update failed: (updated %v) %v\n", updated, err)
-	}
+	err = cleanupOlmDeployment()
 	require.NoError(t, err)
 
+	cleanupCRD()
+
 	err = c.KubernetesInterface().CoreV1().Namespaces().Delete(otherNamespaceName, &metav1.DeleteOptions{})
 	require.NoError(t, err)
 	err = crc.OperatorsV1alpha2().OperatorGroups(testNamespace).Delete(operatorGroup.Name, &metav1.DeleteOptions{})