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

r/aws_launch_template: metadata_options issues #13506

Closed
wants to merge 2 commits into from
Closed

r/aws_launch_template: metadata_options issues #13506

wants to merge 2 commits into from

Conversation

stefansundin
Copy link
Contributor

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

The metadata_options is riddled with bugs. I started this PR to attempt to fix the specific case where you can't get rid of the metadata options by removing the block, i.e.:

  1. Initial config:
    resource "aws_launch_template" "foo" {
      name = "foo"
      metadata_options {
        http_endpoint = "enabled"
        http_tokens = "required"
      }
    }
  2. Apply.
  3. Update config to:
    resource "aws_launch_template" "foo" {
      name = "foo"
    }
  4. Try to apply, but there are no changes.

So this PR will fix that specific scenario, but this thing is so broken in so many other ways. Here are a couple of scenarios:

  1. Initial config:
    resource "aws_launch_template" "foo" {
      name = "foo"
      metadata_options {
        http_endpoint = "enabled"
        http_tokens = "required"
      }
    }
  2. Apply.
  3. Update config to:
    resource "aws_launch_template" "foo" {
      name = "foo"
      metadata_options {
        http_endpoint = "enabled"
      }
    }
  4. Try to apply, but there are no changes. http_tokens should be unset.

The same problems exist with aws_instance. But it's even worse there since you can't unset the values like a launch template, they really need to be set back to the default. But since the notion of Default values were rejected in my original PR (#11076), I can't imagine any sane way of fixing it without bringing that back.

  1. Initial config:
    resource "aws_instance" "foo" {
      instance_type="t2.small"
      ami="ami-0e0e5738da01fdef5"
      metadata_options {
        http_endpoint = "enabled"
        http_tokens = "required"
      }
    }
  2. Apply.
  3. Update config to:
    resource "aws_instance" "foo" {
      instance_type="t2.small"
      ami="ami-0e0e5738da01fdef5"
      metadata_options {
        http_endpoint = "enabled"
      }
    }
  4. Try to apply, but there are no changes. http_tokens should be set to "optional", as if it was never specified in the first place.

I don't agree with this comment by ewbankkit that says The new metadata_options attribute is defined as Computed: true for backwards compatibility reasons. If the metadata_options block isn't specified, then the MetadataOptions simply won't be set and the API call won't contain it.

This PR will fix one tiny bug, but honestly, this thing needs an overhaul. It's so broken. In my opinion, the aws_instance resource cannot be fixed unless we make it aware of the defaults. It is like no one fully tested the end result.


Output from acceptance testing:

# after the first commit:
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate_metadataOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLaunchTemplate_metadataOptions -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_metadataOptions
=== PAUSE TestAccAWSLaunchTemplate_metadataOptions
=== CONT  TestAccAWSLaunchTemplate_metadataOptions
    TestAccAWSLaunchTemplate_metadataOptions: testing.go:684: Step 1 error: Check failed: Check 2/2 error: aws_launch_template.test: Attribute 'metadata_options.#' expected "0", got "1"
--- FAIL: TestAccAWSLaunchTemplate_metadataOptions (27.76s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	29.446s
FAIL
make: *** [testacc] Error 1

# after the second commit:
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSLaunchTemplate_metadataOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSLaunchTemplate_metadataOptions -timeout 120m
=== RUN   TestAccAWSLaunchTemplate_metadataOptions
=== PAUSE TestAccAWSLaunchTemplate_metadataOptions
=== CONT  TestAccAWSLaunchTemplate_metadataOptions
--- PASS: TestAccAWSLaunchTemplate_metadataOptions (41.76s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	43.367s

@stefansundin stefansundin requested a review from a team May 26, 2020 20:58
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 26, 2020
Base automatically changed from master to main January 23, 2021 00:58
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:58
@breathingdust breathingdust added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 21, 2021
@zhelding
Copy link
Contributor

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, 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 May 12, 2022
This pull request was closed.
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. service/ec2 Issues and PRs that pertain to the ec2 service. size/XS 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
Development

Successfully merging this pull request may close these issues.

3 participants