Install flag for helm and additional kustomize file for raw yaml for pure Namespace scoping #149

ryansteakley · 2021-08-05T23:06:11Z

Issue #, if available:
aws-controllers-k8s/community#770

Description of changes:

Adds namespacedInstallation flag for helm values.yaml to specify that Role and RoleBinding should be used instead of ClusterRole and ClusterRoleBinding
Specifies that watchNamespace in helmvalues.yaml must be set if using namespacedInstallation flag.
Contains a sed to replace the original output of the role-controller.yaml file with the template code that allows for the programmatic setting of namespacedInstallation in the helm values.yaml
Adds a overlays/namespaced directory for the raw yamls in config. This folder contains the json patch files, role.yaml and role-binding.yaml that will modify the ClusterRole and ClusterRoleBinding files to be Role and RoleBinding. This allows the user to apply these changes using raw yaml and not helm if these wish.
Chose to reuse the names of cluster-role-binding.yaml and cluster-role-controller.yaml so that every service repo doesn't need to rm these files since they already exist in their repo and any other naming of the yaml files will cause multiple roles/bindings to install causing installation failures.

Tested locally

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot · 2021-08-05T23:06:20Z

Hi @ryansteakley. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

RedbackThomson

+1 on using JSON patches with Kustomize
+1 on working around the limitation with the autogenerated role YAML file

I have some inline suggestions about file extensions and names.

templates/config/overlays/namespaced/kustomization.yaml.tpl

templates/helm/values.yaml.tpl

RedbackThomson · 2021-08-05T23:30:48Z

templates/config/overlays/namespaced/role-binding.json

