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

Add SigV4 for SES SMTP passwords #11144

Merged
merged 5 commits into from
Feb 20, 2020
Merged

Add SigV4 for SES SMTP passwords #11144

merged 5 commits into from
Feb 20, 2020

Conversation

n3ph
Copy link
Contributor

@n3ph n3ph commented Dec 4, 2019 •
edited
Loading

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #10903
Closes #11143
Closes #11930

Release note for CHANGELOG:

  • resource/aws_iam_access_key: add provider region specific attribute ses_smtp_password_v4 for SigV4 support (#11143)

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestSesSmtpPasswordFromSecretKeySigV4'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestSesSmtpPasswordFromSecretKeySigV4 -timeout 120m
=== RUN   TestSesSmtpPasswordFromSecretKeySigV4
--- PASS: TestSesSmtpPasswordFromSecretKeySigV4 (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.024s

@n3ph n3ph requested a review from a team December 4, 2019 21:46
@ghost ghost added size/L Managed by automation to categorize the size of a PR. service/iam Issues and PRs that pertain to the iam service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. labels Dec 4, 2019
@n3ph n3ph changed the title WIP: Initial commit WIP: Add SigV4 for SES SMTP passwords Dec 4, 2019
@n3ph n3ph mentioned this pull request Dec 4, 2019
Closed
@n3ph
Copy link
Contributor Author

n3ph commented Dec 4, 2019 •
edited
Loading

I am not really satisfied with the structure of ses_smtp_passwords. I was thinking of a map with the region as key, but since the region includes dashes it could not be used as a key.

Any ideas?

@n3ph n3ph changed the title WIP: Add SigV4 for SES SMTP passwords Add SigV4 for SES SMTP passwords Dec 5, 2019
vutny
vutny approved these changes Jan 21, 2020
View reviewed changes
Copy link

@vutny vutny left a comment

Choose a reason for hiding this comment

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

Thanks for this, @n3ph ! I'm looking forward for this to be merged.
Even in current implementation it is useful and fixes the issue with non-working AWS SES creds.

As a future improvement, I guess you might consider to generate just a single SMTP password for current region (or explicitly specified one) and define a user could obtain only one password per access key. That could make HCL code more clear, but it's just my own view on a problem.

bflad
bflad suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @n3ph 👋 Thank you for submitting this. Please see the below feedback items and let us know if you have any questions or do not have time to implement them. 👍

aws/resource_aws_iam_access_key.go Outdated Show resolved Hide resolved
aws/resource_aws_iam_access_key.go Outdated Show resolved Hide resolved
@bflad bflad 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 Feb 20, 2020
@bflad bflad self-assigned this Feb 20, 2020
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 20, 2020
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. waiting-response Maintainers are waiting on response from community or contributor. labels Feb 20, 2020
@n3ph
rework resource after review
21a40a8 
* move deprecation warning to attribute `ses_smtp_password`
* rename new attribute to `ses_smtp_password_v4`
* rewrite attribute to simple string
* use current provider region for SigV4
* update docs accordingly
@n3ph n3ph requested a review from bflad February 20, 2020 14:58
n3ph
n3ph commented Feb 20, 2020
View reviewed changes
Copy link
Contributor Author

@n3ph n3ph left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @vutny and @bflad,

thanks for the input. I reworked the code base and heavily simplified it by using the current provider region. The new attribute is now a simple string named ses_smtp_password_v4 and deprecation warning is attached to the old attribute ses_smtp_password.

bflad
bflad approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Look good, thanks for those updates, @n3ph 🚀

Output from acceptance testing:

--- PASS: TestSesSmtpPasswordFromSecretKeySigV4 (0.00s)
--- PASS: TestSesSmtpPasswordFromSecretKeySigV2 (0.00s)
--- PASS: TestAccAWSAccessKey_basic (11.20s)
--- PASS: TestAccAWSAccessKey_encrypted (11.31s)
--- PASS: TestAccAWSAccessKey_inactive (18.44s)

@bflad bflad added this to the v2.50.0 milestone Feb 20, 2020
@bflad bflad merged commit a30eb3e into hashicorp:master Feb 20, 2020
@n3ph n3ph deleted the sigv4-smtp-password branch February 20, 2020 20:04
bflad added a commit that referenced this pull request Feb 20, 2020
@bflad
Update CHANGELOG for #11144
5cffb1d
@ghost
Copy link

ghost commented Feb 21, 2020

This has been released in version 2.50.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 for triage. Thanks!

@vutny
Copy link

vutny commented Feb 24, 2020

Thank you!

@ghost
Copy link

ghost commented Mar 22, 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 Mar 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vutny vutny vutny approved these changes

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/iam Issues and PRs that pertain to the iam 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
Milestone
v2.50.0
3 participants
@n3ph @vutny @bflad