Validate CABundle when writing CRD #124061
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/apiserver
kind/api-change
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
from
91ef3a6 to
eda7cd6
Compare
k8s-ci-robot
added
size/L
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
from
eda7cd6 to
b66a7cf
Compare
liggitt
reviewed
Mar 26, 2024
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
Jefftree
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
flake #123786 |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
liggitt
reviewed
Mar 26, 2024
liggitt
reviewed
Mar 26, 2024
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
from
08e155c to
6417510
Compare
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
liggitt
reviewed
Mar 27, 2024
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
from
0ea4bdb to
72c7a59
Compare
liggitt
reviewed
Apr 30, 2024
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Outdated
Show resolved
Hide resolved
liggitt
reviewed
Jun 7, 2024
staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
Show resolved
Hide resolved
liggitt
reviewed
Jun 7, 2024
| // Allows invalid CA Bundle to be specified only if the existing CABundle is invalid | ||
| // or if the CRD is not established yet. | ||
| func allowInvalidCABundle(oldCRD *apiextensions.CustomResourceDefinition) bool { | ||
| if !apiextensions.IsCRDConditionTrue(oldCRD, apiextensions.Established) { |
There was a problem hiding this comment.
in the controller where we set Established=true (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/establish/establishing_controller.go#L119), after names are accepted, we now need to check if the caBundle is valid before setting Established=True
if it is not, set Established=False with a specific reason indicating the caBundle is not valid
There was a problem hiding this comment.
Updated and added tests in staging/src/k8s.io/apiextensions-apiserver/test/integration/cabundle_test.go
|
@Jefftree can we get this in for 1.31? |
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
2 times, most recently
from
1b72efd to
99fb350
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jefftree, liggitt 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 |
Jefftree
force-pushed
the
conversion-webhook-invalidca
branch
from
99fb350 to
a5791b3
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: a0d51675e22df120e27cf58b9080a9a1dca10d5e
|
Next steps discussed via #124061 (comment) and this PR addresses part of it. /unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
is there an open issue tracking the remaining work to do? |
|
updated release note to accurately describe the new behavior |
|
#126447 tracks the remaining work |
akutz
added a commit
to akutz/vm-operator
that referenced
this pull request
Jul 31, 2024
This patch removes the caBundle field from the conversion webhook definitions. This field was set to the base-64 encoded newline, i.e. Cg==, to allow the creation of the webhooks since the field was previously marked as required. This was in error. The field was supposed to be marked optional, which was fixed in 2018. By then the pattern had already been established to ensure Cert-Manager could find the webhook def and replace the CABundle field with a valid value. However, this pattern created havoc on updates, replacing certs when updating infra components with newline chars. A change in K8s 1.30 rc1 disallows this pattern, requiring clients to omit the field entirely. This patch is in accordance with the new guidelines. Now that kubernetes/kubernetes#124061 is merged, continuing to use "caBundle: Cg==" will cause the manifests to fail when used to update an existing CRD. The K8s Slack thread https://kubernetes.slack.com/archives/C0EG7JC6T/p1722441161968339 has more information.
akutz
added a commit
to akutz/vm-operator
that referenced
this pull request
Jul 31, 2024
This patch removes the caBundle field from the conversion webhook definitions. This field was set to the base64-encoded newline, i.e. Cg==, to allow the creation of the webhooks since the field was previously marked as required. This was in error. The field was supposed to be marked optional, which was fixed in 2018. By then the pattern had already been established to ensure Cert-Manager could find the webhook def and replace the CABundle field with a valid value. However, this pattern created havoc on updates, replacing certs when updating infra components with newline chars. A change in K8s 1.31-rc1 disallows this pattern, requiring clients to omit the field entirely. This patch is in accordance with the new guidelines. Now that kubernetes/kubernetes#124061 is merged, continuing to use "caBundle: Cg==" will cause the manifests to fail when used to update an existing CRD. The K8s Slack thread https://kubernetes.slack.com/archives/C0EG7JC6T/p1722441161968339 has more information.
akutz
added a commit
to akutz/vm-operator
that referenced
this pull request
Jul 31, 2024
This patch removes the caBundle field from the conversion webhook definitions. This field was set to the base64-encoded newline, i.e. Cg==, to allow the creation of the webhooks since the field was previously marked as required. This was in error. The field was supposed to be marked optional, which was fixed in 2018. By then the pattern had already been established to ensure Cert-Manager could find the webhook def and replace the CABundle field with a valid value. However, this pattern created havoc on updates, replacing certs when updating infra components with newline chars. A change in K8s 1.31-rc1 disallows this pattern, requiring clients to omit the field entirely. This patch is in accordance with the new guidelines. Now that kubernetes/kubernetes#124061 is merged, continuing to use "caBundle: Cg==" will cause the manifests to fail when used to update an existing CRD. The K8s Slack thread https://kubernetes.slack.com/archives/C0EG7JC6T/p1722441161968339 has more information.
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #123835
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: