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

feat(deployments) add Helm chart for kuma #916

Merged
merged 27 commits into from
Jul 31, 2020

Conversation

austince
Copy link
Contributor

@austince austince commented Jul 21, 2020

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 the gh-pages branch.

Full changelog

  • Add Helm 3 chart
  • Add Helm support in e2e tests
  • Add chart-releaser to CI

Issues resolved

Fix #852
Fix #706

Documentation

  • N/A for now

@austince austince force-pushed the feat/helm/initial-chart branch 3 times, most recently from 319c6ff to 545762f Compare July 21, 2020 22:09
# resources:
# requests:
# cpu: 100m
# memory: 256Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these?

Copy link
Contributor Author

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: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

And these?

@nickolaev nickolaev marked this pull request as ready for review July 22, 2020 09:48
@nickolaev nickolaev requested a review from a team as a code owner July 22, 2020 09:48

kubectl patch namespace/"$NAMESPACE" \
--type merge \
--patch '{ "metadata": { "labels": { "kuma.io/system-namespace": "true" } } }'
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor

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 ?

Copy link
Contributor Author

@austince austince Jul 22, 2020

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

@austince austince Jul 23, 2020

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@austince austince force-pushed the feat/helm/initial-chart branch 11 times, most recently from d259c8c to f7e9703 Compare July 23, 2020 19:01
@austince austince force-pushed the feat/helm/initial-chart branch 4 times, most recently from 9fc48fe to 064f6e1 Compare July 24, 2020 12:44
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
@austince austince force-pushed the feat/helm/initial-chart branch 2 times, most recently from bfa168d to 3cf0749 Compare July 29, 2020 20:21
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
@austince
Copy link
Contributor Author

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"
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Nikolay Nikolaev and others added 9 commits July 30, 2020 15:42
…plates

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…expected naming

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
@austince
Copy link
Contributor Author

austince commented Jul 30, 2020

Still a bit of flakiness with the e2e test, but works up until deletion where we then get an RBAC error:

Test App deployment with Helm chart Should deploy two apps 2020-07-30T15:45:19Z logger.go:66: Error: uninstallation completed with 3 error(s): admission webhook "secret.validator.kuma-admission.kuma.io" denied the request: secrets "kuma-kds-tls-cert" is forbidden: User "system:serviceaccount:kuma-system:kuma-control-plane" cannot get resource "secrets" in API group "" in the namespace "kuma-system"; admission webhook "secret.validator.kuma-admission.kuma.io" denied the request: secrets "kuma-admission-server-tls-cert" is forbidden: User "system:serviceaccount:kuma-system:kuma-control-plane" cannot get resource "secrets" in API group "" in the namespace "kuma-system"; admission webhook "secret.validator.kuma-admission.kuma.io" denied the request: secrets "kuma-sds-tls-cert" is forbidden: User "system:serviceaccount:kuma-system:kuma-control-plane" cannot get resource "secrets" in API group "" in the namespace "kuma-system"

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

@austince austince changed the title feat(deployments) add Helm chart for kuma-cp feat(deployments) add Helm chart for kuma Jul 30, 2020
@nickolaev nickolaev merged commit 7a6d6dd into kumahq:master Jul 31, 2020
@austince austince deleted the feat/helm/initial-chart branch July 31, 2020 14:36

function usage {
echo "Usage: $0 [--package|--release]"
exit 0
Copy link
Contributor

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 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart
5 participants