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 webhook secret and ValidatingWebhookConfiguration certificate management #2126

Merged
merged 7 commits into from
Nov 20, 2019

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Nov 18, 2019

This pr automatically fills the Secret and the ValidatingWebhookConfiguration of the webhook with some certificates.

A few notes:

  • since the Secret and the ValidatingWebhookConfiguration must be setup before the manager is started this code use a "native" K8S client.
  • a controller is started to watch the content of the resources and trigger an update before the certificates expire.
  • contrary to what I said a CA is setup and is used to sign the webhook server certificate. The secret key of the CA is not kept, a new one is created if needed.

TODO:

  • still need to do some tests with the cert. manager - it works, user can switch to a "self-managed" certificates to a "cert manager" scenario
  • update the already opened pr
  • it needs follow-up pr is to add some documentations.

@barkbay
Copy link
Contributor Author

barkbay commented Nov 18, 2019

I have updated the comment, I just did a quick test and the user can switch from a "self-managed" certificates mode to a "cert manager" scenario. The existing Secret, even if there already are some certificates, is "adopted" by the certificate manager.

I will create some documentation in a dedicated pr and update the existing one if we agree on the general idea proposed by this one.

@barkbay barkbay added the >enhancement Enhancement of existing functionality label Nov 18, 2019
@pebrc pebrc self-assigned this Nov 18, 2019
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. A few initial comments. Still trying to understand what are the trade offs of using a native client. Also want to give it a spin.

cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
pkg/controller/webhook/cert.go Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
terminationGracePeriodSeconds: 10
volumes:
Copy link
Contributor

@anyasabo anyasabo Nov 20, 2019

Choose a reason for hiding this comment

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

This is maybe overthinking it. I wonder if we want the secret to be in this file, because right now this yaml on its own will fail to deploy because the secret doesn't exist. You would need to deploy the webhook yaml also.

That might be a usability thing we can punt on in this PR though. I think right now most people will have the ability to deploy everything, and those that don't have the ability to edit yaml files. We might want to adjust in the future but this should work for now.

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM

@barkbay barkbay merged commit d91cd40 into elastic:master Nov 20, 2019
@barkbay barkbay deleted the webhook-3 branch November 20, 2019 15:54
@barkbay barkbay added the v1.0.0 label Nov 23, 2019
@thbkrkr thbkrkr changed the title Add webhook Secret and ValidatingWebhookConfiguration certificate management Add webhook secret and ValidatingWebhookConfiguration certificate management Jan 9, 2020
@pebrc pebrc mentioned this pull request Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants