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

[Umbrella] ISO-friendly Tagging #22532

Closed
27 tasks done
YakDriver opened this issue Jan 11, 2022 · 25 comments
Closed
27 tasks done

[Umbrella] ISO-friendly Tagging #22532

YakDriver opened this issue Jan 11, 2022 · 25 comments
Labels
enhancement Requests to existing resources that expand the functionality or scope. partition/aws-iso Pertains to the aws-iso partition. partition/aws-iso-b Pertains to the aws-iso-b partition. stale Old or inactive issues managed by automation, if no further action taken these will get closed.

Comments

@YakDriver
Copy link
Member

YakDriver commented Jan 11, 2022

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Relates #18593

Considerations

Five (at least) considerations relate to the problem of tagging support in non-standard AWS partitions:

  • Tag on Create - When this is available, it's highly desireable. I.e., we want to prioritize its use.
  • Tagging support (at all) - Not working at all contrasts with not supporting tag on create.
  • Default tags - Ideally, practitioners will be able to use default tags even if not all resources in non-standard partitions support tagging.
  • Changing AWS functionality - Ideally, we will not need to change the code when AWS adds levels of tagging support to other partitions. Features will simply start working.
  • Avoid eating errors that should be fatal - Eventual consistency or actual tagging problems should result in errors reported to practitioners. Thus the challenge is to narrowly eat errors that specifically relate to lack of tagging support.

Design

A design to handle these considerations:

Tag on Create

Attempt tag on create

Tags? DTags? Error? Result
0 0 0 👍
0 0 1 Don't ToC if len(tags) == 0
0 1 0 👍
0 1 1 Attempt post-create tag
1 0 0 👍
1 0 1 Attempt post-create tag
1 1 0 👍
1 1 1 Attempt post-create tag

Tag after Create

Only if tag on create fails, check error and, if appropriate, try create without tags.

Tags? DTags? Error? Result
0 1 0 👍
0 1 1 Log error, continue
1 0 0 👍
1 0 1 Fatal error (explicit setting of tags when tagging not possible)
1 1 0 👍
1 1 1 Fatal error (explicit setting of tags when tagging not possible)

Read

  1. Attempt to read tags
  2. If tag read fails, check error and, if appropriate, skip tag read.
Tags? DTags? Error? Result
0 0 0 👍
0 0 1 Regular, non-tag error
0 1 0 👍
0 1 1 Log error, continue

Update

  1. Attempt tag update
  2. If update fails, check error and, if appropriate, skip tag update.
Tags? DTags? Error? Result
0 0 0 👍
0 0 1 Regular, non-tag error
0 1 0 👍
0 1 1 Log error, continue

New or Affected Resource(s)

References

@YakDriver YakDriver added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 11, 2022
@YakDriver YakDriver added partition/aws-iso Pertains to the aws-iso partition. partition/aws-iso-b Pertains to the aws-iso-b partition. labels Jan 11, 2022
@blafrisch
Copy link

aws_cloudwatch_event_rule and aws_lb_listener are additional resources that we know of to have tagging issues in the ISO regions.

@YakDriver
Copy link
Member Author

YakDriver commented Jan 12, 2022

@blafrisch @lorengordon We're doing our best to anticipate the errors but since they often vary from service to service, we are not 100% confident these fixes will guard against the exact errors for all these services. This is attempt #1. 🤞

Now with the guard structure in place, updating for the correct error is pretty trivial. But, as we cannot test ourselves, any help with that would be much appreciated. The error code and message would be ideal.

Here are the error codes that v3.72.0 will be guarding against:

CloudWatch:

  • AccessDenied
  • InternalServiceFault

ECR:

  • AccessDenied
  • InvalidParameter
  • Validation

ECS:

  • AccessDenied
  • InvalidParameter
  • UnsupportedFeature

ELBv2:

  • AccessDenied
  • InvalidConfigurationRequest
  • OperationNotPermitted

EventBridge (CloudWatch Events):

  • AccessDenied
  • InternalException
  • OperationDisabled

IAM

  • AccessDenied
  • InvalidInput
  • ServiceFailure

SNS:

  • AccessDenied
  • AuthorizationError
  • InvalidAction

SQS:

  • AccessDenied
  • AuthorizationError
  • InvalidAction
  • UnsupportedOperation

@egon010
Copy link

egon010 commented Jan 12, 2022

Is there a test suite yet to gather results from various partitions/regions?

@YakDriver
Copy link
Member Author

Yes and no. Formally, we only run daily tests across aws (standard partition) and aws-us-gov (GovCloud), and mostly us-west-2 and us-gov-west-1. We track those results. We don't have access to iso regions in house. There are some similarities between the China and iso partitions. We don't have information from AWS on whether they are the same but that could be a potential avenue for testing.

@lorengordon
Copy link
Contributor

