-
Notifications
You must be signed in to change notification settings - Fork 21
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
Helm charts #184
Helm charts #184
Conversation
3c65434
to
7960b6a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
=======================================
Coverage 61.78% 61.78%
=======================================
Files 2 2
Lines 785 785
=======================================
Hits 485 485
Misses 249 249
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7960b6a
to
bef7a42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing Authorino Operator should also install the base resources that allows deploying Authorino instances later on, including a Deployment of authorino-webhooks, as well as registering the AuthConfig CRD and a couple of ClusterRoles.
Because of the webhooks, there's also a dependency on cert-manager (when not installing via OLM.)
The manifests of reference for all the above are: https://github.com/Kuadrant/authorino-operator/blob/main/config/deploy/manifests.yaml, along with the install script: https://github.com/Kuadrant/authorino-operator/blob/main/utils/install.sh.
I imagine that installing Authorino Operator via Helm chart should be a replacement to the install script.
On a different take... If we're planning on having an "Authorino Helm chart" too, then the Authorino one needs to be introduced first, and then this one with a dependency on it. The Authorino Helm chart could then install the missing bits of this PR, without which installing Authorino Operator would only result in broken deployments of the operand.
a8a6504
to
13d504c
Compare
Makefile
Outdated
set -e ;\ | ||
mkdir -p $(dir $(HELM)) ;\ | ||
OS=$(shell go env GOOS) && ARCH=$(shell go env GOARCH) && \ | ||
wget -O helm.tar.gz https://get.helm.sh/helm-$(HELM_VERSION)-$${OS}-$${ARCH}.tar.gz ;\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verification steps failed to me due to missing wget
. Can we use curl instead, since there are other commands that depend on it too?
$ make helm-install
bash: wget: command not found
make: *** [bin/helm] Error 127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wget -O helm.tar.gz https://get.helm.sh/helm-$(HELM_VERSION)-$${OS}-$${ARCH}.tar.gz ;\ | |
curl -sL -o helm.tar.gz https://get.helm.sh/helm-$(HELM_VERSION)-$${OS}-$${ARCH}.tar.gz ;\ |
make/helm.mk
Outdated
$(KUSTOMIZE) build config/helm > charts/authorino-operator/templates/manifests.yaml | ||
V="$(BUNDLE_VERSION)" $(YQ) -i e '.version = strenv(V)' charts/authorino-operator/Chart.yaml | ||
V="$(CERT_MANAGER_VERSION)" $(YQ) -i e '(.dependencies[] | select(.name == "cert-manager").version) = strenv(V)' -i charts/authorino-operator/Chart.yaml | ||
wget -O charts/authorino-operator/crds/cert-manager-manifests.yaml https://github.com/cert-manager/cert-manager/releases/download/v${CERT_MANAGER_VERSION}/cert-manager.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename charts/crds
to something like charts/dependencies
or so?
cert-manager's manifests include a lot more than CRDs (namespaces, deployments, etc.) Additionally, authorino-operator's manifests (the one under charts/templates
) also include CRDs, yet it's not in charts/crds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my fault regarding the set of manifests installed related to cert-manager, now it's installed just the CRDs.
Regarding the charts/crds
directory, Helm is using that directory to install any needed CRD before its mechanism of installing charts and its dependencies kicks in. I thought it was a good place to host the cert-manager CRDs that are require by its chart https://artifacthub.io/packages/helm/cert-manager/cert-manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, decided to remove the dependency and add the extra steps, will be documented on the repo as well.
Verification steps seem to be applying 2 sets of deployments of cert-manager. One set in the |
* When a release is published, the `release-helm-chart` workflow will package the chart and upload it to the release page. Then sync with the kuadrant repo. * When a release is deleted, it will sync with the kuadrant repo
* Using curl instead of wget
13d504c
to
cbb6197
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verification steps work. Let's merge the PR. Thanks for the work, @didierofrivia!
As a side note, it's a shame that Helm decided to keep the installation of the CRDs as a separate step though. I can see how users could easily end up with mismatching versions between API and backend.
For now, I wouldn't replace the install script to default to helm install cert-manager; helm install authorino-operator
, as I think the script providers users with better experience.
Maybe in the future there will be a better way to handle dependencies in Helm that doesn't involve deploying dedicated instances of cert-manager for Authorino Operator nor duplicate cluster-scope installs.
This PR introduces a way to manage an Authorino Operator Helm Chart. This is not meant to replace the way we are building and delivering our manifests (Kustomize, OLM) but to provide an alternative (complementary) way of delivering the operator.
This early implementation uses Kustomize to create the chart template, instead of creating and maintaining new ones with Helm, to later customize the Helm only settings with its
values.yaml
NOTES
Verification Steps
You should see it installed:
latest
) Authorino Operator chartOR
kubectl get deployments/authorino-operator -n authorino-operator -o yaml | grep image:
it should return:
It should also have installed Authorino manifests and webhooks i.e:
kubectl get crds | grep auth authconfigs.authorino.kuadrant.io 2024-07-01T08:26:05Z authorinos.operator.authorino.kuadrant.io 2024-07-01T08:26:05Z
kubectl get deployments/authorino-operator -n authorino-operator -o yaml | grep image:
it should return: