diff --git a/atc/exec/task_config_source.go b/atc/exec/task_config_source.go index b63de8b..0b85ac3 100644 --- a/atc/exec/task_config_source.go +++ b/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 atc.TaskConfig{}, err - } + return byteConfig, configSource.ConfigPath, 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) - if err != nil { - return atc.TaskConfig{}, fmt.Errorf("failed to create task config from bytes: %s", err) + return newTaskConfig(byteConfig, from) +} + +// 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 { diff --git a/atc/exec/task_config_source_test.go b/atc/exec/task_config_source_test.go index 4ba3c9a..98cf516 100644 --- a/atc/exec/task_config_source_test.go +++ b/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() { diff --git a/atc/exec/task_step.go b/atc/exec/task_step.go index 3014520..dbc10b2 100644 --- a/atc/exec/task_step.go +++ b/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{ - ConfigSource: taskConfigSource, - Vars: []vars.Variables{vars.StaticVariables(step.plan.Vars)}, - ExpectAllKeys: false, - } + configVars = append(configVars, vars.StaticVariables(step.plan.Vars)) + } + configVars = append(configVars, state) + + taskConfigSource = InterpolateTemplateConfigSource{ + ConfigSource: taskConfigSource, + Vars: configVars, + ExpectAllKeys: false, } taskVars = []vars.Variables{state} } else { diff --git a/atc/exec/task_step_test.go b/atc/exec/task_step_test.go index b599624..1427f1c 100644 --- a/atc/exec/task_step_test.go +++ b/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"))) + }) + }) })