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

Use CAPI capability of Flux's HelmRelease to install apps into target cluster #226

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

wahabmk
Copy link
Contributor

@wahabmk wahabmk commented Aug 22, 2024

Description

There is no change in HMC code needed to use the CAPI capability of Flux Helm Controller to install applications into target cluster. So this PR adds the following to the templates/aws-standalone-cp chart as an example of how to use this feature with HMC:

  1. A helm release for cert-manager which references a HelmRepository type source.
  2. A helm release for nginx-ingress which references an OCIRepository type source.

Manual Testing

Management Cluster

The Flux objects for cert-manager and nginx-ingress are created and reconciled in the management cluster:

~ kubectl -n hmc-system get helmrepository
NAME            URL                                    AGE     READY   STATUS
cert-manager    https://charts.jetstack.io             3h21m   True    stored artifact: revision 'sha256:3aee147c8a57ff5256d669c1f41486633d5c7f313a8ef7c5e4826cb033548e9b'
hmc-templates   oci://hmc-local-registry:5000/charts   3h23m           
➜  ~~~~ kubectl -n hmc-system get ocirepository                            
NAME            URL                                           READY   STATUS                                                                                                       AGE
nginx-ingress   oci://ghcr.io/nginxinc/charts/nginx-ingress   True    stored artifact for digest '1.3.2@sha256:c2810b728c7f735d26ab024ec4569037c1eb2e11ff1fb2e867dccbef3b1dfcf9'   3h21m
➜  ~~~~ kubectl -n hmc-system get helmrelease  
NAME                         AGE     READY   STATUS
cluster-api                  3h23m   True    Helm install succeeded for release hmc-system/cluster-api.v1 with chart [email protected]
cluster-api-provider-aws     3h23m   True    Helm install succeeded for release hmc-system/cluster-api-provider-aws.v1 with chart [email protected]
hmc                          3h23m   True    Helm upgrade succeeded for release hmc-system/hmc.v2 with chart [email protected]
k0smotron                    3h23m   True    Helm install succeeded for release hmc-system/k0smotron.v1 with chart [email protected]
wali-aws-dev                 3h21m   True    Helm install succeeded for release hmc-system/wali-aws-dev.v1 with chart [email protected]
wali-aws-dev-cert-manager    3h21m   True    Helm install succeeded for release cert-manager/cert-manager.v1 with chart [email protected]
wali-aws-dev-nginx-ingress   3h21m   True    Helm install succeeded for release nginx-ingress/nginx-ingress-wali-aws-dev-nginx-ingress.v1 with chart [email protected]+c2810b728c7f

Target Cluster

The CRDs for cert-manager and nginx-ingress have been installed:

~ kubectl get crd | grep cert-manager
certificaterequests.cert-manager.io                   2024-08-22T18:22:14Z
certificates.cert-manager.io                          2024-08-22T18:22:14Z
challenges.acme.cert-manager.io                       2024-08-22T18:22:14Z
clusterissuers.cert-manager.io                        2024-08-22T18:22:15Z
issuers.cert-manager.io                               2024-08-22T18:22:15Z
orders.acme.cert-manager.io                           2024-08-22T18:22:14Z
➜  ~~~~ kubectl get crd | grep nginx
dnsendpoints.externaldns.nginx.org                    2024-08-22T18:22:12Z
globalconfigurations.k8s.nginx.org                    2024-08-22T18:22:12Z
policies.k8s.nginx.org                                2024-08-22T18:22:12Z
transportservers.k8s.nginx.org                        2024-08-22T18:22:12Z
virtualserverroutes.k8s.nginx.org                     2024-08-22T18:22:12Z
virtualservers.k8s.nginx.org                          2024-08-22T18:22:12Z

The controller deployments for both and their respective secrets (that have release info) are present in each's respective namespace:

