d/aws_ssm_instances #23162

kamilturek · 2022-02-13T18:51:03Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Output from acceptance testing:

$ make testacc PKG_NAME=internal/service/ssm TESTARGS="-run=TestAccSSMInstancesDataSource_filter"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssm/... -v -count 1 -parallel 20  -run=TestAccSSMInstancesDataSource_filter -timeout 180m
=== RUN   TestAccSSMInstancesDataSource_filter
=== PAUSE TestAccSSMInstancesDataSource_filter
=== CONT  TestAccSSMInstancesDataSource_filter
--- PASS: TestAccSSMInstancesDataSource_filter (170.09s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ssm        171.736s

ewbankkit · 2022-02-14T12:54:05Z

@kamilturek The code changes LGTM but the acceptance tests continually fail:

% make testacc TESTS=TestAccSSMInstancesDataSource_ PKG=ssm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssm/... -v -count 1 -parallel 20 -run='TestAccSSMInstancesDataSource_'  -timeout 180m
=== RUN   TestAccSSMInstancesDataSource_filter
=== PAUSE TestAccSSMInstancesDataSource_filter
=== CONT  TestAccSSMInstancesDataSource_filter
    instances_data_source_test.go:18: Step 1/1 error: Check failed: 2 errors occurred:
        	* Check 1/2 error: data.aws_ssm_instances.test: Attribute 'ids.#' expected "1", got "0"
        	* Check 2/2 error: data.aws_ssm_instances.test: Attribute "ids.0" not set, but "id" is set in aws_instance.test as "i-08cceda591b59bee6"
        
--- FAIL: TestAccSSMInstancesDataSource_filter (133.40s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-aws/internal/service/ssm	138.191s
FAIL
make: *** [testacc] Error 1

Do you think this could be a timing issue - the SSM agent on the EC2 instance hasn't registered at the time the DescribeInstanceInformation API call is made?

kamilturek · 2022-02-14T21:06:31Z

@ewbankkit Thanks for reviewing the PR! Indeed, I believe the failing test is a result of a timing issue. It passes on my machine, but I guess it's because the test takes longer to execute on my side (171 s vs 138 s on yours) and the delay is sufficient for the instance to register itself in SSM.

I'd think about increasing a time period before the check by moving it to a separate step:

diff --git a/internal/service/ssm/instances_data_source_test.go b/internal/service/ssm/instances_data_source_test.go
index ebc86bfeab..a2cfc945ea 100644
--- a/internal/service/ssm/instances_data_source_test.go
+++ b/internal/service/ssm/instances_data_source_test.go
@@ -21,7 +21,10 @@ func TestAccSSMInstancesDataSource_filter(t *testing.T) {
 		Providers:  acctest.Providers,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccCheckInstancesDataSourceConfig_filter(rName),
+				Config: testAccCheckInstancesDataSourceConfig_filter_instance(rName),
+			},
+			{
+				Config: testAccCheckInstancesDataSourceConfig_filter_dataSource(rName),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr(dataSourceName, "ids.#", "1"),
 					resource.TestCheckResourceAttrPair(dataSourceName, "ids.0", resourceName, "id"),
@@ -31,7 +34,7 @@ func TestAccSSMInstancesDataSource_filter(t *testing.T) {
 	})
 }
 
-func testAccCheckInstancesDataSourceConfig_filter(rName string) string {
+func testAccCheckInstancesDataSourceConfig_filter_instance(rName string) string {
 	return acctest.ConfigCompose(
 		acctest.AvailableEC2InstanceTypeForRegion("t2.micro", "t3.micro"),
 		fmt.Sprintf(`
@@ -83,12 +86,18 @@ resource "aws_instance" "test" {
   instance_type        = data.aws_ec2_instance_type_offering.available.instance_type
   iam_instance_profile = aws_iam_instance_profile.test.name
 }
+`, rName))
+}
 
+func testAccCheckInstancesDataSourceConfig_filter_dataSource(rName string) string {
+	return acctest.ConfigCompose(
+		testAccCheckInstancesDataSourceConfig_filter_instance(rName),
+		`
 data "aws_ssm_instances" "test" {
   filter {
     name   = "InstanceIds"
     values = [aws_instance.test.id]
   }
 }
-`, rName))
+`)
 }

Or even an explicit sleep:

diff --git a/internal/service/ssm/instances_data_source_test.go b/internal/service/ssm/instances_data_source_test.go
index ebc86bfeab..ea4f2aec1e 100644
--- a/internal/service/ssm/instances_data_source_test.go
+++ b/internal/service/ssm/instances_data_source_test.go
@@ -3,10 +3,12 @@ package ssm_test
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/aws/aws-sdk-go/service/ssm"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
 	"github.com/hashicorp/terraform-provider-aws/internal/acctest"
 )
 
@@ -15,14 +17,25 @@ func TestAccSSMInstancesDataSource_filter(t *testing.T) {
 	dataSourceName := "data.aws_ssm_instances.test"
 	resourceName := "aws_instance.test"
 
+	fulfillSleep := func() resource.TestCheckFunc {
+		return func(s *terraform.State) error {
+			time.Sleep(1 * time.Minute)
+			return nil
+		}
+	}
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:   func() { acctest.PreCheck(t) },
 		ErrorCheck: acctest.ErrorCheck(t, ssm.EndpointsID),
 		Providers:  acctest.Providers,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccCheckInstancesDataSourceConfig_filter(rName),
+				Config: testAccCheckInstancesDataSourceConfig_filter_instance(rName),
+			},
+			{
+				Config: testAccCheckInstancesDataSourceConfig_filter_dataSource(rName),
 				Check: resource.ComposeAggregateTestCheckFunc(
+					fulfillSleep(),
 					resource.TestCheckResourceAttr(dataSourceName, "ids.#", "1"),
 					resource.TestCheckResourceAttrPair(dataSourceName, "ids.0", resourceName, "id"),
 				),
@@ -31,7 +44,7 @@ func TestAccSSMInstancesDataSource_filter(t *testing.T) {
 	})
 }
 
-func testAccCheckInstancesDataSourceConfig_filter(rName string) string {
+func testAccCheckInstancesDataSourceConfig_filter_instance(rName string) string {
 	return acctest.ConfigCompose(
 		acctest.AvailableEC2InstanceTypeForRegion("t2.micro", "t3.micro"),
 		fmt.Sprintf(`
@@ -83,12 +96,18 @@ resource "aws_instance" "test" {
   instance_type        = data.aws_ec2_instance_type_offering.available.instance_type
   iam_instance_profile = aws_iam_instance_profile.test.name
 }
+`, rName))
+}
 
+func testAccCheckInstancesDataSourceConfig_filter_dataSource(rName string) string {
+	return acctest.ConfigCompose(
+		testAccCheckInstancesDataSourceConfig_filter_instance(rName),
+		`
 data "aws_ssm_instances" "test" {
   filter {
     name   = "InstanceIds"
     values = [aws_instance.test.id]
   }
 }
-`, rName))
+`)
 }

