generate: consider service accounts when generating a CSV by estroz · Pull Request #3610 · operator-framework/operator-sdk

estroz · 2020-07-30T01:59:32Z

Description of the change:

internal/cmd/operator-sdk/generate: write RBAC objects to stdout or files named with object.Name + GVK, and rename --update-crds to --update-objects
internal/generate/{collector/clusterserviceversion}: consider (cluster) role bindings so CSV generator can assign the correct service account names to roles

Motivation for the change: This PR adds handling for extra RBAC objects present in generate <bundle|packagemanifests> input. These objects will be written to the resulting bundle. For now, only Roles, RoleBindings, their Cluster equivalents, and ServiceAccounts are written.

This PR also correctly names service account for (cluster) role permissions. These are currently incorrect because the CSV generator is naively using (cluster) role names instead of actual service account names. Previously this was ok because the names match the service account, but this is no longer the case. See #3600.

Old test data has been removed, and a static basic.operator.yaml containing the output of kustomize build config/manifests added; the static file's contents match a current project manifest build.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

estroz · 2020-07-31T20:25:42Z

The algorithm for writing permissions, as a result of discussion in #3610 (comment) and with @joelanford offline:

for each Deployment in input:
1. set operatorSA to .spec....serviceAccountName, or to "default" if empty
2. for each (Cluster)Role in the input:
  1. find a referent (Cluster)RoleBinding
  2. if a referent does not exist, write the (Cluster)Role to the manifests directory
  3. if a referent does exist:
    1. if it binds to operatorSA, add the (Cluster)Role rules to the CSV (cluster)permissions for operatorSA
    2. if it does not bind to operatorSA, write the referent (Cluster)RoleBinding and the referenced (Cluster)Role to the manifests directory

`generate <bundle|packagemanifests>` input. These objects will be written to the resulting bundle. For now, only Roles, RoleBindings, their Cluster equivalents, and ServiceAccounts are written. internal/cmd/operator-sdk/generate: write RBAC objects to stdout or files named with object.Name + GVK internal/generate/{collector/clusterserviceversion}: consider (cluster) role bindings so CSV generator can assign the correct service account names to roles

jmrodri

/lgtm

estroz · 2020-08-01T00:47:43Z

@@ -8,10 +8,10 @@ metadata:
          "apiVersion": "cache.example.com/v1alpha1",


These testdata changes can be refactored into another PR, they're not directly related to the bugfix.

joelanford

/lgtm

…3610) (#3714)

The bundle generated by operator-sdk v0.17.0 contains incorrect cluster permission service account name. This issue is fixed in v0.19.0. Patch using yq with the correct value. Refer: operator-framework/operator-sdk#3610 Processing the CSV file via yq changes the indentations, resulting in unrelated changes to the file.

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 Jul 30, 2020

openshift-ci-robot requested review from hasbro17, joelanford and rashmigottipati July 30, 2020 01:59

estroz commented Jul 30, 2020

View reviewed changes

Comment thread internal/generate/clusterserviceversion/clusterserviceversion_updaters.go Outdated

estroz commented Jul 30, 2020

View reviewed changes

Comment thread internal/generate/clusterserviceversion/clusterserviceversion_updaters.go Outdated

estroz force-pushed the bugfix/csv-role-bindings branch 4 times, most recently from 8ae4709 to 2389149 Compare July 30, 2020 16:05

estroz changed the title ~~[WIP] internal/generate: consider role bindings when generating a CSV~~ internal/generate: consider role bindings when generating a CSV Jul 30, 2020

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 Jul 30, 2020

estroz added this to the v1.0.0 milestone Jul 30, 2020

estroz added the kind/bug Categorizes issue or PR as related to a bug. label Jul 30, 2020

joelanford mentioned this pull request Jul 31, 2020

Operator SDK 1.0 Meta-issue #3327

Closed

92 tasks

estroz force-pushed the bugfix/csv-role-bindings branch 3 times, most recently from f99bba0 to 452d972 Compare July 31, 2020 19:57

estroz force-pushed the bugfix/csv-role-bindings branch from 452d972 to 1adee98 Compare August 1, 2020 00:22

jmrodri approved these changes Aug 1, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Aug 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2020

joelanford reviewed Aug 1, 2020

View reviewed changes

Comment thread internal/generate/clusterserviceversion/clusterserviceversion_test.go Outdated

estroz force-pushed the bugfix/csv-role-bindings branch from 1adee98 to 5c7a47d Compare August 1, 2020 00:38

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2020

estroz force-pushed the bugfix/csv-role-bindings branch 2 times, most recently from 14ec7f9 to b4ec8e4 Compare August 1, 2020 00:43

estroz commented Aug 1, 2020

View reviewed changes

split CSV and non-CSV RBAC using collector.Manifests method

8aebd0c

estroz force-pushed the bugfix/csv-role-bindings branch from b4ec8e4 to 8aebd0c Compare August 1, 2020 00:51

joelanford approved these changes Aug 1, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Aug 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2020

estroz changed the title ~~internal/generate: consider role bindings when generating a CSV~~ generate: consider service accounts when generating a CSV Aug 1, 2020

estroz merged commit 3270033 into operator-framework:master Aug 1, 2020

estroz deleted the bugfix/csv-role-bindings branch August 1, 2020 01:26

This was referenced Aug 3, 2020

Generate Bundle Incorrect ClusterPermissions ServiceAccountName #3600

Closed

test/integration: use default service account for CSV perms #3668

Merged

estroz pushed a commit to estroz/operator-sdk that referenced this pull request Aug 10, 2020

cherry-pick operator-framework#3610

d01e9e7

estroz pushed a commit to estroz/operator-sdk that referenced this pull request Aug 10, 2020

cherry-pick operator-framework#3610

2116703

estroz mentioned this pull request Aug 10, 2020

[v0.19.x] generate: consider service accounts when generating a CSV (#3610) #3714

Merged

estroz pushed a commit to estroz/operator-sdk that referenced this pull request Aug 10, 2020

cherry-pick operator-framework#3610

b9e10a6

estroz pushed a commit that referenced this pull request Aug 17, 2020

[v0.19.x] generate: consider service accounts when generating a CSV (#…

48929b8

…3610) (#3714)

darkowlzz mentioned this pull request Nov 6, 2020

Fix ServiceAccountName in the OLM bundle storageos/cluster-operator#290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate: consider service accounts when generating a CSV#3610

generate: consider service accounts when generating a CSV#3610
estroz merged 2 commits intooperator-framework:masterfrom
estroz:bugfix/csv-role-bindings

estroz commented Jul 30, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

estroz commented Jul 31, 2020 •

edited

Loading

Uh oh!

jmrodri left a comment

Uh oh!

Uh oh!

estroz Aug 1, 2020

Uh oh!

joelanford left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -8,10 +8,10 @@ metadata:
		"apiVersion": "cache.example.com/v1alpha1",

Conversation

estroz commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

estroz commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

estroz Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

estroz commented Jul 30, 2020 •

edited

Loading

estroz commented Jul 31, 2020 •

edited

Loading