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

Add ECS Metadata Integration Test for detect changes in ECS Container Agent Metadata Endpoint #458

Merged
merged 52 commits into from
May 6, 2022

Conversation

khanhntd
Copy link
Contributor

@khanhntd khanhntd commented May 2, 2022

Fixed #118 and continue of #453

Description of the issue

Adding an integration test which detect the changes in scrapping metadata URL when Container Agent Version changes.

Description of changes

Able to detect unnoticed changes in URL with ECS Fargate or EC2.

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Requirements

Before commit the code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make linter

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022

Codecov Report

Merging #458 (ada9cd7) into master (c4e07d7) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #458   +/-   ##
=======================================
  Coverage   57.28%   57.28%           
=======================================
  Files         370      370           
  Lines       17346    17352    +6     
=======================================
+ Hits         9936     9940    +4     
- Misses       6824     6826    +2     
  Partials      586      586           
Impacted Files Coverage Δ
translator/totomlconfig/toTomlConfig.go 72.72% <0.00%> (ø)
.../logs_collected/files/collect_list/collect_list.go 91.30% <ø> (ø)
...llected/windows_events/collect_list/collectlist.go 85.00% <ø> (ø)
plugins/outputs/cloudwatch/cloudwatch.go 73.90% <0.00%> (-0.47%) ⬇️
...ogs_collected/files/collect_list/ruleLogFilters.go 100.00% <0.00%> (ø)
...ollected/files/collect_list/ruleTimestampFormat.go 95.52% <0.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4e07d7...ada9cd7. Read the comment docs.

@khanhntd khanhntd changed the title Ecs metadata Add ECS Metadata Integration Test for detect changes in ECS Container Agent Metadata Endpoint May 2, 2022
@khanhntd khanhntd marked this pull request as ready for review May 2, 2022 15:58
@khanhntd khanhntd requested a review from a team as a code owner May 2, 2022 15:58
Makefile Outdated Show resolved Hide resolved
mac: {},
"ec2_windows": {""},
"ec2_mac": {},
"ecs_fargate": {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to run this from the root dir so we can use ./

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree at some point. However, it would pose a different question? Would the developers prefer to look at the root dir or from the current path since for ECS Fargate test, it use

resource "null_resource" "validator" {
  provisioner "local-exec" {
    command = <<-EOT
    EOT
  }
}

instead of like EC2

resource "aws_instance" "integration-test" {
  provisioner "remote-exec" {
}

Therefore, would prefer this path over root_dir. One more addition point to support mine, since I use reduce the variables by using local, so it would be also a pain for using the root dir if don't do something to the root dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer using root.

Copy link
Contributor Author

@khanhntd khanhntd May 4, 2022

Choose a reason for hiding this comment

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

If you guys prefer root module, then its fine by me also. Fixed it in the newest commit.

Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,57 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

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

For the ECS set up can you write a README for what it all does? It's not easy for me to gleam without ECS experience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasonable. I added how ECS Fargate is setup in the README.doc. Let's me know if its cover enough details for everyone.

mac: {},
"ec2_windows": {""},
"ec2_mac": {},
"ecs_fargate": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer using root.

"github.com/aws/amazon-cloudwatch-agent/integration/test"
)

// Purpose: Detect the changes in metadata endpoint for ECS Container Agent https://github.com/aws/amazon-cloudwatch-agent/blob/master/translator/util/ecsutil/ecsutil.go#L67-L75
Copy link
Contributor

Choose a reason for hiding this comment

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

How does verifying the existence of the log group indicate that the v4 metadata endpoint worked? Does this imply that if you use an ECS cluster that doesn't have the v4 endpoint, the log group wouldn't be created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more specifically a bit, during the scrapping metadata endpoint, it would extract the Region, ClusterName, TaskARN. After extracting those variables, it would create a log group based on that format that includes the ClusterName. So to speak, if the clustername which scrapped from metadataendpointv4 does not exist, then it would fail the test. So to your point, yes. Also if the ECS cluster does have the v5 endpoint but we have not updated that, then it would fail the test based on the aforementioned points.

Copy link
Contributor

@SaxyPandaBear SaxyPandaBear left a comment

Choose a reason for hiding this comment

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

lgtm

@khanhntd khanhntd merged commit be2a6bd into aws:master May 6, 2022
@khanhntd khanhntd deleted the ecs_metadata branch May 6, 2022 13:41
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.

Fargate instances incorrectly detected as OnPrem
4 participants