Allow to install kubeapps in several namespaces #508

andresmgot · 2018-08-20T09:24:35Z

Fixes #463

Install AppRepositories CRD only if it's not present (and keep it on deletion).
Add cleanup job for deleting AppRepository instances.
Remove user predefined ClusterRoles.
Adapt documentation.

migmartri

Thank you!

migmartri · 2018-08-20T18:26:39Z

chart/kubeapps/README.md

 ```

-The command removes all the Kubernetes components associated with the chart and deletes the release.
+The first command removes most of the Kubernetes components associated with the chart and deletes the release. After that, if there are no more instances of Kubeapps in the cluster you can manually delete the `apprepositories.kubeapps.com` CRD used by Kubeapps that is shared for the entire cluster.


I'd make it more explicit that if you delete the CRD you will break other instances of Kubeapps running in the same cluster.

migmartri · 2018-08-20T18:27:44Z

chart/kubeapps/templates/apprepository-crd.yaml

@@ -1,7 +1,10 @@
+{{- if not (.Capabilities.APIVersions.Has "kubeapps.com/v1alpha1") -}}


Would you mind adding a comment mentioning that this might have happened if another instance of kubeapps has been installed.

migmartri · 2018-08-20T18:28:50Z

chart/kubeapps/templates/apprepository-jobs-cleanup-rbac.yaml

+  - kubeapps.com
+  resources:
+  - apprepositories
+  verbs:


Do you need all these verbs?

nope, only list and delete

migmartri · 2018-08-20T18:29:15Z

chart/kubeapps/templates/apprepository-jobs-cleanup-rbac.yaml

@@ -0,0 +1,47 @@
+{{- if .Values.rbac.create -}}


Ditto previous comment about adding some context on why this this job is needed in a comment.

added in all the cleanup related files.

migmartri · 2018-08-20T18:34:48Z

docs/user/access-control.md


 ```
+kubectl apply -f - << EOF


Maybe for a first version it might make sense to store these yaml in the repo so it can be easier installed via kubectl create -f 'http://github.com/foobar.yaml'

I like having the role definition here because it's more verbose and users won't be applying them blindly. Also we'd avoid possible broken links if we move/delete those files from the repo.

In any case, if you think it's a price worth to pay for the simplicity it provides I am fine as well.

