Create KEP for built-in Defaulting #1928

apelisse · 2020-08-06T20:11:44Z

Create KEP for built-in defaulting, along with mechanisms so that defaulting can behave the same for CRDs.

Proposes the creation of a new default marker for built-in types, and align the default marker in kubebuilder.
That marker will decide what default field we end-up with in the openapi and how this flag will be used.

apelisse · 2020-08-06T20:14:46Z

/assign @sttts

jpbetz · 2020-08-06T21:48:13Z

keps/sig-api-machinery/NNNN-built-in-default/README.md

+### Goals
+
+The goal is to add a new `// +default` tag to our current built-in Go
+IDL. That tag will be transformed into the OpenAPI `default` tag and


What are the formatting rules for the +default tag value? I'd expect it to match what OpenAPI defines, roughly, or what we have in CRDs today, but might be worth referencing the primary documentation for that here and commenting about any special considerations for putting the value into a godoc tag.

Oh, also, How malformed default tags are detected and reported back to the developer is an interesting detail.

Yes, will solve the comment: we should probably take this exactly as the openapi takes it.
And yes, we can fail when we generate the openapi for the types.

The default tag in OpenAPI is not what you think it is. It's not "our default". We might want x-kubernetes-default instead.

Also note that we will never publish the default. But of course it's fine to get it into SSA via OpenAPi.

OpenAPI uses just json for the default. kubebuilder has some other, home-grown syntax.

I talked to Solly, we changed the format to yaml so that it's easier to format (and still allows structures through its json form). kubebuilder would probably be able to change the format since it's a different marker name anyway.

The default tag in OpenAPI is not what you think it is. It's not "our default". We might want x-kubernetes-default instead.

Can you elaborate on how it's different?

This seems to be what defines a default: https://swagger.io/specification/v2/:

Declares the value of the item that the server will use if none is provided.

That seems to match with what I think we're doing.

There is one maybe ugly detail which makes it different to the default of our native types:

A default in a struct (= object in OpenAPI slang) for native types applies even if the whole struct is omitted (as long as the struct is not a pointer).

A default in a struct in the OpenAPI sense (and CRDs for that matter) only applies when the struct / object is specified.

This leads to this kind of constructions to workaround the latter:

type: object properties: foo: type: object properties: bar: type: string default: "abc" default: {}

In other words, the implicit default of objects is undefined.

At least we defined the default semantics of defaults for CRDs in a way, that the outer default {} is applied first, and then the value for bar is set. I.e. the effective default is {"foo": {"bar": "abc"}} here.

Problem with ^^: due to Golang unmarshal semantics, a default of a PodSepc in native types and a default of a PodSpec in a CRD will differ.

I would feel better if we operate on JSON in either case instead of generation of defaulters for native types with different semantics.

Thanks, that's a very good comment. I've added a section that addresses this. I'll try to provide more example.

At least we defined the default semantics of defaults for CRDs in a way, that the outer default {} is applied first, and then the value for bar is set. I.e. the effective default is {"foo": {"bar": "abc"}} here.

That saves our lives. I think that imposing a {} default on non-pointer structs solves that problem.

I would feel better if we operate on JSON in either case instead of generation of defaulters for native types with different semantics.

I think if we follow the steps that I've specified now, operating on json or built-in types should be equivalent.

lavalamp

I'd love to see motion on this!

keps/sig-api-machinery/NNNN-built-in-default/README.md

lavalamp · 2020-08-06T21:43:13Z

keps/sig-api-machinery/NNNN-built-in-default/README.md

