Skip to content

fix: avoid verify empty container cmd/args #9869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
reingart wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: avoid verify empty container cmd/args #9869

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions pkg/skaffold/verify/k8sjob/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
kubernetesclient "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/client"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/loader"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/log"
olog "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/status"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util"
Expand Down Expand Up @@ -213,6 +214,7 @@ func (v *Verifier) createAndRunJob(ctx context.Context, tc latest.VerifyTestCase
// This is because the k8s API server can be unresponsive when hit with a large
// intitial set of Job CREATE requests
if waitErr := wait.Poll(100*time.Millisecond, 30*time.Second, func() (bool, error) {
olog.Entry(context.TODO()).Debugf("Creating verify job in cluster: %+v\n", job)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It's great that you're adding more logging. However, using context.TODO() should be avoided when a more specific context is available. In this function, ctx is passed as an argument and should be used here instead. This ensures that trace information and other context-scoped values are propagated correctly.

This same feedback applies to the other new logging calls in this file where ctx is available (e.g., in watchJob and Cleanup). For the logs in createJobFromManifestPath, which doesn't currently accept a context, consider refactoring that function to accept a context from its caller.

Suggested change
olog.Entry(context.TODO()).Debugf("Creating verify job in cluster: %+v\n", job)
olog.Entry(ctx).Debugf("Creating verify job in cluster: %+v\n", job)

_, err = clientset.BatchV1().Jobs(job.Namespace).Create(ctx, job, metav1.CreateOptions{})
if err != nil {
return false, nil
Expand Down Expand Up @@ -275,10 +277,12 @@ func (v *Verifier) watchJob(ctx context.Context, clientset k8sclient.Interface,
pod, ok := event.Object.(*corev1.Pod)
if ok {
if pod.Status.Phase == corev1.PodSucceeded {
olog.Entry(context.TODO()).Debugf("Verify pod succeeded: %+v\n", pod)
// TODO(aaron-prindle) add support for jobs w/ multiple pods in the future
break
}
if pod.Status.Phase == corev1.PodFailed {
olog.Entry(context.TODO()).Debugf("Verify pod failed: %+v\n", pod)
failReason := pod.Status.Reason
if failReason == "" {
failReason = "<empty>"
Expand Down Expand Up @@ -326,6 +330,7 @@ func (v *Verifier) Cleanup(ctx context.Context, out io.Writer, dryRun bool) erro
// assumes the job namespace is set and not "" which is the case as createJob
// & createJobFromManifestPath set the namespace in the created Job
namespace := job.Namespace
olog.Entry(context.TODO()).Debugf("Cleaning up job %q in namespace %q", job.Name, namespace)
if err := k8sjobutil.ForceJobDelete(ctx, job.Name, clientset.BatchV1().Jobs(namespace), &v.kubectl); err != nil {
// TODO(aaron-prindle): replace with actionable error
return errors.Wrap(err, "cleaning up deployed job")
Expand Down Expand Up @@ -389,13 +394,17 @@ func (v *Verifier) createJobFromManifestPath(jobName string, container latest.Ve
job.Name = jobName
job.Labels["skaffold.dev/run-id"] = v.labeller.GetRunID()
var original corev1.Container
olog.Entry(context.TODO()).Tracef("Lookging for container %s in %+v\n", container.Name, job.Spec.Template.Spec.Containers)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a typo here: 'Lookging' should be 'Looking'.

Suggested change
olog.Entry(context.TODO()).Tracef("Lookging for container %s in %+v\n", container.Name, job.Spec.Template.Spec.Containers)
olog.Entry(context.TODO()).Tracef("Looking for container %s in %+v\n", container.Name, job.Spec.Template.Spec.Containers)

for _, c := range job.Spec.Template.Spec.Containers {
if c.Name == container.Name {
original = c
olog.Entry(context.TODO()).Tracef("Found container %+v\n", c)
break
}
}
olog.Entry(context.TODO()).Tracef("Original containers from manifest: %+v\n", original)
patchToK8sContainer(container, &original)
olog.Entry(context.TODO()).Tracef("Patched containers: %+v\n", original)

job.Spec.Template.Spec.Containers = []corev1.Container{original}
job.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyNever
Expand All @@ -411,8 +420,12 @@ func (v *Verifier) createJobFromManifestPath(jobName string, container latest.Ve

func patchToK8sContainer(container latest.VerifyContainer, dst *corev1.Container) {
dst.Image = container.Image
dst.Command = container.Command
dst.Args = container.Args
if container.Command != nil {
dst.Command = container.Command
}
if container.Args != nil {
dst.Args = container.Args
}
Comment on lines +423 to +428
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This fix correctly prevents overwriting the command/args when they are not specified in the skaffold.yaml. However, the PR description mentions that the 'bogus' behavior can be reproduced with args: []. With the current check (container.Args != nil), an empty slice [] is not nil, so it would still overwrite the original arguments from the manifest with an empty list. To fully address the case described, it would be better to check if the slice has any elements before overwriting. This applies to both Command and Args.

Suggested change
if container.Command != nil {
dst.Command = container.Command
}
if container.Args != nil {
dst.Args = container.Args
}
if len(container.Command) > 0 {
dst.Command = container.Command
}
if len(container.Args) > 0 {
dst.Args = container.Args
}

dst.Name = container.Name

for _, e := range container.Env {
Expand Down
Loading