changed mind about oenps

keps/sig-api-machinery/4153-declarative-validation/README.md

@@ -799,7 +799,7 @@ We will need to add the ability for fields that re-use a type to specify a
 field-level for its nested fields. Kube-openapi gen will inline the custom 
 definition.
 
-#### Representing Go-isms in Schema
+#### Zero values, `null`, and `omitempty`
 
 Our native types are backed by Go and serialized into Go for mutation, defaulting
 and other transformations before being used for validation. The schemas presented
@@ -822,7 +822,7 @@ and propose additional schema annotations to make our native type schemas more
 representative of their implicit constraints to ensure all validations are
 ported correctly.
 
-##### Dropped Fields in Unstructured Conversion
+##### `omitempty` Primitives Have Incorrect Schema
 
 Consider the following omitempty non-pointer field from `DeploymentSpec`:
 
@@ -857,23 +857,30 @@ field not to be considered during validation, even if it was included.
 This would surface as:
 1. `has(self.paused)` in CEL on the Container object always returns `false` if
 that value is `false`. But a CRD with identical schema would return `true`.
+A proof of concept example of how this bug could bite users of ValidatingAdmissionPolicy 
+is [here](https://github.com/kubernetes/kubernetes/pull/120244).
 2. CEL and JSONSchema Validations of the field if they exist also will not run.
-
-This class of problem exclusively affects `omitempty` non-pointer fields: pointers
-to the openapi-generater are treated as optionals and nil used to disambiguate
-set/unset.
-
-There are two alternative approaches to address this in the schema:
-
-1. Express in schema that zero values are considered unset by adding
-an x-kubernetes extension like `x-kubernetes-zero-unset: true` to fields where
-this is true.
-2. Express in schema that unset values are considered zero by settings
-its `default` to the empty value.
-
-Option 2 seems best, since this is already done for `struct` types, there is 
-already support for `default` in OpenAPI, and the solution naturally expresses 
-Go's behavior of `unset` being implicitly defaulted to zero.
+But this is not a concern as long as empty values are assumed to match the schema.
+
+There are two alternative approaches to address the difference in behavior
+of native types vs CRDs in the schema.
+
+1. Express in schema that zero values are considered unset: by adding an 
+x-kubernetes extension like `x-kubernetes-zero-is-unset: true` to fields where
+ this is true. Clients should know to strip zero values from fields with this
+ annotation.
+2. Express in schema that unset values are considered zero: by settings its
+`default` to the empty value for the type.
+
+Option 1 has the benefit of being consistent with interpreting the object as JSON.
+It has the drawback of requiring a new `x-kubernetes` extension.
+
+Option 2 has the drawback of applying a default to fields where the zero
+value is not considered a default. For cases like `Paused` this is correct,
+but for most cases involving `string`, the empty string is treated as `unset`.
+Additionally option 2 is not consistent with validation of the object as 
+unstructured: in JSON `paused` is not present, so it would be awkward for 
+`has(paused)` to be true.
 
 An exhaustive list of all 730 omit-empty non-pointer is [found here](https://gist.github.com/alexzielenski/05f594f3974aec9b3e74620b8dc7dde1). Struct types already have their
 defaults populated with the go zero value, so they are excluded from this list.