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 validating webhook and webhook tls autoconfiguration #478

Merged
merged 9 commits into from Sep 5, 2018

Conversation

munnerz
Copy link
Member

@munnerz munnerz commented Apr 16, 2018

What this PR does / why we need it:

This PR adds support for ValidatingWebhookConfiguration for cert-manager resources.

This is comprised of a few parts that make it more complex, mostly around securing the TLS endpoint for the webhooks.

Roughly:

  • Adds 'webhook tls bootstrap' - when cert-manager starts, it will create an 'internal' CA issuer based on CLI arguments passed to it on startup. We do this because the APIService has failurePolice: Fail, meaning we cannot create real Issuers or Certificates.

Once the secret resource has been provisioned, the webhook will be able to start, and cert-manager will then create the Issuer and Certificate resource in the apiserver, allowing standard renewal to take place from then on.

  • Adds a 'apiextensions-ca-sync` CronJob as part of the standard deployment (repo here: https://github.com/munnerz/apiextensions-ca-helper). This periodically syncs Secret resources with APIService and {Validating,Mutating}WebhookConfiguration resources. This job is configured to automatically configure the cert-manager webhook configuration resources with the appropriate caBundle field.

This lays out a general pattern for how webhook TLS can be configured in Kubernetes more generally. I have a doc I'm putting together that details a recommendation for how to secure webhooks other than cert-manager's own webhooks.

Release note:

Add validation webhooks for API types

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 16, 2018
@jetstack-bot jetstack-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 16, 2018
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2018
@munnerz
Copy link
Member Author

munnerz commented Apr 25, 2018

So a few more notes on the status of this:

  • We need to add a 'selfsigned' issuer first, to make this truly user intervention free. This will allow us to automatically provision the actual signing CA which is then used by the internal ca issuer. Right now, a signing cert still has to be manually provisioned (which cert manager uses to sign webhook serving certs)

  • In implementing this, a bug in kube-apiserver was discovered that causes validations to fail in 1/N cases (where N is the number of apiservers) in multi master setups only. This PR fixes that: Ensure webhook service routing resolves kubernetes.default.svc correctly kubernetes/kubernetes#62649 and has been cherry picked into 1.10. We need to provide a way to disable validation checks for 1.10 clusters in the meantime, else nothing will work.

@munnerz munnerz added this to the v0.4 milestone May 9, 2018
@munnerz munnerz added this to To do in v0.4 May 9, 2018
@munnerz munnerz moved this from To do to In progress in v0.4 May 9, 2018
@munnerz munnerz self-assigned this May 9, 2018
@munnerz munnerz mentioned this pull request Jun 5, 2018
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2018
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2018
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2018
@munnerz munnerz force-pushed the webhooks branch 4 times, most recently from c4a5226 to 3e54d3e Compare June 16, 2018 17:21
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2018
@munnerz munnerz force-pushed the webhooks branch 2 times, most recently from e1c1cab to b142850 Compare June 25, 2018 11:38
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2018
@munnerz
Copy link
Member Author

munnerz commented Jun 25, 2018

I am currently blocked on helm/helm#4231 being merged, and helm/helm#3982 making it into a tagged release.

Signed-off-by: James Munnelly <james.munnelly@jetstack.io>
Update docs

Signed-off-by: James Munnelly <james.munnelly@jetstack.io>
Signed-off-by: James Munnelly <james.munnelly@jetstack.io>
Signed-off-by: James Munnelly <james.munnelly@jetstack.io>
Signed-off-by: James Munnelly <james.munnelly@jetstack.io>
@jetstack-bot
Copy link
Collaborator

jetstack-bot commented Aug 20, 2018

@munnerz: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
cert-manager-e2e-v1-7 221fb6f link /test e2e v1.7
cert-manager-e2e-v1-8 221fb6f link /test e2e v1.8

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@kragniz
Copy link
Contributor

kragniz commented Aug 20, 2018

I've tested this locally and it worked well for me. Releasing v0.5 with this turned off by default (and a note in the release notes of how to opt in), maybe defaulting it to on in v0.6 sounds like a good idea.

I got this error when deploying with helm:

helm upgrade cert-manager contrib/charts/cert-manager --reuse-values --set webhook.enabled=true   
Error: found in requirements.yaml, but missing in charts/ directory: webhook

I moved webhook into contrib/charts/cert-manager/charts, which made helm happy.

@munnerz
Copy link
Member Author

munnerz commented Aug 29, 2018

Sorry @kragniz - didn't see your review of this PR until now!

Thanks for taking a look 😄

I got this error when deploying with helm:

Yes, if you run helm dep update contrib/charts/cert-manager first, it should build the .tgz in the charts/ directory. I structured it this way so that when the chart is published to helm/charts, it will be package up correctly.

@munnerz
Copy link
Member Author

munnerz commented Sep 5, 2018

/check-dco

@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Sep 5, 2018
@munnerz
Copy link
Member Author

munnerz commented Sep 5, 2018

As this is disabled by default, I'm going to go ahead and merge this so we can start gathering feedback from users 😄

There may be changes still to make, but let's see what the world thinks first!

@munnerz munnerz added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Sep 5, 2018
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot merged commit 834fda1 into cert-manager:master Sep 5, 2018
munnerz added a commit to munnerz/charts-1 that referenced this pull request Sep 10, 2018
* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>
k8s-ci-robot pushed a commit to helm/charts that referenced this pull request Sep 13, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: jenkin-x <jicowan@hotmail.com>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
* cert-manager: fast-forward to upstream bcffc635

* Update version numbers for v0.5.0 (cert-manager/cert-manager#885)
* added affinity and tolerations (cert-manager/cert-manager#869)
* Add validating webhook and webhook tls autoconfiguration (cert-manager/cert-manager#478)
* chart: annotate all CRDs with "crd-install" hook (cert-manager/cert-manager#823)
* helm chart: remove endpoints from rbac resources (cert-manager/cert-manager#769)

Signed-off-by: James Munnelly <james@munnelly.eu>

* Update image tag and add description

Signed-off-by: James Munnelly <james@munnelly.eu>
@munnerz munnerz deleted the webhooks branch January 29, 2019 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
No open projects
v0.4
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants