Add log driver secret to LogConfig and agent capability #1818

tommyhahn · 2019-01-29T18:52:29Z

Summary

Add the secret value of Splunk logging driver into the Container's hostconfig section and add the agent capability for secret that supports Splunk

Implementation details

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=30s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

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

samuelkarp · 2019-01-29T18:56:37Z

agent/api/task/task.go

@@ -900,6 +903,27 @@ func (task *Task) ApplyExecutionRoleLogsAuth(hostConfig *dockercontainer.HostCon
 	return nil
 }

+//ApplyLogDriverSecret will add the generated splunk secret into the associated HostConfig


There's nothing specific to Splunk here.

samuelkarp · 2019-01-29T18:57:23Z

agent/api/task/task.go

+		container.MergeEnvironmentVariables(envVars)
+	}
+
+	//Check if the "splunk-token" has already been initialized and stored in the LogCofig map


We should not include logic specific to Splunk or keys like "splunk-token". If we need logic like this, it should be handled in the ECS backend so we do not need to keep updating the agent.

I agree - this was discussed during design phase, to not cater implementation to one specific log driver.

Figured out a way. Will discuss and update this pr shortly.

agent/engine/docker_task_engine.go

suneyz · 2019-01-29T22:25:18Z

agent/app/agent_capability.go

