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 diff --git a/pkg/skaffold/docker/auth.go b/pkg/skaffold/docker/auth.go index 70c41b9e157..8f319464235 100644 --- a/pkg/skaffold/docker/auth.go +++ b/pkg/skaffold/docker/auth.go @@ -18,18 +18,20 @@ package docker import ( "context" - "encoding/base64" "encoding/json" "fmt" "os" "path/filepath" + "github.com/distribution/reference" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" - "github.com/docker/distribution/reference" + clitypes "github.com/docker/cli/cli/config/types" types "github.com/docker/docker/api/types/registry" "github.com/docker/docker/pkg/homedir" "github.com/docker/docker/registry" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/v1/google" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/gcp" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" @@ -79,14 +81,72 @@ func (h credsHelper) GetAuthConfig(registry string) (types.AuthConfig, error) { return types.AuthConfig{}, err } + return h.loadCredentials(cf, registry) +} + +func (h credsHelper) loadCredentials(cf *configfile.ConfigFile, registry string) (types.AuthConfig, error) { + if helper := cf.CredentialHelpers[registry]; helper == "gcloud" { + authCfg, err := h.getGoogleAuthConfig(registry) + if err == nil { + return authCfg, nil + } + log.Entry(context.TODO()).Debugf("error getting google authenticator, falling back to docker auth: %v", err) + } + + var anonymous clitypes.AuthConfig auth, err := cf.GetAuthConfig(registry) if err != nil { return types.AuthConfig{}, err } + // From go-containerrergistry logic, the ServerAddress is not considered when determining if returned auth is anonymous. + anonymous.ServerAddress = auth.ServerAddress + if auth != anonymous { + return types.AuthConfig(auth), nil + } + + if isGoogleRegistry(registry) { + authCfg, err := h.getGoogleAuthConfig(registry) + if err == nil { + return authCfg, nil + } + } + return types.AuthConfig(auth), nil } +func (h credsHelper) getGoogleAuthConfig(registry string) (types.AuthConfig, error) { + auth, err := google.NewEnvAuthenticator() + if err != nil { + return types.AuthConfig{}, err + } + + if auth == authn.Anonymous { + return types.AuthConfig{}, fmt.Errorf("error getting google authenticator") + } + + cfg, err := auth.Authorization() + if err != nil { + return types.AuthConfig{}, err + } + + bCfg, err := cfg.MarshalJSON() + if err != nil { + return types.AuthConfig{}, err + } + + var authCfg types.AuthConfig + err = json.Unmarshal(bCfg, &authCfg) + if err != nil { + return types.AuthConfig{}, err + } + + // The docker library does the same when we request the credentials + authCfg.ServerAddress = registry + + return authCfg, nil +} + // GetAllAuthConfigs retrieves all the auth configs. // Because this can take a long time, we make sure it can be interrupted by the user. func (h credsHelper) GetAllAuthConfigs(ctx context.Context) (map[string]types.AuthConfig, error) { @@ -111,22 +171,31 @@ func (h credsHelper) GetAllAuthConfigs(ctx context.Context) (map[string]types.Au } func (h credsHelper) doGetAllAuthConfigs() (map[string]types.AuthConfig, error) { + credentials := make(map[string]types.AuthConfig) cf, err := loadDockerConfig() if err != nil { return nil, err } - credentials, err := cf.GetAllCredentials() + defaultCreds, err := cf.GetCredentialsStore("").GetAll() if err != nil { return nil, err } - authConfigs := make(map[string]types.AuthConfig, len(credentials)) - for k, auth := range credentials { - authConfigs[k] = types.AuthConfig(auth) + for registry, cred := range defaultCreds { + credentials[registry] = types.AuthConfig(cred) + } + + for registry := range cf.CredentialHelpers { + authCfg, err := h.loadCredentials(cf, registry) + if err != nil { + log.Entry(context.TODO()).Debugf("failed to get credentials for registry %v: %v", registry, err) + continue + } + credentials[registry] = authCfg } - return authConfigs, nil + return credentials, nil } func (l *localDaemon) encodedRegistryAuth(ctx context.Context, a AuthConfigHelper, image string) (string, error) { @@ -150,12 +219,7 @@ func (l *localDaemon) encodedRegistryAuth(ctx context.Context, a AuthConfigHelpe return "", fmt.Errorf("getting auth config: %w", err) } - buf, err := json.Marshal(ac) - if err != nil { - return "", err - } - - return base64.URLEncoding.EncodeToString(buf), nil + return types.EncodeAuthConfig(ac) } func (l *localDaemon) officialRegistry(ctx context.Context) string { diff --git a/pkg/skaffold/docker/auth_test.go b/pkg/skaffold/docker/auth_test.go index 32a0103ab36..99ed7e21e4a 100644 --- a/pkg/skaffold/docker/auth_test.go +++ b/pkg/skaffold/docker/auth_test.go @@ -58,14 +58,18 @@ func TestGetAllAuthConfigs(t *testing.T) { tmpDir := t.NewTempDir(). Write("config.json", `{"credHelpers":{"my.registry":"helper"}}`). Write("docker-credential-gcloud", `#!/bin/sh -read server -echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`). + read server + echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`). Write("docker-credential-helper", `#!/bin/sh -read server -echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`) + read server + echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`) t.Override(&configDir, tmpDir.Root()) t.Setenv("PATH", tmpDir.Root()) + // These env values will prevent the authenticator to use the Google authenticator, it will use docker logic instead. + t.Setenv("HOME", tmpDir.Root()) + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") + auth, err := DefaultAuthHelper.GetAllAuthConfigs(context.Background()) t.CheckNoError(err) @@ -89,6 +93,46 @@ echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`) t.CheckError(true, err) t.CheckEmpty(auth) }) + + testutil.Run(t, "Application Default Credentials authentication", func(t *testutil.T) { + if runtime.GOOS == "windows" { + t.Skip("test doesn't work on windows") + } + + authToken := `{"access_token":"TOKEN","expires_in": 3599}` + authServerURL := startTokenServer(t, authToken) + credentialsFile := getCredentialsFile(t, map[string]string{ + "client_id": "123456.apps.googleusercontent.com", + "client_secret": "THE-SECRET", + "refresh_token": "REFRESH-TOKEN", + "type": "authorized_user", + }, authServerURL) + + tmpDir := t.NewTempDir().Write("credentials.json", credentialsFile) + tmpDir.Write("config.json", `{"credHelpers":{"my.registry1":"helper","my.registry2":"missinghelper","gcr.io":"gcloud","us-central1-docker.pkg.dev":"otherhelper","us-east1-docker.pkg.dev":"gcloud"}}`) + tmpDir.Write("docker-credential-helper", `#!/bin/sh + read server + echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`) + tmpDir.Write("docker-credential-otherhelper", `#!/bin/sh + read server + echo "{\"Username\":\"\",\"Secret\":\"TOKEN_$server\"}"`) + + t.Override(&configDir, tmpDir.Root()) + t.SetEnvs(map[string]string{ + "PATH": tmpDir.Root(), + "HOME": tmpDir.Root(), // This is to prevent the go-containerregistry library from using ADCs that are already present on the computer. + "GOOGLE_APPLICATION_CREDENTIALS": tmpDir.Path("credentials.json"), + }) + + auth, err := DefaultAuthHelper.GetAllAuthConfigs(context.Background()) + t.CheckNoError(err) + t.CheckDeepEqual(map[string]registry.AuthConfig{ + "gcr.io": {Username: "_token", Password: "TOKEN", Auth: "X3Rva2VuOlRPS0VO", ServerAddress: "gcr.io"}, + "us-east1-docker.pkg.dev": {Username: "_token", Password: "TOKEN", Auth: "X3Rva2VuOlRPS0VO", ServerAddress: "us-east1-docker.pkg.dev"}, + "us-central1-docker.pkg.dev": {IdentityToken: "TOKEN_us-central1-docker.pkg.dev", ServerAddress: "us-central1-docker.pkg.dev"}, + "my.registry1": {IdentityToken: "TOKEN_my.registry1", ServerAddress: "my.registry1"}, + }, auth) + }) } func TestGetEncodedRegistryAuth(t *testing.T) {