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.

Sign up for GitHub

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

Jump to bottom

CABundle should not be required in Webhook ClientConfig #69590

Closed
tamalsaha opened this issue Oct 9, 2018 · 10 comments · Fixed by #70138
Closed

CABundle should not be required in Webhook ClientConfig #69590

tamalsaha opened this issue Oct 9, 2018 · 10 comments · Fixed by #70138
Assignees
@liggitt
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@tamalsaha
Copy link
Member

tamalsaha commented Oct 9, 2018 •
edited
Loading

Rancher users seem to use Let's Encrypt to expose their kube-apiserver. This means that their kubeconfig file has no client-ca. Any custom controller that uses self-hosted EAS to implement admission webhooks has no place to look for CABundle.

But CABundle is a required field for Webhook ClientConfig . https://github.com/kubernetes/api/blob/e3c5c37695fbce2b0b46845e3bb4806c4b8faf47/admissionregistration/v1beta1/types.go#L290

We use the self-hosted webhook pattern shown in https://github.com/openshift/kubernetes-namespace-reservation with our tools . This makes installing such tools in Rancher cluster very cumbersome.

CABundle should be an optional field to indicate that webhook server might be using a trusted certificate.

xref: https://github.com/kubedb/project/issues/309
@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Oct 9, 2018
@tamalsaha
Copy link
Member Author

/kind enhancement
/sig api-machinery

cc: @deads2k

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 9, 2018
@deads2k
Copy link
Contributor

deads2k commented Oct 9, 2018

Using system certs when the CA is empty seems fine. Semi-related, I do not want to support an insecure option. No CA cannot mean insecure, it must mean system certs.

@liggitt
Copy link
Member

liggitt commented Oct 9, 2018

Using system certs when the CA is empty seems fine. Semi-related, I do not want to support an insecure option. No CA cannot mean insecure, it must mean system certs.

Agree on both counts. If you leave it empty today (which is allowable), I think that is the current behavior (use system certs)

@tamalsaha
Copy link
Member Author

So, is the fix just to relax the constraint in the types.go?

@deads2k
Copy link
Contributor

deads2k commented Oct 9, 2018 •
edited
Loading

So, is the fix just to relax the constraint in the types.go?

Check for an existing test that confirms that it defaults safely (not insecure). If one exists, link it in. If one doesn't, I'd like to see such a test.

Otherwise, yeah.

@mattmoor
Copy link
Contributor

Any chance this would land in 1.13?

@liggitt
Copy link
Member

liggitt commented Oct 12, 2018

Any chance this would land in 1.13?

I don't see why not. The change is trivial, just needs a testcase added.

@liggitt liggitt mentioned this issue Oct 23, 2018
Merged
@liggitt liggitt self-assigned this Oct 23, 2018
@liggitt liggitt mentioned this issue Oct 23, 2018
Merged
@liggitt
Copy link
Member

liggitt commented Oct 23, 2018

opened #70138 to fix the optional indicator and added a test to make sure the transport created from that config uses default rootca bundle and is not made insecure

@mattmoor mattmoor mentioned this issue Oct 31, 2018
Closed
@bgarcial
Copy link

bgarcial commented May 6, 2019 •
edited
Loading

I had the same error, but it was related with the management of the helm dependencies

My cluster version is 1.12.7, but the error in my case was by the following:

I was trying attach cert-manager as a helm dependency in my requirements.yaml helm chart file and when I do that, I execute helm dep update to tell to helm that attach to cert-manager.

Then a *.tgz cert-manager package is downloaded to my chart/ directory.
My idea was to remove cert-manager like dependency and so I had to remove the *.tgz cert-manager package from chart/ directory and also update the helm dependencies executing again executing helm dep update in order to don't have any dependency attached.

⟩ helm dep update
Hang tight while we grab the latest from your chart repositories...
...Unable to get an update from the "local" chart repository (http://127.0.0.1:8879/charts):
        Get http://127.0.0.1:8879/charts/index.yaml: dial tcp 127.0.0.1:8879: connect: connection refused
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 0 charts
Deleting outdated charts

And my problem with the "caBundle"has disappear.

@ZhiminXiang ZhiminXiang mentioned this issue May 7, 2019
Merged
@alejandrox1 alejandrox1 mentioned this issue May 30, 2019
Closed
@elithrar
Copy link

elithrar commented Aug 21, 2019 via email

@matthchr matthchr mentioned this issue Sep 18, 2020
Closed
@vihangm vihangm mentioned this issue Jun 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@liggitt liggitt

Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Correct optional/omitempty indicator on webhook cabundle liggitt/kubernetes
7 participants
@elithrar @tamalsaha @liggitt @bgarcial @mattmoor @deads2k @k8s-ci-robot