API conventions: add more on defaulting #6764
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin 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 |
k8s-ci-robot
added
area/developer-guide
|
Also @apelisse - it might be interesting to denote some fields as |
|
Oh, this needs a TOC update - didn't we have a tool for that? Edit: done |
| the values applied when using the "v2" API. In most cases, these values are | ||
| defined as literal values by the API version (e.g. "if this field is not | ||
| specified it defaults to 0"). In some cases, these values may be | ||
| deterministically derived from other fields (e.g. "if this field is not |
There was a problem hiding this comment.
fwiw, deriving a default from another mutable field makes for really confusing semantics on apply update when the other field gets changed, but the derived default sticks. I'd go so far as to call derived defaults an anti-pattern.
There was a problem hiding this comment.
I can note this in the text, but consider even things like:
if strategy.Type == appsv1.RollingUpdateDeploymentStrategyType {
if strategy.RollingUpdate == nil {
rollingUpdate := appsv1.RollingUpdateDeployment{}
strategy.RollingUpdate = &rollingUpdate
}
if strategy.RollingUpdate.MaxUnavailable == nil {
// Set default MaxUnavailable as 25% by default.
maxUnavailable := intstr.FromString("25%")
strategy.RollingUpdate.MaxUnavailable = &maxUnavailable
}
if strategy.RollingUpdate.MaxSurge == nil {
// Set default MaxSurge as 25% by default.
maxSurge := intstr.FromString("25%")
strategy.RollingUpdate.MaxSurge = &maxSurge
}
}
| considered. If the field is immutable after creation, consider adding logic to | ||
| manually "patch" the value from the "old" object into the "new" one when it has |
There was a problem hiding this comment.
for my understanding, you are saying that if my v1 immutable field defaults to false, and my v2 default to true; I have to mutate the field from false to true if the PUT doesn't specify a value on that field?
There was a problem hiding this comment.
The PUT with v2 will default the unspecified field to the v2 default value. The validate-update code will see the field change from false (v1) to true (v2) and SHOULD fail with an error because the field is immutable.
The only fixes here are:
- require RMW
- don't use defaults.go, but do it like Service IPs - in registry - and manually patch it
|
All comments addressed in the text, I hope. Compare: https://github.com/kubernetes/community/compare/86597c5414674efde1ef8d3ab8de2b3306d87990..7cbe84067ec732f27e2d73d9fa3facc649354757 |
| Some care is required when deciding which mechanism to use and managing the | ||
| semantics. | ||
|
|
||
| ### Static Defaults |
There was a problem hiding this comment.
Would it be worth describing what CEL will now allow? @jpbetz And it's probably a little premature but also possibly in the admission webhook category below.
There was a problem hiding this comment.
Good question. CEL for admission control (only an idea right now, not even a provisional KEP) would be the best fit and would align with the below "Admission Controlled Defaults". I would just be another mechanism, in addition to admission controllers and admission webhooks to perform the mutation.
There was a problem hiding this comment.
can we take that as a followup (since I myself am not sure what to write..)?
There was a problem hiding this comment.
I'm totally fine with that, thanks!
|
Looks great to me, thanks |
|
LGTM defer to @liggitt |
| solution in the face of version skew. | ||
|
|
||
| When an object is updated with a PUT, the API server will see the "old" | ||
| object with defaulted values and the "new" object without. For static defaults |
There was a problem hiding this comment.
This sentence is a bit confusing... the new object is handled with defaults freshly applied to the new object, not without defaults. Maybe something like ... and the "new" object with independently defaulted values.?
| When an object is updated with a PUT, the API server will see the "old" | ||
| object with defaulted values and the "new" object without. For static defaults | ||
| this is mainly a problem if the CREATE and the PUT used different API versions | ||
| (and even then it might be semantically correct). For example, "v1" of an API |
There was a problem hiding this comment.
the parenthetical makes this sentence difficult to understand - "this is a problem ... but might be correct"?
There was a problem hiding this comment.
Yeah, the next para talks thru this, so removing parens.
In reviewing an issue/PR I realized this lesson had not been documented anywhere. This is not the ideal mechanism, IMO, but better than nothing.
| solution in the face of version skew. | ||
|
|
||
| When an object is updated with a PUT, the API server will see the "old" | ||
| object with defaulted values and the "new" object without. For static defaults |
| When an object is updated with a PUT, the API server will see the "old" | ||
| object with defaulted values and the "new" object without. For static defaults | ||
| this is mainly a problem if the CREATE and the PUT used different API versions | ||
| (and even then it might be semantically correct). For example, "v1" of an API |
There was a problem hiding this comment.
Yeah, the next para talks thru this, so removing parens.
|
/lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
In reviewing an issue/PR I realized this lesson had not been documented
anywhere. This is not the ideal mechanism, IMO, but better than
nothing.
This comes out of Service ClusterIP and NodePorts, plus #103546
@robscott @aojea @khenidak @liggitt @msau42