From c9917ad3fc593f4739873039428da638f12fdbf2 Mon Sep 17 00:00:00 2001 From: Jeff Peeler Date: Mon, 12 Nov 2018 20:27:04 -0500 Subject: [PATCH 1/4] fix(olm): add additional validation for operator groups --- .../0000_30_14-operatorgroup.crd.yaml | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/deploy/chart/templates/0000_30_14-operatorgroup.crd.yaml b/deploy/chart/templates/0000_30_14-operatorgroup.crd.yaml index b8102198f27..82068373d34 100644 --- a/deploy/chart/templates/0000_30_14-operatorgroup.crd.yaml +++ b/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: From 4570fa52c7e333362c55efdb3a0a738d8b26e770 Mon Sep 17 00:00:00 2001 From: Jeff Peeler Date: Mon, 12 Nov 2018 20:28:11 -0500 Subject: [PATCH 2/4] chore(olm): add additional debug in requirements This logging makes it clearer that CRDs are required to be created to pass requirement checks. --- pkg/controller/operators/olm/requirements.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller/operators/olm/requirements.go b/pkg/controller/operators/olm/requirements.go index cc8254ad272..1ffd03777d3 100644 --- a/pkg/controller/operators/olm/requirements.go +++ b/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.OpClient.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Get(r.Name, metav1.GetOptions{}) 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 } From 59334426776920c6e4c4bf35340b7b03d0a03836 Mon Sep 17 00:00:00 2001 From: Jeff Peeler Date: Mon, 12 Nov 2018 20:30:09 -0500 Subject: [PATCH 3/4] fix(olm): fix RBAC cluster role creation specifically in this case the resource names must use the plural form --- pkg/controller/operators/olm/operator_test.go | 13 +++-- pkg/controller/operators/olm/operatorgroup.go | 22 ++++---- test/e2e/operator_groups_e2e_test.go | 52 ++++++++++++++++--- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 746657c3c5f..41b9efda56f 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -133,10 +133,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, } @@ -2175,13 +2175,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, ), @@ -2232,7 +2231,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, ), diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 6b50811620c..fa1a458d311 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/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{ diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index d4969797c55..867aa573081 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/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" @@ -78,6 +80,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 @@ -103,8 +106,16 @@ func TestOperatorGroup(t *testing.T) { oldCommand := 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 +130,7 @@ func TestOperatorGroup(t *testing.T) { MatchLabels: matchingLabel, }, }, + //ServiceAccountName: "default-sa", } _, err = crc.OperatorsV1alpha2().OperatorGroups(testNamespace).Create(&operatorGroup) require.NoError(t, err) @@ -133,12 +145,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 +207,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) { @@ -218,6 +254,8 @@ func TestOperatorGroup(t *testing.T) { } 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{}) From 843f96b472bbd45dd0d09b9feefbb8139e02efeb Mon Sep 17 00:00:00 2001 From: Jeff Peeler Date: Mon, 12 Nov 2018 21:01:32 -0500 Subject: [PATCH 4/4] fix(olm): create cleanup function for olm deploy mod also use waitForDelete instead of writing out more polling code --- test/e2e/operator_groups_e2e_test.go | 49 +++++++++++++--------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/test/e2e/operator_groups_e2e_test.go b/test/e2e/operator_groups_e2e_test.go index 867aa573081..1b8e841aef7 100644 --- a/test/e2e/operator_groups_e2e_test.go +++ b/test/e2e/operator_groups_e2e_test.go @@ -29,20 +29,20 @@ 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) } @@ -50,18 +50,28 @@ func patchOlmDeployment(t *testing.T, c operatorclient.ClientInterface, newNames 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 { @@ -104,7 +114,7 @@ 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") @@ -231,27 +241,14 @@ 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()