Skip to content

KEP-4222: Add CBOR feature gates. #128539

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

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

benluddy
Copy link
Contributor

@benluddy benluddy commented Nov 4, 2024

What type of PR is this?

/kind feature
/sig api-machinery

What this PR does / why we need it:

For alpha, there is one apiserver feature gate and two client-go feature gates controlling CBOR. They were initially wired to separate test-only feature gate instances in order to prevent them from being configurable at runtime via command-line flags or environment variables (for client-go feature gates outside of Kubernetes components). All of the integration tests required by the KEP as alpha criteria have been implemented. This adds the feature gates to the usual feature gate instances and removes the temporary code to support separate test-only feature gate instances.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This PR excludes wiring the codecs used for serving built-in API resources (by request from @deads2k). That will follow before 1.32 code freeze.

Does this PR introduce a user-facing change?

Added the feature gate CBORServingAndStorage to allow CBOR as the encoding for API request and response bodies, and as the storage encoding for custom resources. Clients must opt in; programs built with client-go can do this using the client-go feature gates ClientsAllowCBOR and ClientsPreferCBOR.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://kep.k8s.io/4222

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/test labels Nov 4, 2024
@benluddy
Copy link
Contributor Author

benluddy commented Nov 4, 2024

/cc @jpbetz @deads2k

@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 4, 2024
@benluddy
Copy link
Contributor Author

benluddy commented Nov 4, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 4, 2024
@jpbetz
Copy link
Contributor

jpbetz commented Nov 4, 2024

/lgtm
/approve

/hold
This appears to activate the alpha of this enhancement, so unless I'm misunderstanding, so please remove hold only when everything else has merged and we're fully ready for alpha.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 357fa09dcef6ca40568361ab83c31dc73ad8a5ea

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024
@tengqm
Copy link
Contributor

tengqm commented Nov 5, 2024

Is there a (placeholder) PR to the website for this feature gate?

@benluddy
Copy link
Contributor Author

benluddy commented Nov 5, 2024

Is there a (placeholder) PR to the website for this feature gate?

Yes: kubernetes/website#48464

@k8s-ci-robot k8s-ci-robot added the sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. label Nov 5, 2024
@benluddy
Copy link
Contributor Author

benluddy commented Nov 5, 2024

/retest-required

@deads2k
Copy link
Contributor

deads2k commented Nov 5, 2024

/approve

Criteria for featuregate in #122921 are met once #128462 (tagged for merge) has landed.

@benluddy please remove the hold after that PR has merged.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, deads2k, jpbetz

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024

Verified

This commit was signed with the committer’s verified signature.
benluddy Ben Luddy
For alpha, there is one apiserver feature gate and two client-go feature gates controlling
CBOR. They were initially wired to separate test-only feature gate instances in order to prevent
them from being configurable at runtime via command-line flags or environment variables (for
client-go feature gates outside of Kubernetes components). All of the integration tests required by
the KEP as alpha criteria have been implemented. This adds the feature gates to the usual feature
gate instances and removes the temporary code to support separate test-only feature gate instances.
@benluddy
Copy link
Contributor Author

benluddy commented Nov 5, 2024

/hold cancel

#128462 has merged.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2024
@deads2k
Copy link
Contributor

deads2k commented Nov 5, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2515126a01b5be807c9db9c03a732dd99f6a825e

Verified

This commit was signed with the committer’s verified signature.
benluddy Ben Luddy

Verified

This commit was signed with the committer’s verified signature.
benluddy Ben Luddy
Setting AllAlpha=true in integration tests changes the dynamic client request encoding and the
custom resource storage encoding to CBOR. The etcd storage path is updated to accept either JSON or
CBOR as storage encoding. The client feature gate controlling the dynamic client request encoding is
temporarily disabled until the serving codecs for builtin APIs are wired to the CBOR apiserver
feature gate.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024
@deads2k
Copy link
Contributor

deads2k commented Nov 5, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5d5110cd09b6ac6eecb01023a98393c28a1bde3c

@k8s-ci-robot k8s-ci-robot merged commit a28f140 into kubernetes:master Nov 5, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Nov 5, 2024
Comment on lines +45 to +50
// If enabled, and only if ClientsAllowCBOR is also enabled, the default request content
// type (if not explicitly configured) and the dynamic client's request content type both
// become "application/cbor" instead of "application/json". The default content type for
// apply patch requests becomes "application/apply-patch+cbor" instead of
// "application/apply-patch+yaml".
ClientsPreferCBOR Feature = "ClientsPreferCBOR"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cause the issues here #128600

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. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants