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

fix(kuma-cp) do not replace autogenerated certs #1215

Merged
merged 4 commits into from
Nov 30, 2020

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

The problem

We were overriding autogenerated certs on Kubernetes therefore you need to restart the pods between upgrades.

The solution

Preserve autogenerated certificates between Kuma upgrades.

HELM
That was tricky. There is no native way to do it in HELM as the issue https://github.com/helm/charts/issues/5167 is still open, but there is a working workaround: read the secret if already exist and take the values from the secret. We cannot just do the if is upgrade don't render secret, because if we don't render the secret it will be deleted.

kumactl
Kumactl uses HELM under the hood to render YAML. Previously we did not require connection to the Kubernetes to render the YAML which was convenient. The lookup function just returns nil if you use Render instead of RenderWithClient, but we want to have the same behavior as in HELM. To do it, kumactl now requires connection to Kubernetes to check the state of secrets and now override them.

Alternative solutions
We could introduce something like initialSetup in Values.yaml and instruct the user to set this to false when installing for the first time + use "helm.sh/resource-policy": keep , but then we won't clean this secret on delete.
On kumactl we could have install and upgrade, but I think it's much easier to have this in one command.

Full changelog

  • Added HELM templates that reads current secret
  • Removed the code in app/kumactl/pkg/k8s. It was from the early days of Kuma.
  • Consistent way of loading the Kube config in both install dns and install control-plane

Documentation

  • No changes in docs.

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner November 25, 2020 13:40
@nickolaev
Copy link
Contributor

Making kumactl install control-plane requiring access is not great indeed. On the other hand, if we merge this, we can follow-up with merging install dns with install control-plane --dns or something similar. So we can finally have a one touch deploy :)

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, otherwise OK.

test/framework/k8s_controlplane.go Show resolved Hide resolved
app/kumactl/cmd/install/install_control_plane.go Outdated Show resolved Hide resolved
…enerated-secret

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

Great! LGTM

@jakubdyszkiewicz jakubdyszkiewicz merged commit dc26d88 into master Nov 30, 2020
@jakubdyszkiewicz jakubdyszkiewicz deleted the fix/helm-keep-autogenerated-secret branch November 30, 2020 14:01
mergify bot pushed a commit that referenced this pull request Nov 30, 2020
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
(cherry picked from commit dc26d88)

# Conflicts:
#	app/kumactl/pkg/install/k8s/control-plane/helmtemplates_vfsdata.go
jakubdyszkiewicz pushed a commit that referenced this pull request Nov 30, 2020
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
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.

None yet

2 participants