Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 16, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
74c5edc
add template files and namespaced installation arg to helm values
ryansteakley Aug 5, 2021
a5f977a
Change non go template files from .tpl to .yaml and add required bloc…
ryansteakley Aug 6, 2021
46bace7
use go templating as to not hardcode service-name
ryansteakley Aug 6, 2021
0cfc640
remove whitespace and rm .yaml file from helm directory after install…
ryansteakley Aug 10, 2021
7d370da
change from sed-fu to tail-fu
ryansteakley Aug 11, 2021
b7b9eb4
Merge remote-tracking branch 'upstream/main' into namespaced-installa…
ryansteakley Aug 12, 2021
66af7d7
remove watchNamespace and change namespacedInstallation to installSco…
ryansteakley Aug 13, 2021
a032924
fix indent
ryansteakley Aug 13, 2021
34bff50
reword comment and move code to remove duplication
ryansteakley Aug 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions scripts/build-controller-release.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,12 @@ controller-gen rbac:roleName=$K8S_RBAC_ROLE_NAME paths=./... output:rbac:artifac
# controller-gen rbac outputs a ClusterRole definition in a
# $config_output_dir/rbac/role.yaml file. We have some other standard Role
# files for a reader and writer role, so here we rename the `role.yaml` file to
# `cluster-role-controller.yaml` to better reflect what is in that file.
mv $helm_output_dir/templates/role.yaml $helm_output_dir/templates/cluster-role-controller.yaml
# `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
cp -r $ROOT_DIR/templates/helm/templates/_namespaced-install.tpl $helm_output_dir/templates/
sed -e '1r '"$helm_output_dir/templates/_namespaced-install.tpl"'' -e '1, 7d' $helm_output_dir/templates/role.yaml > $helm_output_dir/templates/cluster-role-controller.yaml
rm $helm_output_dir/templates/role.yaml

popd 1>/dev/null

Expand Down
9 changes: 7 additions & 2 deletions scripts/build-controller.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,13 @@ controller-gen rbac:roleName=$K8S_RBAC_ROLE_NAME paths=./... output:rbac:artifac
# controller-gen rbac outputs a ClusterRole definition in a
# $config_output_dir/rbac/role.yaml file. We have some other standard Role
# files for a reader and writer role, so here we rename the `role.yaml` file to
# `cluster-role-controller.yaml` to better reflect what is in that file.
mv $config_output_dir/rbac/role.yaml $config_output_dir/rbac/cluster-role-controller.yaml
# `role-controller.yaml` to better reflect what is in that file.
mv $config_output_dir/rbac/role.yaml $config_output_dir/rbac/role-controller.yaml
# Copy definitions for json patches which allow the user to patch the controller
# with Role/Rolebinding and be purely namespaced scoped instead of using Cluster/ClusterRoleBinding
# using kustomize
mkdir -p $config_output_dir/overlays/namespaced
cp -r $ROOT_DIR/templates/config/overlays/namespaced/* $config_output_dir/overlays/namespaced

popd 1>/dev/null

Expand Down
15 changes: 15 additions & 0 deletions templates/config/overlays/namespaced/kustomization.yaml.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
resources:
- ../../default
patches:
- path: role.json
target:
group: rbac.authorization.k8s.io
version: v1
kind: ClusterRole
name: ack-sagemaker-controller
- path: role-binding.json
target:
group: rbac.authorization.k8s.io
version: v1
kind: ClusterRoleBinding
name: ack-sagemaker-controller-rolebinding
14 changes: 14 additions & 0 deletions templates/config/overlays/namespaced/role-binding.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[{"op": "replace",
"path": "/kind",
"value": "RoleBinding"},

Copy link
Contributor

@RedbackThomson RedbackThomson Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove excess whitespace

Copy link
Contributor

@RedbackThomson RedbackThomson Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"}]


{"op": "add",
"path": "/metadata/namespace",
"value": "ack-system"},


{"op": "replace",
"path": "/roleRef/kind",
"value": "Role"}
]
9 changes: 9 additions & 0 deletions templates/config/overlays/namespaced/role.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[{"op": "replace",
"path": "/kind",
"value": "Role"},


{"op": "add",
"path": "/metadata/namespace",
"value": "ack-system"}
]
15 changes: 15 additions & 0 deletions templates/helm/templates/_namespaced-install.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@

---
apiVersion: rbac.authorization.k8s.io/v1
{{ if not .Values.namespacedInstallation }}
kind: ClusterRole
metadata:
creationTimestamp: null
name: ack-sagemaker-controller
{{ else }}
kind: Role
metadata:
creationTimestamp: null
name: ack-sagemaker-controller
namespace: {{ .Release.Namespace }}
{{ end }}
11 changes: 11 additions & 0 deletions templates/helm/templates/cluster-role-binding.yaml.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
apiVersion: rbac.authorization.k8s.io/v1
{{ "{{ if not .Values.namespacedInstallation }}" }}
Copy link
Collaborator

@jaypipes jaypipes Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below... this could simply be:

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

kind: ClusterRoleBinding
metadata:
name: {{ "{{ include \"app.fullname\" . }}" }}
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: ack-{{ .ServiceIDClean }}-controller
{{ "{{ else }}" }}
kind: RoleBinding
metadata:
name: {{ "{{ include \"app.fullname\" . }}" }}
namespace: {{ "{{ .Release.Namespace }}" }}
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: ack-{{ .ServiceIDClean }}-controller
Copy link
Contributor

@RedbackThomson RedbackThomson Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{{ "{{ end }}" }}
subjects:
- kind: ServiceAccount
name: {{ "{{ include \"service-account.name\" . }}" }}
Expand Down
4 changes: 4 additions & 0 deletions templates/helm/values.yaml.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ log:
# If specified, the service controller will watch for object creation only in the provided namespace
watchNamespace: ""

# If specified, the service controller will use Role/Rolebinding instead of ClusterRole/ClusterRoleBindings
# If set to true, user must set watchNamespace as well.
namespacedInstallation: false
Copy link
Collaborator

@jaypipes jaypipes Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@RedbackThomson RedbackThomson Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@jaypipes jaypipes Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@RedbackThomson RedbackThomson Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to not conflate the two ideas:

  1. Watch a single namespace
  2. 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.


resourceTags:
# Configures the ACK service controller to always set key/value pairs tags on resources that it manages.
- services.k8s.aws/managed=true
Expand Down