diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/BUILD b/staging/src/k8s.io/kubectl/pkg/cmd/apply/BUILD index 59829bac05a..63cc83e0402 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/BUILD +++ b/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", 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 1d1b6525571..fc1ffc7d2d4 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go +++ b/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 } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go index 15c34fa7a2a..2f6fe3dec01 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go +++ b/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 diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go index c634fae20ae..9ec40a2617a 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/patcher.go +++ b/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,7 +71,7 @@ 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 @@ -82,22 +79,22 @@ func newPatcher(o *ApplyOptions, info *resource.Info) (*Patcher, error) { 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 { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go index 60b9d5ec64c..6856fcea6dc 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go +++ b/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 { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go b/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go index b04adec7c43..c9e7df497b5 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go +++ b/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, } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go index 9273e02cec2..799c86b6035 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go +++ b/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 ) diff --git a/test/cmd/apply.sh b/test/cmd/apply.sh index 68b6283d4e9..5e8d32a17d8 100755 --- a/test/cmd/apply.sh +++ b/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[@]:?}"