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

[kong] re-use admission certificates if present #256

Merged
merged 3 commits into from
Feb 9, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 15, 2021

What this PR does / why we need it:

Reuses admission webhook certificate Secrets if they are already present.

Which issue this PR fixes

Special notes for your reviewer:

We currently generate our checksum by using the entire admission webhook template output as input to SHA256. For some reason I cannot determine, the checksum still changes with the new lookup functionality between installs and upgrades, although a diff between the two shows no difference in the generated resources. Subsequent upgrades do not change the checksum.

To avoid this, I've modified the checksum generator to extract the tls.crt lines from the template, concatenate those, and checksum that, since we shouldn't need to care about anything else. It's ugly, but it works.

IMO this de facto solves #252 also: the certificate and CA Secret names are predictable, and if you create those Secrets in advance of the install, it should use them instead. Need to check to confirm. We'd only need additional changes for that if we want to make the Secret names user-selectable.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not main
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@rainest
Copy link
Contributor Author

rainest commented Jan 15, 2021

Hold because this change requires Helm 3 and cannot go in until we've removed support for Helm 2 in chart 2.x. It should otherwise be fine to review (and if anyone can figure out why the install/upgrade checksum thing happens or some neater way of working around that, I'd be quite grateful).

As CI helpfully notes, we also need to upgrade CI to use Helm 3.

charts/kong/templates/deployment.yaml Outdated Show resolved Hide resolved
@rainest rainest mentioned this pull request Jan 19, 2021
2 tasks
Copy link
Member

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

Looks good in general; some comments attached.

Also, why is the test failing (on failure to find the lookup function)?

charts/kong/templates/admission-webhook.yaml Outdated Show resolved Hide resolved
charts/kong/templates/admission-webhook.yaml Show resolved Hide resolved
charts/kong/templates/admission-webhook.yaml Outdated Show resolved Hide resolved
@rainest
Copy link
Contributor Author

rainest commented Jan 21, 2021

Also, why is the test failing (on failure to find the lookup function)?

It requires Helm 3. CI still uses Helm 2.

@rainest rainest changed the base branch from next to stage/kong-2.x January 27, 2021 00:05
@rainest
Copy link
Contributor Author

rainest commented Jan 27, 2021

CI now has a different problem!

Error: templates/controller-rbac-resources.yaml: the kind "rbac.authorization.k8s.io/v1beta1 Role" is deprecated in favor of "rbac.authorization.k8s.io/v1 Role"

https://github.com/rainest/charts has a PoC of the updated flow with a cheat: rainest@f4c7da0

This should work with a proper fix for Kong/kubernetes-ingress-controller#801. For roles, apparently just bumping the version is fine. CRDs require other changes, however.

@rainest
Copy link
Contributor Author

rainest commented Feb 4, 2021

The checksum hack wasn't actually necessary--I thought about why we even had it, looked back in ancient history, and realized it was a workaround to intentionally redeploy the controller because we couldn't keep the certificate stable 🤦 helm/charts#20051

If admission webhook certificates are already present on the cluster,
use them instead of generating a new certificate.

Fix #253
On review of the history behind this, we don't need this checksum if we
don't expect the certificate to change. We added it as a workaround for
failures caused by the certificate rotating on update:
helm/charts#20050
@rainest
Copy link
Contributor Author

rainest commented Feb 6, 2021

Rebase following 2635265. CI is now happy/this is no longer blocked on KIC#801.

@rainest rainest merged commit ddf87d4 into stage/kong-2.x Feb 9, 2021
@rainest rainest deleted the fix/lookup-cert branch February 9, 2021 22:34
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

3 participants