Ensure diff doesn't persist patches

julianvmodesto · julianvmodesto · commit 7444c1e865b6 · 2020-04-03T12:07:52.000-04:00
diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go
@@ -80,16 +80,9 @@ func newPatcher(o *ApplyOptions, info *resource.Info) (*Patcher, error) {
 		openapiSchema = o.OpenAPISchema
 	}
 
-	helper := resource.NewHelper(info.Client, info.Mapping)
-	if o.DryRunStrategy == cmdutil.DryRunServer {
-		if err := o.DryRunVerifier.HasSupport(info.Mapping.GroupVersionKind); err != nil {
-			return nil, err
-		}
-		helper.DryRun(true)
-	}
 	return &Patcher{
 		Mapping:       info.Mapping,
-		Helper:        helper,
+		Helper:        resource.NewHelper(info.Client, info.Mapping),
 		DynamicClient: o.DynamicClient,
 		Overwrite:     o.Overwrite,
 		BackOff:       clockwork.NewRealClock(),
@@ -185,7 +178,7 @@ func (p *Patcher) patchSimple(obj runtime.Object, modified []byte, source, names
 		}
 	}
 
-	patchedObj, err := p.Helper.Patch(namespace, name, patchType, patch, nil)
+	patchedObj, err := p.Helper.DryRun(p.ServerDryRun).Patch(namespace, name, patchType, patch, nil)
 	return patch, patchedObj, err
 }
 
@@ -230,11 +223,11 @@ func (p *Patcher) deleteAndCreate(original runtime.Object, modified []byte, name
 	if err != nil {
 		return modified, nil, err
 	}
-	createdObject, err := p.Helper.Create(namespace, true, versionedObject)
+	createdObject, err := p.Helper.DryRun(p.ServerDryRun).Create(namespace, true, versionedObject)
 	if err != nil {
 		// restore the original object if we fail to create the new one
 		// but still propagate and advertise error to user
-		recreated, recreateErr := p.Helper.Create(namespace, true, original)
+		recreated, recreateErr := p.Helper.DryRun(p.ServerDryRun).Create(namespace, true, original)
 		if recreateErr != nil {
 			err = fmt.Errorf("An error occurred force-replacing the existing object with the newly provided one:\n\n%v.\n\nAdditionally, an error occurred attempting to restore the original object:\n\n%v", err, recreateErr)
 		} else {
diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go b/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go
@@ -312,6 +312,7 @@ func (obj InfoObject) Live() runtime.Object {
 // Returns the "merged" object, as it would look like if applied or
 // created.
 func (obj InfoObject) Merged() (runtime.Object, error) {
+	helper := resource.NewHelper(obj.Info.Client, obj.Info.Mapping).DryRun(true)
 	if obj.ServerSideApply {
 		data, err := runtime.Encode(unstructured.UnstructuredJSONScheme, obj.LocalObj)
 		if err != nil {
@@ -320,9 +321,8 @@ func (obj InfoObject) Merged() (runtime.Object, error) {
 		options := metav1.PatchOptions{
 			Force:        &obj.ForceConflicts,
 			FieldManager: obj.FieldManager,
-			DryRun:       []string{metav1.DryRunAll},
 		}
-		return resource.NewHelper(obj.Info.Client, obj.Info.Mapping).Patch(
+		return helper.Patch(
 			obj.Info.Namespace,
 			obj.Info.Name,
 			types.ApplyPatchType,
@@ -334,11 +334,11 @@ func (obj InfoObject) Merged() (runtime.Object, error) {
 	// Build the patcher, and then apply the patch with dry-run, unless the object doesn't exist, in which case we need to create it.
 	if obj.Live() == nil {
 		// Dry-run create if the object doesn't exist.
-		return resource.NewHelper(obj.Info.Client, obj.Info.Mapping).CreateWithOptions(
+		return helper.CreateWithOptions(
 			obj.Info.Namespace,
 			true,
 			obj.LocalObj,
-			&metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}},
+			&metav1.CreateOptions{},
 		)
 	}
 
@@ -361,7 +361,7 @@ func (obj InfoObject) Merged() (runtime.Object, error) {
 	// We plan on replacing this with server-side apply when it becomes available.
 	patcher := &apply.Patcher{
 		Mapping:         obj.Info.Mapping,
-		Helper:          resource.NewHelper(obj.Info.Client, obj.Info.Mapping),
+		Helper:          helper,
 		Overwrite:       true,
 		BackOff:         clockwork.NewRealClock(),
 		ServerDryRun:    true,
diff --git a/test/cmd/diff.sh b/test/cmd/diff.sh
@@ -34,21 +34,34 @@ run_kubectl_diff_tests() {
 
     kubectl apply -f hack/testdata/pod.yaml
     kube::test::get_object_assert 'pod' "{{range.items}}{{ if eq ${id_field:?} \\\"test-pod\\\" }}found{{end}}{{end}}:" 'found:'
+    initialResourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
 
     # Make sure that diffing the resource right after returns nothing (0 exit code).
     kubectl diff -f hack/testdata/pod.yaml
 
+    # Ensure diff only dry-runs and doesn't persist change
+    resourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
+    kube::test::if_has_string "${resourceVersion}" "${initialResourceVersion}"
+
     # Make sure that:
     # 1. the exit code for diff is 1 because it found a difference
     # 2. the difference contains the changed image
     output_message=$(kubectl diff -f hack/testdata/pod-changed.yaml || test $? -eq 1)
     kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0'
 
+    # Ensure diff only dry-runs and doesn't persist change
+    resourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
+    kube::test::if_has_string "${resourceVersion}" "${initialResourceVersion}"
+
     # Test found diff with server-side apply
     kubectl apply -f hack/testdata/pod.yaml
     output_message=$(kubectl diff -f hack/testdata/pod-changed.yaml --server-side --force-conflicts || test $? -eq 1)
     kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0'
 
+    # Ensure diff --server-side only dry-runs and doesn't persist change
+    resourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
+    kube::test::if_has_string "${resourceVersion}" "${initialResourceVersion}"
+
     # Test that we have a return code bigger than 1 if there is an error when diffing
     kubectl diff -f hack/testdata/invalid-pod.yaml || test $? -gt 1
 