rethinking about this, having a file is more windows-friendly (I don't think the << EOF works there)

migmartri · 2018-08-20T18:35:28Z

docs/user/access-control.md

@@ -60,10 +73,28 @@ available in most Kubernetes distributions, you can find more information about
 that role
 [here](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#user-facing-roles).

+Apart from that role we need to create an additional one to have read access to the app repositories.


I'd put the creation of the repositories-read role just after this sentence since you set the stage.

Apart from that role we need to create an additional one to have read access to the app repositories.

Additionally, we need to create a role to give read access to App Repositories and bind it to our service account.

migmartri · 2018-08-20T18:37:34Z

docs/user/access-control.md

+  - clusterserviceplans
+  verbs:
+  - list
+EOF


I definitely think that we need to extract these yamls to external files since these documentation is becoming too dense.

In the meantime I'd add spaces and comments between the different steps.

andresmgot · 2018-08-21T11:05:44Z

waiting for @prydonius's input here

prydonius · 2018-08-22T20:19:39Z

chart/kubeapps/README.md

-The command removes all the Kubernetes components associated with the chart and deletes the release.
+The first command removes most of the Kubernetes components associated with the chart and deletes the release. After that, if there are no more instances of Kubeapps in the cluster you can manually delete the `apprepositories.kubeapps.com` CRD used by Kubeapps that is shared for the entire cluster.
+
+> **NOTE**: If you delete the CRD for `apprepositories.kubeapps.com` it will delete the repositories for **all** the installed instances of `kubeapps`. That'll break existing installations of `kubeapps` if they exist.


That'll -> This will

prydonius · 2018-08-22T20:21:46Z

chart/kubeapps/templates/apprepository-jobs-cleanup-rbac.yaml

+metadata:
+  name: {{ template "kubeapps.apprepository-jobs-cleanup.fullname" . }}
+  annotations:
+    helm.sh/hook: pre-delete


should this be post-delete? What if deletion fails?

yes, it will fit better in the post-delete hook

prydonius · 2018-08-22T20:22:39Z

chart/kubeapps/templates/apprepository-jobs-cleanup-rbac.yaml

@@ -0,0 +1,47 @@
+{{- if .Values.rbac.create -}}
+# The cleanup job is necessary to remove "apprepositories" created


I don't think we need this comment here, just in the Job

prydonius · 2018-08-22T20:22:50Z

chart/kubeapps/templates/apprepository-jobs-cleanup-serviceaccount.yaml

@@ -0,0 +1,15 @@
+# The cleanup job is necessary to remove "apprepositories" created


I don't think we need this comment here, just in the Job

prydonius · 2018-08-22T20:23:15Z

chart/kubeapps/templates/apprepository-jobs-cleanup.yaml

@@ -0,0 +1,47 @@
+# The cleanup job is necessary to remove "apprepositories" created
+# in the bootstrap job or in the web application


I would make this comment simpler, e.g.: "Clean up the AppRepository resources used by this Kubeapps instance"

prydonius · 2018-08-22T20:29:23Z

docs/user/access-control.md

@@ -60,10 +73,28 @@ available in most Kubernetes distributions, you can find more information about
 that role
 [here](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#user-facing-roles).

+Apart from that role we need to create an additional one to have read access to the app repositories.


Apart from that role we need to create an additional one to have read access to the app repositories.

Additionally, we need to create a role to give read access to App Repositories and bind it to our service account.

prydonius · 2018-08-23T19:45:39Z

chart/kubeapps/templates/apprepository-jobs-cleanup-rbac.yaml

+metadata:
+  name: {{ template "kubeapps.apprepository-jobs-cleanup.fullname" . }}
+  annotations:
+    helm.sh/hook: pre-delete


@andresmgot I think this should be post-delete right? I'll make a PR: #530

Andres added 2 commits August 20, 2018 10:57

Allow to install kubeapps in several namespaces

3a7d427

Minor doc update

5d0de77

migmartri approved these changes Aug 20, 2018

View reviewed changes

Review

8f02c6c

andresmgot requested a review from prydonius August 21, 2018 11:05

prydonius approved these changes Aug 22, 2018

View reviewed changes

Review

364f4b0

andresmgot merged commit 53ad739 into vmware-tanzu:master Aug 23, 2018

prydonius reviewed Aug 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to install kubeapps in several namespaces #508

Allow to install kubeapps in several namespaces #508

andresmgot commented Aug 20, 2018

migmartri left a comment

migmartri Aug 20, 2018

migmartri Aug 20, 2018

migmartri Aug 20, 2018

andresmgot Aug 21, 2018

migmartri Aug 20, 2018

andresmgot Aug 21, 2018

migmartri Aug 20, 2018

andresmgot Aug 21, 2018

andresmgot Aug 21, 2018

migmartri Aug 20, 2018

prydonius Aug 22, 2018

migmartri Aug 20, 2018

andresmgot commented Aug 21, 2018

prydonius Aug 22, 2018

prydonius Aug 22, 2018

andresmgot Aug 23, 2018

prydonius Aug 22, 2018

prydonius Aug 22, 2018

prydonius Aug 22, 2018

prydonius Aug 22, 2018

prydonius Aug 23, 2018 •

edited

Loading

		@@ -1,7 +1,10 @@
		{{- if not (.Capabilities.APIVersions.Has "kubeapps.com/v1alpha1") -}}

		@@ -0,0 +1,47 @@
		{{- if .Values.rbac.create -}}
		# The cleanup job is necessary to remove "apprepositories" created

		@@ -0,0 +1,15 @@
		# The cleanup job is necessary to remove "apprepositories" created

		@@ -0,0 +1,47 @@
		# The cleanup job is necessary to remove "apprepositories" created
		# in the bootstrap job or in the web application

Allow to install kubeapps in several namespaces #508

Allow to install kubeapps in several namespaces #508

Conversation

andresmgot commented Aug 20, 2018

migmartri left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Aug 21, 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

prydonius Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

prydonius Aug 23, 2018 •

edited

Loading