+Since the goal of this proposal is to increase consistency with the CRD
+defaulting, we propose to use the same mechanism (both in OpenAPI and
+similar to what kubebuilder does). This defaulting will happen BEFORE
+the existing defaulting functions are called (existing functions can be


I guess I would have expected this to actually just generate those functions, rather than be a separate mechanism?

keps/sig-api-machinery/NNNN-built-in-default/README.md

sttts · 2020-08-07T07:50:41Z

keps/sig-api-machinery/NNNN-built-in-default/README.md

+There are multiple challenges to running defaults declaratively and
+changing the current mechanisms:
+
+- We don't use OpenAPI to drive validation nor defaulting


Why mention validation? Validation is acting on internal types. That's too late. We have to get into the decoding step, i.e. it must be part of the scheme.

keps/sig-api-machinery/NNNN-built-in-default/README.md

keps/sig-api-machinery/1929-built-in-default/README.md

apelisse · 2020-09-21T23:08:59Z

keps/sig-api-machinery/1929-built-in-default/README.md

+# CRD Normalization
+
+In order to have a consistent behavior between CRDs and built-in, the
+following two things need to happen:
+1. Using the flag with CRDs requires the present of a
+`x-kubernetes-remove-nulls` flag (automatically set when using defaults
+in built-in types). When present, the apiserver will remove all null
+values from CRDs before defaulting, after validation, on all updates.
+2. Non-pointer structs will, in both kubebuilder and for built-in types,
+disallow the use of `+default` and force a value of `{}`, since this is
+automatically done by Go deserialization.


@thockin @DirectXMan12 This is a much simpler version of the normalization solution that I was looking for. I'll try to add additional examples here, as discussed with Solly, to show that this makes sense.

keps/sig-api-machinery/1929-built-in-default/README.md

sttts · 2020-10-02T13:52:25Z

keps/sig-api-machinery/1929-built-in-default/README.md

+
+Forcing the "one-line" subset allows us to sidestep a lot of the YAML
+weirdness in syntax, but we can still just use a YAML parser to parse
+the expressions if we want.


a detail: null is allowed in default values?

Also already mentioned above somewhere: we should only allow validating values. We will see how clever the generator and kubebuilder can be made to check that (best effort, doesn't have to be perfect). CRDs will fails to be created if the value does not validate.

sttts · 2020-10-02T13:54:33Z

@apelisse This is much much better now.

One major gap I still see: CRDs need not only null removal, but also zero-value removal. Today's default semantics say that only unspecified values are defaulted.

apelisse · 2020-10-05T17:37:43Z

One major gap I still see: CRDs need not only null removal, but also zero-value removal. Today's default semantics say that only unspecified values are defaulted.

I don't think they are specifically a problem: https://github.com/kubernetes/enhancements/pull/1928/files#diff-f3bb704316dddab7a3f5f1bb6b5b7a50R401

lavalamp · 2020-10-05T18:28:23Z

I'd like this to make the enhancements freeze and it's 99% there. If you could send a follow-up getting the remaining comments addressed, that'd be great, thanks.

/lgtm

apelisse · 2020-10-05T19:19:07Z

/hold cancel

Happy to address feedback as needed even after this is merged.

sttts · 2020-10-06T09:19:35Z

keps/sig-api-machinery/1929-built-in-default/README.md

+      type: string
+      default: default-name
+    defaulted:
+      type: boolean


it's an int above, not a bool.

sttts · 2020-10-06T09:20:08Z

keps/sig-api-machinery/1929-built-in-default/README.md

+  properties:
+    invalid:
+      type: string
+      default: default-name


why invalid zero value? Where is it a) in the type above and b) here in OpenAPI.

With this OpenAPI spec "" is a completely valid value.

sttts · 2020-10-06T09:22:09Z

I don't think they are specifically a problem: https://github.com/kubernetes/enhancements/pull/1928/files#diff-f3bb704316dddab7a3f5f1bb6b5b7a50R401

I disagree. At least there is a major part missing in the KEP. Please add that. Without I question the status "implementable".

liggitt · 2020-10-06T12:38:40Z

Without I question the status "implementable".

agree it should not be marked implementable with TBD approvers and unresolved questions

keps/sig-api-machinery/1929-built-in-default/README.md

liggitt · 2020-10-06T13:50:42Z

keps/sig-api-machinery/1929-built-in-default/README.md

+  // +default=0
+  Number int
+}
+```


We could even go one step further and disallow non-{} defaults for structs.

That would be a lot simpler and avoid the same field getting different defaults specified for it at different levels, like this:

properties: foo: type: object default: {"bar":{"baz":1}} properties: bar: type: object default: {"baz":2} properties: baz: type: number default: 3

Would that restriction break any use cases we care about?

keps/sig-api-machinery/1929-built-in-default/README.md

liggitt · 2020-10-06T14:02:28Z

keps/sig-api-machinery/1929-built-in-default/README.md

+  // +default=0
+  Number int
+}
+```


Limiting the defaults of complex types (objects and arrays) to single-level values ({} and []) would make generating the defaulting functions significantly easier.

keps/sig-api-machinery/1929-built-in-default/README.md

thockin · 2020-10-20T17:07:28Z

keps/sig-api-machinery/1929-built-in-default/README.md

