Allow white-spaced CABundle during webhook client creation and validation #132514
Conversation
…ient config and validation - Updates webhookClientConfigForCRD to treat caBundle values containing only whitespace as empty, ensuring system trust roots are used in this case. - Updates ValidateCABundle to treat whitespace-only caBundle as valid, consistent with empty or nil values. - Adds/updates unit tests to verify that whitespace-only caBundle is handled equivalently to empty or nil. - Ensures consistent and user-friendly handling of caBundle across CRD conversion webhook configuration and validation.
|
Hi @tiffanny29631. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
/ok-to-test |
|
/assign @liggitt |
| @@ -28,6 +29,10 @@ import ( | |||
|
|
|||
| func ValidateCABundle(fldPath *field.Path, caBundle []byte) field.ErrorList { | |||
| var allErrors field.ErrorList | |||
| // Treat whitespace-only caBundle as empty | |||
There was a problem hiding this comment.
I don't think we really want to relax validation here ... this is what keeps us from allowing new invalid data into CRDs, and keeps us from establishing CRDs with webhooks configured with CABundles that won't work properly
| caBundle := apiConfig.CABundle | ||
| if len(caBundle) > 0 && len(bytes.TrimSpace(caBundle)) == 0 { | ||
| // treat whitespace-only caBundle as empty | ||
| caBundle = nil |
There was a problem hiding this comment.
is this sufficient to allow a CRD which 1) was already persisted and 2) already established to construct a handler which will serve reads / writes as long as they don't have to successfully talk to a conversion webhook?
I think we need an integration test that:
- creates a multi-version no-op conversion CRD
- persists a CR instance
- updates the CRD to use webhook conversion via the API
- updates the CRD to set a whitespace-only CABundle via direct etcd manipulation
- verifies we can still read the CR instances for the CRD which don't require webhook conversion (this step should error if the fix in this file is commented out)
There was a problem hiding this comment.
Help on step 4 is appreciated, I was using fixtures.CreateNewV1CustomResourceDefinition to update the CRD but fails as same name / kind already exists.
There was a problem hiding this comment.
We have https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/test/integration/storage/objectreader.go#L70-L101 as an example to read/write to etcd. My thinking is we can unmarshal, change the field, and write it back to etcd for step 4?
|
/triage accepted |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tiffanny29631 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
f4ecb2b to
a93ee8e
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR is related to:
Fixes #126447
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: