Clean up code and add tests

aws · May 18, 2023 · f95a0cc · f95a0cc
1 parent 10002ef
commit f95a0cc
Show file tree

Hide file tree

Showing 3 changed files with 316 additions and 152 deletions.
diff --git a/agent/api/task/task.go b/agent/api/task/task.go
@@ -3443,6 +3443,10 @@ func (task *Task) IsServiceConnectEnabled() bool {
 	return task.GetServiceConnectContainer() != nil
 }
 
+func (task *Task) IsServiceConnectBridgeModeApplicationContainer(container *apicontainer.Container) bool {
+	return container.GetNetworkModeFromHostConfig() == "container" && task.IsServiceConnectEnabled()
+}
+
 // PopulateServiceConnectContainerMappingEnvVar populates APPNET_CONTAINER_IP_MAPPING env var for AppNet Agent container
 // aka SC container
 func (task *Task) PopulateServiceConnectContainerMappingEnvVar() error {

diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go
@@ -1385,11 +1385,19 @@ func (engine *DockerTaskEngine) createContainer(task *apitask.Task, container *a
 				var err error
 				targetContainer, err = task.GetBridgeModePauseContainerForTaskContainer(targetContainer)
 				if err != nil {
+					logger.Error("Failed to create container", logger.Fields{
+						field.TaskID:    task.GetID(),
+						field.Container: container.Name,
+						field.Error: errors.New(fmt.Sprintf(
+							"container uses awsfirelens log driver but we failed to resolve Firelens bridge IP: %v", err)),
+					})
 					return dockerapi.DockerContainerMetadata{
-						Error: dockerapi.CannotStartContainerError{FromError: errors.New(fmt.Sprintf(
-							"failed to start firelens container: %v", err))},
+						Error: dockerapi.CannotCreateContainerError{FromError: errors.New(fmt.Sprintf(
+							"failed to create container - container uses awsfirelens log driver but we failed to "+
+								"resolve Firelens bridge IP: %v", err))},
 					}
 				}
+				logger.Info(fmt.Sprintf("successfully resolved firelens pause container in bridge SC task"))
 			}
 			ipAddress, ok := getContainerHostIP(targetContainer.GetNetworkSettings())
 			if !ok {
@@ -1647,60 +1655,64 @@ func (engine *DockerTaskEngine) startContainer(task *apitask.Task, container *ap
 	// For bridge-mode ServiceConnect-enabled tasks, we inject pause container for each application container
 	// including the firelens container. Therefore, when resolving the container IP, we should be checking that
 	// of the associated pause container. In such case, the firelens container has network mode "container" since it's
-	//launched into its pause container's network namespace.
-	if container.GetFirelensConfig() != nil {
-		if !task.IsNetworkModeAWSVPC() &&
-			(container.GetNetworkModeFromHostConfig() == "" ||
-				container.GetNetworkModeFromHostConfig() == apitask.BridgeNetworkMode ||
-				(container.GetNetworkModeFromHostConfig() == "container" && task.IsServiceConnectEnabled())) {
-			_, gotContainerIP := getContainerHostIP(dockerContainerMD.NetworkSettings)
-			if task.IsServiceConnectEnabled() {
-				targetContainer, err := task.GetBridgeModePauseContainerForTaskContainer(container)
-				if err != nil {
-					return dockerapi.DockerContainerMetadata{
-						Error: dockerapi.CannotStartContainerError{FromError: errors.New(fmt.Sprintf(
-							"failed to start firelens container: %v", err))},
-					}
+	// launched into its pause container's network namespace.
+	if container.GetFirelensConfig() != nil && task.IsNetworkModeBridge() {
+		_, gotContainerIP := getContainerHostIP(dockerContainerMD.NetworkSettings)
+		if task.IsServiceConnectEnabled() {
+			targetContainer, err := task.GetBridgeModePauseContainerForTaskContainer(container)
+			if err != nil {
+				logger.Error("Failed to start Firelens container", logger.Fields{
+					field.TaskID:    task.GetID(),
+					field.Container: container.Name,
+					field.Error:     err,
+				})
+				return dockerapi.DockerContainerMetadata{
+					Error: dockerapi.CannotStartContainerError{FromError: errors.New(fmt.Sprintf(
+						"failed to start firelens container: %v", err))},
 				}
-				_, gotContainerIP = getContainerHostIP(targetContainer.GetNetworkSettings())
 			}
-
-			if !gotContainerIP {
-				getIPBridgeBackoff := retry.NewExponentialBackoff(minGetIPBridgeTimeout, maxGetIPBridgeTimeout, getIPBridgeRetryJitterMultiplier, getIPBridgeRetryDelayMultiplier)
-				contextWithTimeout, cancel := context.WithTimeout(engine.ctx, time.Minute)
-				defer cancel()
-				err := retry.RetryWithBackoffCtx(contextWithTimeout, getIPBridgeBackoff, func() error {
-					gotIPBridge := false
-					if task.IsServiceConnectEnabled() {
-						targetContainer, err := task.GetBridgeModePauseContainerForTaskContainer(container)
-						if err != nil {
-							return err
-						}
-						_, gotIPBridge = getContainerHostIP(targetContainer.GetNetworkSettings())
-						if gotIPBridge {
-							return nil
-						}
-					} else {
-						inspectOutput, err := engine.client.InspectContainer(engine.ctx, dockerContainerMD.DockerID,
-							dockerclient.InspectContainerTimeout)
-						if err != nil {
-							return err
-						}
-						_, gotIPBridge = getContainerHostIP(inspectOutput.NetworkSettings)
-						if gotIPBridge {
-							dockerContainerMD.NetworkSettings = inspectOutput.NetworkSettings
-							return nil
-						}
+			_, gotContainerIP = getContainerHostIP(targetContainer.GetNetworkSettings())
+		}
+
+		if !gotContainerIP {
+			getIPBridgeBackoff := retry.NewExponentialBackoff(minGetIPBridgeTimeout, maxGetIPBridgeTimeout, getIPBridgeRetryJitterMultiplier, getIPBridgeRetryDelayMultiplier)
+			contextWithTimeout, cancel := context.WithTimeout(engine.ctx, time.Minute)
+			defer cancel()
+			err := retry.RetryWithBackoffCtx(contextWithTimeout, getIPBridgeBackoff, func() error {
+				gotIPBridge := false
+				if task.IsServiceConnectEnabled() {
+					targetContainer, err := task.GetBridgeModePauseContainerForTaskContainer(container)
+					if err != nil {
+						return err
 					}
-					return errors.New("Bridge IP not available to use for firelens")
-				})
-				if err != nil {
-					return dockerapi.DockerContainerMetadata{
-						Error: dockerapi.CannotStartContainerError{FromError: err},
+					_, gotIPBridge = getContainerHostIP(targetContainer.GetNetworkSettings())
+					if gotIPBridge {
+						return nil
+					}
+				} else {
+					inspectOutput, err := engine.client.InspectContainer(engine.ctx, dockerContainerMD.DockerID,
+						dockerclient.InspectContainerTimeout)
+					if err != nil {
+						return err
 					}
+					_, gotIPBridge = getContainerHostIP(inspectOutput.NetworkSettings)
+					if gotIPBridge {
+						dockerContainerMD.NetworkSettings = inspectOutput.NetworkSettings
+						return nil
+					}
+				}
+				return errors.New("Bridge IP not available to use for firelens")
+			})
+			if err != nil {
+				logger.Error("Failed to start Firelens container", logger.Fields{
+					field.TaskID:    task.GetID(),
+					field.Container: container.Name,
+					field.Error:     err,
+				})
+				return dockerapi.DockerContainerMetadata{
+					Error: dockerapi.CannotStartContainerError{FromError: err},
 				}
 			}
-
 		}
 	}
 	if execcmd.IsExecEnabledContainer(container) {