Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

resource/aws_cloudwatch_log_group: Automatically trim :* suffix from ARN in API response #14214

Merged
merged 2 commits into from
Jul 30, 2020

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jul 16, 2020

Community Note

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

Closes #13046
Closes #13509

Release note for CHANGELOG:

* resource/aws_cloudwatch_log_group: Automatically trim `:*` suffix from `arn` attribute

Previously:

    TestAccAWSDataSyncTask_CloudWatchLogGroupARN: testing.go:684: Step 0 error: errors during apply:

        Error: error creating DataSync Task: ValidationException: 1 validation error detected: Value 'arn:aws:logs:us-west-2:123456789012:log-group:tf-acc-test-4735468151095290255:*' at 'cloudWatchLogGroupArn' failed to satisfy constraint: Member must satisfy regular expression pattern: ^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b):logs:[a-z\-0-9]*:[0-9]{12}:log-group:([^:\*]*)$

Output from acceptance testing (aws_route53_query_log failure related to similar issue #13510):

--- PASS: TestAccAWSCloudWatchLogGroup_disappears (9.19s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix (13.55s)
--- PASS: TestAccAWSCloudWatchLogGroup_generatedName (13.99s)
--- PASS: TestAccAWSCloudWatchLogGroup_basic (15.24s)
--- PASS: TestAccAWSCloudWatchLogGroup_multiple (15.65s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix_retention (21.29s)
--- PASS: TestAccAWSCloudWatchLogGroup_retentionPolicy (24.99s)
--- PASS: TestAccAWSCloudWatchLogGroup_kmsKey (29.00s)
--- PASS: TestAccAWSCloudWatchLogGroup_tagging (35.60s)

--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (225.36s)
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings_kinesis (332.67s)

--- PASS: TestAccAWSAPIGatewayV2Stage_AccessLogSettings (56.73s)

--- PASS: TestAccAWSDataSyncTask_CloudWatchLogGroupARN (304.98s)

--- PASS: TestAccAWSDirectoryServiceLogSubscription_basic (1764.25s)

--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (688.17s)

--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (26.43s)

--- FAIL: TestAccAWSRoute53QueryLog_Basic (42.80s)
    TestAccAWSRoute53QueryLog_Basic: testing.go:684: Step 0 error: errors during apply:

        Error: Provider produced inconsistent final plan

        When expanding the plan for aws_cloudwatch_log_group.test to include new
        values learned so far during apply, provider "aws" produced an invalid new
        value for .name: was
        cty.StringVal("/aws/route53/testaccawsroute53querylog_basic-rsbvm.com"), but
        now cty.StringVal("/aws/route53/testaccawsroute53querylog_basic-rsbvm.com.").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

--- PASS: TestAccAWSStorageGatewayGateway_CloudWatchLogs (220.06s)

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. labels Jul 16, 2020
@bflad bflad added this to the v3.0.0 milestone Jul 16, 2020
@bflad bflad requested a review from a team July 16, 2020 19:45
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/apigateway Issues and PRs that pertain to the apigateway service. service/apigatewayv2 Issues and PRs that pertain to the apigatewayv2 service. service/cloudwatchlogs service/datasync Issues and PRs that pertain to the datasync service. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Jul 16, 2020
website/docs/guides/version-3-upgrade.html.md Outdated Show resolved Hide resolved
website/docs/guides/version-3-upgrade.html.md Outdated Show resolved Hide resolved
website/docs/guides/version-3-upgrade.html.md Outdated Show resolved Hide resolved
@bflad bflad force-pushed the td-aws_cloudwatch_log_group-arn-suffix branch from 310f5c5 to a8cc742 Compare July 25, 2020 01:16
@bflad
Copy link
Contributor Author

bflad commented Jul 25, 2020

Rebased to fix merge conflicts and re-verified:

--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (2265.03s)
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings_kinesis (384.03s)

--- PASS: TestAccAWSAPIGatewayV2Stage_AccessLogSettings (48.86s)

--- PASS: TestAccAWSCloudWatchLogGroup_basic (14.86s)
--- PASS: TestAccAWSCloudWatchLogGroup_disappears (11.99s)
--- PASS: TestAccAWSCloudWatchLogGroup_generatedName (13.52s)
--- PASS: TestAccAWSCloudWatchLogGroup_kmsKey (25.59s)
--- PASS: TestAccAWSCloudWatchLogGroup_multiple (16.88s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix (17.02s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix_retention (20.61s)
--- PASS: TestAccAWSCloudWatchLogGroup_retentionPolicy (22.33s)
--- PASS: TestAccAWSCloudWatchLogGroup_tagging (34.85s)

--- PASS: TestAccAWSDataSyncTask_CloudWatchLogGroupARN (343.63s)

--- PASS: TestAccAWSDirectoryServiceLogSubscription_basic (1801.29s)

--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (663.00s)

--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (24.65s)

--- PASS: TestAccAWSStorageGatewayGateway_CloudWatchLogs (240.30s)

…ARN in API response

Reference: #13046
Reference: #13509

Previously:

```
    TestAccAWSDataSyncTask_CloudWatchLogGroupARN: testing.go:684: Step 0 error: errors during apply:

        Error: error creating DataSync Task: ValidationException: 1 validation error detected: Value 'arn:aws:logs:us-west-2:123456789012:log-group:tf-acc-test-4735468151095290255:*' at 'cloudWatchLogGroupArn' failed to satisfy constraint: Member must satisfy regular expression pattern: ^arn:(aws|aws-cn|aws-us-gov|aws-iso|aws-iso-b):logs:[a-z\-0-9]*:[0-9]{12}:log-group:([^:\*]*)$
```

Output from acceptance testing (`aws_route53_query_log` failure related to similar issue #13510):

```
--- PASS: TestAccAWSCloudWatchLogGroup_disappears (9.19s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix (13.55s)
--- PASS: TestAccAWSCloudWatchLogGroup_generatedName (13.99s)
--- PASS: TestAccAWSCloudWatchLogGroup_basic (15.24s)
--- PASS: TestAccAWSCloudWatchLogGroup_multiple (15.65s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix_retention (21.29s)
--- PASS: TestAccAWSCloudWatchLogGroup_retentionPolicy (24.99s)
--- PASS: TestAccAWSCloudWatchLogGroup_kmsKey (29.00s)
--- PASS: TestAccAWSCloudWatchLogGroup_tagging (35.60s)

--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (225.36s)
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings_kinesis (332.67s)

--- PASS: TestAccAWSAPIGatewayV2Stage_AccessLogSettings (56.73s)

--- PASS: TestAccAWSDataSyncTask_CloudWatchLogGroupARN (304.98s)

--- PASS: TestAccAWSDirectoryServiceLogSubscription_basic (1764.25s)

--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (688.17s)

--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (26.43s)

--- FAIL: TestAccAWSRoute53QueryLog_Basic (42.80s)
    TestAccAWSRoute53QueryLog_Basic: testing.go:684: Step 0 error: errors during apply:

        Error: Provider produced inconsistent final plan

        When expanding the plan for aws_cloudwatch_log_group.test to include new
        values learned so far during apply, provider "aws" produced an invalid new
        value for .name: was
        cty.StringVal("/aws/route53/testaccawsroute53querylog_basic-rsbvm.com"), but
        now cty.StringVal("/aws/route53/testaccawsroute53querylog_basic-rsbvm.com.").

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

--- PASS: TestAccAWSStorageGatewayGateway_CloudWatchLogs (220.06s)
```
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

LGTM (aside from merge conflicts that have come up) 👍

Output of acceptance tests:

--- FAIL: TestAccAWSFlowLog_LogFormat (8.77s) -- related to #14397
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (288.05s)
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings_kinesis (225.13s)
--- PASS: TestAccAWSAPIGatewayStage_basic (398.55s)
--- PASS: TestAccAWSAPIGatewayStage_disappears (58.65s)
--- PASS: TestAccAWSAPIGatewayStage_disappears_ReferencingDeployment (25.91s)
--- PASS: TestAccAWSAPIGatewayV2Stage_AccessLogSettings (109.17s)
--- PASS: TestAccAWSAPIGatewayV2Stage_ClientCertificateIdAndDescription (21.10s)
--- PASS: TestAccAWSAPIGatewayV2Stage_DefaultRouteSettingsHttp (65.88s)
--- PASS: TestAccAWSAPIGatewayV2Stage_DefaultRouteSettingsWebSocket (48.56s)
--- PASS: TestAccAWSAPIGatewayV2Stage_Deployment (16.23s)
--- PASS: TestAccAWSAPIGatewayV2Stage_RouteSettingsHttp (30.19s)
--- PASS: TestAccAWSAPIGatewayV2Stage_RouteSettingsWebSocket (39.12s)
--- PASS: TestAccAWSAPIGatewayV2Stage_StageVariables (49.19s)
--- PASS: TestAccAWSAPIGatewayV2Stage_Tags (69.90s)
--- PASS: TestAccAWSAPIGatewayV2Stage_autoDeployHttp (47.32s)
--- PASS: TestAccAWSAPIGatewayV2Stage_basicHttp (16.37s)
--- PASS: TestAccAWSAPIGatewayV2Stage_basicWebSocket (25.56s)
--- PASS: TestAccAWSAPIGatewayV2Stage_defaultHttpStage (27.42s)
--- PASS: TestAccAWSAPIGatewayV2Stage_disappears (14.07s)
--- PASS: TestAccAWSCloudWatchLogGroup_basic (13.39s)
--- PASS: TestAccAWSCloudWatchLogGroup_disappears (7.12s)
--- PASS: TestAccAWSCloudWatchLogGroup_generatedName (9.27s)
--- PASS: TestAccAWSCloudWatchLogGroup_kmsKey (15.59s)
--- PASS: TestAccAWSCloudWatchLogGroup_multiple (9.20s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix (9.12s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix_retention (14.63s)
--- PASS: TestAccAWSCloudWatchLogGroup_retentionPolicy (15.37s)
--- PASS: TestAccAWSCloudWatchLogGroup_tagging (36.01s)
--- PASS: TestAccAWSDataSyncTask_CloudWatchLogGroupARN (324.81s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_AtimeMtime (327.79s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_BytesPerSecond (337.09s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_Gid (312.85s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_PosixPermissions (314.98s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_PreserveDeletedFiles (296.06s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_Uid (293.08s)
--- PASS: TestAccAWSDataSyncTask_DefaultSyncOptions_VerifyMode (304.82s)
--- PASS: TestAccAWSDataSyncTask_basic (334.72s)
--- PASS: TestAccAWSDataSyncTask_disappears (273.81s)
--- PASS: TestAccAWSDirectoryServiceLogSubscription_basic (1756.88s)
--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (20.50s)
--- PASS: TestAccAWSFlowLog_LogDestinationType_MaxAggregationInterval (10.96s)
--- PASS: TestAccAWSFlowLog_LogDestinationType_S3 (11.56s)
--- PASS: TestAccAWSFlowLog_LogDestinationType_S3_Invalid (7.31s)
--- PASS: TestAccAWSFlowLog_SubnetID (12.19s)
--- PASS: TestAccAWSFlowLog_VPCID (16.78s)
--- PASS: TestAccAWSFlowLog_disappears (19.25s)
--- PASS: TestAccAWSFlowLog_tags (22.37s)
--- SKIP: TestAccAWSDataSyncTask_Tags (0.00s)

@bflad bflad force-pushed the td-aws_cloudwatch_log_group-arn-suffix branch from a8cc742 to 649e609 Compare July 30, 2020 15:50
@bflad
Copy link
Contributor Author

bflad commented Jul 30, 2020

Rebased and re-verified. Will merge once CI is green.

Output from acceptance testing:

--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings (203.18s)
--- PASS: TestAccAWSAPIGatewayStage_accessLogSettings_kinesis (323.02s)

--- PASS: TestAccAWSAPIGatewayV2Stage_AccessLogSettings (54.52s)

--- PASS: TestAccAWSCloudWatchLogGroup_basic (13.29s)
--- PASS: TestAccAWSCloudWatchLogGroup_disappears (11.46s)
--- PASS: TestAccAWSCloudWatchLogGroup_generatedName (17.54s)
--- PASS: TestAccAWSCloudWatchLogGroup_kmsKey (27.65s)
--- PASS: TestAccAWSCloudWatchLogGroup_multiple (17.09s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix (12.35s)
--- PASS: TestAccAWSCloudWatchLogGroup_namePrefix_retention (19.08s)
--- PASS: TestAccAWSCloudWatchLogGroup_retentionPolicy (20.78s)
--- PASS: TestAccAWSCloudWatchLogGroup_tagging (30.79s)

--- PASS: TestAccAWSDataSyncTask_CloudWatchLogGroupARN (260.79s)

--- PASS: TestAccAWSDirectoryServiceLogSubscription_basic (1847.51s)

--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (881.77s)

--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (22.35s)

--- PASS: TestAccAWSStorageGatewayGateway_CloudWatchLogs (249.29s)

@bflad bflad merged commit 9fb4164 into master Jul 30, 2020
@bflad bflad deleted the td-aws_cloudwatch_log_group-arn-suffix branch July 30, 2020 17:05
bflad added a commit that referenced this pull request Jul 30, 2020
@ghost
Copy link

ghost commented Jul 31, 2020

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

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

@ttaveira
Copy link

ttaveira commented Aug 5, 2020

Seems like that this might introduce unexpected changes when the ARN attribute is used to configure other resources (e.g. by using it on aws_storagegateway_gateway.cloudwatch_log_group_arn).

From reading #13509 it makes sense to keep ARNs consistent, but is it expected to add the :* suffix when configuring other resources...? Any suggestions?

@bflad
Copy link
Contributor Author

bflad commented Aug 5, 2020

Hi @ttaveira 👋 Please see the Version 3 Upgrade Guide section on the aws_cloudwatch_log_group resource. In particular, you can re-implement the suffix where necessary with string interpolation, e.g.

"${aws_cloudwatch_log_group.example.arn}:*"

Terraform CLI nor the Terraform AWS Provider will know when it is appropriate to include the suffix as neither has any information about the context when references are used, but generally the inclusion of the unexpected suffix was causing enough user experience problems to necessitate the change.

@olhado
Copy link

olhado commented Aug 7, 2020

This is not awesome, as it breaks public modules that want to support the 2.X AWS provider and the 3.X AWS provider. This has happened to us already. Is there a recommended way to support both?

@bflad
Copy link
Contributor Author

bflad commented Aug 7, 2020

@olhado its not pretty but something like the below should get you started.

To always include the suffix no matter which version:

"${replace(aws_cloudwatch_log_group.example.arn, "/:\\*$/", "")}:*"

e.g. in terraform console

> "${replace("arn:aws:logs:us-east-1:123456789012:example:*", "/:\\*$/", "")}:*"
arn:aws:logs:us-east-1:123456789012:example:*
> "${replace("arn:aws:logs:us-east-1:123456789012:example", "/:\\*$/", "")}:*"
arn:aws:logs:us-east-1:123456789012:example:*

And to always remove the suffix no matter which version:

replace(aws_cloudwatch_log_group.example.arn, "/:\\*$/", "")
> replace("arn:aws:logs:us-east-1:123456789012:example:*", "/:\\*$/", "")
arn:aws:logs:us-east-1:123456789012:example
> replace("arn:aws:logs:us-east-1:123456789012:example", "/:\\*$/", "")
arn:aws:logs:us-east-1:123456789012:example

@olhado
Copy link

olhado commented Aug 7, 2020

In our case, we need the suffix. If I understand correctly, the first option removes the :* if it finds it, and then the interpolation adds it back. This way you don't get :*:* when running in the 2.X world?

@bflad
Copy link
Contributor Author

bflad commented Aug 7, 2020

@olhado correct.

@MeMan-MasterOfTheUniverse

I am confused why the "goal" was to make the ARN consistent. The ARN is the ARN and it's not defined by terraform or the AWS provider. You're changing an identifying string, which makes it not work with the services terraform is used to provision. This change was to address a validation error with data sync and it seems like instead of fixing the issue, the problem statement was just changed instead.

It seems a more appropriate solution here would have been to create an additional attribute that is 'trimmed_arn' or something. Instead, now when I want to use the interpolated ARN, like in a lambda permission policy statement, lambda doesn't register it b/c terraform has invalidated the ARN.

Please revert.

@ghost
Copy link

ghost commented Aug 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. service/apigatewayv2 Issues and PRs that pertain to the apigatewayv2 service. service/datasync Issues and PRs that pertain to the datasync service. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
6 participants