We'll give it a go, thanks for the heads up!

@YakDriver YakDriver added this to the v3.72.0 milestone Jan 12, 2022
@YakDriver
Copy link
Member Author

I'll keep this issue open until we hear back on success and/or failure.

@github-actions
Copy link

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

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

@blafrisch
Copy link

@YakDriver we were able to deploy v3.72.0 and test with the aws_cloudwatch_event_rule and aws_lb_listener resources; both still experienced errors. Note that these messages are hand-transcribed and have some info redacted:

For aws_cloudwatch_event_rule:

Error: error listing tags for EventBridge Rule (arn:aws-iso:events:<REDACTED>:rule/<REDACTED>): UnknownOperationException Operation is disabled in this region
status code: 400

For aws_lb_listener:

Error: error listing tags for (arn:aws-iso:elasticloadbalancing:<REDACTED>:listener/<REDACTED>): ValidationError: Resource type 'listener' does not support tagging
status code: 400

@spavuluri
Copy link
Contributor

@YakDriver can confirm the same error with v3.72.0 in aws-iso.

In the console, the resource appears like its created, at least in the case of cloudwatch event rule. But the apply reports above error.

Testing continues.

@YakDriver
Copy link
Member Author

YakDriver commented Jan 21, 2022

These are services we've heard back about where tags are now not causing problems.

@lorengordon @spavuluri @blafrisch Let us know if you run into problems! Or, if these are working...

(These may differ between iso and iso-b.)

@YakDriver YakDriver removed this from the v3.72.0 milestone Jan 25, 2022
@blafrisch
Copy link

@YakDriver I can confirm that the most recent changes were successful in aws-iso for aws_cloudwatch_event_rule and aws_lb_listener. 🎉

@spavuluri
Copy link
Contributor

@YakDriver no issues encountered for aws_sqs_queue, aws_sns_topic, aws_cloudwatch_metric_alarm, aws_ecr_repository in aws-iso

@YakDriver
Copy link
Member Author

@spavuluri @blafrisch Millions of thanks!!

We have heard back that ECS continues to be a problem but still need to find out what the exact error message is. If anyone has the AWS error code (e.g., UnknownOperationException) and message, that would be super helpful! Once we have that, we can get a fix out quickly.

@YakDriver
Copy link
Member Author

ECS handles tagging differently than (most?) every other service. As a result, it took a different approach than the others but hopefully it should be working now with #23030. If we don't hear back for anyone in the next 30 days on more tagging problems in ISO, I'll close this issue on or after March 8, 2022. Thanks for all the help!

@lorengordon
Copy link
Contributor

@YakDriver I see the PR fixing ECS tags was marked for the v4 release... Does that mean it won't make it into a v3 release?

@YakDriver
Copy link
Member Author

That is likely correct. Any estimate of showstoppers with v4 and ISO?

@lorengordon
Copy link
Contributor

Just makes it harder to test, is all, with all the breaking changes. Folks might need some time to rewrite configs to account for that, before they can report that things are or are not working.

@caleb-devops
Copy link

@YakDriver v3.74.0 did fix most of the errors relating to tagging, however I am receiving the following error during terraform apply for aws_iam_instance_profile resources:

error updating tags for IAM Instance Profile (<REDACTED>): Error tagging resource (<REDACTED>): InvalidAction: Unsupported operation.

@YakDriver
Copy link
Member Author

The ECS fixes have been backported to v3.74 and are available in v3.74.3.

@AbrohamLincoln
Copy link

I ran into this this today with the following resources:
aws_elasticache_subnet_group
aws_elasticache_replication_group

@YakDriver
Copy link
Member Author

@AbrohamLincoln Thank you for letting us know. We'll work on a fix.

@YakDriver
Copy link
Member Author

@AbrohamLincoln For ElastiCache fixes, see #23980. When 4.9 is released (next week I believe), please give it a test and let us know if it works. We are unable to test in iso or iso-b on our own.

@cvockrodt
Copy link

I've seen some differing information here regarding IAM tags

You cannot tag the following resources in aws-iso (not sure about aws-iso-b:

  • IAM SAML identity providers
  • Instance Profiles
  • OpenID Connect (OIDC) identity providers
  • Policies
  • Server certificates
  • Virtual MFA devices

This affects the ability to use provider level default tags when managing any of the above resources.

Copy link

github-actions bot commented Jun 4, 2024

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

@github-actions github-actions bot added the stale Old or inactive issues managed by automation, if no further action taken these will get closed. label Jun 4, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2024
Copy link

github-actions bot commented Aug 7, 2024

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. partition/aws-iso Pertains to the aws-iso partition. partition/aws-iso-b Pertains to the aws-iso-b partition. stale Old or inactive issues managed by automation, if no further action taken these will get closed.
Projects
None yet
Development

No branches or pull requests

8 participants