pkg/scaffold/role,pkg/operator-sdk/new.go: generate role rules from helm chart #1188

joelanford · 2019-03-08T18:01:59Z

Description of the change:
When creating a helm-based project, parse the Helm chart templates to generate the role

Motivation for the change:
Instead of scaffolding a static role, which will likely have unnecessary rules and be missing necessary rules, we can generate the role rules dynamically from the chart templates.

This will enable operators to be generated with the correct RBAC rules for nearly all existing Helm charts.

/cc @dmesser @robszumski

joelanford · 2019-03-11T16:21:05Z

~~This PR currently has a couple of rough edges (and probably others I haven't thought about):~~

Many Helm charts (at least 82 of 266 in the official stable repo) have templates for cluster-scoped resources (e.g. ClusterRole and ClusterRoleBinding), but this PR makes no attempt to discern between templates for namespace-scoped resources and cluster-scoped resources, and therefore groups all the rules together into either a Role or ClusterRole, depending on whether --cluster-scoped is set.

~~The result is that rules that need to be in a ClusterRole incorrectly end up in a Role when --cluster-scoped is not set.~~
To create a rule, we need the pluralized resource name, but all we have in the template is a GVK. This PR makes a best effort attempt to convert a kind to a resource, but some resources may have plural names that don't match our conversion.

EDIT: Both of these issues are now solved in the PR, which now uses the discovery API to lookup all the resources to get their scope and plural name.

lilic

Did a quick review and left some suggestions.

I am not sure if we can somehow make use of https://github.com/liggitt/audit2rbac to generate the RBAC? Or at least have a look what the project does, as it solves a similar problem, but guess that would be more for go operators.

pkg/scaffold/helm/chart.go

lilic · 2019-03-18T12:50:50Z

pkg/scaffold/helm/chart.go

+func GenerateRoleRules(chart *chart.Chart) ([]rbacv1.PolicyRule, []error) {
+	helmOperatorRules := []rbacv1.PolicyRule{
+		// We need this rule so tiller can read namespaces to ensure they exist
+		{


Should we document the following "default" rules we have in place so the user understands and doesn't remove them?

That's a good idea. Unfortunately, the comments get stomped out by scaffold.UpdateRoleForResource because of the marshaling and unmarshaling that happens.

Kubernetes has an open issue addressing this problem: kubernetes/kubernetes#15157

pkg/scaffold/helm/chart.go

lilic · 2019-03-18T13:19:54Z

pkg/scaffold/helm/chart.go

+			}
+
+			// Skip templates that don't look like Kubernetes resources
+			if apiVersion == "" && kind == "" {


What if we just do:

if apiVersion == "" || kind == "" { errs = append(errs, fmt.Errorf("could not find resource kind or apiVersion %s[%d]", name, i)) continue }

This has also been slightly reworked. I left the separate checks for apiVersion and kind so that the logged warning would be more specific.

lilic · 2019-03-18T13:26:35Z

pkg/scaffold/helm/chart.go

+		rules = append(rules, rbacv1.PolicyRule{
+			APIGroups: []string{group},
+			Resources: resources,
+			Verbs:     []string{rbacv1.VerbAll},


So if I understand correctly we are just giving it *, I understand we can't fine tune this, but maybe we can document that the user double checks the generated rules?

I think * is reasonable for these rules because the Helm operator needs to create, get, update, and delete all of the templated resources.

However, since the code now renders the default manifest and uses that to build rules, it is possible that some resources require custom values to be set so that they are present in the release manifest. I'll add a log at the end, that says something along the lines of:

Double check deploy/role.yaml to verify correct RBAC policy rules. The existing rules were created from the default chart manifest, so some rules may be overly broad or missing.

internal/pkg/scaffold/helm/chart.go

…helm chart

…g/scaffold/constants.go to avoid unnecessary dep imports in non-helm projects

internal/pkg/scaffold/constants.go

internal/pkg/scaffold/helm/role.go

internal/pkg/scaffold/helm/chart.go

When fetching chart dependencies, collect the Helm output in a buffer. If an error occurs, output the buffer before returning the error.

…ernal/pkg/scaffold/constants.go to avoid unnecessary dep imports in non-helm projects" This reverts commit fc556e8.

estroz

/lgtm

hasbro17 · 2019-04-10T21:52:21Z

internal/pkg/scaffold/helm/chart.go

+	}
+	if err := man.Build(); err != nil {
+		fmt.Println(out.String())
+		return err


Nit but can we wrap this and other errors like in Line 148. Either here or in the caller.

Otherwise we could end up with error messages that aren't too clear like in #1281
Right now I think this could end up showing something like:

failed to create helm chart: requirements.yaml cannot be opened: ....

https://github.com/helm/helm/blob/master/pkg/downloader/manager.go#L83

whereas I'd prefer to see:

failed to create helm chart: failed to fetch/build chart dependencies: requirements.yaml cannot be opened: ....

hasbro17

LGTM

Worth mentioning this in the CHANGELOG.
And do you think we should point out this behavior out in the helm user-guide?

lilic

lgtm 👍

…eration

openshift-ci-robot · 2019-04-11T13:54:34Z

New changes are detected. LGTM label has been removed.

joelanford added the language/helm Issue is related to a Helm operator project label Mar 8, 2019

joelanford requested review from AlexNPavel, lilic, hasbro17, theishshah and estroz March 8, 2019 18:01

openshift-ci-robot requested review from dmesser and robszumski March 8, 2019 18:02

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 8, 2019

lilic suggested changes Mar 18, 2019

View reviewed changes

joelanford force-pushed the helm-generate-role branch from 68ef514 to 49f4e44 Compare April 1, 2019 14:33

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 Apr 1, 2019

joelanford commented Apr 1, 2019

View reviewed changes

internal/pkg/scaffold/helm/chart.go Show resolved Hide resolved

joelanford force-pushed the helm-generate-role branch 2 times, most recently from f73232e to 1d1800e Compare April 2, 2019 00:32

joelanford added 4 commits April 2, 2019 14:16

pkg/scaffold/role.go: support adding custom rules

d92791b

pkg/scaffold/,commands/operator-sdk/new.go: generate role rules from …

d4959c5

…helm chart

internal/pkg/scaffold/helm/chart.go: fetch chart dependencies

d1004a9

cmd/operator-sdk,internal: move HelmChartsDir constant to internal/pk…

fc556e8

…g/scaffold/constants.go to avoid unnecessary dep imports in non-helm projects

joelanford force-pushed the helm-generate-role branch from 1d1800e to fc556e8 Compare April 2, 2019 18:16

lilic reviewed Apr 8, 2019

View reviewed changes

internal/pkg/scaffold/constants.go Outdated Show resolved Hide resolved

lilic reviewed Apr 8, 2019

View reviewed changes

internal/pkg/scaffold/helm/role.go Show resolved Hide resolved

estroz reviewed Apr 8, 2019

View reviewed changes

internal/pkg/scaffold/helm/chart.go Outdated Show resolved Hide resolved

joelanford added 3 commits April 8, 2019 18:08

internal/pkg/scaffold/helm/chart.go: better error output

f54ec13

When fetching chart dependencies, collect the Helm output in a buffer. If an error occurs, output the buffer before returning the error.

Revert "cmd/operator-sdk,internal: move HelmChartsDir constant to int…

f6b6573

…ernal/pkg/scaffold/constants.go to avoid unnecessary dep imports in non-helm projects" This reverts commit fc556e8.

Merge branch 'master' into helm-generate-role

e8517dc

estroz approved these changes Apr 10, 2019

View reviewed changes

openshift-ci-robot assigned estroz Apr 10, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019

hasbro17 reviewed Apr 10, 2019

View reviewed changes

hasbro17 approved these changes Apr 10, 2019

View reviewed changes

lilic approved these changes Apr 11, 2019

View reviewed changes

joelanford added 2 commits April 11, 2019 09:52

internal/pkg/scaffold/helm/chart.go: better error wrapping

31d4647

CHANGELOG.md,doc/helm/user-guide.md: include note about RBAC rule gen…

6719441

…eration

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2019

joelanford merged commit 76d13fc into operator-framework:master Apr 11, 2019

joelanford deleted the helm-generate-role branch April 12, 2019 01:21

joelanford mentioned this pull request Jun 4, 2019

pkg/helm/run.go,pkg/internal/scaffold: helm operator metrics #1482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/scaffold/role,pkg/operator-sdk/new.go: generate role rules from helm chart #1188

pkg/scaffold/role,pkg/operator-sdk/new.go: generate role rules from helm chart #1188

joelanford commented Mar 8, 2019

joelanford commented Mar 11, 2019 •

edited

Loading

lilic left a comment •

edited

Loading

lilic Mar 18, 2019

joelanford Apr 1, 2019

lilic Mar 18, 2019

joelanford Apr 1, 2019

lilic Mar 18, 2019

joelanford Apr 1, 2019

estroz left a comment

hasbro17 Apr 10, 2019

hasbro17 left a comment

lilic left a comment

openshift-ci-robot commented Apr 11, 2019

pkg/scaffold/role,pkg/operator-sdk/new.go: generate role rules from helm chart #1188

pkg/scaffold/role,pkg/operator-sdk/new.go: generate role rules from helm chart #1188

Conversation

joelanford commented Mar 8, 2019

joelanford commented Mar 11, 2019 • edited Loading

lilic left a comment • edited Loading

Choose a reason for hiding this comment

lilic Mar 18, 2019

Choose a reason for hiding this comment

joelanford Apr 1, 2019

Choose a reason for hiding this comment

lilic Mar 18, 2019

Choose a reason for hiding this comment

joelanford Apr 1, 2019

Choose a reason for hiding this comment

lilic Mar 18, 2019

Choose a reason for hiding this comment

joelanford Apr 1, 2019

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

hasbro17 Apr 10, 2019

Choose a reason for hiding this comment

hasbro17 left a comment

Choose a reason for hiding this comment

lilic left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 11, 2019

joelanford commented Mar 11, 2019 •

edited

Loading

lilic left a comment •

edited

Loading