Repository: GoogleContainerTools/skaffold
Pull request #7579 (2022-06-23 18:45:40)

fix: switch cloud run deployer to prefer project in Skaffold manifest over Cloud Run manifest

**Related**: #3217

**Description**
The initial implementation of the Cloud Run Deployer would use the project specified in the Skaffold manifest only if a project wasn't specified in the Cloud Run manifest.  This inverts the precedence: a project specified in a Skaffold manifest will override the project specified in the Cloud Run manifest.

This way, if you build your Skaffold config  using an existing Cloud Run manifest, you can change the projects for different profiles entirely in the Skaffold config, without having to use Kustomize etc. to modify the Cloud Run manifest.

**User facing changes (remove if N/A)**
For Cloud Run deploys, prefer the Google Cloud project id in the Skaffold manifest over the project id in the Cloud Run manifest.
*Before*:
Given a Cloud Run manifest with
metadata:
 &nbsp; &nbsp;namespace: project1

and a Skaffold manifest with
deploy:
 &nbsp; &nbsp;cloudrun:
  &nbsp; &nbsp; &nbsp; &nbsp;defaultprojectid: project2

The service would be deployed to project1.

*After*:
Given a Cloud Run manifest with
metadata:
 &nbsp; &nbsp;namespace: project1

and a Skaffold manifest with
deploy:
 &nbsp; &nbsp;cloudrun:
  &nbsp; &nbsp; &nbsp; &nbsp;projectid: project2 

The service will be deployed to project2


