From 25a48dc47fcf31c3e89e9f0f46d87e1a8c963dec Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Fri, 6 Dec 2024 18:16:04 +0200 Subject: [PATCH 1/3] fix(sync): add validation for namespaces and handle empty pods --- pkg/skaffold/sync/sync.go | 10 ++++ pkg/skaffold/sync/sync_test.go | 92 ++++++++++++++++++---------------- 2 files changed, 59 insertions(+), 43 deletions(-) diff --git a/pkg/skaffold/sync/sync.go b/pkg/skaffold/sync/sync.go index 528133589ea..a05b6f476c1 100644 --- a/pkg/skaffold/sync/sync.go +++ b/pkg/skaffold/sync/sync.go @@ -328,6 +328,10 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex return nil } + if len(namespaces) == 0 { + return fmt.Errorf("no namespaces provided for syncing") + } + errs, ctx := errgroup.WithContext(ctx) client, err := kubernetesclient.Client(kubeContext) @@ -340,10 +344,16 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex pods, err := client.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{ FieldSelector: fmt.Sprintf("status.phase=%s", v1.PodRunning), }) + if err != nil { return fmt.Errorf("getting pods for namespace %q: %w", ns, err) } + if len(pods.Items) == 0 { + log.Entry(ctx).Warnf("no running pods found in namespace %q", ns) + continue + } + for _, p := range pods.Items { for _, c := range p.Spec.Containers { if c.Image != image { diff --git a/pkg/skaffold/sync/sync_test.go b/pkg/skaffold/sync/sync_test.go index 6ed0baf1bb8..99716ce7e8f 100644 --- a/pkg/skaffold/sync/sync_test.go +++ b/pkg/skaffold/sync/sync_test.go @@ -903,7 +903,7 @@ func fakeCmd(ctx context.Context, _ v1.Pod, _ v1.Container, files syncMap) *exec return exec.CommandContext(ctx, "copy", args...) } -var pod = &v1.Pod{ +var runningPod = &v1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: "podname", }, @@ -921,58 +921,64 @@ var pod = &v1.Pod{ } func TestPerform(t *testing.T) { - tests := []struct { - description string - image string - files syncMap - pod *v1.Pod - cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd - cmdErr error - clientErr error - expected []string - shouldErr bool + tests := map[string]struct { + image string + files syncMap + pod *v1.Pod + cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd + cmdErr error + clientErr error + expected []string + shouldErr bool }{ - { - description: "no error", - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: pod, - cmdFn: fakeCmd, - expected: []string{"copy test.go /test.go"}, + "no error": { + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + expected: []string{"copy test.go /test.go"}, }, - { - description: "cmd error", - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: pod, - cmdFn: fakeCmd, - cmdErr: fmt.Errorf(""), - shouldErr: true, + "cmd error": { + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + cmdErr: fmt.Errorf(""), + shouldErr: true, }, - { - description: "client error", - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: pod, - cmdFn: fakeCmd, - clientErr: fmt.Errorf(""), - shouldErr: true, + "client error": { + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + clientErr: fmt.Errorf(""), + shouldErr: true, }, - { - description: "no copy", - image: "gcr.io/different-pod:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: pod, - cmdFn: fakeCmd, - shouldErr: true, + "pod not running": { + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: nil, + cmdFn: fakeCmd, + shouldErr: true, + }, + "no copy": { + image: "gcr.io/different-pod:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + shouldErr: true, }, } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { + for name, test := range tests { + testutil.Run(t, name, func(t *testutil.T) { cmdRecord := &TestCmdRecorder{err: test.cmdErr} t.Override(&util.DefaultExecCommand, cmdRecord) t.Override(&client.Client, func(string) (kubernetes.Interface, error) { + if test.pod == nil { + return fake.NewSimpleClientset(), nil + } + return fake.NewSimpleClientset(test.pod), test.clientErr }) From ff1fd910c5bbc6072fb84309b92f23ab94c855c7 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Sat, 7 Dec 2024 07:07:17 +0200 Subject: [PATCH 2/3] fixes --- pkg/skaffold/sync/sync_test.go | 102 +++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 43 deletions(-) diff --git a/pkg/skaffold/sync/sync_test.go b/pkg/skaffold/sync/sync_test.go index 99716ce7e8f..bc464b03d22 100644 --- a/pkg/skaffold/sync/sync_test.go +++ b/pkg/skaffold/sync/sync_test.go @@ -921,56 +921,62 @@ var runningPod = &v1.Pod{ } func TestPerform(t *testing.T) { - tests := map[string]struct { - image string - files syncMap - pod *v1.Pod - cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd - cmdErr error - clientErr error - expected []string - shouldErr bool + tests := []struct { + description string + image string + files syncMap + pod *v1.Pod + cmdFn func(context.Context, v1.Pod, v1.Container, syncMap) *exec.Cmd + cmdErr error + clientErr error + expected []string + shouldErr bool }{ - "no error": { - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: runningPod, - cmdFn: fakeCmd, - expected: []string{"copy test.go /test.go"}, + { + description: "no error", + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + expected: []string{"copy test.go /test.go"}, }, - "cmd error": { - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: runningPod, - cmdFn: fakeCmd, - cmdErr: fmt.Errorf(""), - shouldErr: true, + { + description: "cmd error", + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + cmdErr: fmt.Errorf(""), + shouldErr: true, }, - "client error": { - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: runningPod, - cmdFn: fakeCmd, - clientErr: fmt.Errorf(""), - shouldErr: true, + { + description: "client error", + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + clientErr: fmt.Errorf(""), + shouldErr: true, }, - "pod not running": { - image: "gcr.io/k8s-skaffold:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: nil, - cmdFn: fakeCmd, - shouldErr: true, + { + description: "pod not running", + image: "gcr.io/k8s-skaffold:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: nil, + cmdFn: fakeCmd, + shouldErr: true, }, - "no copy": { - image: "gcr.io/different-pod:123", - files: syncMap{"test.go": {"/test.go"}}, - pod: runningPod, - cmdFn: fakeCmd, - shouldErr: true, + { + description: "no copy", + image: "gcr.io/different-pod:123", + files: syncMap{"test.go": {"/test.go"}}, + pod: runningPod, + cmdFn: fakeCmd, + shouldErr: true, }, } - for name, test := range tests { - testutil.Run(t, name, func(t *testutil.T) { + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { cmdRecord := &TestCmdRecorder{err: test.cmdErr} t.Override(&util.DefaultExecCommand, cmdRecord) @@ -989,6 +995,16 @@ func TestPerform(t *testing.T) { } } +func TestPerform_WithoutNamespaces(t *testing.T) { + err := Perform(context.Background(), "", syncMap{"test.go": {"/test.go"}}, nil, []string{}, "") + + if err != nil { + testutil.CheckErrorAndDeepEqual(t, true, err, "no namespaces provided for syncing", err.Error()) + } else { + testutil.CheckError(t, true, err) + } +} + func TestSyncMap(t *testing.T) { tests := []struct { description string From c848fb1393e6109fab2667e62d1325f2154bb7b1 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Wed, 11 Dec 2024 06:44:38 +0200 Subject: [PATCH 3/3] fixes --- pkg/skaffold/sync/sync.go | 4 ---- pkg/skaffold/sync/sync_test.go | 10 ---------- 2 files changed, 14 deletions(-) diff --git a/pkg/skaffold/sync/sync.go b/pkg/skaffold/sync/sync.go index a05b6f476c1..cdf2f6aa2bd 100644 --- a/pkg/skaffold/sync/sync.go +++ b/pkg/skaffold/sync/sync.go @@ -328,10 +328,6 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex return nil } - if len(namespaces) == 0 { - return fmt.Errorf("no namespaces provided for syncing") - } - errs, ctx := errgroup.WithContext(ctx) client, err := kubernetesclient.Client(kubeContext) diff --git a/pkg/skaffold/sync/sync_test.go b/pkg/skaffold/sync/sync_test.go index bc464b03d22..8a0e4facc9c 100644 --- a/pkg/skaffold/sync/sync_test.go +++ b/pkg/skaffold/sync/sync_test.go @@ -995,16 +995,6 @@ func TestPerform(t *testing.T) { } } -func TestPerform_WithoutNamespaces(t *testing.T) { - err := Perform(context.Background(), "", syncMap{"test.go": {"/test.go"}}, nil, []string{}, "") - - if err != nil { - testutil.CheckErrorAndDeepEqual(t, true, err, "no namespaces provided for syncing", err.Error()) - } else { - testutil.CheckError(t, true, err) - } -} - func TestSyncMap(t *testing.T) { tests := []struct { description string