All the variants pass on my machine so I really don't have a way to verify if they solve the timing issue 😞

ewbankkit · 2022-02-14T22:03:47Z

Let's try with the explicit Sleep.

kamilturek · 2022-02-15T19:42:53Z

@ewbankkit That's done 🙂 Please give it a try.

$ make testacc PKG_NAME=internal/service/ssm TESTARGS="-run=TestAccSSMInstancesDataSource_filter"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssm/... -v -count 1 -parallel 20  -run=TestAccSSMInstancesDataSource_filter -timeout 180m
=== RUN   TestAccSSMInstancesDataSource_filter
=== PAUSE TestAccSSMInstancesDataSource_filter
=== CONT  TestAccSSMInstancesDataSource_filter
--- PASS: TestAccSSMInstancesDataSource_filter (329.81s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ssm        331.460s

Let me know please if there's anything else I could do here.

…cate with the SSM service.

ewbankkit

LGTM 🚀.

% make testacc TESTS=TestAccSSMInstancesDataSource_ PKG=ssm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssm/... -v -count 1 -parallel 20 -run='TestAccSSMInstancesDataSource_'  -timeout 180m
=== RUN   TestAccSSMInstancesDataSource_filter
=== PAUSE TestAccSSMInstancesDataSource_filter
=== CONT  TestAccSSMInstancesDataSource_filter
--- PASS: TestAccSSMInstancesDataSource_filter (327.96s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ssm	332.257s

ewbankkit · 2022-02-16T14:26:15Z

@kamilturek Thanks for the contribution 🎉 👏.
I think the underlying issue was with the default VPC in our default test region. Because of years of testing it is in an unknown state so in the case where the instance requires external access (as in this case for the SSM Agent) we need to create the VPC infrastructure to allow that (VPC, subnet, internet gateway, route table etc.).
I did that and the tests now pass.
I left the 1 minute sleep in there just to make sure 😄.

kamilturek · 2022-02-16T17:04:04Z

@ewbankkit Ah, that's why! It makes sense. Thanks, that's nice learning!

github-actions · 2022-02-19T00:07:58Z

This functionality has been released in v4.2.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2022-05-14T02:42:13Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

kamilturek added 5 commits February 13, 2022 19:23

Create aws_ssm_instances data source

ea2c3e3

Add acceptance test

852b373

Add data source to provider

01ee2eb

Change testing approach

865abe2

Add documentation page

542d83a

Add changelog

1d52301

kamilturek marked this pull request as ready for review February 13, 2022 20:55

ewbankkit removed the needs-triage Waiting for first response or review from a maintainer. label Feb 13, 2022

Add explicit sleep to let the SSM agent register instance

f24b167

d/aws_ssm_instances: Add VPC networking to allow SSM Agent to communi…

89b8034

…cate with the SSM service.

github-actions bot added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 16, 2022

ewbankkit approved these changes Feb 16, 2022

View reviewed changes

ewbankkit merged commit dbb210e into hashicorp:main Feb 16, 2022

github-actions bot added this to the v4.2.0 milestone Feb 16, 2022

kamilturek deleted the f-data-aws-ssm-instances branch February 16, 2022 17:04

github-actions bot locked as resolved and limited conversation to collaborators May 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

d/aws_ssm_instances #23162

d/aws_ssm_instances #23162

kamilturek commented Feb 13, 2022

ewbankkit commented Feb 14, 2022

kamilturek commented Feb 14, 2022

ewbankkit commented Feb 14, 2022

kamilturek commented Feb 15, 2022

ewbankkit left a comment

ewbankkit commented Feb 16, 2022

kamilturek commented Feb 16, 2022

github-actions bot commented Feb 19, 2022

github-actions bot commented May 14, 2022

d/aws_ssm_instances #23162

d/aws_ssm_instances #23162

Conversation

kamilturek commented Feb 13, 2022

Community Note

ewbankkit commented Feb 14, 2022

kamilturek commented Feb 14, 2022

ewbankkit commented Feb 14, 2022

kamilturek commented Feb 15, 2022

ewbankkit left a comment

Choose a reason for hiding this comment

ewbankkit commented Feb 16, 2022

kamilturek commented Feb 16, 2022

github-actions bot commented Feb 19, 2022

github-actions bot commented May 14, 2022