From 7a7b2a177cc0636ba95a31fb21288acd7a65288f Mon Sep 17 00:00:00 2001 From: Brian Pursley Date: Tue, 21 Jun 2022 10:51:30 -0400 Subject: [PATCH] Remove unused flags from kubectl run The following flags, which do not apply to kubectl run, have been removed: --cascade --filename --force --grace-period --kustomize --recursive --timeout --wait These flags were being added to the run command to support pod deletion after attach, but they are not used if set, so they effectively do nothing. This PR also displays an error message if the pod fails to be deleted (when the --rm flag is used). Previously any error during deletion would be suppressed and the pod would remain. This PR also adds some unit tests for run and attach with and without the --rm flag. As such, some minor refactoring of the run command has been done to support mocking dependencies. Kubernetes-commit: 25e713ba777ec1158fad749e9467601526ba096a --- pkg/cmd/run/run.go | 35 ++++---- pkg/cmd/run/run_test.go | 190 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/run/run.go b/pkg/cmd/run/run.go index dd154ce24..c57d2e6b8 100644 --- a/pkg/cmd/run/run.go +++ b/pkg/cmd/run/run.go @@ -36,7 +36,6 @@ import ( "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/cli-runtime/pkg/resource" - "k8s.io/client-go/kubernetes" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/tools/cache" watchtools "k8s.io/client-go/tools/watch" @@ -94,6 +93,8 @@ const ( var metadataAccessor = meta.NewAccessor() +var attachFunc = attach.DefaultAttachFunc + type RunObject struct { Object runtime.Object Mapping *meta.RESTMapping @@ -105,7 +106,6 @@ type RunOptions struct { PrintFlags *genericclioptions.PrintFlags RecordFlags *genericclioptions.RecordFlags - DeleteFlags *cmddelete.DeleteFlags DeleteOptions *cmddelete.DeleteOptions DryRunStrategy cmdutil.DryRunStrategy @@ -135,7 +135,6 @@ type RunOptions struct { func NewRunOptions(streams genericclioptions.IOStreams) *RunOptions { return &RunOptions{ PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme), - DeleteFlags: cmddelete.NewDeleteFlags("to use to replace the resource."), RecordFlags: genericclioptions.NewRecordFlags(), Recorder: genericclioptions.NoopRecorder{}, @@ -159,7 +158,6 @@ func NewCmdRun(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.Co }, } - o.DeleteFlags.AddFlags(cmd) o.PrintFlags.AddFlags(cmd) o.RecordFlags.AddFlags(cmd) @@ -226,18 +224,17 @@ func (o *RunOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { return printer.PrintObj(obj, o.Out) } - deleteOpts, err := o.DeleteFlags.ToOptions(dynamicClient, o.IOStreams) - if err != nil { - return err + o.DeleteOptions = &cmddelete.DeleteOptions{ + CascadingStrategy: metav1.DeletePropagationBackground, + DynamicClient: dynamicClient, + GracePeriod: -1, + IgnoreNotFound: true, + IOStreams: o.IOStreams, + Quiet: o.Quiet, + Timeout: time.Duration(0), + WaitForDeletion: false, } - deleteOpts.IgnoreNotFound = true - deleteOpts.WaitForDeletion = false - deleteOpts.GracePeriod = -1 - deleteOpts.Quiet = o.Quiet - - o.DeleteOptions = deleteOpts - return nil } @@ -325,7 +322,11 @@ func (o *RunOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e if o.Attach { if remove { - defer o.removeCreatedObjects(f, createdObjects) + defer func() { + if err := o.removeCreatedObjects(f, createdObjects); err != nil { + fmt.Fprintf(o.ErrOut, "Delete failed: %v\n", err) + } + }() } opts := &attach.AttachOptions{ @@ -345,9 +346,9 @@ func (o *RunOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e return err } opts.Config = config - opts.AttachFunc = attach.DefaultAttachFunc + opts.AttachFunc = attachFunc - clientset, err := kubernetes.NewForConfig(config) + clientset, err := f.KubernetesClientSet() if err != nil { return err } diff --git a/pkg/cmd/run/run_test.go b/pkg/cmd/run/run_test.go index 0d3491a37..5ebf6a807 100644 --- a/pkg/cmd/run/run_test.go +++ b/pkg/cmd/run/run_test.go @@ -33,10 +33,13 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/cli-runtime/pkg/genericclioptions" restclient "k8s.io/client-go/rest" "k8s.io/client-go/rest/fake" + "k8s.io/client-go/tools/remotecommand" + "k8s.io/kubectl/pkg/cmd/attach" "k8s.io/kubectl/pkg/cmd/delete" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" cmdutil "k8s.io/kubectl/pkg/cmd/util" @@ -640,6 +643,193 @@ func TestExpose(t *testing.T) { } } +func TestRunAttach(t *testing.T) { + tests := []struct { + name string + rm bool + quiet bool + deleteErrorMessage string + expectedDeleteCount int + expectedOut string + expectedErrOut string + }{ + { + name: "test attach", + rm: false, + quiet: false, + expectedDeleteCount: 0, + expectedOut: "", + expectedErrOut: "If you don't see a command prompt, try pressing enter.\n", + }, + { + name: "test attach with quiet", + rm: false, + quiet: true, + expectedDeleteCount: 0, + expectedOut: "", + expectedErrOut: "", + }, + { + name: "test attach with rm", + rm: true, + quiet: false, + expectedDeleteCount: 1, + expectedOut: "pod \"foo\" deleted\n", + expectedErrOut: "If you don't see a command prompt, try pressing enter.\n", + }, + { + name: "test attach with rm should not print message if quiet is specified", + rm: true, + quiet: true, + expectedDeleteCount: 1, + expectedOut: "", + expectedErrOut: "", + }, + { + name: "error should be displayed if delete fails", + rm: true, + quiet: false, + deleteErrorMessage: "delete error message", + expectedDeleteCount: 1, + expectedOut: "", + expectedErrOut: "If you don't see a command prompt, try pressing enter.\nDelete failed: delete error message\n", + }, + } + + fakePod := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "bar", + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + postCount := 0 + attachCount := 0 + deleteCount := 0 + + attachFunc = func(o *attach.AttachOptions, containerToAttach *corev1.Container, raw bool, sizeQueue remotecommand.TerminalSizeQueue) func() error { + if containerToAttach.Name != "bar" { + tt.Fatalf("expected attach to container name \"bar\", but got %q", containerToAttach.Name) + } + return func() error { + attachCount++ + return nil + } + } + + tf := cmdtesting.NewTestFactory().WithNamespace("test") + defer tf.Cleanup() + + codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) + ns := scheme.Codecs.WithoutConversion() + tf.Client = &fake.RESTClient{ + GroupVersion: schema.GroupVersion{Version: ""}, + NegotiatedSerializer: ns, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p, m := req.URL.Path, req.Method; { + case m == "POST" && p == "/namespaces/test/pods": + postCount++ + body := cmdtesting.ObjBody(codec, fakePod) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil + case m == "GET" && p == "/api/v1/namespaces/default/pods": + event := &metav1.WatchEvent{ + Type: "ADDED", + Object: runtime.RawExtension{Object: fakePod}, + } + body := cmdtesting.ObjBody(codec, event) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil + case m == "GET" && p == "/namespaces/default/pods/foo": + body := cmdtesting.ObjBody(codec, fakePod) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil + case m == "DELETE" && p == "/namespaces/default/pods/foo": + deleteCount++ + if test.deleteErrorMessage != "" { + body := cmdtesting.ObjBody(codec, &metav1.Status{ + Status: metav1.StatusFailure, + Message: test.deleteErrorMessage, + }) + return &http.Response{StatusCode: http.StatusInternalServerError, Header: cmdtesting.DefaultHeader(), Body: body}, nil + } else { + body := cmdtesting.ObjBody(codec, fakePod) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: body}, nil + } + default: + tt.Errorf("unexpected request: %s %#v\n%#v", req.Method, req.URL, req) + return nil, fmt.Errorf("unexpected request") + } + }), + } + + tf.ClientConfigVal = &restclient.Config{ + ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Version: ""}, NegotiatedSerializer: ns}, + } + + streams, _, bufOut, bufErr := genericclioptions.NewTestIOStreams() + cmdutil.BehaviorOnFatal(func(str string, code int) { + bufErr.Write([]byte(str)) + }) + + cmd := NewCmdRun(tf, streams) + cmd.Flags().Set("image", "test-image") + cmd.Flags().Set("attach", "true") + if test.rm { + cmd.Flags().Set("rm", "true") + } + if test.quiet { + cmd.Flags().Set("quiet", "true") + } + + parentCmd := cobra.Command{} + parentCmd.AddCommand(cmd) + + cmd.Run(cmd, []string{"test-pod"}) + + if postCount != 1 { + tt.Fatalf("expected 1 post request, but got %d", postCount) + } + + if attachCount != 1 { + tt.Fatalf("expected 1 attach call, but got %d", attachCount) + } + + if deleteCount != test.expectedDeleteCount { + tt.Fatalf("expected %d delete requests, but got %d", test.expectedDeleteCount, deleteCount) + } + + if bufErr.String() != test.expectedErrOut { + tt.Fatalf("unexpected error. got: %q, expected: %q", bufErr.String(), test.expectedErrOut) + } + + if bufOut.String() != test.expectedOut { + tt.Fatalf("unexpected output. got: %q, expected: %q", bufOut.String(), test.expectedOut) + } + }) + + } +} + func TestRunOverride(t *testing.T) { tests := []struct { name string