CRs: Default non-nullable nulls #95423

apelisse · 2020-10-08T23:37:51Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

CRDs: For structural schemas, non-nullable null map fields will now be dropped and defaulted if a default is available. null items in list will continue being preserved, and fail validation if not nullable.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

apelisse · 2020-10-08T23:38:11Z

/assign @sttts @liggitt

fedebongio · 2020-10-15T20:11:27Z

/triage accepted

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go

sttts · 2020-10-19T13:22:16Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go

 			} else if s.AdditionalProperties != nil {
-				Default(v, s.AdditionalProperties.Structural)
+				if isNonNullableNull(x[k], s.AdditionalProperties.Structural) {
+					x[k] = runtime.DeepCopyJSONValue(s.AdditionalProperties.Structural.Default.Object)


you don't check that there is a default.

Does it matter? At that point, we know that x[k] is null, then that's going to reset it to null? That's a no-op.

don't we need to check if s.AdditionalProperties.Structural is non-nil so we don't panic? if so, add a unit test that would have caught this

Yeah good point

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go

apelisse · 2020-11-02T16:33:28Z

Updated, still missing some higher-level tests specifically.

sttts · 2020-11-03T09:01:52Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -1251,6 +1252,7 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error {
 			// TODO: switch over pruning and coercing at the root to  schemaobjectmeta.Coerce too
 			structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false)
 		}
+		structuraldefaulting.PruneNulls(u.Object, v.structuralSchemas[gv.Version])


why not inside the if statement?

Now I am pretty sure we want this only for !v.preserveUnknownFields.

Now I am pretty sure we want this only for !v.preserveUnknownFields.

I'm trying to understand why. Clearly, unknownfields don't have a schema, so basically, they're always non-nullable non-defaulted, so all the nulls would be dropped. But I don't see why that's a problem, or consistent with what we're doing?

unknownfields don't have a schema, so basically, they're always non-nullable non-defaulted

I don't think that's true... with no schema, we allow any value, including null, so they are nullable non-defaulted

Ah, that's a good point. My perspective has been that "nullable" is false by default, so a lack of schema would imply non-nullable.

Yes, good, that's what I expected.

I consider the non-structural case as legacy we will never ever touch again. Hence, only change pruning of nulls in the if.

As @apelisse found in our validation (he was 2s faster than me):

if spec.PreserveUnknownFields == nil || *spec.PreserveUnknownFields == true { allErrs = append(allErrs, field.Invalid(fldPath.Child("preserveUnknownFields"), true, "must be false in order to use defaults in the schema")) }

We forbid defaulting for preserveUnknownFields:true. Hence, pruning some nulls without defaulting would be confusing.

@sttts changed my mind by showing me this: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go#L252-L254

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.go

liggitt · 2020-11-06T23:55:55Z

/approve
/lgtm

k8s-ci-robot · 2020-11-06T23:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, liggitt

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:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

apelisse · 2020-11-07T03:27:36Z

/priority important-soon
/retest

Kubernetes 1.20 tests are currently broken, since our old invalid config is now considered valid (see kubernetes/kubernetes#95423). In fixing this, I realized we don't have any validation for WorkloadGroup! This adds validation, by applying the same validation WorkloadEntry has to the template of workload group. However, workload entry validation is pretty much nil, so this change isn't very beneficial beyond getting tests passing.

* Add validation for workload group Kubernetes 1.20 tests are currently broken, since our old invalid config is now considered valid (see kubernetes/kubernetes#95423). In fixing this, I realized we don't have any validation for WorkloadGroup! This adds validation, by applying the same validation WorkloadEntry has to the template of workload group. However, workload entry validation is pretty much nil, so this change isn't very beneficial beyond getting tests passing. * gen * fixes * Update * fmt

* Add validation for workload group Kubernetes 1.20 tests are currently broken, since our old invalid config is now considered valid (see kubernetes/kubernetes#95423). In fixing this, I realized we don't have any validation for WorkloadGroup! This adds validation, by applying the same validation WorkloadEntry has to the template of workload group. However, workload entry validation is pretty much nil, so this change isn't very beneficial beyond getting tests passing. * gen * fixes * Update * fmt (cherry picked from commit 6e649ab)

* Add validation for workload group (#29355) * Add validation for workload group Kubernetes 1.20 tests are currently broken, since our old invalid config is now considered valid (see kubernetes/kubernetes#95423). In fixing this, I realized we don't have any validation for WorkloadGroup! This adds validation, by applying the same validation WorkloadEntry has to the template of workload group. However, workload entry validation is pretty much nil, so this change isn't very beneficial beyond getting tests passing. * gen * fixes * Update * fmt (cherry picked from commit 6e649ab) * fix extra file

Currently, Whenever a `SP` is created with `Spec.Routes` field not being set from [golang types](https://github.com/linkerd/linkerd2/blob/main/controller/gen/apis/serviceprofile/v1alpha2/types.go#L13) , k8s API rejects them with the following error ```bash ServiceProfile.linkerd.io \"backend-svc.linkerd-smi-app.svc.cluster.local\" is invalid: spec.routes: Invalid value: \"null\": spec.routes in body must be of type array: \"null\" ``` This happens because, Golang automatically renders them it as `Routes: Null` whenever it marshalled into json. This is rejected by k8s API server as it expects that field to be an array. [**This is fixed in k8s >= 1.20**](kubernetes/kubernetes#95423) as non-nullable nulls are defaulted, and hence this error happens only in `<=1.19`. As `1.19` is a pretty recent version of k8s, and things like [smi-adaptor](https://github.com/linkerd/linkerd-smi/pull) may not want to manage and make sure `Spec.Routes` is not null all the time. This can be easily be fixed by marking `Spec.Routes` as `omitempty` in its json tagswhich means that the field is omitted whenever it is not set while being marshalled. This means that the k8s API won't error out, as that field isn't set to anything invalid. Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

## Context Currently, Whenever a `SP` is created with `Spec.Routes` field not being set from [golang types](https://github.com/linkerd/linkerd2/blob/main/controller/gen/apis/serviceprofile/v1alpha2/types.go#L13), k8s API rejects them with the following error ```bash ServiceProfile.linkerd.io \"backend-svc.linkerd-smi-app.svc.cluster.local\" is invalid: spec.routes: Invalid value: \"null\": spec.routes in body must be of type array: \"null\" ``` This happens because, Golang automatically renders them it as `Routes: Null` whenever it marshaled into json. This is rejected by k8s API server as it expects that field to be an array. [This is fixed in k8s >= 1.20](kubernetes/kubernetes#95423) as non-nullable nulls are defaulted, and hence this error happens only in `<=1.19`. ## Problem As `1.19` is a pretty recent version of k8s, and things like [smi-adaptor](https://github.com/linkerd/linkerd-smi/pull) may not want to manage and make sure `Spec.Routes` is not null all the time. ## Fix This can be easily be fixed by marking `Spec.Routes` as `omitempty` in its json tags which means that the field is omitted whenever it is not set while being marshaled. This means that the k8s API won't error out, as that field isn't set to anything invalid. Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>

k8s-ci-robot requested review from deads2k and logicalhan October 8, 2020 23:38

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 8, 2020

k8s-ci-robot assigned liggitt and sttts Oct 8, 2020

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 15, 2020

sttts reviewed Oct 19, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go Show resolved Hide resolved

sttts reviewed Oct 19, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go Show resolved Hide resolved

sttts reviewed Oct 19, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go Show resolved Hide resolved

apelisse force-pushed the default-null-crds branch from 7de1bdf to 159ce40 Compare November 2, 2020 16:32

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2020

apelisse force-pushed the default-null-crds branch from 159ce40 to c8aac5d Compare November 2, 2020 20:28

sttts reviewed Nov 3, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go Show resolved Hide resolved

sttts reviewed Nov 3, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.go Outdated Show resolved Hide resolved

reylejano mentioned this pull request Nov 6, 2020

Placeholder for Built-in Defaulting kubernetes/website#24640

Closed

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2020

k8s-ci-robot merged commit 9253aa9 into kubernetes:master Nov 7, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 7, 2020

github-actions bot mentioned this pull request Nov 10, 2020

Week Ending November 8, 2020 dev-obs/actus#265

Open

kikisdeliveryservice mentioned this pull request Nov 12, 2020

Built-in API Types Defaults kubernetes/enhancements#1929

Closed

howardjohn mentioned this pull request Dec 3, 2020

Add validation for workload group istio/istio#29355

Merged

chymy mentioned this pull request Feb 2, 2021

Validation fails for CustomResource with array of pointers #98079

Closed

howardjohn mentioned this pull request Feb 25, 2021

[1.8] Add validation for workload group (#29355) istio/istio#31083

Merged

liggitt mentioned this pull request Mar 28, 2021

metav1.Time fails CRD validation #86811

Closed

Pothulapati mentioned this pull request Jun 16, 2021

sp: prevent sp.Spec.Routes from being null'ed linkerd/linkerd2#6271

Merged

wu0407 mentioned this pull request Nov 8, 2021

cannot create serviceScrape VictoriaMetrics/operator#365

Closed

derekbit mentioned this pull request Dec 2, 2021

[TASK] Handle the CRD validation error longhorn/longhorn#3352

Closed

skidder mentioned this pull request Jul 25, 2022

Add status object schema to CRD. lyft/flinkk8soperator#254

Open

pavannd1 mentioned this pull request Jul 29, 2022

[BUG] Kanister Controller cannot update Status on Kubernetes versions < v1.20 kanisterio/kanister#1576

Closed

This was referenced Oct 5, 2022

🐛 Unable to create a VirtualCluster on k8 v1.20.2 kubernetes-sigs/cluster-api-provider-nested#63

Open

🐛Fix VirtualCluster CRD and reconciler for work in 1.19+1.20 environment kubernetes-sigs/cluster-api-provider-nested#316

Closed

XiShanYongYe-Chang mentioned this pull request Dec 29, 2023

add cluster taint, expect pod be evicted, but failed karmada-io/karmada#4402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRs: Default non-nullable nulls #95423

CRs: Default non-nullable nulls #95423

apelisse commented Oct 8, 2020 •

edited

apelisse commented Oct 8, 2020

fedebongio commented Oct 15, 2020

sttts Oct 19, 2020

apelisse Oct 19, 2020

liggitt Nov 5, 2020

apelisse Nov 6, 2020

apelisse commented Nov 2, 2020

sttts Nov 3, 2020

sttts Nov 3, 2020

apelisse Nov 3, 2020

liggitt Nov 3, 2020

apelisse Nov 3, 2020

apelisse Nov 4, 2020

apelisse Nov 4, 2020

sttts Nov 5, 2020

sttts Nov 5, 2020

apelisse Nov 5, 2020

liggitt commented Nov 6, 2020

k8s-ci-robot commented Nov 6, 2020

apelisse commented Nov 7, 2020

CRs: Default non-nullable nulls #95423

CRs: Default non-nullable nulls #95423

Conversation

apelisse commented Oct 8, 2020 • edited

apelisse commented Oct 8, 2020

fedebongio commented Oct 15, 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

apelisse commented Nov 2, 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

liggitt commented Nov 6, 2020

k8s-ci-robot commented Nov 6, 2020

apelisse commented Nov 7, 2020

apelisse commented Oct 8, 2020 •

edited