bitnami/kubeapps Improved handling of packaging options for Kubeapps #9063

absoludity · 2022-02-17T20:01:33Z

Description of the change

This change contains the changes landed in our Kubeapps chart at vmware-tanzu/kubeapps#4265 , but without the update to the plugin paths for now.

Adding Antonio and Rafa as reviewers just to ensure we're on the same page about landing this change now.

Benefits

It gives us a better UX for choosing which packaging system to use with Kubeapps, by allowing the user to choose just the packaging system, while the chart then calculates the required plugins. It also allows the chart to conditionally enable/disable chart dependencies based on the chosen packaging.

Backwards compatible as setting the kubeappsapis.enabledPlugins will override (see paste below)
Possible drawbacks

I understand the readme-generator-for-helm check failure is known (it doesn't deal with nil values yet), but I'm confused about the Error: Detected changes in charts without version bump in Chart.yaml. Charts changed: 1 bitnami/kubeapps Version bumps detected: 99
error, given that I have bumped the patch version of the chart?

Applicable issues

fixes #

Additional information

Here's the output from testing the change by comparing the output of helm template:

# Default plugins are helm and resources:
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 5              
            - /kubeapps-apis
          args:
            - --plugin-dir
            - /plugins/helm
            - --plugin-dir
            - /plugins/resources

# Enabling carvel updates the enabled plugins.
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
diff --git a/docs/user/manifests/kubeapps-local-dev-values.yaml b/docs/user/manifests/kubeapps-local-dev-values.yaml
index 3b7518cc..ff73074b 100644
--- a/docs/user/manifests/kubeapps-local-dev-values.yaml
+++ b/docs/user/manifests/kubeapps-local-dev-values.yaml
@@ -30,3 +30,6 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  carvel:
+    enabled: true
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 7
            - /kubeapps-apis
          args:
            - --plugin-dir
            - /plugins/kapp-controller
            - --plugin-dir
            - /plugins/helm
            - --plugin-dir
            - /plugins/resources

# Enabling flux results in an error as helm is also enabled:
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
diff --git a/docs/user/manifests/kubeapps-local-dev-values.yaml b/docs/user/manifests/kubeapps-local-dev-values.yaml
index 3b7518cc..7117cbaa 100644
--- a/docs/user/manifests/kubeapps-local-dev-values.yaml
+++ b/docs/user/manifests/kubeapps-local-dev-values.yaml
@@ -30,3 +30,8 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  carvel:
+    enabled: true
+  flux:
+    enabled: true
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 7              
Error: execution error at (kubeapps/templates/kubeappsapis/deployment.yaml:71:35): packaging: Please enable only one of the flux and helm plugins, since they both operate on Helm releases.

Use --debug flag to render out invalid YAML
make: *** [script/makefiles/deploy-dev.mk:40: deploy-dev-kubeapps] Error 1

# Disabling helm gets things going as expected:
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
diff --git a/docs/user/manifests/kubeapps-local-dev-values.yaml b/docs/user/manifests/kubeapps-local-dev-values.yaml
index 3b7518cc..ba928fcc 100644
--- a/docs/user/manifests/kubeapps-local-dev-values.yaml
+++ b/docs/user/manifests/kubeapps-local-dev-values.yaml
@@ -30,3 +30,10 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  carvel:
+    enabled: true
+  flux:
+    enabled: true
+  helm:
+    enabled: false
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 7              
            - /kubeapps-apis
          args:
            - --plugin-dir
            - /plugins/kapp-controller
            - --plugin-dir
            - /plugins/fluxv2
            - --plugin-dir
            - /plugins/resources

# Adding some other key to the packaging value causes a fail:
Use --debug flag to render out invalid YAML
make: *** [script/makefiles/deploy-dev.mk:40: deploy-dev-kubeapps] Error 1
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
diff --git a/docs/user/manifests/kubeapps-local-dev-values.yaml b/docs/user/manifests/kubeapps-local-dev-values.yaml
index 3b7518cc..c3a43886 100644
--- a/docs/user/manifests/kubeapps-local-dev-values.yaml
+++ b/docs/user/manifests/kubeapps-local-dev-values.yaml
@@ -30,3 +30,12 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  carvel:
+    enabled: true
+  flux:
+    enabled: true
+  helm:
+    enabled: false
+  unknownpkg:
+    enabled: true
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 7              
Error: execution error at (kubeapps/templates/kubeappsapis/deployment.yaml:71:35): packaging: Unsupported packaging option: unknownpkg

Use --debug flag to render out invalid YAML
make: *** [script/makefiles/deploy-dev.mk:40: deploy-dev-kubeapps] Error 1

# Manually overriding the enabled plugins is still possible:
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ git --no-pager diff docs/user/manifests/kubeapps-local-dev-values.yaml
diff --git a/docs/user/manifests/kubeapps-local-dev-values.yaml b/docs/user/manifests/kubeapps-local-dev-values.yaml
index 3b7518cc..e1fe4b91 100644
--- a/docs/user/manifests/kubeapps-local-dev-values.yaml
+++ b/docs/user/manifests/kubeapps-local-dev-values.yaml
@@ -18,6 +18,9 @@ postgresql:
   existingSecret: postgresql-db
 kubeappsapis:
   replicaCount: 1
+  enabledPlugins:
+  - unknownpkg
+  - anotherunknownpkg
 ingress:
   enabled: true
   hostname: localhost
@@ -30,3 +33,12 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  carvel:
+    enabled: true
+  flux:
+    enabled: true
+  helm:
+    enabled: false
+  unknownpkg:
+    enabled: true
➜  kubeapps git:(3695-dont-require-explicit-enable-redis-2) ✗ make deploy-dev-kubeapps | grep "\- /kubeapps-apis" -A 7              
            - /kubeapps-apis
          args:
            - --plugin-dir
            - /plugins/unknownpkg
            - --plugin-dir
            - /plugins/anotherunknownpkg
            - --clusters-config-path=/config/clusters.conf
            - --plugin-config-path=/config/kubeapps-apis/plugins.conf

Checklist

Chart version bumped in Chart.yaml according to semver.
Variables are documented in the values.yaml and added to the README.md using (readme-generator-for-helm)[https://github.com/bitnami-labs/readme-generator-for-helm]
Title of the PR starts with chart name (e.g. [bitnami/<name_of_the_chart>])
All commits signed off and in agreement of Developer Certificate of Origin (DCO)

Signed-off-by: Michael Nelson <[email protected]>

castelblanque · 2022-02-18T08:31:10Z

LGTM!

Sorry for not pointing this out in the original PR on Kubeapps, if this is set up:

packaging:
  [...]
  flux:
    enabled: true
[...]
kubeappsapis:
  enabledPlugins:
    - helm
    - resources

Installation ends up having Redis but no Flux. I know it might be an edge case, but to avoid this kind of duplicity why not removing the enabledPlugins option completely?

absoludity · 2022-02-18T08:57:22Z

Switching this back to draft as I've just noticed an issue with it (we still refer to .Values.redis.enabled in other parts. I need to test this more thoroughly on Monday.

absoludity · 2022-02-18T09:00:07Z

LGTM!

Sorry for not pointing this out in the original PR on Kubeapps, if this is set up:
packaging:
  [...]
  flux:
    enabled: true
[...]
kubeappsapis:
  enabledPlugins:
    - helm
    - resources
Installation ends up having Redis but no Flux. I know it might be an edge case, but to avoid this kind of duplicity why not removing the enabledPlugins option completely?

As Antonio pointed out elsewhere, if people create their own plugins, or we work on new plugins, we may want to set the enabledPlugins explicitly.

javsalgar · 2022-02-18T09:15:47Z

Could you merge from master? We just found an issue with the GH action. Sorry for the inconvenience

Signed-off-by: Michael Nelson <[email protected]>

absoludity · 2022-02-18T11:34:49Z

Merged master and fixed the issue I mentioned earlier (changes from vmware-tanzu/kubeapps#4309 )

antgamdia

Great! Thanks for the changes, much better UX for enabling the packaging plugins.

javsalgar · 2022-02-21T08:29:24Z

Could you check the issues in the actions? It seems that there's missing metadata in the README

antgamdia · 2022-02-21T08:31:04Z

Could you check the issues in the actions? It seems that there's missing metadata in the README

It's a known issue in the readme-generator project, bitnami/readme-generator-for-helm#31

absoludity · 2022-02-21T22:48:51Z

Could you check the issues in the actions? It seems that there's missing metadata in the README

It's a known issue in the readme-generator project, bitnami-labs/readme-generator-for-helm#31

Can we merge this, given that the issue for the failing check is known. We don't need to wait for that issue in the readme generator to be fixed to land this (as we similarly landed #8981)

…itnami#9063) * Improved handling of packaging options for Kubeapps Signed-off-by: Michael Nelson <[email protected]> * Run readme generator. Signed-off-by: Michael Nelson <[email protected]> * Fix other use of redis.enabled from vmware-tanzu/kubeapps#4309 Signed-off-by: Michael Nelson <[email protected]> Signed-off-by: JMSwag <[email protected]>

…itnami#9063) * Improved handling of packaging options for Kubeapps Signed-off-by: Michael Nelson <[email protected]> * Run readme generator. Signed-off-by: Michael Nelson <[email protected]> * Fix other use of redis.enabled from vmware-tanzu/kubeapps#4309 Signed-off-by: Michael Nelson <[email protected]> Signed-off-by: Pavel Sokolov <[email protected]>

absoludity changed the title ~~Improved handling of packaging options for Kubeapps~~ bitnami/kubeapps Improved handling of packaging options for Kubeapps Feb 17, 2022

absoludity added 2 commits February 18, 2022 10:26

Improved handling of packaging options for Kubeapps

25d55c5

Signed-off-by: Michael Nelson <[email protected]>

Run readme generator.

1c04af7

Signed-off-by: Michael Nelson <[email protected]>

absoludity force-pushed the kubeapps-better-packaging-options branch from 159d1f5 to 1c04af7 Compare February 17, 2022 23:39

absoludity marked this pull request as ready for review February 18, 2022 05:25

absoludity requested review from castelblanque and antgamdia February 18, 2022 05:26

absoludity marked this pull request as draft February 18, 2022 08:57

absoludity added 2 commits February 18, 2022 22:28

Fix other use of redis.enabled from vmware-tanzu/kubeapps#4309

8bc81ba

Signed-off-by: Michael Nelson <[email protected]>

Merge branch 'master' into kubeapps-better-packaging-options

05296a2

Signed-off-by: Michael Nelson <[email protected]>

absoludity force-pushed the kubeapps-better-packaging-options branch from 69c0899 to 05296a2 Compare February 18, 2022 11:32

absoludity marked this pull request as ready for review February 18, 2022 11:33

antgamdia approved these changes Feb 18, 2022

View reviewed changes

javsalgar merged commit a000a97 into bitnami:master Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitnami/kubeapps Improved handling of packaging options for Kubeapps #9063

bitnami/kubeapps Improved handling of packaging options for Kubeapps #9063

absoludity commented Feb 17, 2022 •

edited

Loading

castelblanque commented Feb 18, 2022

absoludity commented Feb 18, 2022

absoludity commented Feb 18, 2022

javsalgar commented Feb 18, 2022

absoludity commented Feb 18, 2022

antgamdia left a comment

javsalgar commented Feb 21, 2022

antgamdia commented Feb 21, 2022

absoludity commented Feb 21, 2022

bitnami/kubeapps Improved handling of packaging options for Kubeapps #9063

bitnami/kubeapps Improved handling of packaging options for Kubeapps #9063

Conversation

absoludity commented Feb 17, 2022 • edited Loading

castelblanque commented Feb 18, 2022

absoludity commented Feb 18, 2022

absoludity commented Feb 18, 2022

javsalgar commented Feb 18, 2022

absoludity commented Feb 18, 2022

antgamdia left a comment

Choose a reason for hiding this comment

javsalgar commented Feb 21, 2022

antgamdia commented Feb 21, 2022

absoludity commented Feb 21, 2022

absoludity commented Feb 17, 2022 •

edited

Loading