Merge pull request kubernetes#125646 from HirazawaUi/apply-null

Prune explicit nulls from client-side apply create

Author: k8s-ci-robot

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

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

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 {

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.
 //

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}}" ''