Moved global repos to their own namespace and outside the kubeapps one #3990

castelblanque · 2021-12-23T17:02:55Z

Description of the change

Sorry for the long PR, but in order to have the feature working it is all or nothing 😄

This PR moves the global repos from the Kubeapps namespace to its own namespace.

Based on the problem stated by @dlaloue-vmware and the discussion in #3680, this allows to give read access to users to the global repos namespace, and therefore not having to grant them with access to the kubeapps namespace.

Changes are done e.g. to Helm templates, repositories-controller, asset-syncer and kubeapps-apis, so it is large change, but basically it is adding the global namespace parameter and substituting the kubeapps namespace where needed.

No changes done to the double secrets kept for non-global repos.
Modifications in Kubeops are done for compatibility reasons.

Benefits

Global repos can be placed into their own namespace, and therefore better RBAC can be applied separated from the kubeapps namespace. Hopefully this helps somehow to implement #1647.

Possible drawbacks

Assets syncing should not be affected, but we could keep an eye on it.

Applicable issues

fixes Update Kubeapps to keep global repositories in a separate namespace #3989

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2021-12-23T17:09:19Z

pkg/kube/kube_handler.go

-	PinnipedProxyURL    string
-	Clusters            map[string]ClusterConfig
+	KubeappsClusterName  string
+	GlobalReposNamespace string


Not sure if this is the best place to add the global repos namespace...otherwise we might need to extend the register RPC method signature.

Yeah, I think that's fine for now.

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2021-12-24T08:45:35Z

chart/kubeapps/templates/apprepository/deployment.yaml

@@ -76,6 +76,7 @@ spec:
            {{- end }}
            - --repo-sync-cmd=/asset-syncer
            - --namespace={{ .Release.Namespace }}
