-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Address "Built-in Defaulting" KEP feedback #2082
Conversation
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.
KEP needs approvers listed as well
@@ -484,27 +501,27 @@ the existing defaulting functions | |||
2. Update kube-openapi to parse the `+default` marker and include it in | |||
the OpenAPI definition. | |||
3. Implement the new CRD semantics that clears `null` values from | |||
objects when the `x-kubernetes-remove-nulls` flag is present and true at | |||
the top-level (or in the CRD). | |||
objects when nullable is unset or false. |
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.
Implement the new CRD semantics for properties with nullable
unset or false (which previously would have resulted in a validation error that null is not a valid value):
- properties and list items with a default have the default applied, replacing the explicit null value
- list items without a default let the explicit null value remain, resulting in a validation error (matches current behavior)
- properties without a default have the explicit
null
value removed
We propose to start removing fields with a null value, when the openapi | ||
nullable flag is missing or false. We assume that existing APIs that use |
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.
maybe tweak the ordering to clarify explicit null values will either be removed or defaulted. saying they'll be removed here, then defaulted later is hard to follow
``` | ||
|
||
Note that the generated code for built-in will look like this: | ||
Note that a simplified version of the generated code for built-ins would look like this: |
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.
give an example of the planned generated code for a complex object as well
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.
That's not exactly what you asked, but I think it has the same purpose
the objects would technically be invalid. | ||
For CRDs, invalid zero-values are omitted, otherwise the objects would | ||
technically be invalid. We can safely default these field if they are | ||
unspecified. |
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.
what does "are omitted" mean? I cannot follow.
Are you saying we are adding a validation step before defaulting for all fields that have zero value?
Note that being invalid is not an easy property to check.
Example:
type: string
maxLength: 3
pattern: ^....$
or
type: string
allOf:
- minLength: 5
- maxLength: 4
In other words, you have to apply the full go-openapi/validator.
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'm not planning on changing any of the validation. If we allow empty strings today, we'll continue allowing empty strings with this proposal, but they won't be defaulted. Defaults will only be applied to unspecified or null values on non-nullable fields.
I agree that this doesn't solve all the problems that we would like to solve.
One such problem is for example, container protocol associative key. If someone embeds that container port structure in a CRD, and the "" string is passed for the protocol, it won't be defaulted and will do the wrong thing when it comes to using the associative key (CRD will think the key is [$port, ""]
, but when the pod template is applied, the empty string will get defaulted to TCP and the key will become [$port, TCP]
).
To be fair, the fact that we can't differentiate between empty strings and missing strings for built-in type is not addressed by this proposal and still needs to be solved.
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 can improve incrementally on that in a backward compatible way since this doesn't close the door to improve the zero-value semantics later, if we decide that it is worth it.
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.
IMO this is a blocker for the whole enhancement. It clearly states in the goals:
we will make the behavior equivalent between built-in types and CRDs
This goal is not reached.
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.
x-kubernetes-prune-zero: true
How about something like that? And kubebuilder automatically adding that.
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.
Yeah, that's the obvious solution, which would be generated on "omitEmpty" non-pointers I assume. I like that this is not an option to users.
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.
Am fine with something more advanced in kubebuilder that knows the Go typesystem and when json.Marshal would apply omitempty and when it doesn't. In the CRD schema API we only have to provide the primitives such that kubebuilder can apply them accordingly.
#2082 (comment) is serious to move forward. Not having a plan about a restriction of the approach which makes not reach the stated goal is a nogo. |
I think we're good with this, can we merge this? |
Found a few things to clarify and a few examples to fixup, mostly lgtm otherwise. Only real question I had was about the " |
@@ -483,28 +512,34 @@ the additional defaulting functions, automatically call them prior to | |||
the existing defaulting functions | |||
2. Update kube-openapi to parse the `+default` marker and include it in |
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 miss the discussion and conclusion about publishing the OpenAPI defaults. For CRDs today we do not publish them intentionally. I suggest to do the same 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.
For CRDs today we do not publish them intentionally.
I didn't know that. I don't really understand it, but I guess I'm fine if we do the same. As long as server-side apply is able to read them since that's one of the reason why we're building that mechanism.
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.
SSA will see the full schema. This happens as part of the OpenAPI v2 conversion.
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.
Have you added a clarification anywhere?
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.
No, not yet, I'm not super satisfied to hide them. it means we can't generate a good "openapi" example to test the behavior in kube-openapi without re-adding the defaults, it means client can't merge properly using listmapkey because they don't have the default either. The more expressive the schema the better, we should keep the defaults, and we should probably do so for CRDs 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.
See my text in the gdoc. I still think we have to describe our plan.
06a2b1d
to
37f578d
Compare
nullable are happy with null values. Also, people can remove the | ||
nullable attribute if they want null values to start being removed. This | ||
will help aligning CRDs and built-in types (there are no ways to set | ||
nullable to true for built-in types). |
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.
Maybe worth a short note:
We will remove null fields also from map[string]T
s, i.e. for type that have additionalProperties
in their schema instead of properties
.
// +default=default-name | ||
Name string | ||
// +default="default-name" | ||
Name string `json:"name,omitempty"` |
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 is no example for the cases above:
""
is valid- the default is not the zero value.
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 do you know "" is valid? The comment reads "empty string is not a valid string", we just can't differentiate between unset and invalid.
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.
Why does it matter whether it is valid or not? Why not the other cases? This is not obvious.
Non-pointer scalar fields that have a default must be omitempty (unless | ||
their default is the zero-value). Kubebuilder and built-in defaulting | ||
generators should both require this. | ||
|
||
This would generate the following OpenAPI: |
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.
where is the golang source for the schema below?
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.
Ah yes, the name doesn't match, good catch
Am still not happy with the examples section. There are still a number of typos and missing cases. I would also like to see a clear distinction between the proposed
Especially the lack of separation of 1-3 vs. 4 makes it read very handwavy. |
|
||
// This field is defaulted to 0 with no defaulter. | ||
// This field would be defaulted to 0 without a 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.
Why would it be default to 0 without a default? What about non-omitempty cases in general?
|
||
// This field is defaulted to 0 with no defaulter. | ||
// +default=0 | ||
// This field would be defaulted to 0 without a 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.
That defaulted below looks a little weird now.
built-in that would behave the same way. | ||
|
||
### CRD Normalization | ||
We will depend on tooling (defaulter-gen and kubebuilder) to enforce the | ||
new type requirements (via linting, warnings or errors), to implicitly |
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: linting and warnings was meant to be the same
+default marker | ||
2. we forbid or warn (liniting) about omitempty on non-pointer structs | ||
3. we require omitempty on scalars with default different from the | ||
zero-value. |
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.
see my 4th rule in the gdoc
// +default="default-name" | ||
Invalid string `json:"name,omitempty"` | ||
Name string `json:"name,omitempty"` |
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.
json:"name,omitempty"
by rule 3
`omitempty`. Non-pointer scalar fields that have a default must be | ||
omitempty (unless their default is the zero-value). Kubebuilder and | ||
built-in defaulting generators should both require this. | ||
`omitempty`. On the other hand, a manifest in YAML |
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 manifest in JSON
|
||
{"entry": {"invalid": "", "number": 0}} | ||
|
||
converted to JSON directly and sent to the server would result in |
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.
remove "converted to JSON direction and"
// +default="apple" | ||
type Item 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.
flesh this one out, same for string maps
|
||
As described by @sttts on many occasions, the immutability is blocked | ||
because we don't know how to distinguish between null and | ||
unspecified. We believe that this mechanism will be able to unblock it. | ||
|
||
### Changes to kubebuilder/default-gen |
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.
@DirectXMan12 please take a look
zero-value. | ||
zero-value | ||
4. we auto-generate `default: <zero-value>` for scalars without | ||
omitempty and forbit the `+default` marker |
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.
forbid
>>> default('{mapping: {foo: null, bar: apple}') | ||
{"mapping": {"foo": "banana", "bar": "apple"} | ||
``` | ||
|
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.
maybe another case with null and no default => removal
>>> default('{list: [null, foo]}') | ||
{"list": ["apple", "foo"]} | ||
``` | ||
|
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.
add case with null and no default => NO removal
Only two nits left: examples for array and string map with null and no default Other than that: LGTM |
f32d5b0
to
d3906e5
Compare
Added the example and rebased! |
properties: | ||
mapping: | ||
type: object | ||
additionalProperties: |
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: tab
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.
😬
Co-authored-by: Dr. Stefan Schimanski <stefan.schimanski@gmail.com>
d3906e5
to
e2baa04
Compare
/approve Maybe @DirectXMan12 can look at the section @sttts tagged them in and do the final LGTM? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, lavalamp 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 |
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.
couple of nits inline, but consider this a soft lgtm, expecially in the part I was directly tagged in
/lgtm
type: string | ||
default: default-name | ||
default: "default-name" |
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: don't actually need the quotes here -- it's the same either way ;-)
sent to the server would result in different behaviour for built-ins | ||
|
||
```python | ||
>>> default('{name: ""}') |
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: it's a bit weird that the example object changes when it looks like you're going to keep talking about the above invalid entry.
and CRDs: | ||
|
||
```python | ||
>>> default('{name:"", number:0}') |
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.
number
vs defaulted
?
cc @sttts @liggitt @DirectXMan12 @jpbetz