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

Proposal: Argument to create instance tags after launch, rather than only hardcoded partition/region restrictions #4437

Closed
lorengordon opened this issue May 3, 2018 · 12 comments
Labels
partition/aws-cn Pertains to the aws-cn partition. partition/aws-us-gov Pertains to the aws-us-gov partition. proposal Proposes new design or functionality. service/ec2 Issues and PRs that pertain to the ec2 service.

Comments

@lorengordon
Copy link
Contributor

I would like to propose using a new argument for EC2 instances, tag_at_launch (or somesuch), that controls how tags are applied to the instance. Whether the tags are applied at launch or after launch.

The problem I run into is when using providers that have AWS-compatible APIs (or AWS partitions otherwise not in the public SDK), but of course have their own endpoints. If they do not yet support tag-at-launch, then the current hardcoded-region/partition approach means that usage of the tags feature becomes quite difficult from a workflow perspective.

It would be much simpler if there were an argument that allowed a user to select the method that applies the tags.

Some options:

  • tag_at_launch - This would be a bool that defaults to true, and that could be flipped to false in the code for GovCloud or ChinaCloud regions
  • tag_method - This would be an enum (or whatever Go calls them) with values that represent each method, e.g. at_launch or after_launch

Or perhaps the behavior could be that if this new argument is absent from the user config, the current behavior is preserved. If set, then the specified method is attempted and if it errors then so be it (user error).

I think this would be in a way just exposing the restricted variable and giving it a nicer name. 😁

If accepted, perhaps a similar approach could be applied to the CloudWatch Log Group tag creation, as well? (The other place where the IsGovCloud() and IsChinaCloud() tests are used...)

@bflad
Copy link
Contributor

bflad commented May 3, 2018

Personally I would like to remove many (all?) of the IsGovCloud()/IsChinaCloud() calls and instead during read operations bypass them by error message (e.g. UnsupportedOperation). This will allow those calls to start working when the API is updated without an underlying code update. We can still present errors during create/update/delete while trying to perform an unsupported action like tagging where it's not implemented.

@lorengordon
Copy link
Contributor Author

Yeah, sure, I can understand that. Though I know as a user I'd certainly still appreciate having an argument to control the method for features like this, where the resource is supported in the partition but has different feature support. That way I can still use the current version of the provider, with just a small change to my configurations.

@lorengordon
Copy link
Contributor Author

Alternatively, perhaps new resources would be warranted, aws_instance_tags and aws_ebs_volume_tags, which might be a more terraform-native approach to tagging instances after launch?

@bflad bflad added proposal Proposes new design or functionality. service/ec2 Issues and PRs that pertain to the ec2 service. partition/aws-us-gov Pertains to the aws-us-gov partition. partition/aws-cn Pertains to the aws-cn partition. labels May 3, 2018
@bflad
Copy link
Contributor

bflad commented May 3, 2018

Have you reached out to AWS support about upgrading the GovCloud EC2 API to support tag on launch?

@lorengordon
Copy link
Contributor Author

I have not, no. I figure the feature just lands when it lands. I don't particularly get the feeling that such requests really have much impact on the schedule.

@bflad
Copy link
Contributor

bflad commented May 3, 2018

A feature request to AWS Support is their recommended approach (at least from what I have heard from AWS Enterprise Support TAMs in my current and previous roles). They need to hear the customer requests to prioritize their backend work. If everyone shares the same opinion that it doesn't matter to submit the requests then it will appear no one cares. 😉

With regards to your specific ask here, I'd lean more towards a separate resource (e.g. aws_ec2_tag -- tagging is generic for the EC2 service) for better composability rather than patching individual resources. Personally I am a little hesitant on the effort vs maintenance burden though.

You should be able to currently workaround this with something like a null resource that calls out to AWS CLI (obviously not ideal though). I believe the Resource Groups service might work with EC2 as well.

@lorengordon
Copy link
Contributor Author

It may be a while, but we can probably, eventually, drum up some time to submit new tagging resources. Would you be amenable to merging such a patch if we put in the time?

@bflad
Copy link
Contributor

bflad commented Jul 9, 2018

As of July 5th, tagging EC2 and EBS on creation is now supported in GovCloud (US). A pull request for this updating this support in the aws_instance resource is available in #5106 and will release with version 1.27.0 of the AWS provider later this week.

Removing these IsGovCloud()/IsChinaCloud() checks with the behavioral approach, outlined in #4437 (comment), will likely be the best path forward for any resources still using it rather than trying to implement separate attributes. For an example of this switch in action, see: https://github.com/terraform-providers/terraform-provider-aws/pull/3794/files

I believe both the above items answer all the questions here, so going to close this issue. 👍

@bflad bflad closed this as completed Jul 9, 2018
@lorengordon
Copy link
Contributor Author

@bflad I don't think this change really addresses the issue at hand. The problem is not about the GovCloud or China partitions specifically. It is about partitions or services with otherwise AWS-compatible APIs that do not happen to support tag-at-launch. In a general sense. This patch will continue to fail in those circumstances.

Catching and ignoring the failure just means the tags are not created at all, right? If so, that's also not desirable.

@bflad
Copy link
Contributor

bflad commented Jul 10, 2018

@lorengordon Instead of talking in the abstract, do you have specific examples/errors you are encountering and with which services/resources? I think I am missing something here. 😄

Its probably important to note that we (in the project sense) do not promise to offer full compatibility with non-AWS API endpoints, but only provide best effort support. We would only accept workarounds for non-AWS API endpoints if the implementation is simple. We do not currently acceptance test for API endpoints outside the three existing AWS partitions so we offer no compatibility guarantees.

If the API endpoint in question does not support tagging at all, Terraform should always return the difference of the tags being missing or error on creation. That is operator error, not an issue with the provider, and should be addressed by not using that configuration at all. The upcoming Terraform 0.12 release will support null values, which should help in this situation.

If the API does not support tagging on creation, that will need to be addressed case by case, but also reference the above. Adding an attribute for non-AWS APIs will likely not be accepted into a resource or the provider configuration. We would potentially accept something like what is mentioned here as it does not introduce too much into the resource code: #5106 (comment)

Hope this helps.

@lorengordon
Copy link
Contributor Author

The public AWS partitions are not the only AWS partitions. We are looking into a partnership with Hashicorp, perhaps we can clarify better through that channel.

@ghost
Copy link

ghost commented Apr 4, 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 Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
partition/aws-cn Pertains to the aws-cn partition. partition/aws-us-gov Pertains to the aws-us-gov partition. proposal Proposes new design or functionality. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
Development

No branches or pull requests

2 participants