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.

Sign up for GitHub

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

Jump to bottom

Fix aws_key_pair import re-creation bug #40075

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix aws_key_pair import re-creation bug #40075

wants to merge 6 commits into from
+81 −46

Conversation

eugercek
Copy link
Contributor

@eugercek eugercek commented Nov 10, 2024 •
edited
Loading

Description

Why I Opened this PR:
I recently imported many ec2 key pairs in our infra. And it's very toil job. One needs to manually edit state files, which may contains secrets. If we've used many terraform modules in a single root, I probably couldn't edit due to permission restrictions. In some organizations this editing may need many tickets (internal in the company) to resolve.

Why this happens:
Currently if one imports a key pair, its public key material is not included. Since there's no a public key (empty) terraform finds a diff between user given value (a proper public_key), even if you write enter correct public_key key, and recreates it.

You can now get public key material of ec2 key pair via DescribeKeyPair api call.
It's added in April 28, 2022, relevant doc

||Describe public keys|You can query the public key and creation date of an Amazon EC2 key pair.|April 28, 2022|

For Example (check Example Code for SDK):

aws ec2 describe-key-pairs --key-names key-pair-name --include-public-key

Some catches of this PR

Since we ignored aws_key_pair.public_key in tests we didn't see the problem of aws DescribeKeyPair api. The API overrides the comment you give to the name of the key pair name, if you use console or the sdk. (It's known in data source of key pair, thus its tests handled that.)

Example Code

Prints:

// ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAINNmjoQb6LuFti6eBe/oeTN017N/A22A4ee9H3SJkLty umut-3
// **Not** ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAINNmjoQb6LuFti6eBe/oeTN017N/A22A4ee9H3SJkLty umut-custom-comment

func main() {
  cfg, err := config.LoadDefaultConfig(context.TODO())
  if err != nil {
  	log.Fatal(err)
  }

  svc := ec2.NewFromConfig(cfg)
  pair, err := svc.ImportKeyPair(context.Background(), &ec2.ImportKeyPairInput{
  	KeyName:           aws.String("umut-3"),
  	PublicKeyMaterial: []byte("ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAINNmjoQb6LuFti6eBe/oeTN017N/A22A4ee9H3SJkLty umut-custom-comment"),
  })
  if err != nil {
  	log.Fatal(err)
  }

  pairs, err := svc.DescribeKeyPairs(context.Background(), &ec2.DescribeKeyPairsInput{
  	IncludePublicKey: aws.Bool(true),
  	Filters: []types.Filter{
  		types.Filter{
  			Name:   aws.String("key-pair-id"),
  			Values: []string{*pair.KeyPairId},
  		},
  	},
  })
  if err != nil {
  	log.Fatal()
  }
  fmt.Println(*pairs.KeyPairs[0].PublicKey)
}

That's why I to added DiffSuppressFunc. Currently we put the required argument from operator directly to the state and never compare with real value. Thus that may effect many workflows. With this diff function it won't happen. Changed tests with this in mind.

Relations

Closes #8529
Closes #5347
Closes #1092

Output from Acceptance Testing

> make testacc TESTS='TestAccEC2KeyPair' PKG=ec2 
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.2 test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2KeyPair'  -timeout 360m
2024/11/10 18:24:50 Initializing Terraform AWS Provider...
=== RUN   TestAccEC2KeyPairDataSource_basic
=== PAUSE TestAccEC2KeyPairDataSource_basic
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== RUN   TestAccEC2KeyPair_basic
=== PAUSE TestAccEC2KeyPair_basic
=== RUN   TestAccEC2KeyPair_tags
=== PAUSE TestAccEC2KeyPair_tags
=== RUN   TestAccEC2KeyPair_nameGenerated
=== PAUSE TestAccEC2KeyPair_nameGenerated
=== RUN   TestAccEC2KeyPair_namePrefix
=== PAUSE TestAccEC2KeyPair_namePrefix
=== RUN   TestAccEC2KeyPair_disappears
=== PAUSE TestAccEC2KeyPair_disappears
=== CONT  TestAccEC2KeyPairDataSource_basic
=== CONT  TestAccEC2KeyPair_nameGenerated
=== CONT  TestAccEC2KeyPair_basic
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPair_tags
=== CONT  TestAccEC2KeyPair_disappears
=== CONT  TestAccEC2KeyPair_namePrefix
--- PASS: TestAccEC2KeyPairDataSource_includePublicKey (28.04s)
--- PASS: TestAccEC2KeyPairDataSource_basic (28.12s)
--- PASS: TestAccEC2KeyPair_disappears (28.23s)
--- PASS: TestAccEC2KeyPair_nameGenerated (31.51s)
--- PASS: TestAccEC2KeyPair_namePrefix (31.70s)
--- PASS: TestAccEC2KeyPair_basic (31.89s)
--- PASS: TestAccEC2KeyPair_tags (59.67s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ec2        66.441s

@eugercek eugercek requested a review from a team as a code owner November 10, 2024 16:03
@github-actions GitHub Actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • 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.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/ec2 Issues and PRs that pertain to the ec2 service. verify Pertains to the verify package (i.e., provider-level validating, diff suppression, etc.) needs-triage Waiting for first response or review from a maintainer. labels Nov 10, 2024
@justinretzolk
Copy link
Member

Related #32502
Related #37713

@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Nov 11, 2024
@eugercek
Copy link
Contributor Author

Hi, are there any updates on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. verify Pertains to the verify package (i.e., provider-level validating, diff suppression, etc.)
Projects
None yet
Milestone
No milestone
2 participants
@eugercek @justinretzolk