Don't process CSVs without operatorgroup #589

ecordell · 2018-11-28T23:34:10Z

This PR also moves the copying logic into the CSV control loop so that we don't have to requeue operatorgroups every time a CSV changes.

We probably shouldn't merge until there's a default operatorgroup shipping with OLM (otherwise you won't be able to install any operators)

/hold

ecordell · 2018-11-28T23:34:33Z

/approve
/hold

openshift-ci-robot · 2018-11-28T23:34:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alecmerdler · 2018-11-29T00:06:27Z

pkg/controller/operators/olm/operator.go

 	return
 }

+// operatorGroupForCSV returns the operatorgroup for the CSV only if the CSV is active one in the group
+func (a *Operator) operatorGroupForActiveCSV(logger *log.Logger, csv *v1alpha1.ClusterServiceVersion) *v1alpha2.OperatorGroup {


Function signature is a bit weird when this takes a log.Logger argument.

openshift-bot · 2018-12-02T07:22:09Z

@ecordell: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

njhale · 2018-12-03T02:18:13Z

pkg/controller/operators/olm/operator_test.go

@@ -391,6 +391,17 @@ func withAnnotations(obj runtime.Object, annotations map[string]string) runtime.
 	return meta.(runtime.Object)
 }

+func addAnnotations(annotations map[string]string, add map[string]string) map[string]string {


ecordell · 2018-12-03T16:09:23Z

/retest

ecordell · 2018-12-04T18:03:42Z

/retest

ecordell · 2018-12-05T16:44:08Z

/retest

this commit also moves the copying / rbac logic into the CSV loop, so that we don't have to requeue operatorgroups frequently

and aggregate them to operatorgroup clusterroles this lets us speed up the tests for operatorgroups and in the future can simplify installplan generation (can remove the clusterrole gen from installplans)

ecordell · 2018-12-11T22:20:23Z

/hold cancel

ecordell · 2018-12-11T22:44:35Z

deploy/chart/templates/0000_30_16-operatorgroup-default.yaml

+  namespace: {{ .Values.operator_namespace }}
+---
+apiVersion: operators.coreos.com/v1alpha2


Added an operatorgroup for the packages server, since they're required now

ecordell · 2018-12-11T22:51:32Z

test/e2e/operator_groups_e2e_test.go

+			sa, err := c.GetServiceAccount(opGroupNamespace, perm.ServiceAccountName)
+			require.NoError(t, err)
+			for _, rule := range perm.Rules {
+				satisfied, err := ruleChecker.RuleSatisfied(sa, otherNamespaceName, rule)


added a test to verify RBAC in target namespaces (kinda funky with informers in the test, but it works)

ecordell · 2018-12-11T22:52:05Z

test/e2e/operator_groups_e2e_test.go

+	})
+
+	// validate provided API clusterroles for the operatorgroup
+	adminRole, err := c.KubernetesInterface().RbacV1().ClusterRoles().Get(operatorGroup.Name+"-admin", metav1.GetOptions{})


This validates both that the operatorgroup clusterrole was created, and that it has correctly aggregated the csv-specific roles

ecordell · 2018-12-11T22:52:34Z

test/e2e/operator_groups_e2e_test.go

@@ -286,8 +397,4 @@ func TestOperatorGroup(t *testing.T) {
 	})
 	require.NoError(t, err)

-	err = c.KubernetesInterface().CoreV1().Namespaces().Delete(otherNamespaceName, &metav1.DeleteOptions{})


these moved into defer blocks further up

ecordell · 2018-12-11T22:53:21Z

pkg/controller/operators/olm/operator.go

@@ -403,7 +404,31 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
 		"phase":     clusterServiceVersion.Status.Phase,
 	})

-	operatorNamespace, ok := clusterServiceVersion.GetAnnotations()["olm.operatorNamespace"]
+	operatorGroup := a.operatorGroupForActiveCSV(logger, clusterServiceVersion)


this is the main part of the PR: gating csv processing on whether there are operatorgroups or not

ecordell · 2018-12-11T22:56:08Z

pkg/controller/operators/olm/operator.go

