Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CONTINT-4460] fix ecs fargate to use env specified agent image #1171

Conversation

ewoodthomas
Copy link
Contributor

@ewoodthomas ewoodthomas commented Oct 8, 2024

What does this PR do?

Sets the agent image for Fargate tasks to the specified environment image tag rather than using public.ecr.aws/datadog/agent:latest

Which scenarios this will impact?

TestECSSuite should now properly test agent changes on Fargate. Prior to these changes, the latest published image tag was used on Fargate.

Motivation

Discovered this issue when adding an aws_account tag to ECS Fargate tasks. I added an expected tag to ecs_test.go, but TestECSSuite failed since the tag did not exist . The CI used the image pushed in the PR when defining the tests, but the Fargate tasks used the latest agent image tag published to the official Datadog ECR.

Additional Notes

Testing

Prior to changes; fargate task definition:

{
    "taskDefinitionArn": "arn:aws:ecs:us-east-1:376334461865:task-definition/ewoodthomas-ecs-cluster-nginx-fg:22",
    "containerDefinitions": [
        {
            "name": "datadog-agent",
            "image": "public.ecr.aws/datadog/agent:latest",
	    ...
	}
}

After changes; fargate task definition:

{
    "taskDefinitionArn": "arn:aws:ecs:us-east-1:376334461865:task-definition/ewoodthomas-ecs-cluster-nginx-fg:26",
    "containerDefinitions": [
        {
            "name": "datadog-agent",
            "image": "376334461865.dkr.ecr.us-east-1.amazonaws.com/agent-e2e-tests:ethan.woodthomas-agent",
            ...
        }
}

When no agent tag is specified, it still uses public.ecr.aws/datadog/agent:latest in the task definition.

@ewoodthomas ewoodthomas marked this pull request as ready for review October 9, 2024 18:42
@ewoodthomas ewoodthomas requested a review from a team as a code owner October 9, 2024 18:42
return &ecs.TaskDefinitionContainerDefinitionArgs{
Cpu: pulumi.IntPtr(0),
Name: pulumi.String("datadog-agent"),
Image: pulumi.String(image),
Image: pulumi.String(dockerAgentFullImagePath(e, "public.ecr.aws/datadog/agent", "latest", false)),
Copy link
Contributor

@pducolin pducolin Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔨 warning
Given dockerAgentFullImagePath implementation

func dockerAgentFullImagePath(e config.Env, repositoryPath, imageTag string, otel bool) string {
	if e.AgentFullImagePath() != "" {
		return e.AgentFullImagePath()
	}

	// if agent pipeline id and commit sha are defined, use the image from the pipeline pushed on agent QA registry
	if e.PipelineID() != "" && e.CommitSHA() != "" && imageTag == "" {
		tag := fmt.Sprintf("%s-%s", e.PipelineID(), e.CommitSHA())
		if otel {
			tag = fmt.Sprintf("%s-7-ot-beta", tag)
		}

		exists, err := e.InternalRegistryImageTagExists(fmt.Sprintf("%s/agent", e.InternalRegistry()), tag)
		if err != nil || !exists {
			panic(fmt.Sprintf("image %s/agent:%s not found in the internal registry", e.InternalRegistry(), tag))
		}
		return utils.BuildDockerImagePath(fmt.Sprintf("%s/agent", e.InternalRegistry()), tag)
	}

	...

and given we pass pipeline id and commit sha from the CI config in datadog-agent, I expect this to still install the latest agent version from "public.ecr.aws/datadog/agent" as imageTag is not empty.

In order to install the pipeline version and fallback to latest by default, I expect this to be

Suggested change
Image: pulumi.String(dockerAgentFullImagePath(e, "public.ecr.aws/datadog/agent", "latest", false)),
Image: pulumi.String(dockerAgentFullImagePath(e, "", "", false)),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how dockerAgentFullImagePath can be confusing for the caller. Probably worth renaming it and add more context in the function description, will track this on our side so that we can follow up

return &ecs.TaskDefinitionContainerDefinitionArgs{
Cpu: pulumi.IntPtr(0),
Name: pulumi.String("datadog-agent"),
Image: pulumi.String(image),
Image: pulumi.String(dockerAgentFullImagePath(e, "public.ecr.aws/datadog/agent", "latest", false)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔨 warning
See my previous comment for the context of this suggestion:

Suggested change
Image: pulumi.String(dockerAgentFullImagePath(e, "public.ecr.aws/datadog/agent", "latest", false)),
Image: pulumi.String(dockerAgentFullImagePath(e, "", "", false)),

@pducolin
Copy link
Contributor

Nice catch, I still expect this to use latest, more details at #1171 (comment)

@ewoodthomas ewoodthomas merged commit 5c3ffa4 into main Oct 15, 2024
8 checks passed
@ewoodthomas ewoodthomas deleted the ewoodthomas/CONTINT-4460_fix_ecs_fargate_to_use_env_specified_agent_image branch October 15, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants