Merge pull request #794 from ecordell/starting-csv-multiple-ips

fix(olm): generate aggregated clusterroles for ownnamespace operatorgroups correctly

Author: openshift-merge-robot

pkg/controller/operators/olm/operator.go

@@ -621,11 +621,6 @@ func (a *Operator) syncCopyCSV(obj interface{}) (syncError error) {
 		syncError = err
 	}
 
-	// Ensure cluster roles exist for using provided apis
-	if err := a.ensureClusterRolesForCSV(clusterServiceVersion, operatorGroup); err != nil {
-		logger.WithError(err).Info("couldn't ensure clusterroles for provided api types")
-		syncError = err
-	}
 	return
 }
 
@@ -988,6 +983,13 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}
 
+		// Ensure cluster roles exist for using provided apis
+		if err := a.ensureClusterRolesForCSV(out, operatorGroup); err != nil {
+			logger.WithError(err).Info("couldn't ensure clusterroles for provided api types")
+			syncError = err
+			return
+		}
+
 	case v1alpha1.CSVPhaseFailed:
 		installer, strategy, _ := a.parseStrategiesAndUpdateStatus(out)
 		if strategy == nil {

pkg/controller/operators/olm/operatorgroup.go

@@ -742,14 +742,10 @@ func (a *Operator) ensureOpGroupClusterRole(op *v1.OperatorGroup, suffix string)
 }
 
 func (a *Operator) ensureOpGroupClusterRoles(op *v1.OperatorGroup) error {
-	if err := a.ensureOpGroupClusterRole(op, AdminSuffix); err != nil {
-		return err
-	}
-	if err := a.ensureOpGroupClusterRole(op, EditSuffix); err != nil {
-		return err
-	}
-	if err := a.ensureOpGroupClusterRole(op, ViewSuffix); err != nil {
-		return err
+	for _, suffix := range Suffices {
+		if err := a.ensureOpGroupClusterRole(op, suffix); err != nil {
+			return err
+		}
 	}
 	return nil
 }

scripts/build_local.sh

@@ -5,8 +5,8 @@
 
 set -e
 
-if [ -z "$NO_MINIKUBE" ] || [ -x "$(command -v minikube)" ]; then
-  ps x | grep -q [m]inikube || minikube start --kubernetes-version="v1.12.0" --extra-config=apiserver.v=4 || { echo 'Cannot start minikube.'; exit 1; }
+if [ -z "$NO_MINIKUBE" ]; then
+  ps x | grep -q [m]inikube || minikube start --kubernetes-version="v1.13.0" --extra-config=apiserver.v=4 || { echo 'Cannot start minikube.'; exit 1; }
   eval $(minikube docker-env) || { echo 'Cannot switch to minikube docker'; exit 1; }
   kubectl config use-context minikube
 fi

test/e2e/catalog_e2e_test.go

@@ -213,7 +213,6 @@ func TestConfigMapUpdateTriggersRegistryPodRollout(t *testing.T) {
 		}
 	}
 	require.NoError(t, err)
