-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Conversation
310f5c5
to
a8cc742
Compare
Rebased to fix merge conflicts and re-verified:
|
…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) ```
There was a problem hiding this 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)
a8cc742
to
649e609
Compare
Rebased and re-verified. Will merge once CI is green. Output from acceptance testing:
|
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! |
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 |
Hi @ttaveira 👋 Please see the Version 3 Upgrade Guide section on the "${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. |
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? |
@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 > "${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 |
In our case, we need the suffix. If I understand correctly, the first option removes the |
@olhado correct. |
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. |
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! |
Community Note
Closes #13046
Closes #13509
Release note for CHANGELOG:
Previously:
Output from acceptance testing (
aws_route53_query_log
failure related to similar issue #13510):