-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat(deployments) add Helm chart for kuma #916
Conversation
319c6ff
to
545762f
Compare
deployments/charts/kuma-cp/templates/cp-webhooks-and-secrets.yaml
Outdated
Show resolved
Hide resolved
# resources: | ||
# requests: | ||
# cpu: 100m | ||
# memory: 256Mi |
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.
Do we need these?
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.
More for documentation purposes. Usually when installing a new chart, I'll copy the chart's values.yaml file and modify from there. We can certainly replace these comments with more structured docs in the README.
# key: "" | ||
# kds: | ||
# cert: "" | ||
# key: "" |
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.
And these?
|
||
kubectl patch namespace/"$NAMESPACE" \ | ||
--type merge \ | ||
--patch '{ "metadata": { "labels": { "kuma.io/system-namespace": "true" } } }' |
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.
Why do you need this one?
Can you just apply this one -> https://github.com/kumahq/kuma/blob/master/app/kumactl/data/install/k8s/control-plane/namespace.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.
Is there a way with kumactl
to install only the namespace? Helm 3 requires a namespace to exist before deployment:
$ helm3 install kuma-cp . --namespace kuma-system
Error: create: failed to create: namespaces "kuma-system" not found
The other way would be to make users specify the namespace in values.yaml
and ignore the Helm .Release.Namespace
entirely, though this is not ideal in terms of fitting with the Helm conventions and would likely lead to confusion.
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.
no ! right now, there is no way to install namespace only through kumactl
. Can we
keep the kuma-system
as a default value in the namespace ?
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.
Hey @tharun208, can you elaborate on the ask? I'm not sure I understand. The issue w/ Helm 3 is that you must create the namespace, whether it is kuma-system
or another, before the actual deployment if you need to modify it (ex: add the system label).
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.
Following up from slack, we've tried to use the Helm 3.2.0 option --create-namespace
but results in the same error.
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.
Another hacky option here would be to run a Job w/ a pre-install hook that updates the namespace with the label using kubectl
directly.
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.
@nickolaev, what do you think about this approach of adding the label with a pre-install job?
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.
Why would this be "hacky"? It looks legitimate to me? I am surprised they did not make that the default behaviour.
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.
Just not something I've seen before, but happy to give it a shot. A bit like the "jump host" solution you were talking about
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 worked! Added in the latest commit
d259c8c
to
f7e9703
Compare
9fc48fe
to
064f6e1
Compare
Signed-off-by: austin ce <[email protected]>
Signed-off-by: austin ce <[email protected]>
Signed-off-by: austin ce <[email protected]>
Signed-off-by: austin ce <[email protected]>
bfa168d
to
3cf0749
Compare
Signed-off-by: austin ce <[email protected]>
Signed-off-by: austin ce <[email protected]>
3cf0749
to
e736f77
Compare
Don't know why the tests are failing, seems to be something with the GUI server |
name: {{ $serviceAccountName }} | ||
annotations: | ||
"helm.sh/hook": "pre-install" | ||
"helm.sh/hook-delete-policy": "hook-succeeded,hook-failed" |
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.
Hmm does this guarantee a proper cleanup when charts are deleted?
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.
Yes, it should be deleted either when the pre-install
hook (i.e. running that job) succeeds or fails. Are you seeing differently?
…plates Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: Nikolay Nikolaev <[email protected]>
…expected naming Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: Nikolay Nikolaev <[email protected]>
Signed-off-by: austin ce <[email protected]>
Signed-off-by: austin ce <[email protected]>
d302cf4
to
cc913ca
Compare
Signed-off-by: austin ce <[email protected]>
d9813d1
to
35eb107
Compare
Still a bit of flakiness with the e2e test, but works up until deletion where we then get an RBAC error:
Might pass on a re-run, and could also be that the namespace is cleaned up before the release is done deleting? Happy to tackle this in either this PR or another |
|
||
function usage { | ||
echo "Usage: $0 [--package|--release]" | ||
exit 0 |
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.
@nickolaev shouldn't do that ;)
Summary
Chart
Add a Helm chart (compatible with Helm 3) for the Kuma Control Plane. As discussed with @nickolaev + @jakubdyszkiewicz, this initial Helm chart is hard-coded but we would like to eventually merge the
kumactl install
commands with generating charts.Based on
kumactl install control-plane
, this chart parameterizes all the possible flags and supports CNI, installing CRDs, and all modes.I'll write more docs when we get closer to "ready to merge". For testing, I'll also need some help, but I think an e2e test using the current framework and Helm 3 as a library to deploy to the testing cluster should work.
CI/CD
Use
helm/chart-releaser
to publish charts to GitHub Releases and then host them on thegh-pages
branch.Full changelog
Issues resolved
Fix #852
Fix #706
Documentation