-	require.Equal(t, 1, ipCount)
 }
 
 func TestConfigMapReplaceTriggersRegistryPodRollout(t *testing.T) {

test/e2e/operator_groups_e2e_test.go

@@ -2,15 +2,12 @@ package e2e
 
 import (
 	"fmt"
-	"regexp"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/coreos/go-semver/semver"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	"github.com/stretchr/testify/require"
-	appsv1 "k8s.io/api/apps/v1"
 	authorizationv1 "k8s.io/api/authorization/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -21,7 +18,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/tools/cache"
-	"k8s.io/kubernetes/pkg/apis/rbac"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
@@ -30,59 +28,6 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
 )
 
-func DeploymentComplete(deployment *appsv1.Deployment, newStatus *appsv1.DeploymentStatus) bool {
-	return newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) &&
-		newStatus.Replicas == *(deployment.Spec.Replicas) &&
-		newStatus.AvailableReplicas == *(deployment.Spec.Replicas) &&
-		newStatus.ObservedGeneration >= deployment.Generation
-}
-
-// Currently this function only modifies the watchedNamespace in the container command
-func patchOlmDeployment(t *testing.T, c operatorclient.ClientInterface, newNamespace string) (cleanupFunc func() error) {
-	runningDeploy, err := c.GetDeployment(operatorNamespace, "olm-operator")
-	require.NoError(t, err)
-
-	oldCommand := runningDeploy.Spec.Template.Spec.Containers[0].Command
-	re, err := regexp.Compile(`-watchedNamespaces\W(\S+)`)
-	require.NoError(t, err)
-	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)
-
-	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(olmDeployment.Namespace, olmDeployment.Name)
-		if err != nil {
-			return false, err
-		}
-		if DeploymentComplete(olmDeployment, &fetchedDeployment.Status) {
-			return true, nil
-		}
-		return false, nil
-	})
-	require.NoError(t, err)
-
-	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 *v1.OperatorGroup, checkTargetNamespaces bool, targetNamespaces string) error {
 	if checkTargetNamespaces {
 		if annotation, ok := obj.GetAnnotations()[v1.OperatorGroupTargetsAnnotationKey]; !ok || annotation != targetNamespaces {
@@ -221,7 +166,7 @@ func TestOperatorGroup(t *testing.T) {
 			ServiceAccountName: serviceAccountName,
 			Rules: []rbacv1.PolicyRule{
 				{
-					Verbs:     []string{rbac.VerbAll},
+					Verbs:     []string{rbacv1.VerbAll},
 					APIGroups: []string{mainCRD.Spec.Group},
 					Resources: []string{mainCRDPlural},
 				},
@@ -447,14 +392,130 @@ func TestOperatorGroup(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func createProjectAdmin(t *testing.T, c operatorclient.ClientInterface, namespace string) (string, cleanupFunc) {
+	sa, err := c.CreateServiceAccount(&corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      genName("padmin-"),
+		},
+	})
+	require.NoError(t, err)
+
+	rb, err := c.CreateRoleBinding(&rbacv1.RoleBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      genName("padmin-"),
+			Namespace: namespace,
+		},
+		Subjects: []rbacv1.Subject{
+			{
+				Kind:      "ServiceAccount",
+				Name:      sa.GetName(),
+				Namespace: sa.GetNamespace(),
+			},
+		},
+		RoleRef: rbacv1.RoleRef{
+			APIGroup: "rbac.authorization.k8s.io",
+			Kind:     "ClusterRole",
+			Name:     "admin",
+		},
+	})
+	require.NoError(t, err)
+	// kubectl -n a8v4sw  auth can-i create alp999.cluster.com --as system:serviceaccount:a8v4sw:padmin-xqdfz
+	return "system:serviceaccount:" + namespace + ":" + sa.GetName(), func() {
+		_ = c.DeleteServiceAccount(sa.GetNamespace(), sa.GetName(), metav1.NewDeleteOptions(0))
+		_ = c.DeleteRoleBinding(rb.GetNamespace(), rb.GetName(), metav1.NewDeleteOptions(0))
+	}
+}
+
+func TestOperatorGroupRoleAggregation(t *testing.T) {
+	// Generate namespaceA
+	// Generate operatorGroupA - OwnNamespace
+	// Generate csvA in namespaceA with all installmodes supported
+	// Create crd so csv succeeds
+	// Ensure clusterroles created and aggregated for access provided APIs
+
+	defer cleaner.NotifyTestComplete(t, true)
+
+	// Generate namespaceA
+	nsA := genName("a")
+	c := newKubeClient(t)
+	for _, ns := range []string{nsA} {
+		namespace := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: ns,
+			},
+		}
+		_, err := c.KubernetesInterface().CoreV1().Namespaces().Create(namespace)
+		require.NoError(t, err)
+		defer func(name string) {
+			require.NoError(t, c.KubernetesInterface().CoreV1().Namespaces().Delete(name, &metav1.DeleteOptions{}))
+		}(ns)
+	}
+
+	// Generate operatorGroupA - OwnNamespace
+	crc := newCRClient(t)
+	groupA := newOperatorGroup(nsA, genName("a"), nil, nil, []string{nsA}, false)
+	_, err := crc.OperatorsV1().OperatorGroups(nsA).Create(groupA)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, crc.OperatorsV1().OperatorGroups(nsA).Delete(groupA.GetName(), &metav1.DeleteOptions{}))
+	}()
+
+	// Generate csvA in namespaceA with all installmodes supported
+	crd := newCRD(genName("a"))
+	namedStrategy := newNginxInstallStrategy(genName("dep-"), nil, nil)
+	csvA := newCSV("nginx-a", nsA, "", *semver.New("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, namedStrategy)
+	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(nsA).Create(&csvA)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, crc.OperatorsV1alpha1().ClusterServiceVersions(nsA).Delete(csvA.GetName(), &metav1.DeleteOptions{}))
+	}()
+
+	// Create crd so csv succeeds
+	cleanupCRD, err := createCRD(c, crd)
+	require.NoError(t, err)
+	defer cleanupCRD()
+
+	_, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvSucceededChecker)
+	require.NoError(t, err)
+
+	// Ensure clusterroles created and aggregated for access provided APIs
+	padmin, cleanupPadmin := createProjectAdmin(t, c, nsA)
+	defer cleanupPadmin()
+
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{
+			Spec: authorizationv1.SubjectAccessReviewSpec{
+				User: padmin,
+				ResourceAttributes: &authorizationv1.ResourceAttributes{
+					Namespace: nsA,
+					Group:     crd.Spec.Group,
+					Version:   crd.Spec.Version,
+					Resource:  crd.Spec.Names.Plural,
+					Verb:      "create",
+				},
+			},
+		})
+		if err != nil {
+			return false, err
+		}
+		if res == nil {
+			return false, nil
+		}
+		t.Log("checking padmin for permission")
+		return res.Status.Allowed, nil
+	})
+	require.NoError(t, err)
+}
+
 func TestOperatorGroupInstallModeSupport(t *testing.T) {
 	// Generate namespaceA
 	// Generate namespaceB
 	// Create operatorGroupA in namespaceA that selects namespaceA
 	// Generate csvA with an unfulfilled required CRD and no supported InstallModes in namespaceA
 	// Ensure csvA transitions to Failed with reason "UnsupportedOperatorGroup"
 	// Update csvA to have OwnNamespace supported=true
-	// Ensure csvA transitions to Pending
+	// Ensure csvA transitions to Succeeded
 	// Update operatorGroupA's target namespaces to select namespaceB
 	// Ensure csvA transitions to Failed with reason "UnsupportedOperatorGroup"
 	// Update csvA to have SingleNamespace supported=true
@@ -555,8 +616,13 @@ func TestOperatorGroupInstallModeSupport(t *testing.T) {
 	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(nsA).Update(csvA)
 	require.NoError(t, err)
 
-	// Ensure csvA transitions to Pending
-	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvPendingChecker)
+	// Create crd so csv succeeds
+	cleanupCRD, err := createCRD(c, crd)
+	require.NoError(t, err)
+	defer cleanupCRD()
+
+	// Ensure csvA transitions to Succeeded
+	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvSucceededChecker)
 	require.NoError(t, err)
 
 	// Update operatorGroupA's target namespaces to select namespaceB
