create envfile task resource #2398

jy19 · 2020-03-17T15:55:59Z

Summary

this commit initializes the environment files resource when task starts up, and reads the environment variables from those files when containers are created

note: i am getting this PR out to get eyes on it, there are additional unit tests that still need to be added.

Implementation details

When task starts up -> containers are checked to have environment files. if so, envfile resource is created. after files are retrieved (done in previous PR), when containers are created, those retrieved files are read and those environment variables are merged into a container's existing environment variables.

Testing

New tests cover the changes: yes
make test is successful

Description for the changelog

create envfile task resource on task start up as needed

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

this commit initializes the environment files resource when task starts up, and reads the environment variables from those files when containers are created

jy19 · 2020-03-17T16:29:32Z

gocyclo fails on task (*Task).PostUnmarshalTask

agent/api/task/task.go

agent/taskresource/envFiles/envfile.go

yhlee-aws · 2020-03-17T21:32:02Z

agent/api/container/container.go

+	// envVarsList is a list of map, where each map is from an envfile
+	// iterate over this sequentially because the original order of the
+	// environment files give precedence to the environment variables
+	for _, envVarsIn := range envVarsList {


any repeated variable in more than one env file needs to be resolved by precedence as well, in the order as defined in the "environmentFiles" list. I don't see how that's handled here?

Looks like the envVarsList already merge all env var from all files together. I think it would be easier to add the logic to handle repeated variable in the ReadEnvVarsFromEnvfiles when creating the envVarsList.

the original thought is that envVarsList is a list that follows the original order of what was entered in the task def, so if an env var already exists in the map, we don't add to it because whatever was entered into the map first has precedence (just based on the list order). however, i add to the list of downloaded paths with a go routine so i need to double check if that's still the case..

yhlee-aws · 2020-03-18T00:15:44Z

agent/taskresource/envFiles/envfile.go

+		}
+		// only read the line that has "="
+		if strings.Contains(line, envVariableDelimiter) {
+			variables := strings.Split(line, "=")


this will read a line that has "=" anywhere, e.g. strings.Split("=xyz ", "=") returns ["", "xyz"]. Im inclined to use regex or any other validation that makes sure either side of = is at least length of one.

we decided not to add any agent specific validations, so at least we should understand what happens when invalid line is passed to docker.

good point - and is the existing logic to ignore any lines that is a comment or doesn't match the regex ok?

im ok with that

instead of regex i added a super simple "if both sides length > 0" lmk if you have any concerns, i'm open to adding it as regex

agent/api/task/task.go

yumex93 · 2020-03-18T19:20:57Z

agent/api/task/task.go

+	for _, container := range task.Containers {
+		envfileResource, err := envFiles.NewEnvironmentFileResource(config.Cluster, task.Arn, config.AWSRegion, config.DataDir,
+			container.EnvironmentFiles, credentialsManager, task.ExecutionCredentialsID)
+		if err != nil {


Since each container has their own task resource, you may want to have container name or other container identifier in the error message so people can understand which one is causing the error.

good point - also added check for containers having env files before creating a resource. instead of checking all containers and then checking again.

yumex93 · 2020-03-18T19:35:37Z

agent/engine/docker_task_engine.go

@@ -1018,6 +1018,14 @@ func (engine *DockerTaskEngine) createContainer(task *apitask.Task, container *a
 		}
 	}

+	if container.ShouldCreateWithEnvFiles() {
+		err := task.ReadEnvVarsFromEnvfiles(container)


I feel it would be better for the name to be MergeEnvVarsFromEnvFiles, it makes me confusing for a second that where you merge the env vars until I read the function logic.

We don't have requirements for the order between secrets and the env file, right?

i think the requirement is just that all existing env vars (in this case secret as well) have precendence - though it's highly unlikely that someone will create a secret but then also store it as plaintext in s3..or is it 🤔

also renamed the method.

agent/taskresource/envFiles/envfile.go

agent/api/container/container.go

fenxiong

mostly nits except for the one regarding environmentFileResourceJSON

agent/api/container/container.go

agent/api/task/task.go

agent/taskresource/envFiles/envfile_test.go

agent/taskresource/envFiles/envfile.go

agent/api/task/task.go

yumex93 · 2020-03-20T23:08:34Z

agent/taskresource/envFiles/envfile_test.go


 	mockOS.EXPECT().RemoveAll(resourceDir).Return(errors.New("error response"))

 	assert.Error(t, envfileResource.Cleanup())
 }
+
+func TestReadEnvVarsFromEnvfiles(t *testing.T) {


Should we change this test to read from multiple env files to verify the env vars with same key are overriden correctly?

i can add an additional test for this use case.

wait this is already tested in container_test.go
this method wouldn't actually override any envvars, but return everything in the file

yumex93

You can merge your pr once you resolve the comments I left.

…nto env_files

jy19 added 2 commits March 11, 2020 14:28

retrieve envfiles from s3 concurrently using go routines

aacd728

read environment variables from downloaded envfiles

e72c799

this commit initializes the environment files resource when task starts up, and reads the environment variables from those files when containers are created

jy19 requested review from yhlee-aws, fenxiong and yumex93 March 17, 2020 15:55

fenxiong reviewed Mar 17, 2020

View reviewed changes

yhlee-aws reviewed Mar 17, 2020

View reviewed changes

yhlee-aws reviewed Mar 18, 2020

View reviewed changes

yumex93 reviewed Mar 18, 2020

View reviewed changes

agent/api/task/task.go Outdated Show resolved Hide resolved

yumex93 reviewed Mar 18, 2020

View reviewed changes

agent/taskresource/envFiles/envfile.go Outdated Show resolved Hide resolved

jy19 force-pushed the env_files branch from b634f8a to 7ec8ed6 Compare March 19, 2020 19:53

yumex93 reviewed Mar 19, 2020

View reviewed changes

agent/api/container/container.go Outdated Show resolved Hide resolved

jy19 force-pushed the env_files branch from 7ec8ed6 to fa9af18 Compare March 20, 2020 00:08

fenxiong reviewed Mar 20, 2020

View reviewed changes

jy19 force-pushed the env_files branch from fa9af18 to 8ab0aff Compare March 20, 2020 19:53

yumex93 reviewed Mar 20, 2020

View reviewed changes

agent/api/task/task.go Outdated Show resolved Hide resolved

yumex93 reviewed Mar 20, 2020

View reviewed changes

agent/api/task/task.go Outdated Show resolved Hide resolved

fenxiong approved these changes Mar 20, 2020

View reviewed changes

agent/api/task/task.go Show resolved Hide resolved

yumex93 reviewed Mar 20, 2020

View reviewed changes

yumex93 approved these changes Mar 20, 2020

View reviewed changes

jy19 force-pushed the env_files branch from 8ab0aff to e044b70 Compare March 21, 2020 00:18

Merge branch 'env_files' of https://github.com/aws/amazon-ecs-agent i…

c8ab428

…nto env_files

jy19 force-pushed the env_files branch from e044b70 to c8ab428 Compare March 21, 2020 00:20

jy19 merged commit c32a89f into aws:env_files Mar 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create envfile task resource #2398

create envfile task resource #2398

jy19 commented Mar 17, 2020

jy19 commented Mar 17, 2020

yhlee-aws Mar 17, 2020

yumex93 Mar 18, 2020 •

edited

Loading

jy19 Mar 18, 2020

yhlee-aws Mar 18, 2020

yhlee-aws Mar 18, 2020

jy19 Mar 18, 2020

yhlee-aws Mar 20, 2020

jy19 Mar 20, 2020

yumex93 Mar 18, 2020

jy19 Mar 18, 2020

yumex93 Mar 18, 2020

yumex93 Mar 18, 2020 •

edited

Loading

jy19 Mar 18, 2020

jy19 Mar 18, 2020

fenxiong left a comment

yumex93 Mar 20, 2020

jy19 Mar 20, 2020

jy19 Mar 21, 2020

yumex93 left a comment

create envfile task resource #2398

create envfile task resource #2398

Conversation

jy19 commented Mar 17, 2020

Summary

Implementation details

Testing

Description for the changelog

Licensing

jy19 commented Mar 17, 2020

Choose a reason for hiding this comment

yumex93 Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yumex93 Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenxiong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yumex93 left a comment

Choose a reason for hiding this comment

yumex93 Mar 18, 2020 •

edited

Loading

yumex93 Mar 18, 2020 •

edited

Loading