feat/msp: do not use tfvars file outside of deploy-type 'subscription' (#62704)

Closes CORE-121

The dependency on the generated `tfvars` file is frustrating for first-time MSP setup because it currently requires `-stable=false` to update, and doesn't actually serve any purpose for deploy types other than `subscription` (which uses it to isolate image changes that happen on via GitHub actions). This makes it so that we don't generate, or depend on, the dynamic `tfvars` file unless you are using `subscription`.

I've also added a rollout spec configuration, `initialImageTag`, to make the initial tag we provision environments with configurable (as some services might not publish `insiders` images) - see the docstring.

## Test plan

Inspect output of `sg msp generate -all`

dev/managedservicesplatform/managedservicesplatform.go

@@ -111,7 +111,7 @@ func (r *Renderer) RenderEnvironment(
 		SecretVolumes:   env.SecretVolumes,
 		PreventDestroys: preventDestroys,
 
-		IsFinalStageOfRollout: rolloutPipeline != nil,
+		IsFinalStageOfRollout: rolloutPipeline.IsFinalStage(),
 	})
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create IAM stack")

dev/managedservicesplatform/spec/environment.go

@@ -171,6 +171,10 @@ func (c EnvironmentCategory) Validate() error {
 	return nil
 }
 
+func (c EnvironmentCategory) IsProduction() bool {
+	return c == EnvironmentCategoryExternal || c == EnvironmentCategoryInternal
+}
+
 type EnvironmentDeployType string
 
 const (
@@ -179,10 +183,6 @@ const (
 	EnvironmentDeployTypeRollout      = "rollout"
 )
 
-func (c EnvironmentCategory) IsProduction() bool {
-	return c == EnvironmentCategoryExternal || c == EnvironmentCategoryInternal
-}
-
 type EnvironmentDeploySpec struct {
 	// Type specifies the deployment method for the environment. There are
 	// 3 supported types:
@@ -197,7 +197,12 @@ type EnvironmentDeploySpec struct {
 
 func (s EnvironmentDeploySpec) Validate() []error {
 	var errs []error
-	if s.Type == EnvironmentDeployTypeSubscription {
+	switch s.Type {
+	case EnvironmentDeployTypeManual:
+		if s.Subscription != nil {
+			errs = append(errs, errors.New("subscription deploy spec provided when type is manual"))
+		}
+	case EnvironmentDeployTypeSubscription:
 		if s.Manual != nil {
 			errs = append(errs, errors.New("manual deploy spec provided when type is subscription"))
 		} else if s.Subscription == nil {
@@ -205,52 +210,47 @@ func (s EnvironmentDeploySpec) Validate() []error {
 		} else if s.Subscription.Tag == "" {
 			errs = append(errs, errors.New("no tag in image subscription specified"))
 		}
-	} else if s.Type == EnvironmentDeployTypeManual {
-		if s.Subscription != nil {
-			errs = append(errs, errors.New("subscription deploy spec provided when type is manual"))
-		}
+	case EnvironmentDeployTypeRollout:
+		// no validation
+	default:
+		errs = append(errs, errors.Newf("invalid deploy type %q", s.Type))
 	}
 	return errs
 }
 
-// ResolveTag uses the deploy spec to resolve an appropriate tag for the environment.
-func (d EnvironmentDeploySpec) ResolveTag(repo string) (string, error) {
-	switch d.Type {
-	case EnvironmentDeployTypeManual:
-		if d.Manual == nil {
-			return "insiders", nil
-		}
-		return d.Manual.Tag, nil
-	case EnvironmentDeployTypeSubscription:
-		// we already validated in Validate(), hence it's fine to assume this won't panic
-		updater, err := imageupdater.New()
-		if err != nil {
-			return "", errors.Wrapf(err, "create image updater")
-		}
-		tagAndDigest, err := updater.ResolveTagAndDigest(repo, d.Subscription.Tag)
-		if err != nil {
-			return "", errors.Wrapf(err, "resolve digest for tag %q", "insiders")
-		}
-		return tagAndDigest, nil
-	case EnvironmentDeployTypeRollout:
-		// Enforce convention
-		return "insiders", nil
-	default:
-		return "", errors.Newf("unable to resolve tag for unknown deploy type %q", d.Type)
-	}
-}
-
 type EnvironmentDeployManualSpec struct {
 	// Tag is the tag to deploy. If empty, defaults to "insiders".
 	Tag string `yaml:"tag,omitempty"`
 }
 
+// GetTag returns the tag to deploy. If empty, defaults to "insiders".
+func (s *EnvironmentDeployManualSpec) GetTag() string {
+	if s == nil {
+		return "insiders"
+	}
+	return s.Tag
+}
+
 type EnvironmentDeployTypeSubscriptionSpec struct {
 	// Tag is the tag to subscribe to.
 	Tag string `yaml:"tag,omitempty"`
 	// TODO: In the future, we may support subscribing by semver constraints.
 }
 
+// ResolveTag fetches the latest digest for the target imageRepo and configured
+// subscription tag, and returns it.
+func (s EnvironmentDeployTypeSubscriptionSpec) ResolveTag(imageRepo string) (string, error) {
+	updater, err := imageupdater.New()
+	if err != nil {
+		return "", errors.Wrapf(err, "create image updater")
+	}
+	tagAndDigest, err := updater.ResolveTagAndDigest(imageRepo, s.Tag)
+	if err != nil {
+		return "", errors.Wrapf(err, "resolve digest for tag %q", "insiders")
+	}
+	return tagAndDigest, nil
+}
+
 type EnvironmentServiceSpec struct {
 	// Domain configures where the resource is externally accessible.
 	//

dev/managedservicesplatform/spec/environment_test.go

@@ -207,3 +207,83 @@ func TestEnvironmentJobScheduleSpecFindMaxCronInterval(t *testing.T) {
 		})
 	}
 }
+
+func TestEnvironmentDeploySpec_Validate(t *testing.T) {
+	tests := []struct {
+		name     string
+		spec     EnvironmentDeploySpec
+		wantErrs autogold.Value
+	}{
+		{
+			name: "manual type with subscription",
+			spec: EnvironmentDeploySpec{
+				Type:         EnvironmentDeployTypeManual,
+				Subscription: &EnvironmentDeployTypeSubscriptionSpec{},
+			},
+			wantErrs: autogold.Expect([]string{"subscription deploy spec provided when type is manual"}),
+		},
+		{
+			name: "subscription type with manual",
+			spec: EnvironmentDeploySpec{
+				Type:   EnvironmentDeployTypeSubscription,
+				Manual: &EnvironmentDeployManualSpec{},
+			},
+			wantErrs: autogold.Expect([]string{"manual deploy spec provided when type is subscription"}),
+		},
+		{
+			name: "subscription type without subscription",
+			spec: EnvironmentDeploySpec{
+				Type: EnvironmentDeployTypeSubscription,
+			},
+			wantErrs: autogold.Expect([]string{"no subscription specified when deploy type is subscription"}),
+		},
+		{
+			name: "subscription type with empty tag",
+			spec: EnvironmentDeploySpec{
+				Type:         EnvironmentDeployTypeSubscription,
+				Subscription: &EnvironmentDeployTypeSubscriptionSpec{},
+			},
+			wantErrs: autogold.Expect([]string{"no tag in image subscription specified"}),
+		},
+		{
+			name: "subscription type with tag",
+			spec: EnvironmentDeploySpec{
+				Type: EnvironmentDeployTypeSubscription,
+				Subscription: &EnvironmentDeployTypeSubscriptionSpec{
+					Tag: "insiders",
+				},
+			},
+		},
+		{
+			name: "rollout type",
+			spec: EnvironmentDeploySpec{
+				Type: EnvironmentDeployTypeRollout,
+			},
+		},
+		{
+			name: "invalid type",
+			spec: EnvironmentDeploySpec{
+				Type: "invalid",
+			},
+			wantErrs: autogold.Expect([]string{`invalid deploy type "invalid"`}),
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			errs := stringifyErrors(tc.spec.Validate())
+			if tc.wantErrs == nil {
+				assert.Empty(t, errs)
+			} else {
+				tc.wantErrs.Equal(t, errs)
+			}
+		})
+	}
+}
+
+func stringifyErrors(errs []error) (values []string) {
+	for _, errs := range errs {
+		values = append(values, errs.Error())
+	}
+	return values
+}

dev/managedservicesplatform/spec/rollout.go

@@ -15,6 +15,13 @@ type RolloutSpec struct {
 	// releases for. Can be used to give access to the Service Account used in your CI pipeline,
 	// instead of using the default releaser SA that MSP provisions.
 	ServiceAccount *string `yaml:"serviceAccount,omitempty"`
+	// InitialImageTag is the image tag to use by default. This is mostly used to
+	// provision the first revision of a Cloud Run service/job for an environment,
+	// after which Cloud Deploy manages the image used for Cloud Run revisions.
+	//
+	// This only needs to be set if the default image tag 'insiders' does not
+	// correspond to an image tag that is available for this service's images.
+	InitialImageTag *string `yaml:"initialImageTag,omitempty"`
 }
 
 func (r *RolloutSpec) GetStageByEnvironment(id string) *RolloutStageSpec {
@@ -29,6 +36,13 @@ func (r *RolloutSpec) GetStageByEnvironment(id string) *RolloutStageSpec {
 	return nil
 }
 
+func (r *RolloutSpec) GetInitialImageTag() string {
+	if r.InitialImageTag != nil {
+		return *r.InitialImageTag
+	}
+	return "insiders"
+}
+
 type RolloutStageSpec struct {
 	// EnvironmentID is the ID of the environment to use in this stage.
 	// The specified environment MUST have 'deploy: { type: "rollout" }' configured.
@@ -38,6 +52,7 @@ type RolloutStageSpec struct {
 // RolloutPipelineConfiguration is rendered from BuildPipelineConfiguration for use in
 // stacks.
 type RolloutPipelineConfiguration struct {
+	isFinalStage bool
 	// Stages is evaluated from OriginalSpec.Stages to include attributes
 	// required to actually configure the stages.
 	Stages []rolloutPipelineTargetConfiguration
@@ -45,6 +60,15 @@ type RolloutPipelineConfiguration struct {
 	OriginalSpec RolloutSpec
 }
 
+// IsFinalStage indicates if the env used for this RolloutPipelineConfiguration
+// is the final stage in the rollout pipeline. If nil, this returns false.
+func (s *RolloutPipelineConfiguration) IsFinalStage() bool {
+	if s == nil {
+		return false
+	}
+	return s.isFinalStage
+}
+
 // rolloutPipelineTargetConfiguration is an internal type that extends
 // RolloutStageSpec with other top-level environment spec.
 type rolloutPipelineTargetConfiguration struct {
@@ -61,11 +85,6 @@ func (s Spec) BuildRolloutPipelineConfiguration(env EnvironmentSpec) *RolloutPip
 		return nil
 	}
 
-	// We only need the configuration
-	if s.Rollout.Stages[len(s.Rollout.Stages)-1].EnvironmentID != env.ID {
-		return nil
-	}
-
 	var targets []rolloutPipelineTargetConfiguration
 	for _, stage := range s.Rollout.Stages {
 		env := s.GetEnvironment(stage.EnvironmentID)
@@ -74,7 +93,9 @@ func (s Spec) BuildRolloutPipelineConfiguration(env EnvironmentSpec) *RolloutPip
 			RolloutStageSpec: stage,
 		})
 	}
+	finalStageEnv := s.Rollout.Stages[len(s.Rollout.Stages)-1].EnvironmentID
 	return &RolloutPipelineConfiguration{
+		isFinalStage: finalStageEnv == env.ID,
 		Stages:       targets,
 		OriginalSpec: *s.Rollout,
 	}

dev/managedservicesplatform/stacks/cloudrun/cloudrun.go

@@ -71,9 +71,9 @@ type Variables struct {
 	Image       string
 	Environment spec.EnvironmentSpec
 
-	// RolloutPipeline is only non-nil if this environment is the final
-	// environment of a rollout spec - the final environment is where the Cloud
-	// Deploy pipeline lives.
+	// RolloutPipeline is only non-nil if this environment is part of rollout
+	// pipeline. The final environment (IsFinalStage) is where the Cloud Deploy
+	// pipeline lives.
 	RolloutPipeline *spec.RolloutPipelineConfiguration
 
 	StableGenerate bool
@@ -109,8 +109,11 @@ func NewStack(stacks *stack.Set, vars Variables) (crossStackOutput *CrossStackOu
 		}),
 		randomprovider.With(),
 		dynamicvariables.With(vars.StableGenerate, func() (stack.TFVars, error) {
-			resolvedImageTag, err := vars.Environment.Deploy.ResolveTag(vars.Image)
-			return stack.TFVars{tfVarKeyResolvedImageTag: resolvedImageTag}, err
+			if d := vars.Environment.Deploy; d.Type == spec.EnvironmentDeployTypeSubscription {
+				resolvedImageTag, err := d.Subscription.ResolveTag(vars.Image)
+				return stack.TFVars{tfVarKeyResolvedImageTag: resolvedImageTag}, err
+			}
+			return nil, nil
 		}),
 		sentryprovider.With(gsmsecret.DataConfig{
 			Secret:    googlesecretsmanager.SecretSentryAuthToken,
@@ -165,11 +168,24 @@ func NewStack(stacks *stack.Set, vars Variables) (crossStackOutput *CrossStackOu
 	// Add user-configured secret volumes
 	addContainerSecretVolumes(cloudRunBuilder, vars.Environment.SecretVolumes)
 
-	// Load image tag from tfvars.
-	imageTag := tfvar.New(stack, id, tfvar.Config{
-		VariableKey: tfVarKeyResolvedImageTag,
-		Description: "Resolved image tag to deploy",
-	})
+	// Determine where to source the image tag from, based on the deploy type.
+	var imageTag string
+	switch d := vars.Environment.Deploy; d.Type {
+	case spec.EnvironmentDeployTypeManual:
+		imageTag = d.Manual.GetTag()
+
+	case spec.EnvironmentDeployTypeRollout:
+		imageTag = vars.RolloutPipeline.OriginalSpec.GetInitialImageTag()
+
+	case spec.EnvironmentDeployTypeSubscription:
+		imageTag = *tfvar.New(stack, id, tfvar.Config{
+			VariableKey: tfVarKeyResolvedImageTag,
+			Description: "Image tag resolved from subscription to deploy",
+		}).StringValue
+
+	default:
+		return nil, errors.Newf("unsupported deploy type %q", d.Type)
+	}
 
 	// privateNetworkEnabled indicates if privateNetwork has been instantiated
 	// before.
@@ -349,7 +365,7 @@ func NewStack(stacks *stack.Set, vars Variables) (crossStackOutput *CrossStackOu
 	cloudRunResource, err := cloudRunBuilder.Build(stack, builder.Variables{
 		Service:           vars.Service,
 		Image:             vars.Image,
-		ResolvedImageTag:  *imageTag.StringValue,
+		ImageTag:          imageTag,
 		Environment:       vars.Environment,
 		GCPProjectID:      vars.ProjectID,
 		GCPRegion:         locationSpec.GCPRegion,
@@ -367,8 +383,9 @@ func NewStack(stacks *stack.Set, vars Variables) (crossStackOutput *CrossStackOu
 		return nil, errors.Wrapf(err, "build Cloud Run resource kind %q", cloudRunBuilder.Kind())
 	}
 
-	// We have a rollout pipeline to configure.
-	if vars.RolloutPipeline != nil {
+	// We have a rollout pipeline to configure - Cloud Deploy pipeline lives in
+	// the final stage of the pipeline.
+	if vars.RolloutPipeline.IsFinalStage() {
 		id := id.Group("rolloutpipeline")
 
 		// For now, we only use 1 region everywhere, but also note that ALL
@@ -511,8 +528,6 @@ func NewStack(stacks *stack.Set, vars Variables) (crossStackOutput *CrossStackOu
 		"Cloud Run resource name")
 	locals.Add("cloud_run_location", *cloudRunResource.Location(),
 		"Cloud Run resource location")
-	locals.Add("image_tag", *imageTag.StringValue,
-		"Resolved tag of service image to deploy")
 	return &CrossStackOutput{
 		DiagnosticsSecret:  diagnosticsSecret,
 		RedisInstanceID:    redisInstanceID,

dev/managedservicesplatform/stacks/cloudrun/internal/builder/builder.go

@@ -17,8 +17,8 @@ type Variables struct {
 
 	// Image and ResolvedImageTag are used to declare the full image reference
 	// to deploy.
-	Image            string
-	ResolvedImageTag string
+	Image    string
+	ImageTag string
 	// GCPProjectID for all resources.
 	GCPProjectID string
 	// GCPRegion for all resources.

dev/managedservicesplatform/stacks/cloudrun/internal/builder/job/job.go

@@ -138,7 +138,7 @@ func (b *jobBuilder) Build(stack cdktf.TerraformStack, vars builder.Variables) (
 				// Configuration for the single service container.
 				Containers: []*cloudrunv2job.CloudRunV2JobTemplateTemplateContainers{{
 					Name:  pointers.Ptr(vars.Service.ID),
-					Image: pointers.Ptr(fmt.Sprintf("%s:%s", vars.Image, vars.ResolvedImageTag)),
+					Image: pointers.Ptr(fmt.Sprintf("%s:%s", vars.Image, vars.ImageTag)),
 
 					Resources: &cloudrunv2job.CloudRunV2JobTemplateTemplateContainersResources{
 						Limits: &vars.ResourceLimits,

dev/managedservicesplatform/stacks/cloudrun/internal/builder/service/service.go

@@ -192,7 +192,7 @@ func (b *serviceBuilder) Build(stack cdktf.TerraformStack, vars builder.Variable
 			// Configuration for the single service container.
 			Containers: []*cloudrunv2service.CloudRunV2ServiceTemplateContainers{{
 				Name:  pointers.Ptr(vars.Service.ID),
-				Image: pointers.Ptr(fmt.Sprintf("%s:%s", vars.Image, vars.ResolvedImageTag)),
+				Image: pointers.Ptr(fmt.Sprintf("%s:%s", vars.Image, vars.ImageTag)),
 
 				Resources: &cloudrunv2service.CloudRunV2ServiceTemplateContainersResources{
 					Limits: &vars.ResourceLimits,