@@ -429,9 +454,67 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
 		}
 		syncError = fmt.Errorf("error transitioning ClusterServiceVersion: %s and error updating CSV status: %s", syncError, updateErr)
 	}
+
+	// Check if we need to do any copying / annotation for the operatorgroup
+	if err := a.copyCsvToTargetNamespace(updatedCSV, operatorGroup); err != nil {


I moved these three pieces from the operatorgroup control loop to here. The symptom was the e2e test taking a very long time (because csv changes didn't requeue their operatorgroups).

One option would've been to retrigger the owning operatorgroup on CSV change, but then in the operatorgroup side, we reach out and collect up each CSV to perform some of these actions. Moving them here removes all of that processing and lets us treat each CSV in isolation.

ecordell · 2018-12-11T22:58:20Z

pkg/controller/operators/olm/operator.go

+	}
+
+	// Ensure cluster roles exist for using provided apis
+	if err := a.ensureClusterRolesForCSV(updatedCSV, operatorGroup); err != nil {


Previously we were generating operatorgroup clusterroles directly (e.g. an operatorgroup with permissions on all provided apis in the group).

To speed things up, the operatorgroup control loop just writes out a ClusterRole with aggregation rules, and each individual CSV writes out clusterroles for its provided apis with the correct label to get aggregated to the operatorgroup clusterrole. There's an e2e test to verify the aggregated roles get the correct permissions.

jpeeler · 2018-12-11T23:05:48Z

pkg/controller/operators/olm/operatorgroup.go

@@ -18,63 +17,137 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )

+const (
+	operatorGroupAnnotationKey          = "olm.operatorGroup"


One of us is going to have to rebase a bit. I ended up pulling this into types so the e2e can also use them in #616. Will finish reviewing this tomorrow.

I think #616 will need to change (slightly) anyway - see the operatorGroupForActiveCSV method.

njhale · 2018-12-12T15:07:51Z

pkg/controller/operators/olm/operatorgroup.go

 			return err
 		}
 	}
