From 8b10f3cbe235357ecb1879761e1a227ee94b23f3 Mon Sep 17 00:00:00 2001 From: Yong Wen Chua Date: Fri, 8 Mar 2024 17:28:03 +0800 Subject: [PATCH 01/24] Fix `aws_securitylake_subscriber` to accept more than one source blocks --- internal/service/securitylake/subscriber.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/securitylake/subscriber.go b/internal/service/securitylake/subscriber.go index 39cd7b84ed8a..0ca4bcce825c 100644 --- a/internal/service/securitylake/subscriber.go +++ b/internal/service/securitylake/subscriber.go @@ -102,7 +102,7 @@ func (r *subscriberResource) Schema(ctx context.Context, request resource.Schema "source": schema.ListNestedBlock{ Validators: []validator.List{ listvalidator.IsRequired(), - listvalidator.SizeAtMost(1), + listvalidator.SizeAtLeast(1), }, NestedObject: schema.NestedBlockObject{ Blocks: map[string]schema.Block{ From 2cf42fb9b2b57608d7a4134372848c463c41aa6f Mon Sep 17 00:00:00 2001 From: Yong Wen Chua Date: Fri, 8 Mar 2024 17:39:31 +0800 Subject: [PATCH 02/24] Add changelog --- .changelog/36268.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/36268.txt diff --git a/.changelog/36268.txt b/.changelog/36268.txt new file mode 100644 index 000000000000..09a514f339d3 --- /dev/null +++ b/.changelog/36268.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_securitylake_subscriber: Allow more than one log source +``` From 9ad8f3faec4595646601bc3dd5baf7b53dbaf1cd Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 25 Apr 2024 11:27:59 -0700 Subject: [PATCH 03/24] Allows running tests from delegated admin account --- .../securitylake/aws_log_source_test.go | 4 +- .../securitylake/custom_log_source_test.go | 3 +- .../service/securitylake/data_lake_test.go | 12 ++--- internal/service/securitylake/exports_test.go | 1 + .../service/securitylake/securitylake_test.go | 50 +++++++++++++++++++ .../subscriber_notification_test.go | 8 +-- .../service/securitylake/subscriber_test.go | 10 ++-- 7 files changed, 71 insertions(+), 17 deletions(-) diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index 94ce12ea42ba..de97a29a52b8 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -28,6 +28,7 @@ func testAccAWSLogSource_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -64,6 +65,7 @@ func testAccAWSLogSource_multiRegion(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -100,7 +102,7 @@ func testAccAWSLogSource_disappears(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index 1033236350d6..6bb617cc17db 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -27,6 +27,7 @@ func testAccCustomLogSource_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -66,7 +67,7 @@ func testAccCustomLogSource_disappears(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, diff --git a/internal/service/securitylake/data_lake_test.go b/internal/service/securitylake/data_lake_test.go index 9b074d69abf3..31119d1d4d85 100644 --- a/internal/service/securitylake/data_lake_test.go +++ b/internal/service/securitylake/data_lake_test.go @@ -28,7 +28,7 @@ func testAccDataLake_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -68,7 +68,7 @@ func testAccDataLake_disappears(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -95,7 +95,7 @@ func testAccDataLake_tags(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -146,7 +146,7 @@ func testAccDataLake_lifeCycle(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -190,7 +190,7 @@ func testAccDataLake_lifeCycleUpdate(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -256,7 +256,7 @@ func testAccDataLake_replication(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) acctest.PreCheckMultipleRegion(t, 2) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), diff --git a/internal/service/securitylake/exports_test.go b/internal/service/securitylake/exports_test.go index ef78744db8ca..dcff7c43d354 100644 --- a/internal/service/securitylake/exports_test.go +++ b/internal/service/securitylake/exports_test.go @@ -14,6 +14,7 @@ var ( FindAWSLogSourceBySourceName = findAWSLogSourceBySourceName FindCustomLogSourceBySourceName = findCustomLogSourceBySourceName FindDataLakeByARN = findDataLakeByARN + FindDataLakes = findDataLakes FindSubscriberByID = findSubscriberByID FindSubscriberNotificationByEndPointID = findSubscriberNotificationByEndPointID ) diff --git a/internal/service/securitylake/securitylake_test.go b/internal/service/securitylake/securitylake_test.go index 1d339fa2da26..5f21f9ca3514 100644 --- a/internal/service/securitylake/securitylake_test.go +++ b/internal/service/securitylake/securitylake_test.go @@ -4,11 +4,25 @@ package securitylake_test import ( + "context" "testing" + "github.com/aws/aws-sdk-go-v2/service/securitylake" + awstypes "github.com/aws/aws-sdk-go-v2/service/securitylake/types" + "github.com/aws/aws-sdk-go/aws" + "github.com/hashicorp/aws-sdk-go-base/v2/tfawserr" "github.com/hashicorp/terraform-provider-aws/internal/acctest" + "github.com/hashicorp/terraform-provider-aws/internal/conns" + tforganizations "github.com/hashicorp/terraform-provider-aws/internal/service/organizations" + tfsecuritylake "github.com/hashicorp/terraform-provider-aws/internal/service/securitylake" + tfsts "github.com/hashicorp/terraform-provider-aws/internal/service/sts" + "github.com/hashicorp/terraform-provider-aws/internal/slices" + "github.com/hashicorp/terraform-provider-aws/internal/tfresource" ) +// Prerequisite: the current account must be either: +// * not a member of an organization +// * a delegated Security Lake administrator account func TestAccSecurityLake_serial(t *testing.T) { t.Parallel() @@ -47,3 +61,39 @@ func TestAccSecurityLake_serial(t *testing.T) { acctest.RunSerialTests2Levels(t, testCases, 0) } + +// testAccPreCheck validates that the current account is either +// * not a member of an organization +// * a member of an organization and is the delegated Security Lake administrator account +func testAccPreCheck(ctx context.Context, t *testing.T) { + t.Helper() + + awsClient := acctest.Provider.Meta().(*conns.AWSClient) + + organization, err := tforganizations.FindOrganization(ctx, awsClient.OrganizationsConn(ctx)) + + // Not a member of an organization + if tfresource.NotFound(err) { + return + } + + callerIdentity, err := tfsts.FindCallerIdentity(ctx, awsClient.STSClient(ctx)) + + if err != nil { + t.Fatalf("getting current identity: %s", err) + } + + if aws.StringValue(organization.MasterAccountId) == aws.StringValue(callerIdentity.Account) { + t.Skip("this AWS account must not be the management account of an AWS Organization") + } + + _, err = tfsecuritylake.FindDataLakes(ctx, awsClient.SecurityLakeClient(ctx), &securitylake.ListDataLakesInput{}, slices.PredicateTrue[*awstypes.DataLakeResource]()) + + if tfawserr.ErrMessageContains(err, "AccessDeniedException", "must be a delegated Security Lake administrator account") { + t.Skip("this AWS account must be a delegate Security Lake administrator account") + } + + if err != nil { + t.Fatalf("finding data lakes: %s", err) + } +} diff --git a/internal/service/securitylake/subscriber_notification_test.go b/internal/service/securitylake/subscriber_notification_test.go index db98128e875f..30a694cc70c5 100644 --- a/internal/service/securitylake/subscriber_notification_test.go +++ b/internal/service/securitylake/subscriber_notification_test.go @@ -28,7 +28,7 @@ func testAccSubscriberNotification_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -63,7 +63,7 @@ func testAccSubscriberNotification_https(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -100,7 +100,7 @@ func testAccSubscriberNotification_disappears(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -129,7 +129,7 @@ func testAccSubscriberNotification_update(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index 78ca83d97a9d..af3ab46f119d 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -29,7 +29,7 @@ func testAccSubscriber_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -69,7 +69,7 @@ func testAccSubscriber_disappears(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -105,7 +105,7 @@ func testAccSubscriber_customLogSource(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -144,7 +144,7 @@ func testAccSubscriber_tags(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, @@ -195,7 +195,7 @@ func testAccSubscriber_update(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) - acctest.PreCheckOrganizationsAccount(ctx, t) + testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, From cfe01ed026949e7b3ec1a3395f4d0dd4f21b3f67 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 25 Apr 2024 18:34:48 -0700 Subject: [PATCH 04/24] Cleanup --- .../service/securitylake/subscriber_test.go | 118 +++++++++--------- 1 file changed, 57 insertions(+), 61 deletions(-) diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index af3ab46f119d..f2ff7cbb0ffb 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -294,39 +294,73 @@ func testAccCheckSubscriberExists(ctx context.Context, n string, v *types.Subscr func testAccSubscriberConfig_basic(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` -data "aws_caller_identity" "test" {} +resource "aws_securitylake_subscriber" "test" { + subscriber_name = %[2]q + access_type = "S3" + source { + aws_log_source_resource { + source_name = "ROUTE53" + source_version = "1.0" + } + } + subscriber_identity { + external_id = "example" + principal = data.aws_caller_identity.current.account_id + } + + depends_on = [aws_securitylake_aws_log_source.test] +} resource "aws_securitylake_aws_log_source" "test" { source { - accounts = [data.aws_caller_identity.test.account_id] + accounts = [data.aws_caller_identity.current.account_id] regions = [%[1]q] source_name = "ROUTE53" source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test] } +`, acctest.Region(), rName)) +} +func testAccSubscriberConfig_customLog(rName string) string { + return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_subscriber" "test" { - subscriber_name = %[2]q - access_type = "S3" + subscriber_name = %[1]q + subscriber_description = "Example" source { - aws_log_source_resource { - source_name = "ROUTE53" - source_version = "1.0" + custom_log_source_resource { + source_name = aws_securitylake_custom_log_source.test.source_name + source_version = aws_securitylake_custom_log_source.test.source_version } } subscriber_identity { external_id = "example" - principal = data.aws_caller_identity.test.account_id + principal = data.aws_caller_identity.current.account_id } - depends_on = [aws_securitylake_aws_log_source.test] + depends_on = [aws_securitylake_custom_log_source.test] } -`, acctest.Region(), rName)) + +resource "aws_securitylake_custom_log_source" "test" { + source_name = "windows-sysmon" + source_version = "1.0" + event_classes = ["FILE_ACTIVITY"] + + configuration { + crawler_configuration { + role_arn = aws_iam_role.test.arn + } + + provider_identity { + external_id = "windows-sysmon-test" + principal = data.aws_caller_identity.current.account_id + } + } + + depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] } -func testAccSubscriberConfig_customLog(rName string) string { - return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_iam_role" "test" { name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" path = "/service-role/" @@ -370,42 +404,6 @@ resource "aws_iam_role_policy_attachment" "test" { policy_arn = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/AWSGlueServiceRole" role = aws_iam_role.test.name } - -resource "aws_securitylake_custom_log_source" "test" { - source_name = "windows-sysmon" - source_version = "1.0" - event_classes = ["FILE_ACTIVITY"] - - configuration { - crawler_configuration { - role_arn = aws_iam_role.test.arn - } - - provider_identity { - external_id = "windows-sysmon-test" - principal = data.aws_caller_identity.current.account_id - } - } - - depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] -} - -resource "aws_securitylake_subscriber" "test" { - subscriber_name = %[1]q - subscriber_description = "Example" - source { - custom_log_source_resource { - source_name = aws_securitylake_custom_log_source.test.source_name - source_version = aws_securitylake_custom_log_source.test.source_version - } - } - subscriber_identity { - external_id = "example" - principal = data.aws_caller_identity.current.account_id - } - - depends_on = [aws_securitylake_custom_log_source.test] -} `, rName)) } @@ -588,18 +586,6 @@ resource "aws_securitylake_subscriber" "test" { func testAccSubscriberConfig_update(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` -data "aws_caller_identity" "test" {} - -resource "aws_securitylake_aws_log_source" "test" { - source { - accounts = [data.aws_caller_identity.test.account_id] - regions = [%[1]q] - source_name = "ROUTE53" - source_version = "1.0" - } - depends_on = [aws_securitylake_data_lake.test] -} - resource "aws_securitylake_subscriber" "test" { subscriber_name = %[2]q access_type = "S3" @@ -611,8 +597,18 @@ resource "aws_securitylake_subscriber" "test" { } subscriber_identity { external_id = "updated" - principal = data.aws_caller_identity.test.account_id + principal = data.aws_caller_identity.current.account_id } } + +resource "aws_securitylake_aws_log_source" "test" { + source { + accounts = [data.aws_caller_identity.current.account_id] + regions = [%[1]q] + source_name = "ROUTE53" + source_version = "1.0" + } + depends_on = [aws_securitylake_data_lake.test] +} `, acctest.Region(), rName)) } From cbd8fa6f6aa3f12dd467e6c7162e6a5f9f3cfba2 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 25 Apr 2024 18:54:25 -0700 Subject: [PATCH 05/24] Uses `aws_region` data source instead of `acctest.Region` --- internal/service/securitylake/subscriber_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index f2ff7cbb0ffb..5c6e40d5dc5b 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -295,7 +295,7 @@ func testAccCheckSubscriberExists(ctx context.Context, n string, v *types.Subscr func testAccSubscriberConfig_basic(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_subscriber" "test" { - subscriber_name = %[2]q + subscriber_name = %[1]q access_type = "S3" source { aws_log_source_resource { @@ -314,13 +314,15 @@ resource "aws_securitylake_subscriber" "test" { resource "aws_securitylake_aws_log_source" "test" { source { accounts = [data.aws_caller_identity.current.account_id] - regions = [%[1]q] + regions = [data.aws_region.current.name] source_name = "ROUTE53" source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test] } -`, acctest.Region(), rName)) + +data "aws_region" "current" {} +`, rName)) } func testAccSubscriberConfig_customLog(rName string) string { @@ -587,7 +589,7 @@ resource "aws_securitylake_subscriber" "test" { func testAccSubscriberConfig_update(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_subscriber" "test" { - subscriber_name = %[2]q + subscriber_name = %[1]q access_type = "S3" source { aws_log_source_resource { @@ -604,11 +606,13 @@ resource "aws_securitylake_subscriber" "test" { resource "aws_securitylake_aws_log_source" "test" { source { accounts = [data.aws_caller_identity.current.account_id] - regions = [%[1]q] + regions = [data.aws_region.current.name] source_name = "ROUTE53" source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test] } -`, acctest.Region(), rName)) + +data "aws_region" "current" {} +`, rName)) } From 6a0169f793103d3fadb4ef4cd17ba2bce86cdaf4 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 25 Apr 2024 18:54:56 -0700 Subject: [PATCH 06/24] Simplifies tag tests --- .../service/securitylake/subscriber_test.go | 170 ++++-------------- 1 file changed, 33 insertions(+), 137 deletions(-) diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index 5c6e40d5dc5b..a9ccae1de565 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -411,76 +411,12 @@ resource "aws_iam_role_policy_attachment" "test" { func testAccSubscriberConfig_tags1(rName, tag1Key, tag1Value string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` -resource "aws_iam_role" "test" { - name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" - path = "/service-role/" - - assume_role_policy = < Date: Fri, 26 Apr 2024 11:30:37 -0700 Subject: [PATCH 07/24] Use `ComposeAggregateTestCheckFunc` --- internal/service/securitylake/subscriber_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index a9ccae1de565..45cb10c121cf 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -77,7 +77,7 @@ func testAccSubscriber_disappears(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccSubscriberConfig_basic(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), acctest.CheckFrameworkResourceDisappears(ctx, acctest.Provider, tfsecuritylake.ResourceSubscriber, resourceName), resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), @@ -152,7 +152,7 @@ func testAccSubscriber_tags(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccSubscriberConfig_tags1(rName, "key1", "value1"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"), @@ -166,7 +166,7 @@ func testAccSubscriber_tags(t *testing.T) { }, { Config: testAccSubscriberConfig_tags2(rName, "key1", "value1updated", "key2", "value2"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "tags.%", "2"), resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"), @@ -175,7 +175,7 @@ func testAccSubscriber_tags(t *testing.T) { }, { Config: testAccSubscriberConfig_tags1(rName, "key2", "value2"), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"), @@ -203,7 +203,7 @@ func testAccSubscriber_update(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccSubscriberConfig_basic(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), resource.TestCheckResourceAttr(resourceName, "access_type", "S3"), @@ -223,7 +223,7 @@ func testAccSubscriber_update(t *testing.T) { }, { Config: testAccSubscriberConfig_update(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), resource.TestCheckResourceAttr(resourceName, "access_type", "S3"), From 342ac0f5dce9e17ce1920b13c2dd456130492d6b Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 26 Apr 2024 14:46:52 -0700 Subject: [PATCH 08/24] Uses `aws_region` data source instead of `acctest.Region` for AWS Log Source --- .../securitylake/aws_log_source_test.go | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index de97a29a52b8..c838971ae466 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -65,10 +65,11 @@ func testAccAWSLogSource_multiRegion(t *testing.T) { PreCheck: func() { acctest.PreCheck(ctx, t) acctest.PreCheckPartitionHasService(t, names.SecurityLake) + acctest.PreCheckMultipleRegion(t, 2) testAccPreCheck(ctx, t) }, ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), - ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + ProtoV5ProviderFactories: acctest.ProtoV5FactoriesAlternate(ctx, t), CheckDestroy: testAccCheckAWSLogSourceDestroy(ctx), Steps: []resource.TestStep{ { @@ -168,34 +169,45 @@ func testAccCheckAWSLogSourceExists(ctx context.Context, n string, v *types.AwsL } func testAccAWSLogSourceConfig_basic() string { - return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), ` data "aws_caller_identity" "test" {} resource "aws_securitylake_aws_log_source" "test" { source { accounts = [data.aws_caller_identity.test.account_id] - regions = [%[1]q] + regions = [data.aws_region.current.name] source_name = "ROUTE53" source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test] } -`, acctest.Region())) + +data "aws_region" "current" {} +`) } func testAccAWSLogSourceConfig_multiRegion(rName string) string { - return acctest.ConfigCompose(testAccDataLakeConfig_replication(rName), fmt.Sprintf(` + return acctest.ConfigCompose( + acctest.ConfigMultipleRegionProvider(2), + testAccDataLakeConfig_replication(rName), ` data "aws_caller_identity" "test" {} resource "aws_securitylake_aws_log_source" "test" { source { accounts = [data.aws_caller_identity.test.account_id] - regions = [%[1]q, %[2]q] + regions = [data.aws_region.current.name, data.aws_region.alternate.name] source_name = "ROUTE53" source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test, aws_securitylake_data_lake.region_2] } -`, acctest.Region(), acctest.AlternateRegion())) + +data "aws_region" "current" {} + +data "aws_region" "alternate" { + provider = awsalternate +} +`) } From 3fefd9728d130e56584e83d7d43ccf03628af453 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 26 Apr 2024 16:26:01 -0700 Subject: [PATCH 09/24] Correctly handles default version --- .../service/securitylake/aws_log_source.go | 1 + .../securitylake/aws_log_source_test.go | 96 ++++++++++++++++--- .../service/securitylake/securitylake_test.go | 7 +- .../securitylake_aws_log_source.html.markdown | 12 ++- 4 files changed, 96 insertions(+), 20 deletions(-) diff --git a/internal/service/securitylake/aws_log_source.go b/internal/service/securitylake/aws_log_source.go index 728b7e195f17..a42b73dd1787 100644 --- a/internal/service/securitylake/aws_log_source.go +++ b/internal/service/securitylake/aws_log_source.go @@ -144,6 +144,7 @@ func (r *awsLogSourceResource) Create(ctx context.Context, request resource.Crea sourceData.Accounts.SetValue = fwflex.FlattenFrameworkStringValueSet(ctx, logSource.Accounts) sourceData.SourceVersion = fwflex.StringToFramework(ctx, logSource.SourceVersion) + data.Source = fwtypes.NewListNestedObjectValueOfPtrMust(ctx, sourceData) response.Diagnostics.Append(response.State.Set(ctx, data)...) } diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index c838971ae466..960425a37d98 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/securitylake/types" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-provider-aws/internal/acctest" "github.com/hashicorp/terraform-provider-aws/internal/conns" @@ -36,6 +37,51 @@ func testAccAWSLogSource_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccAWSLogSourceConfig_basic(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), + resource.TestCheckResourceAttr(resourceName, "source.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.accounts.#", "1"), + acctest.CheckResourceAttrAccountID(resourceName, "source.0.accounts.0"), + resource.TestCheckResourceAttr(resourceName, "source.0.regions.#", "1"), + resource.TestCheckTypeSetElemAttr(resourceName, "source.0.regions.*", acctest.Region()), + resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), + resource.TestCheckResourceAttr(resourceName, "source.0.source_version", "2.0"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAWSLogSourceConfig_sourceVersion("2.0"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + }, + }, + }) +} + +func testAccAWSLogSource_sourceVersion(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_aws_log_source.test" + var logSource types.AwsLogSourceConfiguration + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckAWSLogSourceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccAWSLogSourceConfig_sourceVersion("1.0"), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), resource.TestCheckResourceAttr(resourceName, "source.#", "1"), @@ -51,6 +97,23 @@ func testAccAWSLogSource_basic(t *testing.T) { ImportState: true, ImportStateVerify: true, }, + { + Config: testAccAWSLogSourceConfig_sourceVersion("2.0"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), + resource.TestCheckResourceAttr(resourceName, "source.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.accounts.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.regions.#", "1"), + resource.TestCheckTypeSetElemAttr(resourceName, "source.0.regions.*", acctest.Region()), + resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), + resource.TestCheckResourceAttr(resourceName, "source.0.source_version", "2.0"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, }, }) } @@ -81,8 +144,6 @@ func testAccAWSLogSource_multiRegion(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "source.0.regions.#", "2"), resource.TestCheckTypeSetElemAttr(resourceName, "source.0.regions.*", acctest.Region()), resource.TestCheckTypeSetElemAttr(resourceName, "source.0.regions.*", acctest.AlternateRegion()), - resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), - resource.TestCheckResourceAttr(resourceName, "source.0.source_version", "1.0"), ), }, { @@ -171,34 +232,45 @@ func testAccCheckAWSLogSourceExists(ctx context.Context, n string, v *types.AwsL func testAccAWSLogSourceConfig_basic() string { return acctest.ConfigCompose( testAccDataLakeConfig_basic(), ` -data "aws_caller_identity" "test" {} +resource "aws_securitylake_aws_log_source" "test" { + source { + accounts = [data.aws_caller_identity.current.account_id] + regions = [data.aws_region.current.name] + source_name = "ROUTE53" + } + depends_on = [aws_securitylake_data_lake.test] +} +data "aws_region" "current" {} +`) +} + +func testAccAWSLogSourceConfig_sourceVersion(version string) string { + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_aws_log_source" "test" { source { - accounts = [data.aws_caller_identity.test.account_id] + accounts = [data.aws_caller_identity.current.account_id] regions = [data.aws_region.current.name] source_name = "ROUTE53" - source_version = "1.0" + source_version = %[1]q } depends_on = [aws_securitylake_data_lake.test] } data "aws_region" "current" {} -`) +`, version)) } func testAccAWSLogSourceConfig_multiRegion(rName string) string { return acctest.ConfigCompose( acctest.ConfigMultipleRegionProvider(2), testAccDataLakeConfig_replication(rName), ` -data "aws_caller_identity" "test" {} - resource "aws_securitylake_aws_log_source" "test" { source { - accounts = [data.aws_caller_identity.test.account_id] - regions = [data.aws_region.current.name, data.aws_region.alternate.name] - source_name = "ROUTE53" - source_version = "1.0" + accounts = [data.aws_caller_identity.current.account_id] + regions = [data.aws_region.current.name, data.aws_region.alternate.name] + source_name = "ROUTE53" } depends_on = [aws_securitylake_data_lake.test, aws_securitylake_data_lake.region_2] diff --git a/internal/service/securitylake/securitylake_test.go b/internal/service/securitylake/securitylake_test.go index 5f21f9ca3514..d862c380520b 100644 --- a/internal/service/securitylake/securitylake_test.go +++ b/internal/service/securitylake/securitylake_test.go @@ -28,9 +28,10 @@ func TestAccSecurityLake_serial(t *testing.T) { testCases := map[string]map[string]func(t *testing.T){ "AWSLogSource": { - "basic": testAccAWSLogSource_basic, - "disappears": testAccAWSLogSource_disappears, - "multiRegion": testAccAWSLogSource_multiRegion, + "basic": testAccAWSLogSource_basic, + "disappears": testAccAWSLogSource_disappears, + "multiRegion": testAccAWSLogSource_multiRegion, + "sourceVersion": testAccAWSLogSource_sourceVersion, }, "CustomLogSource": { "basic": testAccCustomLogSource_basic, diff --git a/website/docs/r/securitylake_aws_log_source.html.markdown b/website/docs/r/securitylake_aws_log_source.html.markdown index 50a856b419d1..810fb1a830ff 100644 --- a/website/docs/r/securitylake_aws_log_source.html.markdown +++ b/website/docs/r/securitylake_aws_log_source.html.markdown @@ -17,10 +17,9 @@ Terraform resource for managing an Amazon Security Lake AWS Log Source. ```terraform resource "aws_securitylake_aws_log_source" "test" { source { - accounts = ["123456789012"] - regions = ["eu-west-1"] - source_name = "ROUTE53" - source_version = "1.0" + accounts = ["123456789012"] + regions = ["eu-west-1"] + source_name = "ROUTE53" } } ``` @@ -34,9 +33,12 @@ The following arguments are required: `source` supports the following: * `accounts` - (Optional) Specify the AWS account information where you want to enable Security Lake. + Defaults to the current account. * `regions` - (Required) Specify the Regions where you want to enable Security Lake. * `source_name` - (Required) The name for a AWS source. This must be a Regionally unique value. Valid values: `ROUTE53`, `VPC_FLOW`, `SH_FINDINGS`, `CLOUD_TRAIL_MGMT`, `LAMBDA_EXECUTION`, `S3_DATA`. -* `source_version` - (Optional) The version for a AWS source. This must be a Regionally unique value. +* `source_version` - (Optional) The version for a AWS source. + If not specified, the version will be the default. + This must be a Regionally unique value. ## Attribute Reference From 04a5b9b9119f765134ffa873a770ed34718abf6d Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 26 Apr 2024 17:27:02 -0700 Subject: [PATCH 10/24] Avoids conflict when creating multiple AWS Log Sources --- .../service/securitylake/aws_log_source.go | 16 ++++- .../securitylake/aws_log_source_test.go | 65 +++++++++++++++++++ internal/service/securitylake/data_lake.go | 12 ++++ .../service/securitylake/securitylake_test.go | 1 + 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/internal/service/securitylake/aws_log_source.go b/internal/service/securitylake/aws_log_source.go index a42b73dd1787..cb4e4b61e38d 100644 --- a/internal/service/securitylake/aws_log_source.go +++ b/internal/service/securitylake/aws_log_source.go @@ -6,6 +6,7 @@ package securitylake import ( "context" "fmt" + "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/securitylake" @@ -20,6 +21,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" + "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/errs" "github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag" "github.com/hashicorp/terraform-provider-aws/internal/framework" @@ -117,7 +119,12 @@ func (r *awsLogSourceResource) Create(ctx context.Context, request resource.Crea return } - _, err := conn.CreateAwsLogSource(ctx, input) + conns.GlobalMutexKV.Lock(dataLakeMutexKey) + defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) + + _, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, 2*time.Minute, func() (any, error) { + return conn.CreateAwsLogSource(ctx, input) + }) if err != nil { response.Diagnostics.AddError("creating Security Lake AWS Log Source", err.Error()) @@ -213,7 +220,12 @@ func (r *awsLogSourceResource) Delete(ctx context.Context, request resource.Dele input.Sources = []awstypes.AwsLogSourceConfiguration{*logSource} } - _, err := conn.DeleteAwsLogSource(ctx, input) + conns.GlobalMutexKV.Lock(dataLakeMutexKey) + defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) + + _, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, 2*time.Minute, func() (any, error) { + return conn.DeleteAwsLogSource(ctx, input) + }) if errs.IsA[*awstypes.ResourceNotFoundException](err) { return diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index 960425a37d98..c75a9d702854 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -182,6 +182,46 @@ func testAccAWSLogSource_disappears(t *testing.T) { }) } +func testAccAWSLogSource_multiple(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_aws_log_source.test" + resource2Name := "aws_securitylake_aws_log_source.test2" + var logSource, logSource2 types.AwsLogSourceConfiguration + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckAWSLogSourceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccAWSLogSourceConfig_multiple(), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), + testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource2), + + resource.TestCheckResourceAttr(resourceName, "source.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), + resource.TestCheckResourceAttr(resourceName, "source.0.source_version", "2.0"), + + resource.TestCheckResourceAttr(resource2Name, "source.#", "1"), + resource.TestCheckResourceAttr(resource2Name, "source.0.source_name", "S3_DATA"), + resource.TestCheckResourceAttr(resource2Name, "source.0.source_version", "2.0"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func testAccCheckAWSLogSourceDestroy(ctx context.Context) resource.TestCheckFunc { return func(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).SecurityLakeClient(ctx) @@ -283,3 +323,28 @@ data "aws_region" "alternate" { } `) } + +func testAccAWSLogSourceConfig_multiple() string { + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), ` +resource "aws_securitylake_aws_log_source" "test" { + source { + accounts = [data.aws_caller_identity.current.account_id] + regions = [data.aws_region.current.name] + source_name = "ROUTE53" + } + depends_on = [aws_securitylake_data_lake.test] +} + +resource "aws_securitylake_aws_log_source" "test2" { + source { + accounts = [data.aws_caller_identity.current.account_id] + regions = [data.aws_region.current.name] + source_name = "S3_DATA" + } + depends_on = [aws_securitylake_data_lake.test] +} + +data "aws_region" "current" {} +`) +} diff --git a/internal/service/securitylake/data_lake.go b/internal/service/securitylake/data_lake.go index f74c1b28507a..ffa1bd2341f1 100644 --- a/internal/service/securitylake/data_lake.go +++ b/internal/service/securitylake/data_lake.go @@ -24,6 +24,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" + "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/enum" "github.com/hashicorp/terraform-provider-aws/internal/errs" "github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag" @@ -36,6 +37,8 @@ import ( "github.com/hashicorp/terraform-provider-aws/names" ) +const dataLakeMutexKey = "aws_securitylake_data_lake" + // @FrameworkResource(name="Data Lake") // @Tags(identifierAttribute="arn") func newDataLakeResource(context.Context) (resource.ResourceWithConfigure, error) { @@ -190,6 +193,9 @@ func (r *dataLakeResource) Create(ctx context.Context, request resource.CreateRe // Additional fields. input.Tags = getTagsIn(ctx) + conns.GlobalMutexKV.Lock(dataLakeMutexKey) + defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) + output, err := conn.CreateDataLake(ctx, input) if err != nil { @@ -294,6 +300,9 @@ func (r *dataLakeResource) Update(ctx context.Context, request resource.UpdateRe return } + conns.GlobalMutexKV.Lock(dataLakeMutexKey) + defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) + _, err := conn.UpdateDataLake(ctx, input) if err != nil { @@ -321,6 +330,9 @@ func (r *dataLakeResource) Delete(ctx context.Context, request resource.DeleteRe conn := r.Meta().SecurityLakeClient(ctx) + conns.GlobalMutexKV.Lock(dataLakeMutexKey) + defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) + // "ConflictException: The request failed because your Security Lake configuration or resources are currently being updated. Wait a few minutes and then try again." const ( timeout = 2 * time.Minute diff --git a/internal/service/securitylake/securitylake_test.go b/internal/service/securitylake/securitylake_test.go index d862c380520b..aa5e76c2cf5c 100644 --- a/internal/service/securitylake/securitylake_test.go +++ b/internal/service/securitylake/securitylake_test.go @@ -30,6 +30,7 @@ func TestAccSecurityLake_serial(t *testing.T) { "AWSLogSource": { "basic": testAccAWSLogSource_basic, "disappears": testAccAWSLogSource_disappears, + "multiple": testAccAWSLogSource_multiple, "multiRegion": testAccAWSLogSource_multiRegion, "sourceVersion": testAccAWSLogSource_sourceVersion, }, From a7b837ad5c83d4b26ff0ca0d8d081e0a35bd0d4d Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 29 Apr 2024 11:19:12 -0700 Subject: [PATCH 11/24] Updates documentation to note need for `depends_on` relationship with `aws_securitylake_data_lake` and all other resources --- website/docs/r/securitylake_aws_log_source.html.markdown | 6 +++++- website/docs/r/securitylake_custom_log_source.html.markdown | 4 ++++ website/docs/r/securitylake_data_lake.html.markdown | 2 ++ website/docs/r/securitylake_subscriber.html.markdown | 4 ++++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/website/docs/r/securitylake_aws_log_source.html.markdown b/website/docs/r/securitylake_aws_log_source.html.markdown index 810fb1a830ff..9a0f59867fd0 100644 --- a/website/docs/r/securitylake_aws_log_source.html.markdown +++ b/website/docs/r/securitylake_aws_log_source.html.markdown @@ -10,17 +10,21 @@ description: |- Terraform resource for managing an Amazon Security Lake AWS Log Source. +~> **NOTE:** The underlying `aws_securitylake_data_lake` must be configured before creating the `aws_securitylake_aws_log_source`. Use a `depends_on` statement. + ## Example Usage ### Basic Usage ```terraform -resource "aws_securitylake_aws_log_source" "test" { +resource "aws_securitylake_aws_log_source" "example" { source { accounts = ["123456789012"] regions = ["eu-west-1"] source_name = "ROUTE53" } + + depends_on = [aws_securitylake_data_lake.example] } ``` diff --git a/website/docs/r/securitylake_custom_log_source.html.markdown b/website/docs/r/securitylake_custom_log_source.html.markdown index edf3caf0d809..198452e64f35 100644 --- a/website/docs/r/securitylake_custom_log_source.html.markdown +++ b/website/docs/r/securitylake_custom_log_source.html.markdown @@ -10,6 +10,8 @@ description: |- Terraform resource for managing an AWS Security Lake Custom Log Source. +~> **NOTE:** The underlying `aws_securitylake_data_lake` must be configured before creating the `aws_securitylake_custom_log_source`. Use a `depends_on` statement. + ## Example Usage ### Basic Usage @@ -30,6 +32,8 @@ resource "aws_securitylake_custom_log_source" "example" { principal = "123456789012" } } + + depends_on = [aws_securitylake_data_lake.example] } ``` diff --git a/website/docs/r/securitylake_data_lake.html.markdown b/website/docs/r/securitylake_data_lake.html.markdown index e983b587be76..f8234332a3c7 100644 --- a/website/docs/r/securitylake_data_lake.html.markdown +++ b/website/docs/r/securitylake_data_lake.html.markdown @@ -10,6 +10,8 @@ description: |- Terraform resource for managing an AWS Security Lake Data Lake. +~> **NOTE:** The underlying `aws_securitylake_data_lake` must be configured before creating other Security Lake resources. Use a `depends_on` statement. + ## Example Usage ```terraform diff --git a/website/docs/r/securitylake_subscriber.html.markdown b/website/docs/r/securitylake_subscriber.html.markdown index 98268632b977..fb23fb4955dc 100644 --- a/website/docs/r/securitylake_subscriber.html.markdown +++ b/website/docs/r/securitylake_subscriber.html.markdown @@ -10,6 +10,8 @@ description: |- Terraform resource for managing an AWS Security Lake Subscriber. +~> **NOTE:** The underlying `aws_securitylake_data_lake` must be configured before creating the `aws_securitylake_subscriber`. Use a `depends_on` statement. + ## Example Usage ```terraform @@ -28,6 +30,8 @@ resource "aws_securitylake_subscriber" "example" { external_id = "example" principal = "1234567890" } + + depends_on = [aws_securitylake_data_lake.example] } ``` From 5a818efe45dd71c16d958a4f6e58f5948bd63778 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 29 Apr 2024 15:15:45 -0700 Subject: [PATCH 12/24] `aws_securitylake_aws_log_source` updates --- internal/service/securitylake/aws_log_source_test.go | 5 ++++- website/docs/r/securitylake_aws_log_source.html.markdown | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index c75a9d702854..9cde5d599fcc 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -40,8 +40,11 @@ func testAccAWSLogSource_basic(t *testing.T) { Check: resource.ComposeAggregateTestCheckFunc( testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), resource.TestCheckResourceAttr(resourceName, "source.#", "1"), - resource.TestCheckResourceAttr(resourceName, "source.0.accounts.#", "1"), + resource.TestCheckResourceAttrSet(resourceName, "source.0.accounts.#"), acctest.CheckResourceAttrAccountID(resourceName, "source.0.accounts.0"), + func(s *terraform.State) error { + return resource.TestCheckTypeSetElemAttr(resourceName, "source.0.accounts.*", acctest.AccountID())(s) + }, resource.TestCheckResourceAttr(resourceName, "source.0.regions.#", "1"), resource.TestCheckTypeSetElemAttr(resourceName, "source.0.regions.*", acctest.Region()), resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), diff --git a/website/docs/r/securitylake_aws_log_source.html.markdown b/website/docs/r/securitylake_aws_log_source.html.markdown index 9a0f59867fd0..0a16f67ce8ea 100644 --- a/website/docs/r/securitylake_aws_log_source.html.markdown +++ b/website/docs/r/securitylake_aws_log_source.html.markdown @@ -10,6 +10,8 @@ description: |- Terraform resource for managing an Amazon Security Lake AWS Log Source. +~> **NOTE:** A single `aws_securitylake_aws_log_source` should be used to configure a log source across all regions and accounts. + ~> **NOTE:** The underlying `aws_securitylake_data_lake` must be configured before creating the `aws_securitylake_aws_log_source`. Use a `depends_on` statement. ## Example Usage @@ -37,7 +39,7 @@ The following arguments are required: `source` supports the following: * `accounts` - (Optional) Specify the AWS account information where you want to enable Security Lake. - Defaults to the current account. + If not specified, uses all accounts included in the Security Lake. * `regions` - (Required) Specify the Regions where you want to enable Security Lake. * `source_name` - (Required) The name for a AWS source. This must be a Regionally unique value. Valid values: `ROUTE53`, `VPC_FLOW`, `SH_FINDINGS`, `CLOUD_TRAIL_MGMT`, `LAMBDA_EXECUTION`, `S3_DATA`. * `source_version` - (Optional) The version for a AWS source. From 05dd4d2fbde25297cf8febf40426f7197ace5981 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Mon, 29 Apr 2024 17:02:43 -0700 Subject: [PATCH 13/24] Tests defaults in `testAccCustomLogSource_basic` --- .../securitylake/custom_log_source_test.go | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index 6bb617cc17db..3733ee7a5694 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -6,8 +6,10 @@ package securitylake_test import ( "context" "fmt" + "strings" "testing" + "github.com/YakDriver/regexache" "github.com/aws/aws-sdk-go-v2/service/securitylake/types" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" @@ -37,22 +39,28 @@ func testAccCustomLogSource_basic(t *testing.T) { Config: testAccCustomLogSourceConfig_basic(), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "attributes.#", "1"), + acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.crawler_arn", "glue", "crawler/windows-sysmon"), + acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.database_arn", "glue", fmt.Sprintf("database/amazon_security_lake_glue_db_%s", strings.Replace(acctest.Region(), "-", "_", -1))), + acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.table_arn", "glue", fmt.Sprintf("table/amazon_security_lake_table_%s_ext_windows_sysmon", strings.Replace(acctest.Region(), "-", "_", -1))), resource.TestCheckResourceAttr(resourceName, "configuration.#", "1"), resource.TestCheckResourceAttr(resourceName, "configuration.0.crawler_configuration.#", "1"), resource.TestCheckResourceAttrPair(resourceName, "configuration.0.crawler_configuration.0.role_arn", "aws_iam_role.test", "arn"), resource.TestCheckResourceAttr(resourceName, "configuration.0.provider_identity.#", "1"), resource.TestCheckResourceAttr(resourceName, "configuration.0.provider_identity.0.external_id", "windows-sysmon-test"), - resource.TestCheckResourceAttr(resourceName, "event_classes.#", "1"), - resource.TestCheckTypeSetElemAttr(resourceName, "event_classes.*", "FILE_ACTIVITY"), + resource.TestCheckNoResourceAttr(resourceName, "event_classes"), + resource.TestCheckResourceAttr(resourceName, "provider_details.#", "1"), + resource.TestMatchResourceAttr(resourceName, "provider_details.0.location", regexache.MustCompile(fmt.Sprintf(`^s3://aws-security-data-lake-%s-[a-z0-9]{30}/ext/windows-sysmon/$`, acctest.Region()))), + acctest.CheckResourceAttrGlobalARN(resourceName, "provider_details.0.role_arn", "iam", fmt.Sprintf("role/AmazonSecurityLake-Provider-windows-sysmon-%s", acctest.Region())), resource.TestCheckResourceAttr(resourceName, "source_name", "windows-sysmon"), - resource.TestCheckResourceAttr(resourceName, "source_version", "1.0"), + resource.TestCheckNoResourceAttr(resourceName, "source_version"), ), }, { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"configuration", "event_classes"}, + ImportStateVerifyIgnore: []string{"configuration"}, }, }, }) @@ -133,7 +141,25 @@ func testAccCheckCustomLogSourceExists(ctx context.Context, n string, v *types.C } func testAccCustomLogSourceConfig_basic() string { - return acctest.ConfigCompose(testAccDataLakeConfig_basic(), ` + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), ` +resource "aws_securitylake_custom_log_source" "test" { + source_name = "windows-sysmon" + + configuration { + crawler_configuration { + role_arn = aws_iam_role.test.arn + } + + provider_identity { + external_id = "windows-sysmon-test" + principal = data.aws_caller_identity.current.account_id + } + } + + depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] +} + resource "aws_iam_role" "test" { name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" path = "/service-role/" @@ -177,6 +203,8 @@ resource "aws_iam_role_policy_attachment" "test" { policy_arn = "arn:${data.aws_partition.current.partition}:iam::aws:policy/service-role/AWSGlueServiceRole" role = aws_iam_role.test.name } +`) +} resource "aws_securitylake_custom_log_source" "test" { source_name = "windows-sysmon" From 4d5946a7298c6c0ee3fdfac3aabf9130ed267c04 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 30 Apr 2024 11:31:41 -0700 Subject: [PATCH 14/24] Additional tests for Custom Log Sources --- .../securitylake/aws_log_source_test.go | 10 +- .../service/securitylake/custom_log_source.go | 4 + .../securitylake/custom_log_source_test.go | 273 ++++++++++++++++-- .../service/securitylake/securitylake_test.go | 6 +- ...curitylake_custom_log_source.html.markdown | 6 +- 5 files changed, 270 insertions(+), 29 deletions(-) diff --git a/internal/service/securitylake/aws_log_source_test.go b/internal/service/securitylake/aws_log_source_test.go index 9cde5d599fcc..b7a6468f9f94 100644 --- a/internal/service/securitylake/aws_log_source_test.go +++ b/internal/service/securitylake/aws_log_source_test.go @@ -188,7 +188,7 @@ func testAccAWSLogSource_disappears(t *testing.T) { func testAccAWSLogSource_multiple(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_aws_log_source.test" - resource2Name := "aws_securitylake_aws_log_source.test2" + resourceName2 := "aws_securitylake_aws_log_source.test2" var logSource, logSource2 types.AwsLogSourceConfiguration resource.Test(t, resource.TestCase{ @@ -205,15 +205,15 @@ func testAccAWSLogSource_multiple(t *testing.T) { Config: testAccAWSLogSourceConfig_multiple(), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource), - testAccCheckAWSLogSourceExists(ctx, resourceName, &logSource2), + testAccCheckAWSLogSourceExists(ctx, resourceName2, &logSource2), resource.TestCheckResourceAttr(resourceName, "source.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.source_name", "ROUTE53"), resource.TestCheckResourceAttr(resourceName, "source.0.source_version", "2.0"), - resource.TestCheckResourceAttr(resource2Name, "source.#", "1"), - resource.TestCheckResourceAttr(resource2Name, "source.0.source_name", "S3_DATA"), - resource.TestCheckResourceAttr(resource2Name, "source.0.source_version", "2.0"), + resource.TestCheckResourceAttr(resourceName2, "source.#", "1"), + resource.TestCheckResourceAttr(resourceName2, "source.0.source_name", "S3_DATA"), + resource.TestCheckResourceAttr(resourceName2, "source.0.source_version", "2.0"), ), }, { diff --git a/internal/service/securitylake/custom_log_source.go b/internal/service/securitylake/custom_log_source.go index f02536097fc3..c718bc4fda04 100644 --- a/internal/service/securitylake/custom_log_source.go +++ b/internal/service/securitylake/custom_log_source.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/securitylake" awstypes "github.com/aws/aws-sdk-go-v2/service/securitylake/types" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" + "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" @@ -88,6 +89,9 @@ func (r *customLogSourceResource) Schema(ctx context.Context, request resource.S }, "source_name": schema.StringAttribute{ Required: true, + Validators: []validator.String{ + stringvalidator.LengthAtMost(20), + }, PlanModifiers: []planmodifier.String{ stringplanmodifier.RequiresReplace(), }, diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index 3733ee7a5694..462d50fe27e2 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -11,11 +11,13 @@ import ( "github.com/YakDriver/regexache" "github.com/aws/aws-sdk-go-v2/service/securitylake/types" + sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-provider-aws/internal/acctest" "github.com/hashicorp/terraform-provider-aws/internal/conns" tfsecuritylake "github.com/hashicorp/terraform-provider-aws/internal/service/securitylake" + "github.com/hashicorp/terraform-provider-aws/internal/slices" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/names" ) @@ -23,6 +25,7 @@ import ( func testAccCustomLogSource_basic(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_custom_log_source.test" + rName := randomCustomLogSourceName() var customLogSource types.CustomLogSourceResource resource.Test(t, resource.TestCase{ @@ -36,23 +39,23 @@ func testAccCustomLogSource_basic(t *testing.T) { CheckDestroy: testAccCheckCustomLogSourceDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccCustomLogSourceConfig_basic(), + Config: testAccCustomLogSourceConfig_basic(rName), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), resource.TestCheckResourceAttr(resourceName, "attributes.#", "1"), - acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.crawler_arn", "glue", "crawler/windows-sysmon"), + acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.crawler_arn", "glue", fmt.Sprintf("crawler/%s", rName)), acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.database_arn", "glue", fmt.Sprintf("database/amazon_security_lake_glue_db_%s", strings.Replace(acctest.Region(), "-", "_", -1))), - acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.table_arn", "glue", fmt.Sprintf("table/amazon_security_lake_table_%s_ext_windows_sysmon", strings.Replace(acctest.Region(), "-", "_", -1))), + acctest.CheckResourceAttrRegionalARN(resourceName, "attributes.0.table_arn", "glue", fmt.Sprintf("table/amazon_security_lake_table_%s_ext_%s", strings.Replace(acctest.Region(), "-", "_", -1), strings.Replace(rName, "-", "_", -1))), resource.TestCheckResourceAttr(resourceName, "configuration.#", "1"), resource.TestCheckResourceAttr(resourceName, "configuration.0.crawler_configuration.#", "1"), resource.TestCheckResourceAttrPair(resourceName, "configuration.0.crawler_configuration.0.role_arn", "aws_iam_role.test", "arn"), resource.TestCheckResourceAttr(resourceName, "configuration.0.provider_identity.#", "1"), - resource.TestCheckResourceAttr(resourceName, "configuration.0.provider_identity.0.external_id", "windows-sysmon-test"), + resource.TestCheckResourceAttr(resourceName, "configuration.0.provider_identity.0.external_id", fmt.Sprintf("%s-test", rName)), resource.TestCheckNoResourceAttr(resourceName, "event_classes"), resource.TestCheckResourceAttr(resourceName, "provider_details.#", "1"), - resource.TestMatchResourceAttr(resourceName, "provider_details.0.location", regexache.MustCompile(fmt.Sprintf(`^s3://aws-security-data-lake-%s-[a-z0-9]{30}/ext/windows-sysmon/$`, acctest.Region()))), - acctest.CheckResourceAttrGlobalARN(resourceName, "provider_details.0.role_arn", "iam", fmt.Sprintf("role/AmazonSecurityLake-Provider-windows-sysmon-%s", acctest.Region())), - resource.TestCheckResourceAttr(resourceName, "source_name", "windows-sysmon"), + resource.TestMatchResourceAttr(resourceName, "provider_details.0.location", regexache.MustCompile(fmt.Sprintf(`^s3://aws-security-data-lake-%s-[a-z0-9]{30}/ext/%s/$`, acctest.Region(), rName))), + acctest.CheckResourceAttrGlobalARN(resourceName, "provider_details.0.role_arn", "iam", fmt.Sprintf("role/AmazonSecurityLake-Provider-%s-%s", rName, acctest.Region())), + resource.TestCheckResourceAttr(resourceName, "source_name", rName), resource.TestCheckNoResourceAttr(resourceName, "source_version"), ), }, @@ -66,9 +69,119 @@ func testAccCustomLogSource_basic(t *testing.T) { }) } +func testAccCustomLogSource_sourceVersion(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_custom_log_source.test" + rName := randomCustomLogSourceName() + var customLogSource types.CustomLogSourceResource + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckCustomLogSourceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccCustomLogSourceConfig_sourceVersion(rName, "1.5"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "source_version", "1.5"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration"}, + }, + { + Config: testAccCustomLogSourceConfig_sourceVersion(rName, "2.5"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "source_version", "2.5"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration"}, + }, + }, + }) +} + +func testAccCustomLogSource_eventClasses(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_custom_log_source.test" + rName := randomCustomLogSourceName() + var customLogSource types.CustomLogSourceResource + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckCustomLogSourceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccCustomLogSourceConfig_eventClasses(rName, "FILE_ACTIVITY"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "event_classes.#", "1"), + resource.TestCheckTypeSetElemAttr(resourceName, "event_classes.*", "FILE_ACTIVITY"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration", "event_classes"}, + }, + { + Config: testAccCustomLogSourceConfig_eventClasses(rName, "MEMORY_ACTIVITY", "FILE_ACTIVITY"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "event_classes.#", "2"), + resource.TestCheckTypeSetElemAttr(resourceName, "event_classes.*", "MEMORY_ACTIVITY"), + resource.TestCheckTypeSetElemAttr(resourceName, "event_classes.*", "FILE_ACTIVITY"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration", "event_classes"}, + }, + { + Config: testAccCustomLogSourceConfig_eventClasses(rName, "MEMORY_ACTIVITY"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + resource.TestCheckResourceAttr(resourceName, "event_classes.#", "1"), + resource.TestCheckTypeSetElemAttr(resourceName, "event_classes.*", "MEMORY_ACTIVITY"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration", "event_classes"}, + }, + }, + }) +} + func testAccCustomLogSource_disappears(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_custom_log_source.test" + rName := randomCustomLogSourceName() var customLogSource types.CustomLogSourceResource resource.Test(t, resource.TestCase{ @@ -82,7 +195,7 @@ func testAccCustomLogSource_disappears(t *testing.T) { CheckDestroy: testAccCheckCustomLogSourceDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccCustomLogSourceConfig_basic(), + Config: testAccCustomLogSourceConfig_basic(rName), Check: resource.ComposeTestCheckFunc( testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), acctest.CheckFrameworkResourceDisappears(ctx, acctest.Provider, tfsecuritylake.ResourceCustomLogSource, resourceName), @@ -93,6 +206,10 @@ func testAccCustomLogSource_disappears(t *testing.T) { }) } +func randomCustomLogSourceName() string { + return fmt.Sprintf("%s-%s", acctest.ResourcePrefix, sdkacctest.RandString(20-(len(acctest.ResourcePrefix)+1))) +} + func testAccCheckCustomLogSourceDestroy(ctx context.Context) resource.TestCheckFunc { return func(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).SecurityLakeClient(ctx) @@ -140,11 +257,11 @@ func testAccCheckCustomLogSourceExists(ctx context.Context, n string, v *types.C } } -func testAccCustomLogSourceConfig_basic() string { +func testAccCustomLogSourceConfig_basic(rName string) string { return acctest.ConfigCompose( - testAccDataLakeConfig_basic(), ` + testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_custom_log_source" "test" { - source_name = "windows-sysmon" + source_name = %[1]q configuration { crawler_configuration { @@ -152,7 +269,7 @@ resource "aws_securitylake_custom_log_source" "test" { } provider_identity { - external_id = "windows-sysmon-test" + external_id = "%[1]s-test" principal = data.aws_caller_identity.current.account_id } } @@ -161,7 +278,7 @@ resource "aws_securitylake_custom_log_source" "test" { } resource "aws_iam_role" "test" { - name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" + name = %[1]q path = "/service-role/" assume_role_policy = < Date: Tue, 30 Apr 2024 12:12:00 -0700 Subject: [PATCH 15/24] Avoids conflict when creating multiple Custom Log Sources --- .../service/securitylake/aws_log_source.go | 11 +- .../service/securitylake/custom_log_source.go | 9 +- .../securitylake/custom_log_source_test.go | 122 ++++++++++++++++++ internal/service/securitylake/data_lake.go | 41 +++--- .../service/securitylake/securitylake_test.go | 3 +- 5 files changed, 155 insertions(+), 31 deletions(-) diff --git a/internal/service/securitylake/aws_log_source.go b/internal/service/securitylake/aws_log_source.go index cb4e4b61e38d..5b60cf198c07 100644 --- a/internal/service/securitylake/aws_log_source.go +++ b/internal/service/securitylake/aws_log_source.go @@ -21,7 +21,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" - "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/errs" "github.com/hashicorp/terraform-provider-aws/internal/errs/fwdiag" "github.com/hashicorp/terraform-provider-aws/internal/framework" @@ -119,10 +118,7 @@ func (r *awsLogSourceResource) Create(ctx context.Context, request resource.Crea return } - conns.GlobalMutexKV.Lock(dataLakeMutexKey) - defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) - - _, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, 2*time.Minute, func() (any, error) { + _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.CreateAwsLogSourceOutput, error) { return conn.CreateAwsLogSource(ctx, input) }) @@ -220,10 +216,7 @@ func (r *awsLogSourceResource) Delete(ctx context.Context, request resource.Dele input.Sources = []awstypes.AwsLogSourceConfiguration{*logSource} } - conns.GlobalMutexKV.Lock(dataLakeMutexKey) - defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) - - _, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, 2*time.Minute, func() (any, error) { + _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.DeleteAwsLogSourceOutput, error) { return conn.DeleteAwsLogSource(ctx, input) }) diff --git a/internal/service/securitylake/custom_log_source.go b/internal/service/securitylake/custom_log_source.go index c718bc4fda04..b8979827e09a 100644 --- a/internal/service/securitylake/custom_log_source.go +++ b/internal/service/securitylake/custom_log_source.go @@ -6,6 +6,7 @@ package securitylake import ( "context" "fmt" + "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/securitylake" @@ -188,7 +189,9 @@ func (r *customLogSourceResource) Create(ctx context.Context, request resource.C return } - output, err := conn.CreateCustomLogSource(ctx, input) + output, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.CreateCustomLogSourceOutput, error) { + return conn.CreateCustomLogSource(ctx, input) + }) if err != nil { response.Diagnostics.AddError("creating Security Lake Custom Log Source", err.Error()) @@ -270,7 +273,9 @@ func (r *customLogSourceResource) Delete(ctx context.Context, request resource.D return } - _, err := conn.DeleteCustomLogSource(ctx, input) + _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.DeleteCustomLogSourceOutput, error) { + return conn.DeleteCustomLogSource(ctx, input) + }) if errs.IsA[*awstypes.ResourceNotFoundException](err) { return diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index 462d50fe27e2..735c33ea8348 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -115,6 +115,45 @@ func testAccCustomLogSource_sourceVersion(t *testing.T) { }) } +func testAccCustomLogSource_multiple(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_custom_log_source.test" + resourceName2 := "aws_securitylake_custom_log_source.test2" + rName := randomCustomLogSourceName() + rName2 := randomCustomLogSourceName() + var customLogSource, customLogSource2 types.CustomLogSourceResource + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckCustomLogSourceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccCustomLogSourceConfig_multiple(rName, rName2), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckCustomLogSourceExists(ctx, resourceName, &customLogSource), + testAccCheckCustomLogSourceExists(ctx, resourceName2, &customLogSource2), + + resource.TestCheckResourceAttr(resourceName, "source_name", rName), + + resource.TestCheckResourceAttr(resourceName2, "source_name", rName2), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"configuration"}, + }, + }, + }) +} + func testAccCustomLogSource_eventClasses(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_custom_log_source.test" @@ -459,3 +498,86 @@ resource "aws_iam_role_policy_attachment" "test" { } `, rName, strings.Join(eventClasses, ", "))) } + +func testAccCustomLogSourceConfig_multiple(rName, rName2 string) string { + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), fmt.Sprintf(` +resource "aws_securitylake_custom_log_source" "test" { + source_name = %[1]q + + configuration { + crawler_configuration { + role_arn = aws_iam_role.test.arn + } + + provider_identity { + external_id = "%[1]s-test" + principal = data.aws_caller_identity.current.account_id + } + } + + depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] +} + +resource "aws_securitylake_custom_log_source" "test2" { + source_name = %[2]q + + configuration { + crawler_configuration { + role_arn = aws_iam_role.test.arn + } + + provider_identity { + external_id = "%[2]s-test" + principal = data.aws_caller_identity.current.account_id + } + } + + depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] +} + +resource "aws_iam_role" "test" { + name = %[1]q + path = "/service-role/" + + assume_role_policy = < Date: Tue, 30 Apr 2024 12:48:45 -0700 Subject: [PATCH 16/24] `terrafmt` --- .../securitylake/custom_log_source_test.go | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index 735c33ea8348..c45575ed1bf2 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -300,7 +300,7 @@ func testAccCustomLogSourceConfig_basic(rName string) string { return acctest.ConfigCompose( testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_custom_log_source" "test" { - source_name = %[1]q + source_name = %[1]q configuration { crawler_configuration { @@ -317,7 +317,7 @@ resource "aws_securitylake_custom_log_source" "test" { } resource "aws_iam_role" "test" { - name = %[1]q + name = %[1]q path = "/service-role/" assume_role_policy = < Date: Wed, 1 May 2024 10:19:59 -0700 Subject: [PATCH 17/24] Tests defaults in `testAccSubscriber_basic` and adds additional tests --- .../securitylake/custom_log_source_test.go | 8 - .../service/securitylake/securitylake_test.go | 1 + internal/service/securitylake/subscriber.go | 22 +-- .../service/securitylake/subscriber_test.go | 151 +++++++++++++----- .../r/securitylake_subscriber.html.markdown | 7 +- 5 files changed, 124 insertions(+), 65 deletions(-) diff --git a/internal/service/securitylake/custom_log_source_test.go b/internal/service/securitylake/custom_log_source_test.go index c45575ed1bf2..95bfa7bc0db9 100644 --- a/internal/service/securitylake/custom_log_source_test.go +++ b/internal/service/securitylake/custom_log_source_test.go @@ -351,8 +351,6 @@ resource "aws_iam_role_policy" "test" { }] } POLICY - - depends_on = [aws_securitylake_data_lake.test] } resource "aws_iam_role_policy_attachment" "test" { @@ -418,8 +416,6 @@ resource "aws_iam_role_policy" "test" { }] } POLICY - - depends_on = [aws_securitylake_data_lake.test] } resource "aws_iam_role_policy_attachment" "test" { @@ -488,8 +484,6 @@ resource "aws_iam_role_policy" "test" { }] } POLICY - - depends_on = [aws_securitylake_data_lake.test] } resource "aws_iam_role_policy_attachment" "test" { @@ -571,8 +565,6 @@ resource "aws_iam_role_policy" "test" { }] } POLICY - - depends_on = [aws_securitylake_data_lake.test] } resource "aws_iam_role_policy_attachment" "test" { diff --git a/internal/service/securitylake/securitylake_test.go b/internal/service/securitylake/securitylake_test.go index fd0a99e2dbaa..8de83f26b1f7 100644 --- a/internal/service/securitylake/securitylake_test.go +++ b/internal/service/securitylake/securitylake_test.go @@ -50,6 +50,7 @@ func TestAccSecurityLake_serial(t *testing.T) { "replication": testAccDataLake_replication, }, "Subscriber": { + "accessType": testAccSubscriber_accessType, "basic": testAccSubscriber_basic, "customLogs": testAccSubscriber_customLogSource, "disappears": testAccSubscriber_disappears, diff --git a/internal/service/securitylake/subscriber.go b/internal/service/securitylake/subscriber.go index 0ca4bcce825c..3f5a04310a56 100644 --- a/internal/service/securitylake/subscriber.go +++ b/internal/service/securitylake/subscriber.go @@ -21,7 +21,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/listplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" - "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" @@ -70,7 +69,9 @@ func (r *subscriberResource) Schema(ctx context.Context, request resource.Schema "access_type": schema.StringAttribute{ Optional: true, Computed: true, - Default: stringdefault.StaticString("S3"), + PlanModifiers: []planmodifier.String{ + stringplanmodifier.UseStateForUnknown(), + }, }, "arn": framework.ARNAttributeComputedOnly(), "resource_share_arn": framework.ARNAttributeComputedOnly(), @@ -113,10 +114,11 @@ func (r *subscriberResource) Schema(ctx context.Context, request resource.Schema NestedObject: schema.NestedBlockObject{ Attributes: map[string]schema.Attribute{ "source_name": schema.StringAttribute{ - Optional: true, + Required: true, }, "source_version": schema.StringAttribute{ Optional: true, + Computed: true, }, }, }, @@ -155,10 +157,11 @@ func (r *subscriberResource) Schema(ctx context.Context, request resource.Schema }, }, "source_name": schema.StringAttribute{ - Optional: true, + Required: true, }, "source_version": schema.StringAttribute{ Optional: true, + Computed: true, }, }, }, @@ -218,7 +221,7 @@ func (r *subscriberResource) Create(ctx context.Context, request resource.Create } // Additional fields - if !data.AccessTypes.IsNull() { + if !data.AccessTypes.IsUnknown() && !data.AccessTypes.IsNull() { input.AccessTypes = []awstypes.AccessType{awstypes.AccessType(data.AccessTypes.ValueString())} } input.Sources = sources @@ -276,12 +279,11 @@ func (r *subscriberResource) Read(ctx context.Context, request resource.ReadRequ return } - subscriber := output - data.ID = fwflex.StringToFramework(ctx, subscriber.SubscriberId) - data.SubscriberArn = fwflex.StringToFramework(ctx, subscriber.SubscriberArn) + data.ID = fwflex.StringToFramework(ctx, output.SubscriberId) + data.SubscriberArn = fwflex.StringToFramework(ctx, output.SubscriberArn) var subscriberIdentity subscriberIdentityModel - response.Diagnostics.Append(fwflex.Flatten(ctx, subscriber.SubscriberIdentity, &subscriberIdentity)...) + response.Diagnostics.Append(fwflex.Flatten(ctx, output.SubscriberIdentity, &subscriberIdentity)...) if response.Diagnostics.HasError() { return } @@ -290,7 +292,7 @@ func (r *subscriberResource) Read(ctx context.Context, request resource.ReadRequ setTagsOut(ctx, Tags(tags)) } - response.Diagnostics.Append(data.refreshFromOutput(ctx, subscriberIdentity, subscriber)...) + response.Diagnostics.Append(data.refreshFromOutput(ctx, subscriberIdentity, output)...) response.Diagnostics.Append(response.State.Set(ctx, &data)...) } diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index 45cb10c121cf..75a9691c8da7 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -8,6 +8,8 @@ import ( "fmt" "testing" + "github.com/YakDriver/regexache" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/securitylake/types" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" @@ -41,19 +43,37 @@ func testAccSubscriber_basic(t *testing.T) { testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), resource.TestCheckResourceAttr(resourceName, "access_type", "S3"), + func(s *terraform.State) error { + id := aws.ToString(subscriber.SubscriberId) + return acctest.CheckResourceAttrRegionalARN(resourceName, "arn", "securitylake", fmt.Sprintf("subscriber/%s", id))(s) + }, + resource.TestCheckResourceAttr(resourceName, "resource_share_arn", ""), + func(s *terraform.State) error { + id := aws.ToString(subscriber.SubscriberId) + return acctest.CheckResourceAttrGlobalARN(resourceName, "role_arn", "iam", fmt.Sprintf("role/AmazonSecurityLake-%s", id))(s) + }, + acctest.MatchResourceAttrGlobalARNNoAccount(resourceName, "s3_bucket_arn", "s3", regexache.MustCompile(fmt.Sprintf(`aws-security-data-lake-%s-[a-z0-9]{30}$`, acctest.Region()))), + resource.TestCheckResourceAttrSet(resourceName, "id"), + func(s *terraform.State) error { + id := aws.ToString(subscriber.SubscriberId) + return resource.TestCheckResourceAttr(resourceName, "id", id)(s) + }, + resource.TestCheckNoResourceAttr(resourceName, "subscriber_description"), + resource.TestCheckNoResourceAttr(resourceName, "resource_share_name"), + resource.TestCheckNoResourceAttr(resourceName, "subscriber_endpoint"), resource.TestCheckResourceAttr(resourceName, "source.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_name", "ROUTE53"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_version", "1.0"), + resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_version", "2.0"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.#", "1"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.0.external_id", "example"), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, }, }) @@ -80,14 +100,6 @@ func testAccSubscriber_disappears(t *testing.T) { Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), acctest.CheckFrameworkResourceDisappears(ctx, acctest.Provider, tfsecuritylake.ResourceSubscriber, resourceName), - resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), - resource.TestCheckResourceAttr(resourceName, "access_type", "S3"), - resource.TestCheckResourceAttr(resourceName, "source.#", "1"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.#", "1"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_name", "ROUTE53"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_version", "1.0"), - resource.TestCheckResourceAttr(resourceName, "subscriber_identity.#", "1"), - resource.TestCheckResourceAttr(resourceName, "subscriber_identity.0.external_id", "example"), ), ExpectNonEmptyPlan: true, }, @@ -120,15 +132,45 @@ func testAccSubscriber_customLogSource(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "source.0.custom_log_source_resource.#", "1"), resource.TestCheckResourceAttrPair(resourceName, "source.0.custom_log_source_resource.0.source_name", "aws_securitylake_custom_log_source.test", "source_name"), resource.TestCheckResourceAttrPair(resourceName, "source.0.custom_log_source_resource.0.source_version", "aws_securitylake_custom_log_source.test", "source_version"), - resource.TestCheckResourceAttr(resourceName, "subscriber_identity.#", "1"), - resource.TestCheckResourceAttr(resourceName, "subscriber_identity.0.external_id", "example"), ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testAccSubscriber_accessType(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_subscriber.test" + var subscriber types.SubscriberResource + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckSubscriberDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccSubscriberConfig_accessType(rName, "S3"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckSubscriberExists(ctx, resourceName, &subscriber), + resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), + resource.TestCheckResourceAttr(resourceName, "access_type", "S3"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, }, }) @@ -159,10 +201,9 @@ func testAccSubscriber_tags(t *testing.T) { ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, { Config: testAccSubscriberConfig_tags2(rName, "key1", "value1updated", "key2", "value2"), @@ -216,10 +257,9 @@ func testAccSubscriber_update(t *testing.T) { ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, { Config: testAccSubscriberConfig_update(rName), @@ -236,10 +276,9 @@ func testAccSubscriber_update(t *testing.T) { ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{}, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, }, }, }) @@ -293,14 +332,13 @@ func testAccCheckSubscriberExists(ctx context.Context, n string, v *types.Subscr } func testAccSubscriberConfig_basic(rName string) string { - return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_subscriber" "test" { subscriber_name = %[1]q - access_type = "S3" source { aws_log_source_resource { source_name = "ROUTE53" - source_version = "1.0" } } subscriber_identity { @@ -316,7 +354,6 @@ resource "aws_securitylake_aws_log_source" "test" { accounts = [data.aws_caller_identity.current.account_id] regions = [data.aws_region.current.name] source_name = "ROUTE53" - source_version = "1.0" } depends_on = [aws_securitylake_data_lake.test] } @@ -345,9 +382,7 @@ resource "aws_securitylake_subscriber" "test" { } resource "aws_securitylake_custom_log_source" "test" { - source_name = "windows-sysmon" - source_version = "1.0" - event_classes = ["FILE_ACTIVITY"] + source_name = %[1]q configuration { crawler_configuration { @@ -355,7 +390,7 @@ resource "aws_securitylake_custom_log_source" "test" { } provider_identity { - external_id = "windows-sysmon-test" + external_id = "%[1]s-test" principal = data.aws_caller_identity.current.account_id } } @@ -364,7 +399,7 @@ resource "aws_securitylake_custom_log_source" "test" { } resource "aws_iam_role" "test" { - name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" + name = %[1]q path = "/service-role/" assume_role_policy = < Date: Thu, 2 May 2024 09:41:43 -0700 Subject: [PATCH 18/24] Prevent panic when API call returns error --- internal/service/securitylake/data_lake.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/service/securitylake/data_lake.go b/internal/service/securitylake/data_lake.go index f3f726c74cd7..071bba399fea 100644 --- a/internal/service/securitylake/data_lake.go +++ b/internal/service/securitylake/data_lake.go @@ -575,8 +575,13 @@ func retryDataLakeConflictWithMutex[T any](ctx context.Context, timeout time.Dur conns.GlobalMutexKV.Lock(dataLakeMutexKey) defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) - result, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, timeout, func() (any, error) { + raw, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, timeout, func() (any, error) { return f() }) - return result.(T), err + if err != nil { + var zero T + return zero, err + } + + return raw.(T), err } From 8413eb036826a5ce8f39bde631445375ffb3e7ca Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 2 May 2024 14:33:17 -0700 Subject: [PATCH 19/24] Converts `source` from List to Set and adds test for handling multiple sources --- .../service/securitylake/securitylake_test.go | 14 +- internal/service/securitylake/subscriber.go | 101 ++++-- .../service/securitylake/subscriber_test.go | 342 ++++++++++++++++-- 3 files changed, 395 insertions(+), 62 deletions(-) diff --git a/internal/service/securitylake/securitylake_test.go b/internal/service/securitylake/securitylake_test.go index 8de83f26b1f7..d47b3aee43ad 100644 --- a/internal/service/securitylake/securitylake_test.go +++ b/internal/service/securitylake/securitylake_test.go @@ -50,12 +50,14 @@ func TestAccSecurityLake_serial(t *testing.T) { "replication": testAccDataLake_replication, }, "Subscriber": { - "accessType": testAccSubscriber_accessType, - "basic": testAccSubscriber_basic, - "customLogs": testAccSubscriber_customLogSource, - "disappears": testAccSubscriber_disappears, - "tags": testAccSubscriber_tags, - "updated": testAccSubscriber_update, + "accessType": testAccSubscriber_accessType, + "basic": testAccSubscriber_basic, + "customLogs": testAccSubscriber_customLogSource, + "disappears": testAccSubscriber_disappears, + "multipleSources": testAccSubscriber_multipleSources, + "tags": testAccSubscriber_tags, + "updated": testAccSubscriber_update, + "migrateSource": testAccSubscriber_migrate_source, }, "SubscriberNotification": { "basic": testAccSubscriberNotification_basic, diff --git a/internal/service/securitylake/subscriber.go b/internal/service/securitylake/subscriber.go index 3f5a04310a56..f80790aa1162 100644 --- a/internal/service/securitylake/subscriber.go +++ b/internal/service/securitylake/subscriber.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/aws-sdk-go-base/v2/tfawserr" "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" + "github.com/hashicorp/terraform-plugin-framework-validators/setvalidator" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/path" @@ -32,6 +33,7 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/framework" fwflex "github.com/hashicorp/terraform-provider-aws/internal/framework/flex" fwtypes "github.com/hashicorp/terraform-provider-aws/internal/framework/types" + "github.com/hashicorp/terraform-provider-aws/internal/slices" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/names" @@ -100,10 +102,10 @@ func (r *subscriberResource) Schema(ctx context.Context, request resource.Schema names.AttrTagsAll: tftags.TagsAttributeComputedOnly(), }, Blocks: map[string]schema.Block{ - "source": schema.ListNestedBlock{ - Validators: []validator.List{ - listvalidator.IsRequired(), - listvalidator.SizeAtLeast(1), + "source": schema.SetNestedBlock{ + Validators: []validator.Set{ + setvalidator.IsRequired(), + setvalidator.SizeAtLeast(1), }, NestedObject: schema.NestedBlockObject{ Blocks: map[string]schema.Block{ @@ -242,7 +244,7 @@ func (r *subscriberResource) Create(ctx context.Context, request resource.Create subscriber, err := waitSubscriberCreated(ctx, conn, data.ID.ValueString(), r.CreateTimeout(ctx, data.Timeouts)) if err != nil { - response.Diagnostics.AddError(fmt.Sprintf("waiting for Security Lake Subscriber (%s) create", data.ID.ValueString()), err.Error()) + response.Diagnostics.AddError(fmt.Sprintf("waiting for Security Lake Subscriber (%s) to be created", data.ID.ValueString()), err.Error()) return } @@ -347,16 +349,20 @@ func (r *subscriberResource) Update(ctx context.Context, request resource.Update return } - _, err = waitSubscriberUpdated(ctx, conn, new.ID.ValueString(), r.CreateTimeout(ctx, new.Timeouts)) + subscriber, err := waitSubscriberUpdated(ctx, conn, new.ID.ValueString(), r.CreateTimeout(ctx, new.Timeouts)) if err != nil { - response.Diagnostics.AddError(fmt.Sprintf("waiting for Security Lake Subscriber (%s) create", new.ID.ValueString()), err.Error()) + response.Diagnostics.AddError(fmt.Sprintf("waiting for Security Lake Subscriber (%s) to be updated", new.ID.ValueString()), err.Error()) + + return + } + var subscriberIdentity subscriberIdentityModel + response.Diagnostics.Append(fwflex.Flatten(ctx, subscriber.SubscriberIdentity, &subscriberIdentity)...) + if response.Diagnostics.HasError() { return } - new.ResourceShareArn = old.ResourceShareArn - new.ResourceShareName = old.ResourceShareName - new.SubscriberEndpoint = old.SubscriberEndpoint + response.Diagnostics.Append(new.refreshFromOutput(ctx, subscriberIdentity, subscriber)...) } response.Diagnostics.Append(response.State.Set(ctx, &new)...) @@ -510,7 +516,7 @@ func expandSubscriptionValueSources(ctx context.Context, subscriberSourcesModels if !item.AwsLogSourceResource.IsNull() && (len(item.AwsLogSourceResource.Elements()) > 0) { var awsLogSources []awsLogSubscriberSourceModel diags.Append(item.AwsLogSourceResource.ElementsAs(ctx, &awsLogSources, false)...) - subscriberLogSource := expandSubscriberLogSourceSource(ctx, awsLogSources) + subscriberLogSource := expandSubscriberAwsLogSourceSource(ctx, awsLogSources) sources = append(sources, subscriberLogSource) } if (!item.CustomLogSourceResource.IsNull()) && (len(item.CustomLogSourceResource.Elements()) > 0) { @@ -524,7 +530,7 @@ func expandSubscriptionValueSources(ctx context.Context, subscriberSourcesModels return sources, diags } -func expandSubscriberLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { +func expandSubscriberAwsLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { if len(awsLogSources) == 0 { return nil } @@ -553,38 +559,63 @@ func expandSubscriberCustomLogSourceSource(ctx context.Context, customLogSources return customLogSourceResource } -func flattenSubscriberSourcesModel(ctx context.Context, apiObject []awstypes.LogSourceResource) (types.List, diag.Diagnostics) { +func flattenSubscriberSources(ctx context.Context, apiObject []awstypes.LogSourceResource) (types.Set, diag.Diagnostics) { var diags diag.Diagnostics elemType := types.ObjectType{AttrTypes: subscriberSourcesModelAttrTypes} + result := types.SetNull(elemType) - obj := map[string]attr.Value{} + var elems []types.Object for _, item := range apiObject { - switch v := item.(type) { - case *awstypes.LogSourceResourceMemberAwsLogSource: - subscriberLogSource, d := flattenSubscriberLogSourceResourceModel(ctx, &v.Value, nil, "aws") - diags.Append(d...) - obj = map[string]attr.Value{ - "aws_log_source_resource": subscriberLogSource, - "custom_log_source_resource": types.ListNull(customLogSubscriberSourceModelAttrTypes), - } - case *awstypes.LogSourceResourceMemberCustomLogSource: - subscriberLogSource, d := flattenSubscriberLogSourceResourceModel(ctx, nil, &v.Value, "custom") - diags.Append(d...) - obj = map[string]attr.Value{ - "aws_log_source_resource": types.ListNull(logSubscriberSourcesModelAttrTypes), - "custom_log_source_resource": subscriberLogSource, - } + elem, d := flattenSubscriberSourcesModel(ctx, item) + diags.Append(d...) + if d.HasError() { + return result, diags } + elems = append(elems, elem) } - objVal, d := types.ObjectValue(subscriberSourcesModelAttrTypes, obj) + setVal, d := types.SetValue(elemType, slices.ApplyToAll(elems, func(o types.Object) attr.Value { + return o + })) diags.Append(d...) - listVal, d := types.ListValue(elemType, []attr.Value{objVal}) + return setVal, diags +} + +func flattenSubscriberSourcesModel(ctx context.Context, apiObject awstypes.LogSourceResource) (types.Object, diag.Diagnostics) { + var diags diag.Diagnostics + result := types.ObjectUnknown(subscriberSourcesModelAttrTypes) + + obj := map[string]attr.Value{} + + switch v := apiObject.(type) { + case *awstypes.LogSourceResourceMemberAwsLogSource: + subscriberLogSource, d := flattenSubscriberLogSourceResourceModel(ctx, &v.Value, nil, "aws") + diags.Append(d...) + if d.HasError() { + return result, diags + } + obj = map[string]attr.Value{ + "aws_log_source_resource": subscriberLogSource, + "custom_log_source_resource": types.ListNull(customLogSubscriberSourceModelAttrTypes), + } + case *awstypes.LogSourceResourceMemberCustomLogSource: + subscriberLogSource, d := flattenSubscriberLogSourceResourceModel(ctx, nil, &v.Value, "custom") + diags.Append(d...) + if d.HasError() { + return result, diags + } + obj = map[string]attr.Value{ + "aws_log_source_resource": types.ListNull(logSubscriberSourcesModelAttrTypes), + "custom_log_source_resource": subscriberLogSource, + } + } + + result, d := types.ObjectValue(subscriberSourcesModelAttrTypes, obj) diags.Append(d...) - return listVal, diags + return result, diags } func flattenSubscriberLogSourceResourceModel(ctx context.Context, awsLogApiObject *awstypes.AwsLogSourceResource, customLogApiObject *awstypes.CustomLogSourceResource, logSourceType string) (types.List, diag.Diagnostics) { @@ -706,7 +737,7 @@ type subscriberResourceModel struct { AccessTypes types.String `tfsdk:"access_type"` SubscriberArn types.String `tfsdk:"arn"` ID types.String `tfsdk:"id"` - Sources types.List `tfsdk:"source"` + Sources types.Set `tfsdk:"source"` SubscriberDescription types.String `tfsdk:"subscriber_description"` SubscriberIdentity fwtypes.ListNestedObjectValueOf[subscriberIdentityModel] `tfsdk:"subscriber_identity"` SubscriberName types.String `tfsdk:"subscriber_name"` @@ -764,14 +795,14 @@ func (rd *subscriberResourceModel) refreshFromOutput(ctx context.Context, subscr rd.AccessTypes = fwflex.StringValueToFramework(ctx, subscriber.AccessTypes[0]) rd.SubscriberIdentity = fwtypes.NewListNestedObjectValueOfPtrMust(ctx, &subscriberIdentity) - sourcesOutput, d := flattenSubscriberSourcesModel(ctx, subscriber.Sources) - diags.Append(d...) rd.ResourceShareArn = fwflex.StringToFrameworkLegacy(ctx, subscriber.ResourceShareArn) rd.ResourceShareName = fwflex.StringToFramework(ctx, subscriber.ResourceShareName) rd.S3BucketArn = fwflex.StringToFramework(ctx, subscriber.S3BucketArn) rd.SubscriberEndpoint = fwflex.StringToFramework(ctx, subscriber.SubscriberEndpoint) rd.SubscriberStatus = fwflex.StringValueToFramework(ctx, subscriber.SubscriberStatus) rd.RoleArn = fwflex.StringToFramework(ctx, subscriber.RoleArn) + sourcesOutput, d := flattenSubscriberSources(ctx, subscriber.Sources) + diags.Append(d...) rd.Sources = sourcesOutput rd.SubscriberName = fwflex.StringToFramework(ctx, subscriber.SubscriberName) rd.SubscriberDescription = fwflex.StringToFramework(ctx, subscriber.SubscriberDescription) diff --git a/internal/service/securitylake/subscriber_test.go b/internal/service/securitylake/subscriber_test.go index 75a9691c8da7..d8a637260e1c 100644 --- a/internal/service/securitylake/subscriber_test.go +++ b/internal/service/securitylake/subscriber_test.go @@ -13,7 +13,11 @@ import ( "github.com/aws/aws-sdk-go-v2/service/securitylake/types" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + "github.com/hashicorp/terraform-plugin-testing/statecheck" "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" "github.com/hashicorp/terraform-provider-aws/internal/acctest" "github.com/hashicorp/terraform-provider-aws/internal/conns" tfsecuritylake "github.com/hashicorp/terraform-provider-aws/internal/service/securitylake" @@ -112,6 +116,7 @@ func testAccSubscriber_customLogSource(t *testing.T) { resourceName := "aws_securitylake_subscriber.test" var subscriber types.SubscriberResource rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + sourceName := randomCustomLogSourceName() resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -124,7 +129,7 @@ func testAccSubscriber_customLogSource(t *testing.T) { CheckDestroy: testAccCheckSubscriberDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccSubscriberConfig_customLog(rName), + Config: testAccSubscriberConfig_customLog(rName, sourceName), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckSubscriberExists(ctx, resourceName, &subscriber), resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), @@ -251,7 +256,6 @@ func testAccSubscriber_update(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "source.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_name", "ROUTE53"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_version", "1.0"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.#", "1"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.0.external_id", "example"), ), @@ -270,7 +274,6 @@ func testAccSubscriber_update(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "source.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.#", "1"), resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_name", "VPC_FLOW"), - resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_version", "1.0"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.#", "1"), resource.TestCheckResourceAttr(resourceName, "subscriber_identity.0.external_id", "updated"), ), @@ -284,6 +287,188 @@ func testAccSubscriber_update(t *testing.T) { }) } +func testAccSubscriber_multipleSources(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_subscriber.test" + var subscriber types.SubscriberResource + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckSubscriberDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccSubscriberConfig_sources2(rName, "VPC_FLOW", "ROUTE53"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckSubscriberExists(ctx, resourceName, &subscriber), + resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, + tfjsonpath.New("source"), + knownvalue.SetExact([]knownvalue.Check{ + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectPartial(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("VPC_FLOW"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectPartial(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("ROUTE53"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + }), + ), + }, + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccSubscriberConfig_sources2(rName, "ROUTE53", "VPC_FLOW"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + ExpectNonEmptyPlan: false, + }, + { + Config: testAccSubscriberConfig_sources1(rName, "ROUTE53"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckSubscriberExists(ctx, resourceName, &subscriber), + resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, + tfjsonpath.New("source"), + knownvalue.SetExact([]knownvalue.Check{ + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectPartial(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("ROUTE53"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + }), + ), + }, + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccSubscriberConfig_sources2(rName, "VPC_FLOW", "S3_DATA"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckSubscriberExists(ctx, resourceName, &subscriber), + resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, + tfjsonpath.New("source"), + knownvalue.SetExact([]knownvalue.Check{ + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectPartial(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("VPC_FLOW"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectPartial(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("S3_DATA"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + }), + ), + }, + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testAccSubscriber_migrate_source(t *testing.T) { + ctx := acctest.Context(t) + resourceName := "aws_securitylake_subscriber.test" + var subscriber types.SubscriberResource + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, names.SecurityLake) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, names.SecurityLakeServiceID), + CheckDestroy: testAccCheckSubscriberDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.47.0", + }, + }, + Config: testAccSubscriberConfig_migrate_basic(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckSubscriberExists(ctx, resourceName, &subscriber), + resource.TestCheckResourceAttr(resourceName, "subscriber_name", rName), + resource.TestCheckResourceAttr(resourceName, "source.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.#", "1"), + resource.TestCheckResourceAttr(resourceName, "source.0.aws_log_source_resource.0.source_name", "ROUTE53"), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, + tfjsonpath.New("source"), + knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "aws_log_source_resource": knownvalue.ListExact([]knownvalue.Check{ + knownvalue.ObjectExact(map[string]knownvalue.Check{ + "source_name": knownvalue.StringExact("ROUTE53"), + "source_version": knownvalue.StringExact("2.0"), + }), + }), + "custom_log_source_resource": knownvalue.ListSizeExact(0), + }), + }), + ), + }, + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccSubscriberConfig_basic(rName), + PlanOnly: true, + }, + }, + }) +} + func testAccCheckSubscriberDestroy(ctx context.Context) resource.TestCheckFunc { return func(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).SecurityLakeClient(ctx) @@ -338,7 +523,7 @@ resource "aws_securitylake_subscriber" "test" { subscriber_name = %[1]q source { aws_log_source_resource { - source_name = "ROUTE53" + source_name = "ROUTE53" } } subscriber_identity { @@ -351,9 +536,9 @@ resource "aws_securitylake_subscriber" "test" { resource "aws_securitylake_aws_log_source" "test" { source { - accounts = [data.aws_caller_identity.current.account_id] - regions = [data.aws_region.current.name] - source_name = "ROUTE53" + accounts = [data.aws_caller_identity.current.account_id] + regions = [data.aws_region.current.name] + source_name = "ROUTE53" } depends_on = [aws_securitylake_data_lake.test] } @@ -362,8 +547,9 @@ data "aws_region" "current" {} `, rName)) } -func testAccSubscriberConfig_customLog(rName string) string { - return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` +func testAccSubscriberConfig_customLog(rName, sourceName string) string { + return acctest.ConfigCompose( + testAccDataLakeConfig_basic(), fmt.Sprintf(` resource "aws_securitylake_subscriber" "test" { subscriber_name = %[1]q subscriber_description = "Example" @@ -382,7 +568,8 @@ resource "aws_securitylake_subscriber" "test" { } resource "aws_securitylake_custom_log_source" "test" { - source_name = %[1]q + source_name = %[2]q + source_version = "1.5" configuration { crawler_configuration { @@ -390,7 +577,7 @@ resource "aws_securitylake_custom_log_source" "test" { } provider_identity { - external_id = "%[1]s-test" + external_id = "%[2]s-test" principal = data.aws_caller_identity.current.account_id } } @@ -399,7 +586,7 @@ resource "aws_securitylake_custom_log_source" "test" { } resource "aws_iam_role" "test" { - name = %[1]q + name = %[2]q path = "/service-role/" assume_role_policy = < Date: Thu, 2 May 2024 15:07:52 -0700 Subject: [PATCH 20/24] Updates CHANGELOG --- .changelog/36268.txt | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/.changelog/36268.txt b/.changelog/36268.txt index 09a514f339d3..d844c7157551 100644 --- a/.changelog/36268.txt +++ b/.changelog/36268.txt @@ -1,3 +1,31 @@ ```release-note:bug resource/aws_securitylake_subscriber: Allow more than one log source ``` + +```release-note:bug +resource/aws_securitylake_aws_log_source: Correctly handles unspecified `source_version` +``` + +```release-note:bug +resource/aws_securitylake_aws_log_source: Prevents errors when creating multiple log sources concurrently +``` + +```release-note:bug +resource/aws_securitylake_custom_log_source: Validates length of `source_name` parameter +``` + +```release-note:bug +resource/aws_securitylake_custom_log_source: Prevents errors when creating multiple log sources concurrently +``` + +```release-note:bug +resource/aws_securitylake_subscriber: Correctly handles unspecified `access_type` +``` + +```release-note:bug +resource/aws_securitylake_subscriber: Correctly requires `source_name` parameter for `aws_log_source_resource` and `custom_log_source_resource` +``` + +```release-note:bug +resource/aws_securitylake_subscriber: Correctly handles unspecified `source_version` parameter for `aws_log_source_resource` and `custom_log_source_resource` +``` From c9ee390e35a5f09cf1a9cf6b519ce4eef0f254d1 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 2 May 2024 16:56:53 -0700 Subject: [PATCH 21/24] Cleanup --- .../subscriber_notification_test.go | 78 ++++++++++--------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/internal/service/securitylake/subscriber_notification_test.go b/internal/service/securitylake/subscriber_notification_test.go index 30a694cc70c5..e26f652f0ed8 100644 --- a/internal/service/securitylake/subscriber_notification_test.go +++ b/internal/service/securitylake/subscriber_notification_test.go @@ -229,8 +229,6 @@ func testAccCheckSubscriberNotificationExists(ctx context.Context, n string) res func testAccSubscriberNotification_config(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` - - resource "aws_apigatewayv2_api" "test" { name = %[1]q protocol_type = "HTTP" @@ -242,14 +240,16 @@ resource "aws_iam_role" "test" { assume_role_policy = < Date: Thu, 2 May 2024 17:45:49 -0700 Subject: [PATCH 22/24] Parameterizes roles for Subscriber Notification tests --- .../subscriber_notification_test.go | 151 ++++++++---------- 1 file changed, 69 insertions(+), 82 deletions(-) diff --git a/internal/service/securitylake/subscriber_notification_test.go b/internal/service/securitylake/subscriber_notification_test.go index e26f652f0ed8..9af420a1e9f8 100644 --- a/internal/service/securitylake/subscriber_notification_test.go +++ b/internal/service/securitylake/subscriber_notification_test.go @@ -8,7 +8,6 @@ import ( "fmt" "testing" - sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-provider-aws/internal/acctest" @@ -22,7 +21,7 @@ func testAccSubscriberNotification_basic(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_subscriber_notification.test" - rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName := randomCustomLogSourceName() resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -57,7 +56,7 @@ func testAccSubscriberNotification_https(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_subscriber_notification.test" - rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName := randomCustomLogSourceName() resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -94,7 +93,7 @@ func testAccSubscriberNotification_disappears(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_subscriber_notification.test" - rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName := randomCustomLogSourceName() resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -111,8 +110,6 @@ func testAccSubscriberNotification_disappears(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckSubscriberNotificationExists(ctx, resourceName), acctest.CheckFrameworkResourceDisappears(ctx, acctest.Provider, tfsecuritylake.ResourceSubscriberNotification, resourceName), - resource.TestCheckResourceAttr(resourceName, "configuration.#", "1"), - resource.TestCheckResourceAttr(resourceName, "configuration.0.sqs_notification_configuration.#", "1"), ), ExpectNonEmptyPlan: true, }, @@ -123,7 +120,7 @@ func testAccSubscriberNotification_disappears(t *testing.T) { func testAccSubscriberNotification_update(t *testing.T) { ctx := acctest.Context(t) resourceName := "aws_securitylake_subscriber_notification.test" - rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + rName := randomCustomLogSourceName() resource.Test(t, resource.TestCase{ PreCheck: func() { @@ -229,52 +226,73 @@ func testAccCheckSubscriberNotificationExists(ctx context.Context, n string) res func testAccSubscriberNotification_config(rName string) string { return acctest.ConfigCompose(testAccDataLakeConfig_basic(), fmt.Sprintf(` -resource "aws_apigatewayv2_api" "test" { - name = %[1]q - protocol_type = "HTTP" +resource "aws_securitylake_subscriber" "test" { + subscriber_name = %[1]q + + source { + custom_log_source_resource { + source_name = aws_securitylake_custom_log_source.test.source_name + source_version = aws_securitylake_custom_log_source.test.source_version + } + } + subscriber_identity { + external_id = "example" + principal = data.aws_caller_identity.current.account_id + } +} + +resource "aws_securitylake_custom_log_source" "test" { + source_name = %[1]q + + configuration { + crawler_configuration { + role_arn = aws_iam_role.test.arn + } + + provider_identity { + external_id = "%[1]s-test" + principal = data.aws_caller_identity.current.account_id + } + } + + depends_on = [aws_securitylake_data_lake.test, aws_iam_role.test] } resource "aws_iam_role" "test" { - name = "AmazonSecurityLakeCustomDataGlueCrawler-windows-sysmon" + name = %[1]q path = "/service-role/" assume_role_policy = < Date: Fri, 3 May 2024 10:40:32 -0700 Subject: [PATCH 23/24] Linting fixes --- internal/service/securitylake/aws_log_source.go | 5 ++--- internal/service/securitylake/custom_log_source.go | 5 ++--- internal/service/securitylake/data_lake.go | 12 +++++++----- internal/service/securitylake/subscriber.go | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/service/securitylake/aws_log_source.go b/internal/service/securitylake/aws_log_source.go index 5b60cf198c07..fbc0227e6d4a 100644 --- a/internal/service/securitylake/aws_log_source.go +++ b/internal/service/securitylake/aws_log_source.go @@ -6,7 +6,6 @@ package securitylake import ( "context" "fmt" - "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/securitylake" @@ -118,7 +117,7 @@ func (r *awsLogSourceResource) Create(ctx context.Context, request resource.Crea return } - _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.CreateAwsLogSourceOutput, error) { + _, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.CreateAwsLogSourceOutput, error) { return conn.CreateAwsLogSource(ctx, input) }) @@ -216,7 +215,7 @@ func (r *awsLogSourceResource) Delete(ctx context.Context, request resource.Dele input.Sources = []awstypes.AwsLogSourceConfiguration{*logSource} } - _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.DeleteAwsLogSourceOutput, error) { + _, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.DeleteAwsLogSourceOutput, error) { return conn.DeleteAwsLogSource(ctx, input) }) diff --git a/internal/service/securitylake/custom_log_source.go b/internal/service/securitylake/custom_log_source.go index b8979827e09a..a374ef0615fc 100644 --- a/internal/service/securitylake/custom_log_source.go +++ b/internal/service/securitylake/custom_log_source.go @@ -6,7 +6,6 @@ package securitylake import ( "context" "fmt" - "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/securitylake" @@ -189,7 +188,7 @@ func (r *customLogSourceResource) Create(ctx context.Context, request resource.C return } - output, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.CreateCustomLogSourceOutput, error) { + output, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.CreateCustomLogSourceOutput, error) { return conn.CreateCustomLogSource(ctx, input) }) @@ -273,7 +272,7 @@ func (r *customLogSourceResource) Delete(ctx context.Context, request resource.D return } - _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.DeleteCustomLogSourceOutput, error) { + _, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.DeleteCustomLogSourceOutput, error) { return conn.DeleteCustomLogSource(ctx, input) }) diff --git a/internal/service/securitylake/data_lake.go b/internal/service/securitylake/data_lake.go index 071bba399fea..cb590849a6f9 100644 --- a/internal/service/securitylake/data_lake.go +++ b/internal/service/securitylake/data_lake.go @@ -193,7 +193,7 @@ func (r *dataLakeResource) Create(ctx context.Context, request resource.CreateRe // Additional fields. input.Tags = getTagsIn(ctx) - output, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.CreateDataLakeOutput, error) { + output, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.CreateDataLakeOutput, error) { return conn.CreateDataLake(ctx, input) }) @@ -299,7 +299,7 @@ func (r *dataLakeResource) Update(ctx context.Context, request resource.UpdateRe return } - _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.UpdateDataLakeOutput, error) { + _, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.UpdateDataLakeOutput, error) { return conn.UpdateDataLake(ctx, input) }) @@ -332,7 +332,7 @@ func (r *dataLakeResource) Delete(ctx context.Context, request resource.DeleteRe Regions: []string{errs.Must(regionFromARNString(data.ID.ValueString()))}, } - _, err := retryDataLakeConflictWithMutex(ctx, 2*time.Minute, func() (*securitylake.DeleteDataLakeOutput, error) { + _, err := retryDataLakeConflictWithMutex(ctx, func() (*securitylake.DeleteDataLakeOutput, error) { return conn.DeleteDataLake(ctx, input) }) @@ -571,11 +571,13 @@ type dataLakeReplicationConfigurationModel struct { RoleARN fwtypes.ARN `tfsdk:"role_arn"` } -func retryDataLakeConflictWithMutex[T any](ctx context.Context, timeout time.Duration, f func() (T, error)) (T, error) { +func retryDataLakeConflictWithMutex[T any](ctx context.Context, f func() (T, error)) (T, error) { conns.GlobalMutexKV.Lock(dataLakeMutexKey) defer conns.GlobalMutexKV.Unlock(dataLakeMutexKey) - raw, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, timeout, func() (any, error) { + const dataLakeTimeout = 2 * time.Minute + + raw, err := tfresource.RetryWhenIsA[*awstypes.ConflictException](ctx, dataLakeTimeout, func() (any, error) { return f() }) if err != nil { diff --git a/internal/service/securitylake/subscriber.go b/internal/service/securitylake/subscriber.go index f80790aa1162..9e5c4dc28bd8 100644 --- a/internal/service/securitylake/subscriber.go +++ b/internal/service/securitylake/subscriber.go @@ -530,7 +530,7 @@ func expandSubscriptionValueSources(ctx context.Context, subscriberSourcesModels return sources, diags } -func expandSubscriberAwsLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { +func expandSubscriberAwsLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { //nosemgrep:ci.aws-in-func-name if len(awsLogSources) == 0 { return nil } From d08b70df3483f50db0dcbff9a08d5530780fe8c2 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 May 2024 10:59:41 -0700 Subject: [PATCH 24/24] Fixes `nosemgrep` annotations --- internal/service/securitylake/subscriber.go | 2 +- internal/service/ssm/parameter_test.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/service/securitylake/subscriber.go b/internal/service/securitylake/subscriber.go index 9e5c4dc28bd8..e1606df21b57 100644 --- a/internal/service/securitylake/subscriber.go +++ b/internal/service/securitylake/subscriber.go @@ -530,7 +530,7 @@ func expandSubscriptionValueSources(ctx context.Context, subscriberSourcesModels return sources, diags } -func expandSubscriberAwsLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { //nosemgrep:ci.aws-in-func-name +func expandSubscriberAwsLogSourceSource(ctx context.Context, awsLogSources []awsLogSubscriberSourceModel) *awstypes.LogSourceResourceMemberAwsLogSource { // nosemgrep:ci.aws-in-func-name if len(awsLogSources) == 0 { return nil } diff --git a/internal/service/ssm/parameter_test.go b/internal/service/ssm/parameter_test.go index ab9b730da153..c75cd52b459e 100644 --- a/internal/service/ssm/parameter_test.go +++ b/internal/service/ssm/parameter_test.go @@ -954,8 +954,7 @@ func TestAccSSMParameter_DataType_ec2Image(t *testing.T) { } func TestAccSSMParameter_DataType_ssmIntegration(t *testing.T) { - ctx := //nosemgrep:ci.ssm-in-func-name - acctest.Context(t) + ctx := acctest.Context(t) var param ssm.Parameter webhookName := sdkacctest.RandString(16) rName := fmt.Sprintf("/d9d01087-4a3f-49e0-b0b4-d568d7826553/ssm/integrations/webhook/%s", webhookName)