+            - --global-repos-namespace={{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}


Preferred to add a proper flag instead of passing the value through env variables

castelblanque · 2021-12-24T08:45:59Z

chart/kubeapps/templates/shared/global-repos-namespace.yaml

+apiVersion: v1
+kind: Namespace
+metadata:
+  name: "{{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}"


Creates the global repos namespace

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

absoludity

Sorry for the long PR,

Heh - at +161 −91 this isn't a long PR, the focused change just touches a lot of files :) Start to worry about that if it's over +400 (IMO).

but in order to have the feature working it is all or nothing smile

Hmm, I'm not sure this is the case... in particular, have you checked how this code handles upgrading an existing deployment, where there are existing global repos? Won't they disappear with this change (anything other than the default bitnami repo, that is). I think it'd be easy to update so this is a non-breaking change by. See what you think (more context inline).

absoludity · 2022-01-04T05:36:21Z

chart/kubeapps/templates/apprepository/apprepositories-secret.yaml

@@ -7,7 +7,7 @@ metadata:
  {{- if .namespace }}
  namespace: {{ .namespace | quote }}
  {{- else }}
-  namespace: {{ $.Release.Namespace | quote }}
+  namespace: {{ printf "%s-%s" $.Release.Namespace $.Values.apprepository.globalReposNamespaceSuffix | quote }}


Regarding making this a non-breaking change, could we instead do:

Suggested change

namespace: {{ printf "%s-%s" $.Release.Namespace $.Values.apprepository.globalReposNamespaceSuffix | quote }}

namespace: {{ printf "%s%s" $.Release.Namespace $.Values.apprepository.globalReposNamespaceSuffix | quote }}

so that if globalReposNamespaceSuffix is empty (which can be the default, initially), we will have the existing behavior? We can then update the default value to -repos-global when we do a major version change, breaking the backwards compatibility?

It'd require a conditional around the namespace creation later too, etc.

Thanks for the suggestion Michael, it makes sense. So, defaulting now to the same kubeapps namespace, would mean leaving without effect the purpose of this change until next major version 3.x?
As-is in a Kubeapps upgrade scenario, new global repos will go to the new ns and old global repos will stay in the kubeapps ns, just not being considered global anymore.

Thanks for the suggestion Michael, it makes sense. So, defaulting now to the same kubeapps namespace, would mean leaving without effect the purpose of this change until next major version 3.x?

By default, yes, but we can verify it works as expected, even update CI to use it (by setting a value for the suffix) if we want, to ensure that switching the default later is a non-event (other than the instructions for users).

As-is in a Kubeapps upgrade scenario, new global repos will go to the new ns and old global repos will stay in the kubeapps ns, just not being considered global anymore.

Yep, we'll just ensure we document that in the release notes so people can move their repos as needed.

absoludity · 2022-01-04T05:36:56Z

chart/kubeapps/templates/apprepository/deployment.yaml

@@ -76,6 +76,7 @@ spec:
            {{- end }}
            - --repo-sync-cmd=/asset-syncer
            - --namespace={{ .Release.Namespace }}
+            - --global-repos-namespace={{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}


absoludity · 2022-01-04T05:40:15Z

pkg/kube/kube_handler.go

-	PinnipedProxyURL    string
-	Clusters            map[string]ClusterConfig
+	KubeappsClusterName  string
+	GlobalReposNamespace string


Yeah, I think that's fine for now.

castelblanque · 2022-01-04T08:05:42Z

Hmm, I'm not sure this is the case... in particular, have you checked how this code handles upgrading an existing deployment, where there are existing global repos? Won't they disappear with this change (anything other than the default bitnami repo, that is). I think it'd be easy to update so this is a non-breaking change by. See what you think (more context inline).

I don't remember from the top of my head if I tested that scenario before Christmas 😅 , will give it a try. We are talking about:

Install Kubeapps v2.4.2 (global repos in ns kubeapps)
Deploy app from one of the global repos
Upgrade Kubeapps to this snapshot (global repos in new global ns)
Upgrade app?

Old global repos will stay in kubeapps and repos coming from Helm values will go to the new ns.

absoludity · 2022-01-05T06:49:56Z

Hmm, I'm not sure this is the case... in particular, have you checked how this code handles upgrading an existing deployment, where there are existing global repos? Won't they disappear with this change (anything other than the default bitnami repo, that is). I think it'd be easy to update so this is a non-breaking change by. See what you think (more context inline).

I don't remember from the top of my head if I tested that scenario before Christmas sweat_smile , will give it a try. We are talking about:
* Install Kubeapps v2.4.2 (global repos in ns `kubeapps`)

* Deploy app from one of the global repos

* Upgrade Kubeapps to this snapshot (global repos in new global ns)

* Upgrade app?
Old global repos will stay in kubeapps and repos coming from Helm values will go to the new ns.

Yeah - in particular, does that affect existing installations (ie. if I'd installed apache in my-namespace, because apache was from the bitnami repo which was in the global repo before the upgrade, after the upgrade I may not be able to upgrade the app, as kubeapps may have lost the link to the available package?)

antgamdia · 2022-01-05T10:53:39Z

chart/kubeapps/values.yaml

@@ -719,6 +719,9 @@ apprepository:
    ##   - myRegistryKeySecretName
    ##
    pullSecrets: []
+  ## @param apprepository.globalReposNamespaceSuffix Suffix for the namespace of global repos
+  ##
+  globalReposNamespaceSuffix: repos-global


Agree with the rest of the comments in that the suffix should also include the - to prevent backward-incompatible changes.

Also, in order to stick close to similar names within our organization, why not go with a similar suffix as Carvel people does: -packaging-global ? (source: carvel-dev/kapp-controller@d199158)

I find using the word repos more intuitive 😄
Is there any accepted terminology for this?

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2022-01-11T15:05:46Z

Yeah - in particular, does that affect existing installations (ie. if I'd installed apache in my-namespace, because apache was from the bitnami repo which was in the global repo before the upgrade, after the upgrade I may not be able to upgrade the app, as kubeapps may have lost the link to the available package?)

@absoludity I tested the scenario of updating Kubeapps to the new global repos ns, and as expected:

Custom global repos created prior to Kubeapps update were kept in kubeapps ns, but they are not global anymore.
When retrieving details of already installed packages from old custom global repos, this error appears:
When switching context to ns kubeapps, old custom global repos packages appear and they can be installed normally, only in kubeapps ns.
If the old global repo is created/moved into the new global repos namespace, existing installations can be updated. Maybe in the future we could provide a mechanism to move repositories from one namespace to another? This way admins could move their repos to other namespaces after updating.

By now we are safe as we are defaulting to kubeapps ns (no global suffix).
When a proper global namespace will be used, existing installations from custom global repositories will be affected.

absoludity · 2022-01-11T23:02:46Z

Excellent, thanks for testing Rafa.

If the old global repo is created/moved into the new global repos namespace, existing installations can be updated. Maybe in the future we could provide a mechanism to move repositories from one namespace to another? This way admins could move their repos to other namespaces after updating.

Great, yes, at the very least, instructions in the release notes when the default changes.

+1, thanks!

Rafa Castelblanque added 3 commits December 23, 2021 16:33

Added global repos namespace to Kubeapps (#3989)

49bbf7a

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed Go tests (#3989)

0e0e491

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed Go tests (#3989)

8829abf

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque changed the title ~~3989 global repos namespace~~ Moved global repos to their own namespace and out of kubeapps Dec 23, 2021

castelblanque commented Dec 23, 2021

View reviewed changes

Fix for IT in CI (#3989)

1258485

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque changed the title ~~Moved global repos to their own namespace and out of kubeapps~~ Moved global repos to their own namespace and out of the kubeapps one Dec 24, 2021

castelblanque changed the title ~~Moved global repos to their own namespace and out of the kubeapps one~~ Moved global repos to their own namespace and outside the kubeapps one Dec 24, 2021

castelblanque commented Dec 24, 2021

View reviewed changes

Fixed Helm values parameter name (#3989)

6883181

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

absoludity reviewed Jan 4, 2022

View reviewed changes

antgamdia reviewed Jan 5, 2022

View reviewed changes

Rafa Castelblanque added 4 commits January 10, 2022 16:45

Merge branch 'master' into 3989-global-repos-namespace

4581daa

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Default global namespace set to Kubeapps by now (#3989)

ca1d2ca

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'master' into 3989-global-repos-namespace

916bd8f

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed tests (#3989)

f681ee5

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque requested review from absoludity and antgamdia January 11, 2022 15:07

absoludity approved these changes Jan 11, 2022

View reviewed changes

castelblanque merged commit 93f3e41 into master Jan 12, 2022

castelblanque deleted the 3989-global-repos-namespace branch January 12, 2022 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved global repos to their own namespace and outside the kubeapps one #3990

Moved global repos to their own namespace and outside the kubeapps one #3990

castelblanque commented Dec 23, 2021

castelblanque Dec 23, 2021 •

edited

absoludity Jan 4, 2022

castelblanque Dec 24, 2021

absoludity Jan 4, 2022

castelblanque Dec 24, 2021

absoludity left a comment

absoludity Jan 4, 2022

castelblanque Jan 4, 2022

absoludity Jan 5, 2022

absoludity Jan 4, 2022

absoludity Jan 4, 2022

castelblanque commented Jan 4, 2022

absoludity commented Jan 5, 2022

antgamdia Jan 5, 2022 •

edited

castelblanque Jan 11, 2022

castelblanque commented Jan 11, 2022

absoludity commented Jan 11, 2022

	namespace: {{ printf "%s-%s" $.Release.Namespace $.Values.apprepository.globalReposNamespaceSuffix \| quote }}
	namespace: {{ printf "%s%s" $.Release.Namespace $.Values.apprepository.globalReposNamespaceSuffix \| quote }}

Moved global repos to their own namespace and outside the kubeapps one #3990

Moved global repos to their own namespace and outside the kubeapps one #3990

Conversation

castelblanque commented Dec 23, 2021

Description of the change

Benefits

Possible drawbacks

Applicable issues

castelblanque Dec 23, 2021 • edited

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

absoludity 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

castelblanque commented Jan 4, 2022

absoludity commented Jan 5, 2022

antgamdia Jan 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque commented Jan 11, 2022

absoludity commented Jan 11, 2022

castelblanque Dec 23, 2021 •

edited

antgamdia Jan 5, 2022 •

edited