Patch:
diff --git a/pkg/skaffold/deploy/cloudrun/deploy.go b/pkg/skaffold/deploy/cloudrun/deploy.go
index 90b86d4c3..edc2d1ed3 100644
--- a/pkg/skaffold/deploy/cloudrun/deploy.go
+++ b/pkg/skaffold/deploy/cloudrun/deploy.go
@@ -56,8 +56,8 @@ type Deployer struct {
 	monitor  *Monitor
 	labeller *label.DefaultLabeller
 
-	DefaultProject string
-	Region         string
+	Project string
+	Region  string
 
 	// additional client options for connecting to Cloud Run, used for tests
 	clientOptions []option.ClientOption
@@ -67,8 +67,8 @@ type Deployer struct {
 // NewDeployer creates a new Deployer for Cloud Run from the Skaffold deploy config.
 func NewDeployer(cfg Config, labeller *label.DefaultLabeller, crDeploy *latest.CloudRunDeploy) (*Deployer, error) {
 	return &Deployer{
-		DefaultProject: crDeploy.DefaultProjectID,
-		Region:         crDeploy.Region,
+		Project: crDeploy.ProjectID,
+		Region:  crDeploy.Region,
 		// TODO: implement logger for Cloud Run.
 		logger:        &log.NoopLogger{},
 		accessor:      NewAccessor(cfg, labeller.GetRunID()),
@@ -163,8 +163,13 @@ func (d *Deployer) deployToCloudRun(ctx context.Context, out io.Writer, manifest
 			ErrCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR,
 		})
 	}
-	if service.Metadata.Namespace == "" {
-		service.Metadata.Namespace = d.DefaultProject
+	if d.Project != "" {
+		service.Metadata.Namespace = d.Project
+	} else if service.Metadata.Namespace == "" {
+		return sErrors.NewError(fmt.Errorf("unable to detect project for Cloud Run"), &proto.ActionableErr{
+			Message: "No Google Cloud project found in Cloud Run Manifest or Skaffold Config",
+			ErrCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR,
+		})
 	}
 
 	// we need to strip "skaffold.dev" from the run-id label because gcp labels don't support domains
@@ -193,7 +198,7 @@ func (d *Deployer) deployToCloudRun(ctx context.Context, out io.Writer, manifest
 		if !ok || gErr.Code != http.StatusNotFound {
 			return sErrors.NewError(fmt.Errorf("error checking Cloud Run State: %w", err), &proto.ActionableErr{
 				Message: err.Error(),
-				ErrCode: proto.StatusCode_DEPLOY_CANCELLED,
+				ErrCode: proto.StatusCode_DEPLOY_CLOUD_RUN_GET_SERVICE_ERR,
 			})
 		}
 		// This is a new service, we need to create it
@@ -230,10 +235,17 @@ func (d *Deployer) deleteRunService(ctx context.Context, out io.Writer, dryRun b
 	}
 
 	var projectID string
-	if service.Metadata.Namespace != "" {
+	switch {
+	case d.Project != "":
+		projectID = d.Project
+	case service.Metadata.Namespace != "":
 		projectID = service.Metadata.Namespace
-	} else {
-		projectID = d.DefaultProject
+	default:
+		// no project specified, we don't know what to delete.
+		return sErrors.NewError(fmt.Errorf("unable to determine Google Cloud Project"), &proto.ActionableErr{
+			Message: "No Google Cloud Project found in Cloud Run manifest or Skaffold Manifest.",
+			ErrCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR,
+		})
 	}
 	parent := fmt.Sprintf("projects/%s/locations/%s", projectID, d.Region)
 	sName := fmt.Sprintf("%s/services/%s", parent, service.Metadata.Name)
diff --git a/pkg/skaffold/runner/deployer.go b/pkg/skaffold/runner/deployer.go
index 20d215251..79a98cb5d 100644
--- a/pkg/skaffold/runner/deployer.go
+++ b/pkg/skaffold/runner/deployer.go
@@ -204,12 +204,13 @@ func GetDeployer(ctx context.Context, runCtx *runcontext.RunContext, labeller *l
 The "default deployer" is used in `skaffold apply`, which uses a `kubectl` deployer to actuate resources
 on a cluster regardless of provided deployer configuration in the skaffold.yaml.
 The default deployer will honor a select set of deploy configuration from an existing skaffold.yaml:
-	- deploy.StatusCheckDeadlineSeconds
-	- deploy.Logs.Prefix
-	- deploy.Kubectl.Flags
-	- deploy.Kubectl.DefaultNamespace
-	- deploy.Kustomize.Flags
-	- deploy.Kustomize.DefaultNamespace
+  - deploy.StatusCheckDeadlineSeconds
+  - deploy.Logs.Prefix
+  - deploy.Kubectl.Flags
+  - deploy.Kubectl.DefaultNamespace
+  - deploy.Kustomize.Flags
+  - deploy.Kustomize.DefaultNamespace
+
 For a multi-config project, we do not currently support resolving conflicts between differing sets of this deploy configuration.
 Therefore, in this function we do implicit validation of the provided configuration, and fail if any conflict cannot be resolved.
 */
@@ -321,10 +322,10 @@ func getCloudRunDeployer(runCtx *runcontext.RunContext, labeller *label.DefaultL
 				return nil, fmt.Errorf("expected all Cloud Run deploys to be in the same region, found deploys to %s and %s", region, crDeploy.Region)
 			}
 			region = crDeploy.Region
-			if defaultProject != "" && defaultProject != crDeploy.DefaultProjectID {
-				return nil, fmt.Errorf("expected all Cloud Run deploys to use the same default project, found deploys to projects %s and %s", defaultProject, crDeploy.DefaultProjectID)
+			if defaultProject != "" && defaultProject != crDeploy.ProjectID {
+				return nil, fmt.Errorf("expected all Cloud Run deploys to use the same project, found deploys to projects %s and %s", defaultProject, crDeploy.ProjectID)
 			}
 		}
 	}
-	return cloudrun.NewDeployer(runCtx, labeller, &latest.CloudRunDeploy{Region: region, DefaultProjectID: defaultProject})
+	return cloudrun.NewDeployer(runCtx, labeller, &latest.CloudRunDeploy{Region: region, ProjectID: defaultProject})
 }


Test patch:
diff --git a/integration/testdata/deploy-cloudrun/skaffold.yaml b/integration/testdata/deploy-cloudrun/skaffold.yaml
index 335556717..f1947d544 100644
--- a/integration/testdata/deploy-cloudrun/skaffold.yaml
+++ b/integration/testdata/deploy-cloudrun/skaffold.yaml
@@ -7,5 +7,5 @@ manifests:
     - service.yaml
 deploy:
   cloudrun:
-    defaultprojectid: k8s-skaffold
+    projectid: k8s-skaffold
     region: us-central1
\ No newline at end of file
diff --git a/pkg/skaffold/deploy/cloudrun/deploy_test.go b/pkg/skaffold/deploy/cloudrun/deploy_test.go
index 4518c5c8e..08c01073b 100644
--- a/pkg/skaffold/deploy/cloudrun/deploy_test.go
+++ b/pkg/skaffold/deploy/cloudrun/deploy_test.go
@@ -28,9 +28,11 @@ import (
 	"google.golang.org/api/run/v1"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/label"
+	sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
 	runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext/v2"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/proto/v1"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -42,6 +44,7 @@ func TestDeploy(tOuter *testing.T) {
 		region         string
 		expectedPath   string
 		httpErr        int
+		errCode        proto.StatusCode
 	}{
 		{
 			description:    "test deploy",
@@ -58,7 +61,7 @@ func TestDeploy(tOuter *testing.T) {
 			description:    "test deploy with specified project",
 			defaultProject: "testProject",
 			region:         "us-central1",
-			expectedPath:   "/v1/projects/my-project/locations/us-central1/services",
+			expectedPath:   "/v1/projects/testProject/locations/us-central1/services",
 			toDeploy: &run.Service{
 				Metadata: &run.ObjectMeta{
 					Name:      "test-service",
@@ -77,6 +80,17 @@ func TestDeploy(tOuter *testing.T) {
 					Namespace: "my-project",
 				},
 			},
+			errCode: proto.StatusCode_DEPLOY_CLOUD_RUN_GET_SERVICE_ERR,
+		},
+		{
+			description: "test no project specified",
+			region:      "us-central1",
+			toDeploy: &run.Service{
+				Metadata: &run.ObjectMeta{
+					Name: "test-service",
+				},
+			},
+			errCode: proto.StatusCode_DEPLOY_READ_MANIFEST_ERR,
 		},
 	}
 	for _, test := range tests {
@@ -107,16 +121,22 @@ func TestDeploy(tOuter *testing.T) {
 				w.Write(b)
 			}))
 
-			deployer, _ := NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{DefaultProjectID: test.defaultProject, Region: test.region})
+			deployer, _ := NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{ProjectID: test.defaultProject, Region: test.region})
 			deployer.clientOptions = append(deployer.clientOptions, option.WithEndpoint(ts.URL), option.WithoutAuthentication())
 			deployer.useGcpOptions = false
 			manifest, _ := json.Marshal(test.toDeploy)
 			manifests := [][]byte{manifest}
 			err := deployer.Deploy(context.Background(), os.Stderr, []graph.Artifact{}, manifests)
-			if test.httpErr == 0 && err != nil {
+			if test.errCode == proto.StatusCode_OK && err != nil {
 				t.Fatalf("Expected success but got err: %v", err)
-			} else if test.httpErr != 0 && err == nil {
-				t.Fatalf("Expected HTTP Error %s but got success", http.StatusText(test.httpErr))
+			} else if test.errCode != proto.StatusCode_OK {
+				if err == nil {
+					t.Fatalf("Expected status code %s but got success", test.errCode)
+				}
+				sErr := err.(sErrors.Error)
+				if sErr.StatusCode() != test.errCode {
+					t.Fatalf("Expected status code %v but got %v", test.errCode, sErr.StatusCode())
+				}
 			}
 		})
 	}
@@ -146,7 +166,7 @@ func TestCleanup(tOuter *testing.T) {
 			description:    "test cleanup with specified project",
 			defaultProject: "testProject",
 			region:         "us-central1",
-			expectedPath:   "/v1/projects/my-project/locations/us-central1/services/test-service",
+			expectedPath:   "/v1/projects/testProject/locations/us-central1/services/test-service",
 			toDelete: &run.Service{
 				Metadata: &run.ObjectMeta{
 					Name:      "test-service",
@@ -187,7 +207,7 @@ func TestCleanup(tOuter *testing.T) {
 				w.Write(b)
 			}))
 			defer ts.Close()
-			deployer, _ := NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{DefaultProjectID: test.defaultProject, Region: test.region})
+			deployer, _ := NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{ProjectID: test.defaultProject, Region: test.region})
 			deployer.clientOptions = append(deployer.clientOptions, option.WithEndpoint(ts.URL), option.WithoutAuthentication())
 			deployer.useGcpOptions = false
 			manifest, _ := json.Marshal(test.toDelete)
diff --git a/pkg/skaffold/runner/deployer_test.go b/pkg/skaffold/runner/deployer_test.go
index 16fba7093..034c4ba57 100644
--- a/pkg/skaffold/runner/deployer_test.go
+++ b/pkg/skaffold/runner/deployer_test.go
@@ -237,11 +237,11 @@ func TestGetDeployer(tOuter *testing.T) {
 				cfg: latest.Pipeline{
 					Deploy: latest.DeployConfig{
 						DeployType: latest.DeployType{
-							CloudRunDeploy: &latest.CloudRunDeploy{DefaultProjectID: "TestProject", Region: "us-central1"},
+							CloudRunDeploy: &latest.CloudRunDeploy{ProjectID: "TestProject", Region: "us-central1"},
 						},
 					},
 				},
-				expected: t.RequireNonNilResult(cloudrun.NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{DefaultProjectID: "TestProject", Region: "us-central1"})).(deploy.Deployer),
+				expected: t.RequireNonNilResult(cloudrun.NewDeployer(&runcontext.RunContext{}, &label.DefaultLabeller{}, &latest.CloudRunDeploy{ProjectID: "TestProject", Region: "us-central1"})).(deploy.Deployer),
 			},
 			{
 				description: "apply does not allow multiple deployers when Cloud Run is used",
diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go
index e739e5e0f..e476284cf 100644
--- a/pkg/skaffold/schema/latest/config.go
+++ b/pkg/skaffold/schema/latest/config.go
@@ -696,7 +696,7 @@ type DeployType struct {
 // CloudRunDeploy *alpha* deploys the container to Google Cloud Run.
 type CloudRunDeploy struct {
 	// ProjectID of the GCP Project to use for Cloud Run.
-	DefaultProjectID string `yaml:"defaultprojectid,omitempty"`
+	ProjectID string `yaml:"projectid,omitempty"`
 
 	// Region in GCP to use for the Cloud Run Deploy.
 	// Must be one of the regions listed in https://cloud.google.com/run/docs/locations.