Skip to content
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.

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

Small changes to check Carvel multicluster support. #5454

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

I was keen to see exactly what was required to use the Carvel plugin with the existing multicluster support. It turns out, nothing (maybe @antgamdia already checked this and got it working a while ago? Not sure). The only changes I needed were:

  • Updating the version of kapp-controller that we run in the cluster (since v0.35 was still requiring secrets attached to service-accounts, which stopped with k8s 1.24, and updated in kapp-controller with v0.37, but I've tested with the latest which works).
  • Adding a makefile target to add kapp-controller to the second dev cluster
  • Fixing an issue in our carvel docs for creating the reconciliation service account.

With those changes, I can deploy kubeapps on the main cluster, with the patched config:

diff --git a/site/content/docs/latest/reference/manifests/kubeapps-local-dev-values.yaml b/site/content/docs/latest/reference/manifests/kubeapps-local-dev-values.yaml
index 3cebfad41..56004b56b 100644
--- a/site/content/docs/latest/reference/manifests/kubeapps-local-dev-values.yaml
+++ b/site/content/docs/latest/reference/manifests/kubeapps-local-dev-values.yaml
@@ -32,3 +32,8 @@ ingress:
     nginx.ingress.kubernetes.io/proxy-buffers: "4"
     # Required for ingress-nginx 1.0.0 for a valid ingress.
     kubernetes.io/ingress.class: nginx
+packaging:
+  helm:
+    enabled: true
+  carvel:
+    enabled: true

as well as the reconciliation service account as per the carvel docs modified here. I can then login, switch to the second cluster, then browse the catalog to see available apps including the carvel ones available on that cluster:

carvel-catalog-cluster-2

I can then select one to deploy

carvel-deploy-cluster-2

and successfully deploy it

carvel-deployed-second-cluster

as well as delete it.

Benefits

Demonstrates that the carvel plugin can be used with the current multicluster support.

Possible drawbacks

None

Applicable issues

  • fixes #

Additional information

Interestingly, we had code that may have been intended to say that this was not supported, when creating a carvel app on a second cluster:

if targetCluster != packageCluster {
return nil, status.Errorf(codes.InvalidArgument, "installing packages in other clusters in not supported yet")
}

but what it actually does is ensure that the package being installed is from the same cluster as that being targeted, which is the case here anyway (since we're browsing available packages on the second cluster). So it allows deploying the package from the second cluster on the second cluster, but will correctly stop a situation where the API is used to deploy a package from the first cluster on the second cluster.

Signed-off-by: Michael Nelson <minelson@vmware.com>
@netlify
Copy link

netlify bot commented Oct 12, 2022

Deploy Preview for kubeapps-dev ready!

Name Link
🔨 Latest commit dcc1609
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/63460acc49de95000924f22d
😎 Deploy Preview https://deploy-preview-5454--kubeapps-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Thanks!

@absoludity absoludity merged commit f597e02 into main Oct 19, 2022
@absoludity absoludity deleted the carvel-multicluster branch October 19, 2022 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants