fix: interpolate a task file before it is parsed

A var used in container_limits broke the build, because FileConfigSource
parses the file into an atc.TaskConfig before any interpolation happens, and
'((container_mem_limit))' is not a memory limit. Only fields that are plain
strings survived that first parse.

FileConfigSource can now hand back the file as written, and
InterpolateTemplateConfigSource resolves those bytes and parses once,
instead of parsing, marshalling back and parsing again. For a task file the
interpolation pass now always runs, with the pipeline vars followed by the
credential variables, so a var resolves the same way wherever it appears in
the file.

atc/exec/task_config_source.go

@@ -25,6 +25,14 @@ type TaskConfigSource interface {
 	Warnings() []string
 }
 
+// RawTaskConfigSource is a TaskConfigSource that can also hand back the config
+// as it was written, along with a description of where it came from. Interpolating
+// those bytes lets a var stand in for a field that is not a string, such as the
+// values under container_limits.
+type RawTaskConfigSource interface {
+	FetchRawConfig(context.Context, lager.Logger, *build.Repository) ([]byte, string, error)
+}
+
 // StaticConfigSource represents a statically configured TaskConfig.
 type StaticConfigSource struct {
 	Config *atc.TaskConfig
@@ -66,9 +74,20 @@ type FileConfigSource struct {
 // If the task config file is not found, or is invalid YAML, or is an invalid
 // task configuration, the respective errors will be bubbled up.
 func (configSource FileConfigSource) FetchConfig(ctx context.Context, logger lager.Logger, repo *build.Repository) (atc.TaskConfig, error) {
+	byteConfig, source, err := configSource.FetchRawConfig(ctx, logger, repo)
+	if err != nil {
+		return atc.TaskConfig{}, err
+	}
+
+	return newTaskConfig(byteConfig, source)
+}
+
+// FetchRawConfig reads the specified file from the artifact.Repository and
+// returns its contents without parsing them.
+func (configSource FileConfigSource) FetchRawConfig(ctx context.Context, logger lager.Logger, repo *build.Repository) ([]byte, string, error) {
 	segs := strings.SplitN(configSource.ConfigPath, "/", 2)
 	if len(segs) != 2 {
-		return atc.TaskConfig{}, UnspecifiedArtifactSourceError{configSource.ConfigPath}
+		return nil, "", UnspecifiedArtifactSourceError{configSource.ConfigPath}
 	}
 
 	sourceName := build.ArtifactName(segs[0])
@@ -76,26 +95,30 @@ func (configSource FileConfigSource) FetchConfig(ctx context.Context, logger lag
 
 	artifact, _, found := repo.ArtifactFor(sourceName)
 	if !found {
-		return atc.TaskConfig{}, UnknownArtifactSourceError{sourceName, configSource.ConfigPath}
+		return nil, "", UnknownArtifactSourceError{sourceName, configSource.ConfigPath}
 	}
 	stream, err := configSource.Streamer.StreamFile(lagerctx.NewContext(ctx, logger), artifact, filePath)
 	if err != nil {
 		if err == baggageclaim.ErrFileNotFound {
-			return atc.TaskConfig{}, fmt.Errorf("task config '%s/%s' not found", sourceName, filePath)
+			return nil, "", fmt.Errorf("task config '%s/%s' not found", sourceName, filePath)
 		}
-		return atc.TaskConfig{}, err
+		return nil, "", err
 	}
 
 	defer stream.Close()
 
 	byteConfig, err := io.ReadAll(stream)
-	if err != nil {
+	return byteConfig, configSource.ConfigPath, err
-		return atc.TaskConfig{}, err
+}
-	}
 
+// newTaskConfig parses a task config, naming the file it came from when it is known.
+func newTaskConfig(byteConfig []byte, source string) (atc.TaskConfig, error) {
 	config, err := atc.NewTaskConfig(byteConfig)
 	if err != nil {
-		return atc.TaskConfig{}, fmt.Errorf("failed to create task config from bytes %s: %s", configSource.ConfigPath, err)
+		if source == "" {
+			return atc.TaskConfig{}, fmt.Errorf("failed to create task config from bytes: %s", err)
+		}
+		return atc.TaskConfig{}, fmt.Errorf("failed to create task config from bytes %s: %s", source, err)
 	}
 
 	return config, nil
@@ -105,6 +128,8 @@ func (configSource FileConfigSource) Warnings() []string {
 	return []string{}
 }
 
+var _ RawTaskConfigSource = FileConfigSource{}
+
 // BaseResourceTypeDefaultsApplySource applies base resource type defaults to image_source.
 type BaseResourceTypeDefaultsApplySource struct {
 	ConfigSource  TaskConfigSource
@@ -207,28 +232,40 @@ type InterpolateTemplateConfigSource struct {
 
 // FetchConfig returns the interpolated configuration
 func (configSource InterpolateTemplateConfigSource) FetchConfig(ctx context.Context, logger lager.Logger, source *build.Repository) (atc.TaskConfig, error) {
-	taskConfig, err := configSource.ConfigSource.FetchConfig(ctx, logger, source)
+	byteConfig, from, err := configSource.fetchByteConfig(ctx, logger, source)
 	if err != nil {
 		return atc.TaskConfig{}, err
 	}
 
-	byteConfig, err := yaml.Marshal(taskConfig)
-	if err != nil {
-		return atc.TaskConfig{}, fmt.Errorf("failed to marshal task config: %s", err)
-	}
-
 	// process task config using the provided variables
 	byteConfig, err = vars.NewTemplateResolver(byteConfig, configSource.Vars).Resolve(configSource.ExpectAllKeys)
 	if err != nil {
 		return atc.TaskConfig{}, fmt.Errorf("failed to interpolate task config: %s", err)
 	}
 
-	taskConfig, err = atc.NewTaskConfig(byteConfig)
+	return newTaskConfig(byteConfig, from)
-	if err != nil {
+}
-		return atc.TaskConfig{}, fmt.Errorf("failed to create task config from bytes: %s", err)
+
+// fetchByteConfig returns the config to interpolate. When the underlying source
+// can hand back the config as written, it is used untouched, so a var may stand
+// in for a field the parser would reject before it is resolved. Otherwise the
+// config has already been parsed and is marshalled back.
+func (configSource InterpolateTemplateConfigSource) fetchByteConfig(ctx context.Context, logger lager.Logger, source *build.Repository) ([]byte, string, error) {
+	if rawSource, ok := configSource.ConfigSource.(RawTaskConfigSource); ok {
+		return rawSource.FetchRawConfig(ctx, logger, source)
 	}
 
-	return taskConfig, nil
+	taskConfig, err := configSource.ConfigSource.FetchConfig(ctx, logger, source)
+	if err != nil {
+		return nil, "", err
+	}
+
+	byteConfig, err := yaml.Marshal(taskConfig)
+	if err != nil {
+		return nil, "", fmt.Errorf("failed to marshal task config: %s", err)
+	}
+
+	return byteConfig, "", nil
 }
 
 func (configSource InterpolateTemplateConfigSource) Warnings() []string {

atc/exec/task_config_source_test.go

@@ -556,6 +556,64 @@ run: {path: a/file}
 				}))
 			})
 		})
+
+		Context("when the config comes from a file that uses vars in container_limits", func() {
+			var (
+				fakeStreamer *execfakes.FakeStreamer
+				artifactName string
+			)
+
+			BeforeEach(func() {
+				artifactName = "some-artifact-name"
+				fakeStreamer = new(execfakes.FakeStreamer)
+				repo.RegisterArtifact(build.ArtifactName(artifactName), runtimetest.NewVolume("some-volume"), false)
+				fakeStreamer.StreamFileReturns(gbytes.BufferWithBytes([]byte(`
+platform: linux
+
+container_limits:
+  cpu: ((container_cpu_limit))
+  memory: ((container_mem_limit))
+
+run: {path: ls}
+`)), nil)
+			})
+
+			JustBeforeEach(func() {
+				configSource = FileConfigSource{
+					ConfigPath: artifactName + "/build.yml",
+					Streamer:   fakeStreamer,
+				}
+				configSource = InterpolateTemplateConfigSource{
+					ConfigSource: configSource,
+					Vars: []vars.Variables{vars.StaticVariables{
+						"container_cpu_limit": 512,
+						"container_mem_limit": "5gb",
+					}},
+					ExpectAllKeys: true,
+				}
+				fetchedConfig, fetchErr = configSource.FetchConfig(context.TODO(), logger, repo)
+			})
+
+			It("interpolates the vars before the config is parsed", func() {
+				Expect(fetchErr).ToNot(HaveOccurred())
+				Expect(fetchedConfig.Limits).To(Equal(&atc.ContainerLimits{
+					CPU:    newCPULimit(512),
+					Memory: newMemoryLimit(5 * 1024 * 1024 * 1024),
+				}))
+			})
+
+			Context("when the file is still invalid once the vars are resolved", func() {
+				BeforeEach(func() {
+					fakeStreamer.StreamFileReturns(gbytes.BufferWithBytes([]byte(`
+container_limits: {cpu: ((container_cpu_limit))}
+`)), nil)
+				})
+
+				It("names the file it could not parse", func() {
+					Expect(fetchErr).To(MatchError(ContainSubstring(artifactName + "/build.yml")))
+				})
+			})
+		})
 	})
 
 	Context("BaseResourceTypeDefaultsApplySource", func() {

atc/exec/task_step.go

@@ -169,13 +169,18 @@ func (step *TaskStep) run(ctx context.Context, state RunState, delegate TaskDele
 		taskConfigSource = FileConfigSource{ConfigPath: step.plan.ConfigPath, Streamer: step.streamer}
 
 		// for interpolation - use 'vars' from the pipeline, and then fill remaining with cred variables.
-		// this 2-phase strategy allows to interpolate 'vars' by cred variables.
+		// this pass reads the file as written, before it is parsed, so a var may
+		// stand in for a field that is not a string, such as container_limits.
+		configVars := []vars.Variables{}
 		if len(step.plan.Vars) > 0 {
-			taskConfigSource = InterpolateTemplateConfigSource{
+			configVars = append(configVars, vars.StaticVariables(step.plan.Vars))
-				ConfigSource:  taskConfigSource,
+		}
-				Vars:          []vars.Variables{vars.StaticVariables(step.plan.Vars)},
+		configVars = append(configVars, state)
-				ExpectAllKeys: false,
+
-			}
+		taskConfigSource = InterpolateTemplateConfigSource{
+			ConfigSource:  taskConfigSource,
+			Vars:          configVars,
+			ExpectAllKeys: false,
 		}
 		taskVars = []vars.Variables{state}
 	} else {

atc/exec/task_step_test.go

@@ -1185,4 +1185,34 @@ var _ = Describe("TaskStep", func() {
 			})
 		})
 	})
+
+	Context("when the plan has a config path whose container_limits use vars", func() {
+		BeforeEach(func() {
+			state = exec.NewRunState(noopStepper, vars.StaticVariables{
+				"cpu-limit": 512,
+				"mem-limit": "5gb",
+			})
+			repo = state.ArtifactRepository()
+			repo.RegisterArtifact("some-artifact", runtimetest.NewVolume("some-volume"), false)
+
+			fakeStreamer.StreamFileReturns(gbytes.BufferWithBytes([]byte(`
+platform: some-platform
+
+container_limits:
+  cpu: ((cpu-limit))
+  memory: ((mem-limit))
+
+run: {path: ls}
+`)), nil)
+
+			taskPlan.ConfigPath = "some-artifact/task.yml"
+
+			fakePool = new(execfakes.FakePool)
+			fakePool.FindOrSelectWorkerReturns(nil, errors.New("no worker for you"))
+		})
+
+		It("interpolates the file before it is parsed, so the step gets as far as picking a worker", func() {
+			Expect(stepErr).To(MatchError(ContainSubstring("no worker for you")))
+		})
+	})
 })