+[{"op": "replace",
+    "path": "/kind",
+    "value": "RoleBinding"},
+


Please remove excess whitespace

Still some excess whitespace here. Probably can go to 1 line per object:

[{"op": "replace", "path": "/kind", "value": "RoleBinding"}, {"op": "add", "path": "/metadata/namespace", "value": "ack-system"}, {"op": "replace","path": "/roleRef/kind","value": "Role"}]

templates/helm/templates/_namespaced-install.tpl

scripts/build-controller-release.sh

templates/helm/templates/_namespaced-install.tpl

templates/helm/values.yaml.tpl

…ked inside of conditional

vijtrip2 · 2021-08-06T16:49:24Z

templates/config/overlays/namespaced/kustomization.yaml

+    group: rbac.authorization.k8s.io
+    version: v1
+    kind: ClusterRole
+    name: ack-sagemaker-controller


Does "sagemaker" need to be hardcoded here?

Thought I removed this must have pushed it again by accident :(

Fair point. These might need to be made into Go templates and inject {{ .ServiceIDClean }} just like the cluster-role-binding.yaml.tpl

Yep had to make them into go templates and put them in release.go and controller.go respectively

templates/helm/templates/_controller-role-kind-patch.yaml

jaypipes

@ryansteakley you marked a number of conversations between you and @RedbackThomson as resolved, but I don't actually see changes made... did you forget to git push your changes?

RedbackThomson

Just have one more concern about our helm patch file - see inline comments

templates/helm/templates/_controller-role-kind-patch.yaml.tpl

RedbackThomson · 2021-08-09T19:21:18Z

templates/config/overlays/namespaced/role-binding.json

+[{"op": "replace",
+    "path": "/kind",
+    "value": "RoleBinding"},
+


Still some excess whitespace here. Probably can go to 1 line per object:

[{"op": "replace", "path": "/kind", "value": "RoleBinding"}, {"op": "add", "path": "/metadata/namespace", "value": "ack-system"}, {"op": "replace","path": "/roleRef/kind","value": "Role"}]

ryansteakley · 2021-08-09T21:22:31Z

@ryansteakley you marked a number of conversations between you and @RedbackThomson as resolved, but I don't actually see changes made... did you forget to git push your changes?

Going to look into it, it is very possible some changes got git stash and never applied again

Some of the conversations related to file naming extensions ended up in keeping the .tpl extension as they were needed to get {{ .ServiceIDClean }} injected

…ation

RedbackThomson

Awesome. I think you've nailed it, now!

jaypipes

this needs a bit more thought put into it. I need to do some more review here.

jaypipes · 2021-08-11T13:45:00Z

templates/helm/values.yaml.tpl


+# If specified, the service controller will only be able to operate in a single namespace. Set by default to K8S_NAMESPACE
+# If set to true, user must set watchNamespace as well.
+namespacedInstallation: false


Why do we need this additional option? If watchNamespace is required, then why can't we just switch on watchNamespace != ""?

A customer may want to set the watchNamespace but still have their resource be cluster-scoped. This allows for flexibility in changing the watchNamespace value later, and not having to redeploy anything except the deployment. It also means that a user could install the controller into the ack-system namespace and watch another namespace. Installing as namespacedInstallation = true requires the Helm chart be installed into the watchNamespace

A customer may want to set the watchNamespace but still have their resource be cluster-scoped.

Why? Isn't the whole point of namespaced installation to limit the CRDs, the RBACs that can read those CRs and the namespace that those CRs can be created in to just a single namespace?

I am trying to not conflate the two ideas:

Watch a single namespace

Be installed into the scope of the single namespace

Users may want 1 but not 2. But if they want 2, it is required that they also do 1.

If you want to bundle them up into the same thing and ship them together, then ok I'm on board. I wanted to offer the choice, at least.

Note: I guess I don't understand how customers are using namespace scoping well enough to draw a judgement on what they would want.

jaypipes · 2021-08-11T13:45:50Z

templates/helm/templates/cluster-role-binding.yaml.tpl

@@ -1,11 +1,22 @@
 apiVersion: rbac.authorization.k8s.io/v1
+{{ "{{ if not .Values.namespacedInstallation }}" }}


See below... this could simply be:

{{ "{{ if not .Values.watchNamespace }}" }}

templates/helm/templates/cluster-role-binding.yaml.tpl

jaypipes · 2021-08-11T13:48:33Z

templates/helm/templates/_controller-role-kind-patch.yaml.tpl

@@ -0,0 +1,13 @@
+apiVersion: rbac.authorization.k8s.io/v1
+{{ "{{ if not .Values.namespacedInstallation }}" }}


See below... I don't see why this can't just be:

{{ "{{ if not .Values.watchNamespace }}" }}

jaypipes · 2021-08-11T13:48:56Z

templates/helm/templates/_controller-role-kind-patch.yaml.tpl

+metadata:
+  creationTimestamp: null
+  name: ack-{{ .ServiceIDClean }}-controller
+{{ "{{ else if and .Values.namespacedInstallation (required \"watchNamespace must be set for namespaced installation\"  .Values.watchNamespace) }}" }}


and this would not be necessary... could just be {{ else }}

templates/helm/templates/_controller-role-kind-patch.yaml.tpl

jaypipes · 2021-08-11T13:58:01Z

scripts/build-controller-release.sh

+# `cluster-role-controller.yaml` to better reflect what is in that file. We additionally add the ability
+# for the user to specify if they want the role to be ClusterRole or Role by specifying installation scope
+# in the helm values.yaml
+sed -e '1r '"$helm_output_dir/templates/_controller-role-kind-patch.yaml"''  -e '1, 7d'  $helm_output_dir/templates/role.yaml > $helm_output_dir/templates/cluster-role-controller.yaml


oh boy...

Way too much magic and sed-fu to be readable and understandable by mere mortals.

We need to find a clearer way of doing this.

Issue is that the role.yaml file is auto-generated by controller-gen. So there will have to be some overwriting of the auto-generated file with the lines of code we want to allow the user to switch programmatically between clusterrole and role.

jaypipes · 2021-08-11T14:00:59Z

templates/config/overlays/namespaced/role-binding.json

@@ -0,0 +1,3 @@
+[{"op": "replace", "path": "/kind", "value": "RoleBinding"},
+{"op": "add", "path": "/metadata/namespace", "value":  "ack-system"},


This is replacing the namespace that should be supplied by the user in .Values.watchNamespace with the hard-coded string "ack-system", which is defeating the original purpose of a namespace-scoped installation, no?

This file is for the Kustomize overlay - no Helm values to replace, here. All of these Kustomize files use ack-system as the default namespace. The user will need to override the values for all resources anyway, so may as well make this one match.

ah, yes, ok that makes sense.

jaypipes · 2021-08-11T14:01:16Z

templates/config/overlays/namespaced/role.json

@@ -0,0 +1,2 @@
+[{"op": "replace", "path": "/kind", "value": "Role"},
+{"op": "add", "path": "/metadata/namespace", "value": "ack-system"}]


same comment as above about hard-coded "ack-system".

jaypipes · 2021-08-11T14:03:52Z

/hold

…tion

…pe with a default of cluster

RedbackThomson · 2021-08-13T20:18:28Z

As described on Slack with @jaypipes and @ryansteakley:
With the current implementation of .Values.watchNamespace, there are two places where the namespace will need to be specified when installing with namespace-scope. Those two places are the {{ .Release.Namespace }} (--namespace when calling helm install) as well as the --set watchNamespace value. When installing in namespace-scope, these values NEED to be identical, otherwise the Role will not have the privilege to access the resources.

I suggest that we remove the watchNamespace value altogether, and default the --watch-namespace argument to {{ .Release.Namespace}}. We should also have a value to track whether the controller should be installed into cluster or namespace scope - e.g. clusterScope: cluster|namespace. If the clusterScope == namespace, then we change all ClusterRole to Role, and set the --watch-namespace argument.

This would reduce the chance of a user accidentally applying the Helm chart into a namespace where the deployment does not have sufficient privileges. It also reduces the number of independent variables that need to align for a correct configuration.

RedbackThomson · 2021-08-13T22:11:38Z

templates/helm/templates/_helpers.tpl

+{{- define "watch-namespace" -}}
+{{- if eq .Values.installScope "namespace" -}}
+{{- .Release.Namespace -}}
+{{- end -}}
+{{- end -}}


Have we confirmed that this defaults to an empty string when install scope is cluster?

Environment: K8S_NAMESPACE: ack-system (v1:metadata.namespace) AWS_ACCOUNT_ID: 123456789012 AWS_REGION: us-west-1 AWS_ENDPOINT_URL: ACK_WATCH_NAMESPACE: ACK_ENABLE_DEVELOPMENT_LOGGING: true ACK_LOG_LEVEL: debug ACK_RESOURCE_TAGS: services.k8s.aws/managed=true,services.k8s.aws/created=%UTCNOW%,services.k8s.aws/namespace=%KUBERNETES_NAMESPACE% AWS_ROLE_ARN: arn:aws:iam::123456789012:role/ack-controller-role-namespaced AWS_WEB_IDENTITY_TOKEN_FILE: /var/run/secrets/eks.amazonaws.com/serviceaccount/token

Defaults to an empty string when using cluster.
When using namespace, value is correctly passed.

Environment: K8S_NAMESPACE: ack-system (v1:metadata.namespace) AWS_ACCOUNT_ID: 123456789012 AWS_REGION: us-west-1 AWS_ENDPOINT_URL: ACK_WATCH_NAMESPACE: ack-system ACK_ENABLE_DEVELOPMENT_LOGGING: true ACK_LOG_LEVEL: debug ACK_RESOURCE_TAGS: services.k8s.aws/managed=true,services.k8s.aws/created=%UTCNOW%,services.k8s.aws/namespace=%KUBERNETES_NAMESPACE% AWS_ROLE_ARN: arn:aws:iam::123456789012:role/ack-controller-role-namespaced AWS_WEB_IDENTITY_TOKEN_FILE: /var/run/secrets/eks.amazonaws.com/serviceaccount/token

RedbackThomson · 2021-08-13T22:13:14Z

templates/helm/templates/cluster-role-binding.yaml.tpl

+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: ack-{{ .ServiceIDClean }}-controller


nit: Remove duplication by moving apiGroup and name to after {{ end }} (and removing from both conditional blocks)

RedbackThomson · 2021-08-13T22:13:57Z

templates/helm/values.yaml.tpl


-# If specified, the service controller will watch for object creation only in the provided namespace
-watchNamespace: ""
+# Set to namespace to install the controller in a namespaced scope, will only watch for object creation 


nit: For clarity,

Suggested change

# Set to namespace to install the controller in a namespaced scope, will only watch for object creation

# Set to "namespace" to install the controller in a namespaced scope, will only watch for object creation

jaypipes

Yeah, I like it.

jaypipes · 2021-08-16T11:45:05Z

templates/config/overlays/namespaced/role-binding.json

@@ -0,0 +1,3 @@
+[{"op": "replace", "path": "/kind", "value": "RoleBinding"},
+{"op": "add", "path": "/metadata/namespace", "value":  "ack-system"},


ah, yes, ok that makes sense.

jaypipes · 2021-08-16T12:45:05Z

@RedbackThomson feel free to lgtm if you're also good with this.

a-hilaly

👍

RedbackThomson

Ship it!
/lgtm

ack-bot · 2021-08-16T19:03:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, ryansteakley

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~~ [A-Hilaly,RedbackThomson,jaypipes]

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

RedbackThomson · 2021-08-16T19:07:17Z

/unhold

add template files and namespaced installation arg to helm values

74c5edc

ack-bot requested review from jaypipes and vijtrip2 August 5, 2021 23:06

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2021

RedbackThomson reviewed Aug 5, 2021

View reviewed changes

Change non go template files from .tpl to .yaml and add required bloc…

a5f977a

…ked inside of conditional

vijtrip2 reviewed Aug 6, 2021

View reviewed changes

use go templating as to not hardcode service-name

46bace7

ryansteakley force-pushed the namespaced-installation branch from 8a9e3c0 to 46bace7 Compare August 6, 2021 18:35

jaypipes reviewed Aug 9, 2021

View reviewed changes

RedbackThomson suggested changes Aug 9, 2021

View reviewed changes

remove whitespace and rm .yaml file from helm directory after install…

0cfc640

…ation

RedbackThomson approved these changes Aug 11, 2021

View reviewed changes

ack-bot added the approved label Aug 11, 2021

jaypipes requested changes Aug 11, 2021

View reviewed changes

ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2021

ryansteakley added 3 commits August 11, 2021 12:43

change from sed-fu to tail-fu

7d370da

Merge remote-tracking branch 'upstream/main' into namespaced-installa…

b7b9eb4

…tion

remove watchNamespace and change namespacedInstallation to installSco…

66af7d7

…pe with a default of cluster

fix indent

a032924

RedbackThomson reviewed Aug 13, 2021

View reviewed changes

jaypipes approved these changes Aug 16, 2021

View reviewed changes

a-hilaly approved these changes Aug 16, 2021

View reviewed changes

reword comment and move code to remove duplication

34bff50

RedbackThomson approved these changes Aug 16, 2021

View reviewed changes

ack-bot assigned RedbackThomson Aug 16, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2021

ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 16, 2021

ack-bot merged commit ac31817 into aws-controllers-k8s:main Aug 16, 2021

		@@ -1,11 +1,22 @@
		apiVersion: rbac.authorization.k8s.io/v1
		{{ "{{ if not .Values.namespacedInstallation }}" }}

		@@ -0,0 +1,13 @@
		apiVersion: rbac.authorization.k8s.io/v1
		{{ "{{ if not .Values.namespacedInstallation }}" }}

		@@ -0,0 +1,3 @@
		[{"op": "replace", "path": "/kind", "value": "RoleBinding"},
		{"op": "add", "path": "/metadata/namespace", "value": "ack-system"},

		@@ -0,0 +1,2 @@
		[{"op": "replace", "path": "/kind", "value": "Role"},
		{"op": "add", "path": "/metadata/namespace", "value": "ack-system"}] No newline at end of file

	# Set to namespace to install the controller in a namespaced scope, will only watch for object creation
	# Set to "namespace" to install the controller in a namespaced scope, will only watch for object creation

Install flag for helm and additional kustomize file for raw yaml for pure Namespace scoping #149

Install flag for helm and additional kustomize file for raw yaml for pure Namespace scoping #149

Uh oh!

Conversation

ryansteakley commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Aug 5, 2021

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryansteakley commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryansteakley Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryansteakley commented Aug 5, 2021 •

edited

Loading

ryansteakley commented Aug 9, 2021 •

edited

Loading

RedbackThomson Aug 12, 2021 •

edited

Loading

ryansteakley Aug 11, 2021 •

edited

Loading