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

VAULT-2230 Only update webhooks CA bundle when needed #336

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

swenson
Copy link

@swenson swenson commented Apr 22, 2022

Instead of every second. Specifically, we now only update the webhooks
bundle if:

  • we have generated a certificate and the CA bundle does not match the
    current webhook CA bundle; this will also happen if a new leader is
    elected, or
  • the webhook itself was changed (e.g., by a helm upgrade), checked by
    Watching the webhooks.

In any case, will only send the Patch if the CA bundle on the
webhook no longer matches our own CA bundle.

This was tested by performing a helm upgrade with a webhook change and
verifying that the webhook change was noticed and the CA bundle was
patched, and also by setting the certificate lifetimes very low and
making sure that the CA bundle was not patched (since the CA did not
change when the certificate was regenerated).

Fixes #332.

@jasonodonnell jasonodonnell self-requested a review April 22, 2022 21:02
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM!

@swenson swenson requested a review from jasonodonnell April 22, 2022 22:34
@swenson
Copy link
Author

swenson commented Apr 25, 2022

Thanks for the suggestions. This is ready for review again. I've switched to using an informer.

@tvoran tvoran self-requested a review April 25, 2022 21:17
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Worked great for me when testing locally. Left a couple thoughts, but I think this is good.

@swenson swenson force-pushed the VAULT-2230-patch-cabundle-on-change branch from 8033400 to e3c5f24 Compare April 27, 2022 16:07
@swenson
Copy link
Author

swenson commented Apr 28, 2022

@calvn let me know if you want to take another look

Christopher Swenson added 8 commits April 28, 2022 14:40
Instead of every second. Specifically, we now only update the webhooks
bundle if:

* we have generated a certificate and the CA bundle does not match the
  current webhook CA bundle; this will also happen if a new leader is
  elected, or
* the webhook itself was changed (e.g., by a helm upgrade), checked by
  `Watch`ing the webhooks.

In any case, will only send the `Patch` if the CA bundle on the
webhook no longer matches our own CA bundle.

This was tested by performing a `helm upgrade` with a webhook change and
verifying that the webhook change was noticed and the CA bundle was
patched, and also by setting the certificate lifetimes very low and
making sure that the CA bundle was *not* patched (since the CA did not
change when the certificate was regenerated).

Fixes #332.
Also use the cache attached to it instead of querying the API ourselves.
* Use exponential backoff
* Also simplify retry immediate logic
* Remove namespace from mutating webhooks
@swenson swenson force-pushed the VAULT-2230-patch-cabundle-on-change branch from e3c5f24 to d484cfa Compare April 28, 2022 21:41
@swenson
Copy link
Author

swenson commented Apr 28, 2022

Thanks!

@swenson swenson merged commit 398ff35 into main Apr 28, 2022
@swenson swenson deleted the VAULT-2230-patch-cabundle-on-change branch April 28, 2022 21:45
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.

Admission controller registration is invoked every second
4 participants