-	log.Debug("CSV annotation completed")
+	for _, owned := range csv.Spec.APIServiceDefinitions.Owned {
+		namePrefix := fmt.Sprintf("%s-%s-", owned.Name, owned.Version)


Is ownee.Name always set for APIServiceDefinitions?

It is not, but I want to review all of the current ones, add them, and then go make it required in the json schema

njhale · 2018-12-12T15:38:43Z

pkg/controller/operators/olm/operatorgroup.go

+					return err
+				}
+			}
+
 			continue
 		} else if k8serrors.IsNotFound(err) {
 			newCSV := csv.DeepCopy()
 			newCSV.SetNamespace(ns)
 			newCSV.SetResourceVersion("")


ObjectMeta.ResourceVersion is read only so I don't think this needs to be cleared.

njhale

/lgtm

jpeeler · 2018-12-12T17:05:03Z

pkg/controller/operators/olm/operator.go

+	}
+
+	// target namespaces don't match
+	if annotations[operatorGroupTargetsAnnotationKey] != strings.Join(operatorGroup.Status.Namespaces, ",") {


Would this scenario only occur if an operator group was modified after annotations had been previously correct?

jpeeler · 2018-12-12T17:16:16Z

pkg/controller/operators/olm/operatorgroup.go

+	ownerutil.AddNonBlockingOwner(clusterRole, csv)
+	existingCR, err := a.OpClient.KubernetesInterface().RbacV1().ClusterRoles().Create(clusterRole)
+	if k8serrors.IsAlreadyExists(err) {
+		if existingCR != nil && reflect.DeepEqual(existingCR.Labels, clusterRole.Labels) && reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) {


Does createPatch not handle a no-op or is this just an optimization?

…v-loop Don't process CSVs without operatorgroup

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2018

openshift-ci-robot requested review from alecmerdler and njhale November 28, 2018 23:34

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2018

ecordell requested review from jpeeler and removed request for njhale and alecmerdler November 28, 2018 23:34

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2018

ecordell requested a review from njhale November 28, 2018 23:34

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2018

ecordell changed the title ~~[WIP] don't process CSVs without operatorgroup~~ Don't process CSVs without operatorgroup Nov 28, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2018

alecmerdler reviewed Nov 29, 2018

View reviewed changes

njhale reviewed Dec 3, 2018

View reviewed changes

ecordell force-pushed the copying-in-csv-loop branch from 1b1188c to 7501021 Compare December 3, 2018 16:04

ecordell force-pushed the copying-in-csv-loop branch from 7501021 to 7667f79 Compare December 3, 2018 16:33

ecordell force-pushed the copying-in-csv-loop branch 2 times, most recently from d6b9a4e to 72ebb02 Compare December 6, 2018 15:52

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2018

ecordell force-pushed the copying-in-csv-loop branch 2 times, most recently from f12785d to e48a2c1 Compare December 6, 2018 19:56

jpeeler mentioned this pull request Dec 6, 2018

docs(arch): add operator groups #573

Merged

alecmerdler mentioned this pull request Dec 7, 2018

Lightly Expose OLM Operator Groups openshift/console#918

Merged

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2018

ecordell force-pushed the copying-in-csv-loop branch from c360b8f to a0c1f69 Compare December 10, 2018 20:34

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2018

ecordell mentioned this pull request Dec 10, 2018

Use CSVPhaseSucceeded for Copied ClusterServiceVersions #613

Closed

feat(operatorgroups): don't process CSVs that aren't in an operatorgroup

85a6c8a

this commit also moves the copying / rbac logic into the CSV loop, so that we don't have to requeue operatorgroups frequently

ecordell force-pushed the copying-in-csv-loop branch 2 times, most recently from 527c0ba to cc773f8 Compare December 11, 2018 20:55

feat(olm): generate clusterroles for provided apis in csv loop

bbc68a9

and aggregate them to operatorgroup clusterroles this lets us speed up the tests for operatorgroups and in the future can simplify installplan generation (can remove the clusterrole gen from installplans)

ecordell force-pushed the copying-in-csv-loop branch from cc773f8 to bbc68a9 Compare December 11, 2018 21:00

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2018

ecordell commented Dec 11, 2018

View reviewed changes

jpeeler reviewed Dec 11, 2018

View reviewed changes

njhale reviewed Dec 12, 2018

View reviewed changes

njhale approved these changes Dec 12, 2018

View reviewed changes

openshift-ci-robot assigned njhale Dec 12, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018

openshift-merge-robot merged commit 0d5b431 into operator-framework:master Dec 12, 2018

jpeeler reviewed Dec 12, 2018

View reviewed changes

ecordell deleted the copying-in-csv-loop branch January 9, 2019 13:42

ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#589 from ecordell/copying-in-cs…

3a7c072

…v-loop Don't process CSVs without operatorgroup

njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't process CSVs without operatorgroup #589

Don't process CSVs without operatorgroup #589

ecordell commented Nov 28, 2018

ecordell commented Nov 28, 2018

openshift-ci-robot commented Nov 28, 2018

alecmerdler Nov 29, 2018

openshift-bot commented Dec 2, 2018

njhale Dec 3, 2018

ecordell commented Dec 3, 2018

ecordell commented Dec 4, 2018

ecordell commented Dec 5, 2018

ecordell commented Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

ecordell Dec 11, 2018

jpeeler Dec 11, 2018

ecordell Dec 11, 2018

njhale Dec 12, 2018

ecordell Dec 12, 2018

njhale Dec 12, 2018 •

edited

Loading

njhale left a comment

jpeeler Dec 12, 2018

jpeeler Dec 12, 2018

Don't process CSVs without operatorgroup #589

Don't process CSVs without operatorgroup #589

Conversation

ecordell commented Nov 28, 2018

ecordell commented Nov 28, 2018

openshift-ci-robot commented Nov 28, 2018

Choose a reason for hiding this comment

openshift-bot commented Dec 2, 2018

Choose a reason for hiding this comment

ecordell commented Dec 3, 2018

ecordell commented Dec 4, 2018

ecordell commented Dec 5, 2018

ecordell commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Dec 12, 2018 •

edited

Loading