diff --git a/hack/testdata/null-propagation/deployment-null.yml b/hack/testdata/null-propagation/deployment-null.yml new file mode 100644 index 00000000000..0d5c78329be --- /dev/null +++ b/hack/testdata/null-propagation/deployment-null.yml @@ -0,0 +1,21 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-dep-null +spec: + selector: + matchLabels: + l1: l1 + template: + metadata: + labels: + l1: l1 + spec: + containers: + - name: nginx + image: registry.k8s.io/nginx:1.7.9 + resources: + requests: + memory: "64Mi" + cpu: null + terminationMessagePolicy: null diff --git a/hack/testdata/null-propagation/resourcesquota-null.yml b/hack/testdata/null-propagation/resourcesquota-null.yml new file mode 100644 index 00000000000..efee46e16f3 --- /dev/null +++ b/hack/testdata/null-propagation/resourcesquota-null.yml @@ -0,0 +1,8 @@ +kind: ResourceQuota +apiVersion: v1 +metadata: + name: my-rq +spec: + hard: + limits.cpu: null + limits.memory: null diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index 920c113bbd7..6825a808e67 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go @@ -1361,6 +1361,10 @@ func mergeMap(original, patch map[string]interface{}, schema LookupPatchMeta, me // original. Otherwise, check if we want to preserve it or skip it. // Preserving the null value is useful when we want to send an explicit // delete to the API server. + // In some cases, this may lead to inconsistent behavior with create. + // ref: https://github.com/kubernetes/kubernetes/issues/123304 + // To avoid breaking compatibility, + // we made corresponding changes on the client side to ensure that the create and patch behaviors are idempotent. if patchV == nil { delete(original, k) if mergeOptions.IgnoreUnmatchedNulls { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go index 68d21e968ff..c3b31e31fb0 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go @@ -43,7 +43,7 @@ import ( "k8s.io/client-go/util/csaupgrade" "k8s.io/component-base/version" "k8s.io/klog/v2" - "k8s.io/kubectl/pkg/cmd/delete" + cmddelete "k8s.io/kubectl/pkg/cmd/delete" cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/scheme" "k8s.io/kubectl/pkg/util" @@ -61,7 +61,7 @@ type ApplyFlags struct { RecordFlags *genericclioptions.RecordFlags PrintFlags *genericclioptions.PrintFlags - DeleteFlags *delete.DeleteFlags + DeleteFlags *cmddelete.DeleteFlags FieldManager string Selector string @@ -84,7 +84,7 @@ type ApplyOptions struct { PrintFlags *genericclioptions.PrintFlags ToPrinter func(string) (printers.ResourcePrinter, error) - DeleteOptions *delete.DeleteOptions + DeleteOptions *cmddelete.DeleteOptions ServerSideApply bool ForceConflicts bool @@ -182,7 +182,7 @@ var ApplySetToolVersion = version.Get().GitVersion func NewApplyFlags(streams genericiooptions.IOStreams) *ApplyFlags { return &ApplyFlags{ RecordFlags: genericclioptions.NewRecordFlags(), - DeleteFlags: delete.NewDeleteFlags("The files that contain the configurations to apply."), + DeleteFlags: cmddelete.NewDeleteFlags("The files that contain the configurations to apply."), PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme), Overwrite: true, @@ -681,6 +681,12 @@ See https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts` return cmdutil.AddSourceToErr("creating", info.Source, err) } + // prune nulls when client-side apply does a create to match what will happen when client-side applying an update. + // do this after CreateApplyAnnotation so the annotation matches what will be persisted on an update apply of the same manifest. + if u, ok := info.Object.(runtime.Unstructured); ok { + pruneNullsFromMap(u.UnstructuredContent()) + } + if o.DryRunStrategy != cmdutil.DryRunClient { // Then create the resource and skip the three-way merge obj, err := helper.Create(info.Namespace, true, info.Object) @@ -759,6 +765,29 @@ See https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts` return nil } +func pruneNullsFromMap(data map[string]interface{}) { + for k, v := range data { + if v == nil { + delete(data, k) + } else { + pruneNulls(v) + } + } +} +func pruneNullsFromSlice(data []interface{}) { + for _, v := range data { + pruneNulls(v) + } +} +func pruneNulls(v interface{}) { + switch v := v.(type) { + case map[string]interface{}: + pruneNullsFromMap(v) + case []interface{}: + pruneNullsFromSlice(v) + } +} + // Saves the last-applied-configuration annotation in a separate SSA field manager // to prevent it from being dropped by users who have transitioned to SSA. // diff --git a/test/cmd/apply.sh b/test/cmd/apply.sh index de690e61c9d..27d0b0dfeeb 100755 --- a/test/cmd/apply.sh +++ b/test/cmd/apply.sh @@ -96,6 +96,34 @@ run_kubectl_apply_tests() { # cleanup kubectl delete pods selector-test-pod + # Create a deployment + kubectl apply -f hack/testdata/null-propagation/deployment-null.yml "${kube_flags[@]:?}" + # resources.limits.cpu should be nil. + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].resources.requests.cpu}" '' + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].resources.requests.memory}" '64Mi' + # The default value of the terminationMessagePolicy field is `File`, so the result will not be changed. + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].terminationMessagePolicy}" 'File' + + # kubectl apply on create should do what kubectl apply on update will accomplish. + kubectl apply -f hack/testdata/null-propagation/deployment-null.yml "${kube_flags[@]}" + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].resources.requests.cpu}" '' + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].resources.requests.memory}" '64Mi' + kube::test::get_object_jsonpath_assert "deployment/my-dep-null" "{.spec.template.spec.containers[0].terminationMessagePolicy}" 'File' + + # hard.limits.cpu should be nil. + kubectl apply -f hack/testdata/null-propagation/resourcesquota-null.yml "${kube_flags[@]}" + kube::test::get_object_jsonpath_assert "resourcequota/my-rq" "{.spec.hard['limits\.cpu']}" '' + kube::test::get_object_jsonpath_assert "resourcequota/my-rq" "{.spec.hard['limits\.memory']}" '' + + # kubectl apply on create should do what kubectl apply on update will accomplish. + kubectl apply -f hack/testdata/null-propagation/resourcesquota-null.yml "${kube_flags[@]}" + kube::test::get_object_jsonpath_assert "resourcequota/my-rq" "{.spec.hard['limits\.cpu']}" '' + kube::test::get_object_jsonpath_assert "resourcequota/my-rq" "{.spec.hard['limits\.memory']}" '' + + # cleanup + kubectl delete deployment my-dep-null + kubectl delete resourcequota my-rq + ## kubectl apply --dry-run=server # Pre-Condition: no POD exists kube::test::get_object_assert pods "{{range.items}}{{${id_field:?}}}:{{end}}" ''