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

F/#25314 - Return the public key material for a aws_key_pair #25371

Merged

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Jun 15, 2022

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

Closes #25314

Output from acceptance testing:

bruno@pop-os ~/G/terraform-provider-aws (f/#25314-aws-key-pair-public-key)> make testacc TESTS=TestAccEC2KeyPairDataSource_ PKG=ec2                                                                                                     (base) 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2KeyPairDataSource_'  -timeout 180m
=== RUN   TestAccEC2KeyPairDataSource_basic
=== PAUSE TestAccEC2KeyPairDataSource_basic
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPairDataSource_basic
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
--- PASS: TestAccEC2KeyPairDataSource_includePublicKey (7.51s)
--- PASS: TestAccEC2KeyPairDataSource_basic (8.10s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ec2        8.177s
bruno@pop-os ~/G/terraform-provider-aws (f/#25314-aws-key-pair-public-key)> make testacc TESTS=TestAccEC2KeyPair_ PKG=ec2                                                                                                               (base) 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2KeyPair_'  -timeout 180m
=== 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  TestAccEC2KeyPair_basic
=== CONT  TestAccEC2KeyPair_namePrefix
=== CONT  TestAccEC2KeyPair_disappears
=== CONT  TestAccEC2KeyPair_nameGenerated
=== CONT  TestAccEC2KeyPair_tags
--- PASS: TestAccEC2KeyPair_disappears (6.91s)
--- PASS: TestAccEC2KeyPair_basic (9.71s)
--- PASS: TestAccEC2KeyPair_namePrefix (9.83s)
--- PASS: TestAccEC2KeyPair_nameGenerated (9.83s)
--- PASS: TestAccEC2KeyPair_tags (21.12s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ec2        21.230s

@github-actions github-actions bot added size/M Managed by automation to categorize the size of a PR. 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. needs-triage Waiting for first response or review from a maintainer. labels Jun 15, 2022
@bschaatsbergen bschaatsbergen changed the title F/#25314 - Return the public key material F/#25314 - Return the public key material for the for aws_key_pair Jun 15, 2022
@bschaatsbergen bschaatsbergen changed the title F/#25314 - Return the public key material for the for aws_key_pair F/#25314 - Return the public key material for a aws_key_pair Jun 15, 2022
@bschaatsbergen
Copy link
Member Author

bschaatsbergen commented Jun 15, 2022

Interesting, the public_key that you provide through the aws_key_pair resource, is not what's coming back from the DescribeKeyPair API call.

For example, in the create I can set PublicKey tossh-rsa xxxx email@example.com and KeyPairName to Foobar.
The PublicKey in the response from the DescribeKeyPair call will have the email replaced with the KeyPairName, so PublicKey will be set to: ssh-rsa xxxx Foobar.


bruno@pop-os ~/G/terraform-provider-aws (f/#25314-aws-key-pair-public-key)> make testacc TESTS=TestAccEC2KeyPairDataSource_includePublicKey PKG=ec2                                                                           (base) 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2KeyPairDataSource_includePublicKey'  -timeout 180m
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
    ec2_key_pair_data_source_test.go:66: Step 1/1 error: Check failed: Check 5/6 error: data.aws_key_pair.by_name: Attribute 'public_key' expected "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABABBBgQDC6tBL6AAm4QWjf9Xf78/UPwd6cgC+YVi/ahn4SMd8NZOEoHIVuRd6ZJjw/sHkk2KsRKIWH7sSazxAHaJI+RIYaStcLIJirDCvLcw7sXyBgeX/bldl5a1uO/QeH+dAMi8wleKX1JvSJzgwcC6tsAd0kT0nGaLnQBANcJd1awjAPQ== no-reply@hashicorp.com", got "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABABBBgQDC6tBL6AAm4QWjf9Xf78/UPwd6cgC+YVi/ahn4SMd8NZOEoHIVuRd6ZJjw/sHkk2KsRKIWH7sSazxAHaJI+RIYaStcLIJirDCvLcw7sXyBgeX/bldl5a1uO/QeH+dAMi8wleKX1JvSJzgwcC6tsAd0kT0nGaLnQBANcJd1awjAPQ== tf-acc-test-4365605084555105140"
--- FAIL: TestAccEC2KeyPairDataSource_includePublicKey (12.66s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-aws/internal/service/ec2        12.704s
FAIL
make: *** [GNUmakefile:45: testacc] Error 1

To make the test pass I'm now not providing a email address and I append the KeyName to the PublicKey (not very nice).

image

Feel free to suggest an alternative, it bugs me that the data source and resource don't share the same public_key.

@justinretzolk justinretzolk 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 Jun 16, 2022
@bschaatsbergen
Copy link
Member Author

@ewbankkit could you please provide me with some feedback on the above

@ewbankkit
Copy link
Contributor

@bschaatsbergen I added a new function OpenSHHPublicKeysEqual that ignores the comment part of the key when comparing.

The tests now pass:

% make testacc TESTARGS='-run=TestAccEC2KeyPairDataSource_' PKG=ec2 ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 2  -run=TestAccEC2KeyPairDataSource_ -timeout 180m
=== RUN   TestAccEC2KeyPairDataSource_basic
=== PAUSE TestAccEC2KeyPairDataSource_basic
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPairDataSource_basic
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
--- PASS: TestAccEC2KeyPairDataSource_includePublicKey (17.49s)
--- PASS: TestAccEC2KeyPairDataSource_basic (18.72s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	25.314s

@ewbankkit
Copy link
Contributor

Could you please also add create_time and key_type attributes to the data source, plus key_type as a Computed attribute to the aws_key_pair resource? Thanks.

@bschaatsbergen
Copy link
Member Author

@bschaatsbergen I added a new function OpenSHHPublicKeysEqual that ignores the comment part of the key when comparing.

The tests now pass:

% make testacc TESTARGS='-run=TestAccEC2KeyPairDataSource_' PKG=ec2 ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 2  -run=TestAccEC2KeyPairDataSource_ -timeout 180m
=== RUN   TestAccEC2KeyPairDataSource_basic
=== PAUSE TestAccEC2KeyPairDataSource_basic
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPairDataSource_basic
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
--- PASS: TestAccEC2KeyPairDataSource_includePublicKey (17.49s)
--- PASS: TestAccEC2KeyPairDataSource_basic (18.72s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	25.314s

Neat fix, I'll take care of what you mentioned in the last comment.

@bschaatsbergen
Copy link
Member Author

bschaatsbergen commented Jun 24, 2022

@ewbankkit, done. Do you suggest that we also fix the existing limitation mentioned through this PR or another one - https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/key_pair#import ?

Please note that still the data source its public key and the public key provided to the resource can differ..

@ewbankkit
Copy link
Contributor

@bschaatsbergen I suggest a separate PR to deal with that - There will have to be a DiffSuppressFunc to handle the different content of the public key string returned by the AWS API.

@bschaatsbergen
Copy link
Member Author

@bschaatsbergen I suggest a separate PR to deal with that - There will have to be a DiffSuppressFunc to handle the different content of the public key string returned by the AWS API.

Right on, will note it down to pick up in the near future. Removed draft status from this PR.

@bschaatsbergen bschaatsbergen marked this pull request as ready for review June 26, 2022 00:13
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccEC2KeyPairDataSource_' PKG=ec2 ACCTEST_PARALLELISM=3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ec2/... -v -count 1 -parallel 3  -run=TestAccEC2KeyPairDataSource_ -timeout 180m
=== RUN   TestAccEC2KeyPairDataSource_basic
=== PAUSE TestAccEC2KeyPairDataSource_basic
=== RUN   TestAccEC2KeyPairDataSource_includePublicKey
=== PAUSE TestAccEC2KeyPairDataSource_includePublicKey
=== CONT  TestAccEC2KeyPairDataSource_basic
=== CONT  TestAccEC2KeyPairDataSource_includePublicKey
--- PASS: TestAccEC2KeyPairDataSource_includePublicKey (16.34s)
--- PASS: TestAccEC2KeyPairDataSource_basic (16.45s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ec2	20.901s

@ewbankkit
Copy link
Contributor

@bschaatsbergen Thanks for the contribution 🎉 👏.

@ewbankkit
Copy link
Contributor

% make providerlint
==> Checking source code with providerlint...

@ewbankkit ewbankkit merged commit ab2ac9f into hashicorp:main Jun 27, 2022
@github-actions github-actions bot added this to the v4.21.0 milestone Jun 27, 2022
@bschaatsbergen bschaatsbergen deleted the f/#25314-aws-key-pair-public-key branch June 27, 2022 12:49
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

This functionality has been released in v4.21.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!

@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 Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/M 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.

Support public_key on aws_key_pair.
3 participants