+  // +default=32
+  Integer int
+
+  // +default=bar


Can we spec WRT whitespace and escapes? Can I have a value with a newline in it? Tab? Space?

https://play.golang.org/p/kxNxW48GJ7Z

package main import ( "encoding/json" "fmt" ) func main() { var i interface{} err := json.Unmarshal([]byte(`{" ": "tab:\t\nspace: "}`), &i) if err != nil { panic(err) } fmt.Println(i) // Output: map[ :tab: // space: ] }

thockin · 2020-10-20T17:11:00Z

keps/sig-api-machinery/1929-built-in-default/README.md

+On the other hand, this is forbidden:
+```golang
+Type Root struct {
+  // Defaults on non-pointer structs are FORBIDDEN:


Forbidden or ignored? What actually happens if someone specifies this? Will CI fail?

Forbidden. It will fail.

thockin · 2020-10-20T17:17:15Z

keps/sig-api-machinery/1929-built-in-default/README.md

+     if obj.Name == "" {
+          obj.Name = "default-name"
+     }
+     if obj.Number == 0 {


we should be able to elide this and replace it with a comment

thockin · 2020-10-20T17:23:41Z

keps/sig-api-machinery/1929-built-in-default/README.md

+
+### Graduation Criteria
+
+As an internal feature, this will go straight to stable.


I think the graduation criteria should be that all eligible manually-written default functions are converted to this mechanism and the manual code deleted. That would leave only "weird" cases in */defaults.go.

if dynamic defaulting leaks into user manually-written defaults, it might be a bit more complex to replicate this in static markers. @apelisse did you discuss this case in the KEP / comments; i may have missed it?

func myDefaultingFunc(obj *SomeObject) { if obj.foo == 0 { obj.foo = 2 } obj.bar = obj.foo - 1 }

Right, that's why the "declarative" marker defaults will be applied after custom defaulting logic.

In your example, if there is a declarative marker for foo or bar, neither will be applied since they both have 2 and (that defaulting function is weird since bar is always set ;-) 1 set respectively.

Also, clearly, declarative markers won't be able to have this kind of logic, that's on purpose, more often than not, you're doing something wrong if you need this.

Right, that's why the "declarative" marker defaults will be applied after custom defaulting logic.

interesting, i would have expected the dynamic (custom) defaulting to apply after the static defaulting from markers.

In your example, if there is a declarative marker for foo or bar, neither will be applied since they both have 2 and (that defaulting function is weird since bar is always set ;-) 1 set respectively.

i guess i've tried to showcase that bar's defaulting may solely depend on the value of foo.

interesting, i would have expected the dynamic (custom) defaulting to apply after the static defaulting from markers.

I haven't been able to think of an example where it would be better one way or another, but this is possibly simpler to reason about. What's left unspecified after the custom function will have the declarative value applied.

i guess, i've tried to showcase that bar's defaulting may solely depend on the value of foo.

Yes, that's what I understood. But we're specifically not allowing this kind of cross-field defaulting

Also, clearly, declarative markers won't be able to have this kind of logic, that's on purpose, more often than not, you're doing something wrong if you need this.

hm, if so, there has to be a way to alert authors of new APIs about similar bad practices (if considered bad practices). potentially in documentation.

Good point, I think we should.

thockin · 2020-10-20T17:49:53Z

For the record, I am a HUGE proponent of this direction and I think we eventually need a proper IDL to do justice to the set of problems we have here. I am eager to see what is left after we move all the trivial cases to this mechanism.

One though - As defined, this leaves little room for any other "smart" defaults. E.g. if I want to say the default value for field X is the same as field Y, I might say +default=util.SameAs("Y") - but that is a syntactically valid string, as per this spec. If we require more strict syntax (e.g. quoted strings) this is no longer ambiguous, and we have room to maneuver later.

Alternately, if we thing that quoted strings we could later add +default-fn=util.SameAs("Y") and say that default and default-fn are mutually exclusive.

apelisse · 2020-10-20T19:17:28Z

I don't think we'll ever do +default=<function> though :-)

thockin · 2020-10-22T19:35:40Z

Famous last words.

…

On Tue, Oct 20, 2020 at 12:17 PM Antoine Pelisse ***@***.***> wrote: I don't think we'll ever do +default=<function> though :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1928 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVDHDQCS6BMAAQHVV73SLXO5TANCNFSM4PW562FA> .

sttts · 2020-10-23T11:12:22Z

+default=<some-javascript>, awesome idea. I heard some cloud native components now have web assembly embedded.

neolit123 · 2020-10-26T16:11:04Z

just wanted to point out that the --api-audiences flag of the kube-apisserver appears to exhibit this "bound defaulting" we discussed here:
#1928 (review)

Identifiers of the API. The service account token authenticator will validate that tokens used against the API are bound to at least one of these audiences. If the --service-account-issuer flag is configured and this flag is not, this field defaults to a single element list containing the issuer URL.

https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/

so if the kube-apiserver one day has a configuration file, the defaulting of the api-audiences field would technically depend on the value of the service-account-issuer field.

lavalamp · 2020-10-26T16:16:52Z

+default=<some-javascript>, awesome idea. I heard some cloud native components now have web assembly embedded.

I'm current a big -1 on this, we need to have a long conversation if we want to make defaults anything other than "plain old data".

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2020

k8s-ci-robot requested review from deads2k and fedebongio August 6, 2020 20:11

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Aug 6, 2020

k8s-ci-robot assigned sttts Aug 6, 2020

apelisse force-pushed the kep-default branch 2 times, most recently from e2262d9 to c85ce91 Compare August 6, 2020 21:02

jpbetz reviewed Aug 6, 2020

View reviewed changes

lavalamp reviewed Aug 6, 2020

View reviewed changes

jpbetz reviewed Aug 6, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

jpbetz reviewed Aug 6, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

jpbetz reviewed Aug 6, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

sttts reviewed Aug 7, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

sttts reviewed Aug 7, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

sttts reviewed Aug 7, 2020

View reviewed changes

keps/sig-api-machinery/NNNN-built-in-default/README.md Outdated Show resolved Hide resolved

apelisse mentioned this pull request Aug 7, 2020

Built-in API Types Defaults #1929

Closed

apelisse force-pushed the kep-default branch 4 times, most recently from 0efa992 to 834fa72 Compare August 7, 2020 22:15

munnerz mentioned this pull request Aug 26, 2020

✨ Set default value for corev1.Protocol type if not already specified kubernetes-sigs/controller-tools#480

Merged

DirectXMan12 approved these changes Sep 9, 2020

View reviewed changes

keps/sig-api-machinery/1929-built-in-default/README.md Outdated Show resolved Hide resolved

apelisse commented Sep 21, 2020

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 22, 2020

sttts reviewed Oct 2, 2020

View reviewed changes

keps/sig-api-machinery/1929-built-in-default/README.md Show resolved Hide resolved

sttts reviewed Oct 2, 2020

View reviewed changes

apelisse force-pushed the kep-default branch from 88d1013 to ab3fbf3 Compare October 5, 2020 17:33

Create KEP for built-in Defaulting

4abd4ad

apelisse force-pushed the kep-default branch from ab3fbf3 to 4abd4ad Compare October 5, 2020 17:35

k8s-ci-robot assigned lavalamp Oct 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2020

k8s-ci-robot merged commit 056a5f2 into kubernetes:master Oct 5, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 5, 2020

github-actions bot mentioned this pull request Oct 6, 2020

Week Ending October 4, 2020 dev-obs/actus#238

Open

sttts reviewed Oct 6, 2020

View reviewed changes

liggitt reviewed Oct 6, 2020

View reviewed changes

thockin reviewed Oct 20, 2020

View reviewed changes


		### Graduation Criteria

		As an internal feature, this will go straight to stable.

Create KEP for built-in Defaulting #1928

Create KEP for built-in Defaulting #1928

Conversation

apelisse commented Aug 6, 2020 • edited Loading

apelisse commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 2, 2020

apelisse commented Oct 5, 2020

lavalamp commented Oct 5, 2020

apelisse commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 6, 2020

liggitt commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

apelisse Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

neolit123 Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Oct 20, 2020

apelisse commented Oct 20, 2020

thockin commented Oct 22, 2020 via email

sttts commented Oct 23, 2020

neolit123 commented Oct 26, 2020

lavalamp commented Oct 26, 2020

apelisse commented Aug 6, 2020 •

edited

Loading

apelisse Oct 20, 2020 •

edited

Loading

apelisse Oct 20, 2020 •

edited

Loading

apelisse Oct 20, 2020 •

edited

Loading

neolit123 Oct 20, 2020 •

edited

Loading

neolit123 Oct 20, 2020 •

edited

Loading