~ kubectl get ns
NAME              STATUS   AGE
cert-manager      Active   3h28m
default           Active   3h30m
k0s-autopilot     Active   3h30m
kube-node-lease   Active   3h30m
kube-public       Active   3h30m
kube-system       Active   3h30m
nginx-ingress     Active   3h28m
➜  ~~~~ kubectl -n cert-manager get deploy
NAME                      READY   UP-TO-DATE   AVAILABLE   AGE
cert-manager              1/1     1            1           3h24m
cert-manager-cainjector   1/1     1            1           3h24m
cert-manager-webhook      1/1     1            1           3h24m
➜  ~ kubectl -n cert-manager get secret
NAME                                 TYPE                 DATA   AGE
cert-manager-webhook-ca              Opaque               3      3h23m
sh.helm.release.v1.cert-manager.v1   helm.sh/release.v1   1      3h24m
➜  ~~~~ kubectl -n nginx-ingress get deploy
NAME                       READY   UP-TO-DATE   AVAILABLE   AGE
nginx-ingress-controller   1/1     1            1           3h25m
➜  ~ kubectl -n nginx-ingress get secret
NAME                                                             TYPE                 DATA   AGE
sh.helm.release.v1.nginx-ingress-wali-aws-dev-nginx-ingress.v1   helm.sh/release.v1   1      3h25m

@wahabmk wahabmk self-assigned this Aug 22, 2024
@wahabmk wahabmk linked an issue Aug 22, 2024 that may be closed by this pull request
slysunkin
slysunkin previously approved these changes Aug 23, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Schnitzel @wahabmk Do we want to keep it as an example just for one template? If not, I think we'd better separate it as a subchart and inject it into all of our templates.

Copy link
Contributor Author

@wahabmk wahabmk Aug 26, 2024

Choose a reason for hiding this comment

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

I think it is a good idea to separate out the YAML for installing custom apps and injecting it into all HMC templates. We could do it either:

  1. By creating hmc/templates/apps folder with the app YAMLs (nginx, cert-manager, etc) and extending the templates-generate make target to copy the YAML files from hmc/templates/apps/ into each HMC template under hmc/templates/<template-name>/templates/apps/. This way we won't have to manage and version a separate helm chart for the apps (if that is desirable).
  2. Or we could make a separate Helm chart (perhaps library chart) with the app YAMLs and include it as a versioned subchart dependency into each HMC template.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with having it right now inside a specific template without a subchart or anything. We're are doing this first version really to just to proof that we can install helmcharts into a target cluster from a template. So lets get it in as we have it here.

The whole thing of having helmcharts installed across all clusters will come with the extended version of State Management that Martin and I are working on defining.

createNamespace: true
remediation:
retries: -1
values:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Schnitzel @wahabmk Do we want to allow overrides for some/all values? I think it also makes sense to allow enabling/disabling of those releases.

Copy link
Contributor Author

@wahabmk wahabmk Aug 26, 2024

Choose a reason for hiding this comment

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

Do we want to allow overrides for some/all values?

I am leaning towards the opinion that we should not control what value overrides to allow/disallow for these apps. Reason being that if we want these HMC templates to be publicly available, anybody should be able to override these values (as per their needs) to values other than what we have used or unused as default.

I think it also makes sense to allow enabling/disabling of those releases.

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

yea agree we should let the customer overwrite anything they want (especially in this current iteration). In the future we might have tighter managed templates by mirantis that will not allow any changes, but here it's ok.

hmc.mirantis.com/managed: "true"
spec:
chart: cert-manager
version: ">=v1.12.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Schnitzel @wahabmk do we want automatic upgrades here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. I should remove the >= and just pin it to a specific version.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea agree, we should not have auto updates, let's pin it.


# Optionally install apps defined under
# templates/apps into target cluster
installApps: false
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this by default true? I think its just easier to demo.
Also let's rename installApps to installBeachHeadServices which is the term we want to use for these type of applications

Copy link
Contributor

Choose a reason for hiding this comment

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

@wahabmk Oh also, can we rename the folder templates/apps to templates/beachheadservices

Copy link
Contributor

@Schnitzel Schnitzel left a comment

Choose a reason for hiding this comment

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

couple of small things from my side, the rest is great

@wahabmk wahabmk requested a review from Schnitzel August 27, 2024 19:20
Copy link
Contributor

@Schnitzel Schnitzel left a comment

Choose a reason for hiding this comment

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

Great! thank you very much!

@wahabmk wahabmk merged commit e9bbdf8 into Mirantis:main Aug 29, 2024
1 check passed
bnallapeta pushed a commit to bnallapeta/hmc that referenced this pull request Nov 15, 2024
Use CAPI capability of Flux's HelmRelease to install apps into target cluster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Initial implementation for the State Management MVP
4 participants