-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
KEP-4222: Restrict supported media types for new apiservers. #121325
KEP-4222: Restrict supported media types for new apiservers. #121325
Conversation
Skipping CI for Draft Pull Request. |
/sig api-machinery |
This looks reasonable to me. If a consumer is broken, we could add a direct code method to skip. But I doubt people wrote customer serializers before and this is very clear. |
This is to prevent the enablement of new data formats (CBOR) in the early stages of phased implementation.
dc2465f
to
ced56a6
Compare
Proof PR #121334 job https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/121334/pull-kubernetes-e2e-kind/1714750722723024896:
|
Proof looks good. /kind feature |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, deads2k 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 |
/cc @liggitt |
not sure I have context to lgtm this, happy to defer to @deads2k's review. is this guarding the |
/triage accepted |
Added for the storage into etcd. This starts gating all k8s.io/apiserver that uses our standard serialization flows, we could add a direct code method to skip. But I doubt people wrote customer serializers before and this is very clear. |
Looks like good defense. /lgtm |
LGTM label has been added. Git tree hash: 092741d7cf4cb0c163c2a8e7a8f08105b92a8175
|
/hold for @jpbetz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit then LGTM
@@ -724,13 +724,31 @@ func (c *RecommendedConfig) Complete() CompletedConfig { | |||
return c.Config.Complete(c.SharedInformerFactory) | |||
} | |||
|
|||
var allowedMediaTypes = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Use a const here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, this can't be a const in Go
@@ -87,6 +88,12 @@ func NewEtcdOptions(backendConfig *storagebackend.Config) *EtcdOptions { | |||
return options | |||
} | |||
|
|||
var storageMediaTypes = sets.New( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Use a const here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, this can't be a const in Go
/lgtm |
/hold cancel |
Changelog suggestion Updated the generic apiserver library to produce an error if a new API server is configured with support for a data format other than JSON, YAML, or Protobuf. |
Adopted your suggestion, thank you! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Refuse to instantiate a generic apiserver if it is configured with support for data formats other than JSON, YAML, or Protobuf. The motivating use case is to prevent accidental enablement of CBOR in the early stages of phased implementation.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: