Merge pull request kubernetes#89799 from julianvmodesto/patcher

Make kubectl client-side apply with server-side dry-run safer

Author: k8s-ci-robot

staging/src/k8s.io/kubectl/pkg/cmd/apply/BUILD

@@ -68,7 +68,6 @@ go_test(
         "//staging/src/k8s.io/client-go/dynamic/fake:go_default_library",
         "//staging/src/k8s.io/client-go/rest:go_default_library",
         "//staging/src/k8s.io/client-go/rest/fake:go_default_library",
-        "//staging/src/k8s.io/client-go/testing:go_default_library",
         "//staging/src/k8s.io/kubectl/pkg/cmd/testing:go_default_library",
         "//staging/src/k8s.io/kubectl/pkg/cmd/util:go_default_library",
         "//staging/src/k8s.io/kubectl/pkg/scheme:go_default_library",

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

@@ -407,6 +407,20 @@ func (o *ApplyOptions) applyOneObject(info *resource.Info) error {
 		klog.V(4).Infof("error recording current command: %v", err)
 	}
 
+	helper := resource.NewHelper(info.Client, info.Mapping).
+		DryRun(o.DryRunStrategy == cmdutil.DryRunServer).
+		WithFieldManager(o.FieldManager)
+
+	if o.DryRunStrategy == cmdutil.DryRunServer {
+		// Ensure the APIServer supports server-side dry-run for the resource,
+		// otherwise fail early.
+		// For APIServers that don't support server-side dry-run will persist
+		// changes.
+		if err := o.DryRunVerifier.HasSupport(info.Mapping.GroupVersionKind); err != nil {
+			return err
+		}
+	}
+
 	if o.ServerSideApply {
 		// Send the full object to be applied on the server side.
 		data, err := runtime.Encode(unstructured.UnstructuredJSONScheme, info.Object)
@@ -417,14 +431,6 @@ func (o *ApplyOptions) applyOneObject(info *resource.Info) error {
 		options := metav1.PatchOptions{
 			Force: &o.ForceConflicts,
 		}
-		helper := resource.NewHelper(info.Client, info.Mapping).
-			WithFieldManager(o.FieldManager)
-		if o.DryRunStrategy == cmdutil.DryRunServer {
-			if err := o.DryRunVerifier.HasSupport(info.Mapping.GroupVersionKind); err != nil {
-				return err
-			}
-			helper.DryRun(true)
-		}
 		obj, err := helper.Patch(
 			info.Namespace,
 			info.Name,
@@ -495,14 +501,6 @@ See http://k8s.io/docs/reference/using-api/api-concepts/#conflicts`, err)
 
 		if o.DryRunStrategy != cmdutil.DryRunClient {
 			// Then create the resource and skip the three-way merge
-			helper := resource.NewHelper(info.Client, info.Mapping).
-				WithFieldManager(o.FieldManager)
-			if o.DryRunStrategy == cmdutil.DryRunServer {
-				if err := o.DryRunVerifier.HasSupport(info.Mapping.GroupVersionKind); err != nil {
-					return cmdutil.AddSourceToErr("creating", info.Source, err)
-				}
-				helper.DryRun(true)
-			}
 			obj, err := helper.Create(info.Namespace, true, info.Object)
 			if err != nil {
 				return cmdutil.AddSourceToErr("creating", info.Source, err)
@@ -539,7 +537,7 @@ See http://k8s.io/docs/reference/using-api/api-concepts/#conflicts`, err)
 			fmt.Fprintf(o.ErrOut, warningNoLastAppliedConfigAnnotation, o.cmdBaseName)
 		}
 
-		patcher, err := newPatcher(o, info)
+		patcher, err := newPatcher(o, info, helper)
 		if err != nil {
 			return err
 		}

staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go

@@ -44,7 +44,6 @@ import (
 	dynamicfakeclient "k8s.io/client-go/dynamic/fake"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/client-go/rest/fake"
-	clienttesting "k8s.io/client-go/testing"
 	cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 	"k8s.io/kubectl/pkg/scheme"
@@ -1333,6 +1332,11 @@ func TestForceApply(t *testing.T) {
 						}
 						t.Fatalf("unexpected request: %#v after %v tries\n%#v", req.URL, counts["patch"], req)
 						return nil, nil
+					case strings.HasSuffix(p, pathRC) && m == "DELETE":
+						counts["delete"]++
+						deleted = true
+						bodyRC := ioutil.NopCloser(bytes.NewReader(currentRC))
+						return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: bodyRC}, nil
 					case strings.HasSuffix(p, pathRC) && m == "PUT":
 						counts["put"]++
 						bodyRC := ioutil.NopCloser(bytes.NewReader(currentRC))
@@ -1351,16 +1355,6 @@ func TestForceApply(t *testing.T) {
 				}),
 			}
 			fakeDynamicClient := dynamicfakeclient.NewSimpleDynamicClient(scheme)
-			fakeDynamicClient.PrependReactor("delete", "replicationcontrollers", func(action clienttesting.Action) (bool, runtime.Object, error) {
-				if deleteAction, ok := action.(clienttesting.DeleteAction); ok {
-					if deleteAction.GetName() == nameRC {
-						counts["delete"]++
-						deleted = true
-						return true, nil, nil
-					}
-				}
-				return false, nil, nil
-			})
 			tf.FakeDynamicClient = fakeDynamicClient
 			tf.OpenAPISchemaFunc = fn
 			tf.Client = tf.UnstructuredClient

staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go

@@ -33,7 +33,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cli-runtime/pkg/resource"
-	"k8s.io/client-go/dynamic"
 	oapi "k8s.io/kube-openapi/pkg/util/proto"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 	"k8s.io/kubectl/pkg/scheme"
@@ -52,18 +51,16 @@ const (
 
 // Patcher defines options to patch OpenAPI objects.
 type Patcher struct {
-	Mapping       *meta.RESTMapping
-	Helper        *resource.Helper
-	DynamicClient dynamic.Interface
+	Mapping *meta.RESTMapping
+	Helper  *resource.Helper
 
 	Overwrite bool
 	BackOff   clockwork.Clock
 
-	Force        bool
-	Cascade      bool
-	Timeout      time.Duration
-	GracePeriod  int
-	ServerDryRun bool
+	Force       bool
+	Cascade     bool
+	Timeout     time.Duration
+	GracePeriod int
 
 	// If set, forces the patch against a specific resourceVersion
 	ResourceVersion *string
@@ -74,30 +71,30 @@ type Patcher struct {
 	OpenapiSchema openapi.Resources
 }
 
-func newPatcher(o *ApplyOptions, info *resource.Info) (*Patcher, error) {
+func newPatcher(o *ApplyOptions, info *resource.Info, helper *resource.Helper) (*Patcher, error) {
 	var openapiSchema openapi.Resources
 	if o.OpenAPIPatch {
 		openapiSchema = o.OpenAPISchema
 	}
 
 	return &Patcher{
 		Mapping:       info.Mapping,
-		Helper:        resource.NewHelper(info.Client, info.Mapping).WithFieldManager(o.FieldManager),
-		DynamicClient: o.DynamicClient,
+		Helper:        helper,
 		Overwrite:     o.Overwrite,
 		BackOff:       clockwork.NewRealClock(),
 		Force:         o.DeleteOptions.ForceDeletion,
 		Cascade:       o.DeleteOptions.Cascade,
 		Timeout:       o.DeleteOptions.Timeout,
 		GracePeriod:   o.DeleteOptions.GracePeriod,
-		ServerDryRun:  o.DryRunStrategy == cmdutil.DryRunServer,
 		OpenapiSchema: openapiSchema,
 		Retries:       maxPatchRetry,
 	}, nil
 }
 
 func (p *Patcher) delete(namespace, name string) error {
-	return runDelete(namespace, name, p.Mapping, p.DynamicClient, p.Cascade, p.GracePeriod, p.ServerDryRun)
+	options := asDeleteOptions(p.Cascade, p.GracePeriod)
+	_, err := p.Helper.DeleteWithOptions(namespace, name, &options)
+	return err
 }
 
 func (p *Patcher) patchSimple(obj runtime.Object, modified []byte, source, namespace, name string, errOut io.Writer) ([]byte, runtime.Object, error) {
@@ -178,7 +175,7 @@ func (p *Patcher) patchSimple(obj runtime.Object, modified []byte, source, names
 		}
 	}
 
-	patchedObj, err := p.Helper.DryRun(p.ServerDryRun).Patch(namespace, name, patchType, patch, nil)
+	patchedObj, err := p.Helper.Patch(namespace, name, patchType, patch, nil)
 	return patch, patchedObj, err
 }
 
@@ -223,11 +220,11 @@ func (p *Patcher) deleteAndCreate(original runtime.Object, modified []byte, name
 	if err != nil {
 		return modified, nil, err
 	}
-	createdObject, err := p.Helper.DryRun(p.ServerDryRun).Create(namespace, true, versionedObject)
+	createdObject, err := p.Helper.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.DryRun(p.ServerDryRun).Create(namespace, true, original)
+		recreated, recreateErr := p.Helper.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 {

staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go

@@ -143,19 +143,24 @@ func (p *pruner) delete(namespace, name string, mapping *meta.RESTMapping) error
 }
 
 func runDelete(namespace, name string, mapping *meta.RESTMapping, c dynamic.Interface, cascade bool, gracePeriod int, serverDryRun bool) error {
+	options := asDeleteOptions(cascade, gracePeriod)
+	if serverDryRun {
+		options.DryRun = []string{metav1.DryRunAll}
+	}
+	return c.Resource(mapping.Resource).Namespace(namespace).Delete(context.TODO(), name, options)
+}
+
+func asDeleteOptions(cascade bool, gracePeriod int) metav1.DeleteOptions {
 	options := metav1.DeleteOptions{}
 	if gracePeriod >= 0 {
 		options = *metav1.NewDeleteOptions(int64(gracePeriod))
 	}
-	if serverDryRun {
-		options.DryRun = []string{metav1.DryRunAll}
-	}
 	policy := metav1.DeletePropagationForeground
 	if !cascade {
 		policy = metav1.DeletePropagationOrphan
 	}
 	options.PropagationPolicy = &policy
-	return c.Resource(mapping.Resource).Namespace(namespace).Delete(context.TODO(), name, options)
+	return options
 }
 
 type pruneResource struct {

staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go

@@ -367,7 +367,6 @@ func (obj InfoObject) Merged() (runtime.Object, error) {
 		Helper:          helper,
 		Overwrite:       true,
 		BackOff:         clockwork.NewRealClock(),
-		ServerDryRun:    true,
 		OpenapiSchema:   obj.OpenAPI,
 		ResourceVersion: resourceVersion,
 	}

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

@@ -523,8 +523,23 @@ func GetFieldManagerFlag(cmd *cobra.Command) string {
 type DryRunStrategy int
 
 const (
+	// DryRunNone indicates the client will make all mutating calls
 	DryRunNone DryRunStrategy = iota
+
+	// DryRunClient, or client-side dry-run, indicates the client will prevent
+	// making mutating calls such as CREATE, PATCH, and DELETE
 	DryRunClient
+
+	// DryRunServer, or server-side dry-run, indicates the client will send
+	// mutating calls to the APIServer with the dry-run parameter to prevent
+	// persisting changes.
+	//
+	// Note that clients sending server-side dry-run calls should verify that
+	// the APIServer and the resource supports server-side dry-run, and otherwise
+	// clients should fail early.
+	//
+	// If a client sends a server-side dry-run call to an APIServer that doesn't
+	// support server-side dry-run, then the APIServer will persist changes inadvertently.
 	DryRunServer
 )
 

test/cmd/apply.sh

@@ -109,10 +109,15 @@ run_kubectl_apply_tests() {
   kube::test::get_object_assert pods "{{range.items}}{{${id_field:?}}}:{{end}}" ''
   # apply non dry-run creates the pod
   kubectl apply -f hack/testdata/pod.yaml "${kube_flags[@]:?}"
+  initialResourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
   # apply changes
+  kubectl apply --dry-run=client -f hack/testdata/pod-apply.yaml "${kube_flags[@]:?}"
   kubectl apply --dry-run=server -f hack/testdata/pod-apply.yaml "${kube_flags[@]:?}"
   # Post-Condition: label still has initial value
   kube::test::get_object_assert 'pods test-pod' "{{${labels_field:?}.name}}" 'test-pod-label'
+  # Ensure dry-run 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}"
 
   # clean-up
   kubectl delete -f hack/testdata/pod.yaml "${kube_flags[@]:?}"
@@ -377,10 +382,14 @@ run_kubectl_server_side_apply_tests() {
   kube::test::get_object_assert pods "{{range.items}}{{${id_field:?}}}:{{end}}" ''
   # apply non dry-run creates the pod
   kubectl apply --server-side -f hack/testdata/pod.yaml "${kube_flags[@]:?}"
+  initialResourceVersion=$(kubectl get "${kube_flags[@]:?}" -f hack/testdata/pod.yaml -o go-template='{{ .metadata.resourceVersion }}')
   # apply changes
   kubectl apply --server-side --dry-run=server -f hack/testdata/pod-apply.yaml "${kube_flags[@]:?}"
   # Post-Condition: label still has initial value
   kube::test::get_object_assert 'pods test-pod' "{{${labels_field:?}.name}}" 'test-pod-label'
+  # Ensure dry-run 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}"
 
   # clean-up
   kubectl delete -f hack/testdata/pod.yaml "${kube_flags[@]:?}"