diff --git a/integration/deploy_cloudrun_test.go b/integration/deploy_cloudrun_test.go index 184134b14db..78f6ebf98e0 100644 --- a/integration/deploy_cloudrun_test.go +++ b/integration/deploy_cloudrun_test.go @@ -22,10 +22,14 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/uuid" + "google.golang.org/api/option" "google.golang.org/api/run/v1" "github.com/GoogleContainerTools/skaffold/v2/integration/skaffold" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/gcp" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/testutil" ) @@ -69,6 +73,142 @@ func TestDeployCloudRunWithHooks(t *testing.T) { }) } +func TestDeployJobWithMaxRetries(t *testing.T) { + MarkIntegrationTest(t, NeedsGcp) + + tests := []struct { + descrition string + jobManifest string + skaffoldCfg string + args []string + expectedMaxRetries int64 + }{ + { + descrition: "maxRetries set to specific value", + expectedMaxRetries: 2, + jobManifest: ` +apiVersion: run.googleapis.com/v1 +kind: Job +metadata: + annotations: + run.googleapis.com/launch-stage: BETA + name: %v +spec: + template: + spec: + template: + spec: + containers: + - image: docker.io/library/busybox:latest + name: job + maxRetries: 2`, + skaffoldCfg: ` +apiVersion: %v +kind: Config +metadata: + name: cloud-run-test +manifests: + rawYaml: + - job.yaml +deploy: + cloudrun: + projectid: %v + region: %v`, + }, + { + descrition: "maxRetries set to 0", + expectedMaxRetries: 0, + jobManifest: ` +apiVersion: run.googleapis.com/v1 +kind: Job +metadata: + annotations: + run.googleapis.com/launch-stage: BETA + name: %v +spec: + template: + spec: + template: + spec: + containers: + - image: docker.io/library/busybox:latest + name: job + maxRetries: 0`, + skaffoldCfg: ` +apiVersion: %v +kind: Config +metadata: + name: cloud-run-test +manifests: + rawYaml: + - job.yaml +deploy: + cloudrun: + projectid: %v + region: %v`, + }, + { + descrition: "maxRetries not specified - default 3", + expectedMaxRetries: 3, + jobManifest: ` +apiVersion: run.googleapis.com/v1 +kind: Job +metadata: + annotations: + run.googleapis.com/launch-stage: BETA + name: %v +spec: + template: + spec: + template: + spec: + containers: + - image: docker.io/library/busybox:latest + name: job`, + skaffoldCfg: ` +apiVersion: %v +kind: Config +metadata: + name: cloud-run-test +manifests: + rawYaml: + - job.yaml +deploy: + cloudrun: + projectid: %v + region: %v`, + }, + } + + for _, test := range tests { + testutil.Run(t, test.descrition, func(t *testutil.T) { + projectID := "k8s-skaffold" + region := "us-central1" + jobName := fmt.Sprintf("job-%v", uuid.New().String()) + skaffoldCfg := fmt.Sprintf(test.skaffoldCfg, latest.Version, projectID, region) + jobManifest := fmt.Sprintf(test.jobManifest, jobName) + + tmpDir := t.NewTempDir() + tmpDir.Write("skaffold.yaml", skaffoldCfg) + tmpDir.Write("job.yaml", jobManifest) + + skaffold.Run().InDir(tmpDir.Root()).RunOrFail(t.T) + t.Cleanup(func() { + skaffold.Delete(test.args...).InDir(tmpDir.Root()).RunOrFail(t.T) + }) + + job, err := getJob(context.Background(), projectID, region, jobName) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(job.Spec.Template.Spec.Template.Spec.MaxRetries, test.expectedMaxRetries); diff != "" { + t.Fatalf("Job MaxRetries differ (-got,+want):\n%s", diff) + } + }) + } +} + // TODO: remove nolint when test is unskipped // //nolint:unused @@ -82,6 +222,18 @@ func getRunService(ctx context.Context, project, region, service string) (*run.S return call.Do() } +func getJob(ctx context.Context, project, region, job string) (*run.Job, error) { + cOptions := []option.ClientOption{option.WithEndpoint(fmt.Sprintf("%s-run.googleapis.com", region))} + cOptions = append(gcp.ClientOptions(ctx), cOptions...) + crclient, err := run.NewService(ctx, cOptions...) + if err != nil { + return nil, err + } + jName := fmt.Sprintf("namespaces/%v/jobs/%v", project, job) + call := crclient.Namespaces.Jobs.Get(jName) + return call.Do() +} + // TODO: remove nolint when test is unskipped // //nolint:unused diff --git a/pkg/skaffold/deploy/cloudrun/deploy.go b/pkg/skaffold/deploy/cloudrun/deploy.go index d8dd0f170f7..9470b0ce3c0 100644 --- a/pkg/skaffold/deploy/cloudrun/deploy.go +++ b/pkg/skaffold/deploy/cloudrun/deploy.go @@ -40,6 +40,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/manifest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" + logger "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/sync" @@ -294,6 +295,37 @@ func (d *Deployer) deployService(crclient *run.APIService, manifest []byte, out return &resName, nil } +func (d *Deployer) forceSendValueOfMaxRetries(job *run.Job, manifest []byte) { + maxRetriesPath := []string{"spec", "template", "spec", "template", "spec"} + node := make(map[string]interface{}) + + if err := k8syaml.Unmarshal(manifest, &node); err != nil { + logger.Entry(context.TODO()).Debugf("Error unmarshaling job into map, skipping maxRetries ForceSendFields logic: %v", err) + return + } + + for _, field := range maxRetriesPath { + value := node[field] + child, ok := value.(map[string]interface{}) + if !ok { + logger.Entry(context.TODO()).Debugf("Job maxRetries parent fields not found") + return + } + node = child + } + + if _, exists := node["maxRetries"]; !exists { + logger.Entry(context.TODO()).Debugf("Job maxRetries property not found") + return + } + + if job.Spec == nil || job.Spec.Template == nil || job.Spec.Template.Spec == nil || job.Spec.Template.Spec.Template == nil || job.Spec.Template.Spec.Template.Spec == nil { + logger.Entry(context.TODO()).Debugf("Job struct doesn't have the required values to force maxRetries sending") + return + } + job.Spec.Template.Spec.Template.Spec.ForceSendFields = append(job.Spec.Template.Spec.Template.Spec.ForceSendFields, "MaxRetries") +} + func (d *Deployer) deployJob(crclient *run.APIService, manifest []byte, out io.Writer) (*RunResourceName, error) { job := &run.Job{} if err := k8syaml.Unmarshal(manifest, job); err != nil { @@ -302,6 +334,9 @@ func (d *Deployer) deployJob(crclient *run.APIService, manifest []byte, out io.W ErrCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR, }) } + + d.forceSendValueOfMaxRetries(job, manifest) + if d.Project != "" { job.Metadata.Namespace = d.Project } else if job.Metadata.Namespace == "" { diff --git a/pkg/skaffold/deploy/cloudrun/deploy_test.go b/pkg/skaffold/deploy/cloudrun/deploy_test.go index 762da933ba1..9906b9b2366 100644 --- a/pkg/skaffold/deploy/cloudrun/deploy_test.go +++ b/pkg/skaffold/deploy/cloudrun/deploy_test.go @@ -29,6 +29,7 @@ import ( "google.golang.org/api/option" "google.golang.org/api/run/v1" "google.golang.org/protobuf/testing/protocmp" + k8syaml "sigs.k8s.io/yaml" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/label" sErrors "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/errors" @@ -36,6 +37,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/manifest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/v2/proto/v1" "github.com/GoogleContainerTools/skaffold/v2/testutil" ) @@ -161,13 +163,14 @@ func TestDeployService(tOuter *testing.T) { func TestDeployJob(tOuter *testing.T) { tests := []struct { - description string - toDeploy *run.Job - defaultProject string - region string - expectedPath string - httpErr int - errCode proto.StatusCode + description string + toDeploy *run.Job + defaultProject string + region string + expectedPath string + httpErr int + errCode proto.StatusCode + expectedMaxRetries *float64 }{ { description: "test deploy", @@ -223,9 +226,62 @@ func TestDeployJob(tOuter *testing.T) { }, errCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR, }, + { + description: "test deploy with maxRetries field set to 0", + defaultProject: "testProject", + region: "us-central1", + expectedPath: "/apis/run.googleapis.com/v1/namespaces/testProject/jobs", + expectedMaxRetries: util.Ptr[float64](0), + toDeploy: &run.Job{ + ApiVersion: "run.googleapis.com/v1", + Kind: "Job", + Metadata: &run.ObjectMeta{ + Name: "test-service", + }, + Spec: &run.JobSpec{ + Template: &run.ExecutionTemplateSpec{ + Spec: &run.ExecutionSpec{ + Template: &run.TaskTemplateSpec{ + Spec: &run.TaskSpec{ + MaxRetries: 0, + ForceSendFields: []string{"MaxRetries"}, + }, + }, + }, + }, + }, + }, + }, + { + description: "test deploy with maxRetries field set to 5", + defaultProject: "testProject", + region: "us-central1", + expectedPath: "/apis/run.googleapis.com/v1/namespaces/testProject/jobs", + expectedMaxRetries: util.Ptr[float64](5), + toDeploy: &run.Job{ + ApiVersion: "run.googleapis.com/v1", + Kind: "Job", + Metadata: &run.ObjectMeta{ + Name: "test-service", + }, + Spec: &run.JobSpec{ + Template: &run.ExecutionTemplateSpec{ + Spec: &run.ExecutionSpec{ + Template: &run.TaskTemplateSpec{ + Spec: &run.TaskSpec{ + MaxRetries: 5, + ForceSendFields: []string{"MaxRetries"}, + }, + }, + }, + }, + }, + }, + }, } for _, test := range tests { testutil.Run(tOuter, test.description, func(t *testutil.T) { + var jobReceivedInServer []byte ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if test.httpErr != 0 { http.Error(w, "test expecting error", test.httpErr) @@ -233,29 +289,32 @@ func TestDeployJob(tOuter *testing.T) { } if r.URL.Path != test.expectedPath { http.Error(w, "unexpected path: "+r.URL.Path, http.StatusNotFound) + return } - var service run.Service + var job run.Job body, err := io.ReadAll(r.Body) if err != nil { http.Error(w, "Unable to read body: "+err.Error(), http.StatusInternalServerError) return } - if err = json.Unmarshal(body, &service); err != nil { + if err = json.Unmarshal(body, &job); err != nil { http.Error(w, "Unable to parse service: "+err.Error(), http.StatusBadRequest) return } - b, err := json.Marshal(service) + b, err := json.Marshal(job) if err != nil { http.Error(w, "unable to marshal response: "+err.Error(), http.StatusInternalServerError) return } + + jobReceivedInServer = body w.Write(b) })) deployer, _ := NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{ProjectID: test.defaultProject, Region: test.region}, configName) deployer.clientOptions = append(deployer.clientOptions, option.WithEndpoint(ts.URL), option.WithoutAuthentication()) deployer.useGcpOptions = false - manifestList, _ := json.Marshal(test.toDeploy) + manifestList, _ := k8syaml.Marshal(test.toDeploy) manifestsByConfig := manifest.NewManifestListByConfig() manifestsByConfig.Add(configName, manifest.ManifestList{manifestList}) err := deployer.Deploy(context.Background(), os.Stderr, []graph.Artifact{}, manifestsByConfig) @@ -270,10 +329,42 @@ func TestDeployJob(tOuter *testing.T) { t.Fatalf("Expected status code %v but got %v", test.errCode, sErr.StatusCode()) } } + + if test.errCode == proto.StatusCode_OK { + checkMaxRetriesValue(t, jobReceivedInServer, test.expectedMaxRetries) + } }) } } +func checkMaxRetriesValue(t *testutil.T, serverJob []byte, expectedMaxRetries *float64) { + maxRetriesPath := []string{"spec", "template", "spec", "template", "spec"} + var foundMaxRetries *float64 + fields := make(map[string]interface{}) + + if err := json.Unmarshal(serverJob, &fields); err != nil { + t.Fatalf("Error unmarshaling job from server: %v", err) + } + + for _, field := range maxRetriesPath { + value := fields[field] + child, ok := value.(map[string]interface{}) + if !ok { + fields = nil + break + } + fields = child + } + + mxRetryVal := fields["maxRetries"] + if val, ok := mxRetryVal.(float64); ok { + foundMaxRetries = util.Ptr(val) + } + if diff := cmp.Diff(expectedMaxRetries, foundMaxRetries); diff != "" { + t.Fatalf("MaxRetries don't match (+got-want):\n%v", diff) + } +} + func TestDeployRewrites(tOuter *testing.T) { tests := []struct { description string