@@ -39,6 +39,8 @@ const (
 	capabilityPrivateRegistryAuthASM            = "private-registry-authentication.secretsmanager"
 	capabilitySecretEnvSSM                      = "secrets.ssm.environment-variables"
 	capabilitySecretEnvASM                      = "secrets.asm.environment-variables"
+	capabilitySecretLogDriverSSM				= "secrets.ssm.logging-drivers"
+	capabilitySecretLogDriverASM				= "secrets.asm.logging-drivers"


nit: run go fmt to align this with other variables

agent/api/task/task.go

suneyz · 2019-01-29T22:29:37Z

agent/api/container/container.go

@@ -261,6 +266,7 @@ type Secret struct {
 	ContainerPath string `json:"containerPath"`
 	Type          string `json:"type"`
 	Provider      string `json:"provider"`
+	Target 		  string `json:"target"`


nit run go fmt

suneyz

Out of curiosity, are you planning to follow up with functional tests as separate PR?

sharanyad · 2019-02-05T22:38:24Z

agent/api/container/container.go

+
+// MergeLogDrivers appends additional pairs to
+// the the container's Logging Drivers structure
+func (c *Container) HasSecretAsEvnAndLogDriver() bool {


Think it should beHasSecretAsEnvOrLogDriver. either way, just calling it HasSecrets or HasSecretsAvailable might be better. also the comment needs to be changed.

Will change it to HasSecretAsEnvOrLogDriver at this time. It is specifically for checking env and log driver secrets.

sharanyad · 2019-02-05T22:38:50Z

agent/api/container/container.go

@@ -807,7 +813,17 @@ func (c *Container) MergeEnvironmentVariables(envVars map[string]string) {
 	}
 }

-func (c *Container) HasSecretAsEnv() bool {
+//Append the logging driver using secret into the container's LogConfig


nit: space after //

sharanyad · 2019-02-05T22:50:15Z

agent/api/task/task.go

-// PopulateSecretsAsEnv appends the container's env var map with secrets
-func (task *Task) PopulateSecretsAsEnv(container *apicontainer.Container) *apierrors.DockerClientConfigError {
+// PopulateSecretsAsEnvAndLogDriver appends the container's env var map with secrets
+func (task *Task) PopulateSecretsAsEnvAndLogDriver(hostConfig *dockercontainer.HostConfig, container *apicontainer.Container) *apierrors.DockerClientConfigError {


Can we just call it PopulateSecrets so we that don't have to change the name again when new secret type is added?

sharanyad · 2019-02-05T22:55:11Z

agent/api/container/container.go

@@ -63,6 +65,9 @@ const (

 	// SecretTypeEnv is to show secret type being ENVIRONMENT_VARIABLE
 	SecretTypeEnv = "ENVIRONMENT_VARIABLE"
+
+	// TargetLogDriver is to show secret target being "LOG_DRIVER", the default will be "CONTAINER"


where is the default being set?

CP will set the target of those env secrets as a default "container" but don't think it will be used somewhere here since they've already had a type field "ENVIRONMENT_VARIABLE"...

samuelkarp · 2019-02-05T23:31:00Z

Since this code is no longer specific to Splunk, can we remove Splunk from the commit message and the PR title?

Splunk concern addressed

fenxiong · 2019-02-08T01:33:39Z

agent/api/container/container.go

+	hostConfig.LogConfig.Config[logDriverSecretName] = logDriverSecretValue
+}
+
+func (c *Container) HasSecretAsEvnOrLogDriver() bool {


fenxiong · 2019-02-08T01:35:14Z

agent/api/container/container.go

@@ -807,7 +813,15 @@ func (c *Container) MergeEnvironmentVariables(envVars map[string]string) {
 	}
 }

-func (c *Container) HasSecretAsEnv() bool {
+// Append the logging driver using secret into the container's LogConfig
+func (c *Container) ApplyLogDriverSecret(hostConfig *dockercontainer.HostConfig, logDriverSecretName string, logDriverSecretValue string) {


i don't think this should be a method of container.. the container is not used. i think this can just be a helper function put below its caller

fenxiong · 2019-02-08T01:39:40Z

agent/api/task/task.go

+			continue
+		}
+		if secret.Target == apicontainer.SecretTargetLogDriver {
+			logDriverTokenName = secret.Name


i think currently you assume there's only one log driver secret, but i don't see why you need to have that assumption.. i think you can call ApplyLogDriverSecret here whenever you find a log driver secret?

agent/api/task/task.go

fenxiong · 2019-02-08T01:49:11Z

agent/app/agent_capability.go

@@ -138,6 +145,9 @@ func (agent *ecsAgent) capabilities() ([]*ecs.Attribute, error) {
 	// ecs agent version 1.23.0 supports ecs secrets integrating with aws secrets manager
 	capabilities = appendNameOnlyAttribute(capabilities, attributePrefix+capabilitySecretEnvASM)

+	// ecs agent version 1.25.0 supports ecs secrets for logging drivers


probably 1.26.0?

fenxiong · 2019-02-12T08:14:54Z

agent/api/container/container.go

@@ -269,6 +272,7 @@ type Secret struct {
 	ContainerPath string `json:"containerPath"`
 	Type          string `json:"type"`
 	Provider      string `json:"provider"`
+	Target        string `json:"target"`


sorry that i forgot.. i think you need to bump state file version for this

tommyhahn · 2019-02-16T00:01:09Z

#1856
Flaky unit test TestStateManager issue

samuelkarp previously requested changes Jan 29, 2019

View reviewed changes

suneyz reviewed Jan 29, 2019

View reviewed changes

agent/engine/docker_task_engine.go Outdated Show resolved Hide resolved

suneyz reviewed Jan 29, 2019

View reviewed changes

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

suneyz reviewed Jan 29, 2019

View reviewed changes

tommyhahn force-pushed the branch_logging_driver branch 5 times, most recently from 0f10ed1 to 0d21068 Compare February 4, 2019 22:30

tommyhahn added bot/test and removed bot/test labels Feb 5, 2019

tommyhahn force-pushed the branch_logging_driver branch 2 times, most recently from a73b9ea to 5717f08 Compare February 5, 2019 20:47

sharanyad reviewed Feb 5, 2019

View reviewed changes

tommyhahn changed the title ~~Add Splunk secret to LogConfig and agent capability~~ Add log driver secret to LogConfig and agent capability Feb 6, 2019

tommyhahn force-pushed the branch_logging_driver branch 6 times, most recently from 4f13abe to 357723a Compare February 7, 2019 01:17

tommyhahn added bot/test and removed bot/test labels Feb 7, 2019

tommyhahn removed the bot/test label Feb 8, 2019

fenxiong reviewed Feb 8, 2019

View reviewed changes

tommyhahn force-pushed the branch_logging_driver branch 4 times, most recently from d0c39fc to 5f10985 Compare February 12, 2019 07:48

fenxiong approved these changes Feb 12, 2019

View reviewed changes

fenxiong reviewed Feb 12, 2019

View reviewed changes

tommyhahn force-pushed the branch_logging_driver branch from 5f10985 to 734963a Compare February 12, 2019 08:34

tommyhahn added bot/test and removed bot/test labels Feb 12, 2019

tommyhahn force-pushed the branch_logging_driver branch from 734963a to ce89c7f Compare February 14, 2019 01:10

suneyz approved these changes Feb 14, 2019

View reviewed changes

tommyhahn force-pushed the branch_logging_driver branch from ce89c7f to f709e9f Compare February 15, 2019 00:24

Add log driver secret to LogConfig and agent capability

4e818ef

tommyhahn force-pushed the branch_logging_driver branch from f709e9f to 4e818ef Compare February 15, 2019 21:44

tommyhahn merged commit e325804 into aws:dev Feb 18, 2019

tommyhahn deleted the branch_logging_driver branch February 18, 2019 21:33

tommyhahn restored the branch_logging_driver branch February 18, 2019 22:04

tommyhahn mentioned this pull request Feb 18, 2019

Revert "Add log driver secret to LogConfig and agent capability" due to pending test process #1865

Merged

tommyhahn deleted the branch_logging_driver branch February 21, 2019 01:06

tommyhahn mentioned this pull request Mar 21, 2019

Log driver secret support functional test #1952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log driver secret to LogConfig and agent capability #1818

Add log driver secret to LogConfig and agent capability #1818

tommyhahn commented Jan 29, 2019

samuelkarp Jan 29, 2019

samuelkarp Jan 29, 2019

yhlee-aws Jan 29, 2019

tommyhahn Jan 29, 2019

suneyz Jan 29, 2019

suneyz Jan 29, 2019

suneyz left a comment

sharanyad Feb 5, 2019

tommyhahn Feb 6, 2019

sharanyad Feb 5, 2019

sharanyad Feb 5, 2019

sharanyad Feb 5, 2019

tommyhahn Feb 5, 2019

samuelkarp commented Feb 5, 2019

fenxiong Feb 8, 2019

fenxiong Feb 8, 2019

fenxiong Feb 8, 2019

fenxiong Feb 8, 2019

fenxiong Feb 12, 2019

tommyhahn Feb 12, 2019

tommyhahn commented Feb 16, 2019

Add log driver secret to LogConfig and agent capability #1818

Add log driver secret to LogConfig and agent capability #1818

Conversation

tommyhahn commented Jan 29, 2019

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

suneyz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelkarp commented Feb 5, 2019

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

tommyhahn commented Feb 16, 2019