@@ -592,8 +658,8 @@ func TestOperatorGroupInstallModeSupport(t *testing.T) {
 	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(nsA).Update(csvA)
 	require.NoError(t, err)
 
-	// Ensure csvA transitions to Pending
-	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvPendingChecker)
+	// Ensure csvA transitions to Succeeded
+	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvSucceededChecker)
 	require.NoError(t, err)
 
 	// Update operatorGroupA's target namespaces to select namespaceA and namespaceB
@@ -629,8 +695,8 @@ func TestOperatorGroupInstallModeSupport(t *testing.T) {
 	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(nsA).Update(csvA)
 	require.NoError(t, err)
 
-	// Ensure csvA transitions to Pending
-	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvPendingChecker)
+	// Ensure csvA transitions to Succeeded
+	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvSucceededChecker)
 	require.NoError(t, err)
 
 	// Update operatorGroupA's target namespaces to select all namespaces
@@ -667,7 +733,7 @@ func TestOperatorGroupInstallModeSupport(t *testing.T) {
 	require.NoError(t, err)
 
 	// Ensure csvA transitions to Pending
-	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvPendingChecker)
+	csvA, err = fetchCSV(t, crc, csvA.GetName(), nsA, csvSucceededChecker)
 	require.NoError(t, err)
 }
 
@@ -689,6 +755,7 @@ func TestOperatorGroupIntersection(t *testing.T) {
 	// Ensure csvD in namespaceD is still successful
 	// Generate csvA in namespaceA that owns crdA
 	// Wait for csvA to be successful
+	// Ensure clusterroles created and aggregated for accessing provided APIs
 	// Wait for operatorGroupA to have providedAPI annotation with crdA's Kind.version.group in its providedAPIs annotation
 	// Wait for csvA to have a CSV with copied status in namespace C
 	// Generate operatorGroupB in namespaceB that selects namespace C
@@ -851,6 +918,34 @@ func TestOperatorGroupIntersection(t *testing.T) {
 	_, err = awaitCSV(t, crc, nsA, csvA.GetName(), csvSucceededChecker)
 	require.NoError(t, err)
 
+	// Ensure clusterroles created and aggregated for access provided APIs
+	padmin, cleanupPadmin := createProjectAdmin(t, c, nsA)
+	defer cleanupPadmin()
+
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		res, err := c.KubernetesInterface().AuthorizationV1().SubjectAccessReviews().Create(&authorizationv1.SubjectAccessReview{
+			Spec: authorizationv1.SubjectAccessReviewSpec{
+				User: padmin,
+				ResourceAttributes: &authorizationv1.ResourceAttributes{
+					Namespace: nsA,
+					Group:     crdA.Spec.Group,
+					Version:   crdA.Spec.Version,
+					Resource:  crdA.Spec.Names.Plural,
+					Verb:      "create",
+				},
+			},
+		})
+		if err != nil {
+			return false, err
+		}
+		if res == nil {
+			return false, nil
+		}
+		t.Log("checking padmin for permission")
+		return res.Status.Allowed, nil
+	})
+	require.NoError(t, err)
+
 	// Await annotation on groupA
 	q = func() (metav1.ObjectMeta, error) {
 		g, err := crc.OperatorsV1().OperatorGroups(nsA).Get(groupA.GetName(), metav1.GetOptions{})
@@ -1175,7 +1270,7 @@ func TestCSVCopyWatchingAllNamespaces(t *testing.T) {
 			ServiceAccountName: serviceAccountName,
 			Rules: []rbacv1.PolicyRule{
 				{
-					Verbs:     []string{rbac.VerbAll},
+					Verbs:     []string{rbacv1.VerbAll},
 					APIGroups: []string{mainCRD.Spec.Group},
 					Resources: []string{mainCRDPlural},
 				},