-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-4827: Update Beta criteria to include versioned JSON response format #5560
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
base: master
Are you sure you want to change the base?
Conversation
8aebf6d
to
819a31b
Compare
/assign @richabanker |
For programmatic access, a structured, versioned JSON response is available. Clients can request the structured format by specifying the appropriate `Accept` header: | ||
`Accept: application/json;v=v1` | ||
|
||
If the `Accept` header is omitted or set to `text/plain`, the endpoint will return the default plain text response. This approach allows for a stable, machine-readable API while preserving the simple text-based output. |
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.
I think we would want to make a structured, versioned format the default for the z-pages endpoints. Defaulting to an unversioned plain text response has the possibility of inadvertently breaking clients that may (despite our warning headers) parse it as the schema evolves.
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.
Also, if we were to make a versioned endpoint as the default, then we might want to also put in place a deprecation policy for this endpoint. We could do something similar to what we do for Kubernetes APIs/metrics today. For example, when we decide to deprecate an old version of the JSON response (e.g., v1) after a new one (v2) is introduced, we could:
- Announce Deprecation: Mark v1 as deprecated in the release notes and have the endpoint return a Warning header for any v1 requests
- Deprecation Window: Keep the v1 endpoint functional for at least 3 releases to give consumers ample time to migrate
- Removal: After the deprecation window, make v2 the default and maybe consider removing v1?
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.
I see, I updated to default return JSON and the deprecation policy, following the similar pattern as https://kubernetes.io/docs/reference/using-api/api-concepts/ and https://kubernetes.io/docs/reference/using-api/deprecation-policy/
"emulationVersion": "1.31.0-beta.0.981", | ||
"minimumCompatibilityVersion": "1.30.0", | ||
"usefulLinks": { | ||
"configz": "/configz", |
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.
having this as a key-value map seems redundant... The current implementation returns a list of available paths as described in this PR description, maybe just having this as an array of strings is ok?
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.
SGTM, updated.
"binaryVersion": "1.31.0-beta.0.981+c6be932655a03b-dirty", | ||
"emulationVersion": "1.31.0-beta.0.981", | ||
"minimumCompatibilityVersion": "1.30.0", | ||
"usefulLinks": { |
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: this is implemented to return Paths
(instead of useful_links
) currently.
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.
Updated.
819a31b
to
d433b09
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yongruilin 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 |
metrics:/metrics | ||
readyz:/readyz | ||
sli metrics:/metrics/slis | ||
configz: /configz |
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.
This doesnt match with the JSON formatted response. Remove the keys from here too?
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.
It is what current KEP has. I've updated to match what we implemented.
### Endpoint Response Format | ||
#### Data format : text | ||
The `/statusz` endpoint supports both structured JSON and plain text formats, with JSON being the default. |
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.
making a versioned response the default means callers who don't think about the format / version they are relying on will expect what they receive to never change
I think callers should have to explicitly request a content-type and version to get structured versioned data
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.
Oh this is my bad, sorry for swaying you in the other direction @yongruilin ! I initially thought that we would want a structured (versioned) response to be the default which we know will not change schema in the future, but considering that we would likely not begin with v1
directly rather a v1alpha1
perhaps and eventually when we do get to graduating v1alpha1
to v1
- we'd need to switch the default - which is breaking clients. Same for a v1
-> v2
change as well.
Defaulting to text/plain
throughout these migrations (which clearly state that this format of the response is not supposed to be parsed), seems like a good compromise then. And for requests that specify application/json
in their request headers, we look for the requested version param as well, if not found - we serve a 406 NotAcceptable
otherwise proceed to handle the request. How does that sound?
The deprecation policy for a given API version (e.g., `v1`) is as follows: | ||
- A deprecated API version must continue to be served for no less than **12 months or 3 releases** (whichever is longer) after the deprecation is announced. |
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.
A deprecated API version must continue to be served for no less than 12 months or 3 releases (whichever is longer) after the deprecation is announced.
That doesn't sound right for stable versions ... once we advertise something as stable, people should be able to integrate with it and expect to ~never have to revisit their integration
Our policy for other GA APIs is that they will remain supported as long as Kubernetes is at 1.x (which I expect to be for the duration of the project)
https://kubernetes.io/docs/reference/using-api/deprecation-policy/
GA API versions may be marked as deprecated, but must not be removed within a major version of Kubernetes
##### Request | ||
* Method: **GET** | ||
* Endpoint: **/statusz** | ||
* Header: `Accept: application/json;v=v1` (or no Accept header) |
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.
Mimic what we do for discovery, where ;as=$kind;v=$version;g=$group
is required to get a specific kind / version
- kind: something like Statusz? do we think we'll ever have to report multiple things from one endpoint in a way that this should be a StatuszList which for now typically just includes one object?
- group: Coming up with a group name for this will be a little meta, but we can think of something... (
config.k8s.io
doesn't seem terrible... all the specific component configs stem off of that, likekubelet.config.k8s.io
,apiserver.config.k8s.io
, etc) - version: it's disconcerting to claim a stable version ("v1") in the very first release we publish it ... especially when combined with the expectation that we will ~never remove support for it... I expected a brand new API surface / shape to start in an alpha
cc @deads2k for visibility / input
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.
how about:
- kind: ComponentStatus
- group: component.k8s.io
- version: start with v1alpha1
I don't think we would need a list in this case, so one object.
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.
component.k8s.io
seems too generic not directly implying what the API is about, not as descriptive as config.k8s.io
. But then I am not sure if statusz is displaying the "config" really. We will perhaps use the same group for Flagz too, maybe that helps narrow down some choices?
- Plain text format is maintained for backward compatibility. | ||
- Feature is implemented for all core Kubernetes components (kube-apiserver, kube-controller-manager, kube-scheduler, kubelet, kube-proxy). | ||
- Integration and e2e tests are added for the JSON response format. | ||
- Gather feedback from users and developers on the structured format. |
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.
we want to gather feedback on the format before we lock ourselves into supporting it forever ... that implies alpha
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.
Updated.
d433b09
to
2616575
Compare
2616575
to
4979cb1
Compare
This PR adds Beta criteria of statusz with versioned structured response.