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

Add ability to reload cert and key from disk. #45

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

george-angel
Copy link
Contributor

Enables frequent rotation of certificates

What and why?

This change will reload certificate + key on the listener when the files change on-disk.

Allows use of cert-manager and avoiding having hardcoded infinite TTL certificates.

Testing Steps

Started controller:

...
k8s-sidecar-injector-5c8bb965cc-98jbt k8s-sidecar-injector I0812 09:25:07.618723       1 main.go:153] configuration replaced

delete the TLS secret, let cert-manager recreate it, and try to recreate injected Pod:

k8s-sidecar-injector-5c8bb965cc-98jbt k8s-sidecar-injector 2020/08/12 09:28:29 http: TLS handshake error from 10.2.3.1:20124: remote error: tls: bad certificate

A minute later, injector controller is able to handle requests again (without restarting):

k8s-sidecar-injector-5c8bb965cc-98jbt k8s-sidecar-injector I0812 09:30:00.085404       1 webhook.go:510] AdmissionReview for Kind=/v1, Kind=Pod, Namespace=sys-prom Name=thanos-store-0 (thanos-store-0) UID=7efecd19-0f72-4422-add3-511a6308345e patchOperation=CREATE UserInfo={system:serviceaccount:kube-system:statefulset
-controller 34071325-acfd-11e7-97e1-0afa8bad3a0a [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] map[]}

Reviewers

Required reviewers: @byxorna
Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

⚠️ this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

Enables frequent rotation of certificates
@george-angel george-angel mentioned this pull request Aug 12, 2020
Copy link
Contributor

@byxorna byxorna left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for this change!

Does this obsolete the changes in #46?

@george-angel
Copy link
Contributor Author

No, this is a stand-alone change for cert reload, ignore #46 for now (although thanks for comments). I will clean it up and raise a PR with general version updates there.

@george-angel
Copy link
Contributor Author

If you are happy with this change, we can merge it in and I'll update the other PR with just changes to modules + Docker image? Thank you.

@byxorna
Copy link
Contributor

byxorna commented Aug 13, 2020

This LGTM - I cant merge it though, so @alex-laties @kirooshu @defect want to take it from here?

@alex-laties alex-laties merged commit 85bf83c into tumblr:master Aug 13, 2020
@george-angel george-angel deleted the reload-cert branch August 14, 2020 06:54
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.

3 participants