backend/s3: Switch from github.com/terraform-providers/terraform-provider-aws to github.com/hashicorp/aws-sdk-go-base by bflad · Pull Request #20374 · hashicorp/terraform

bflad · 2019-02-18T07:36:13Z

Output from acceptance testing (no new failures):

--- PASS: TestBackend_impl (0.00s)
--- PASS: TestBackendConfig (0.37s)
--- PASS: TestBackendConfig_invalidKey (0.00s)
--- PASS: TestBackend (3.26s)
--- PASS: TestBackendLocked (6.80s)
--- FAIL: TestBackendExtraPaths (2.32s)
--- PASS: TestBackendPrefixInWorkspace (2.06s)
--- PASS: TestKeyEnv (8.20s)
--- PASS: TestRemoteClient_impl (0.00s)
--- PASS: TestRemoteClient (2.42s)
--- PASS: TestRemoteClientLocks (6.33s)
--- PASS: TestForceUnlock (13.31s)
--- PASS: TestRemoteClient_clientMD5 (11.75s)
--- PASS: TestRemoteClient_stateChecksum (10.07s)

…ider-aws to github.com/hashicorp/aws-sdk-go-base Output from acceptance testing (no new failures): ``` --- PASS: TestBackend_impl (0.00s) --- PASS: TestBackendConfig (0.37s) --- PASS: TestBackendConfig_invalidKey (0.00s) --- PASS: TestBackend (3.26s) --- PASS: TestBackendLocked (6.80s) --- FAIL: TestBackendExtraPaths (2.32s) --- PASS: TestBackendPrefixInWorkspace (2.06s) --- PASS: TestKeyEnv (8.20s) --- PASS: TestRemoteClient_impl (0.00s) --- PASS: TestRemoteClient (2.42s) --- PASS: TestRemoteClientLocks (6.33s) --- PASS: TestForceUnlock (13.31s) --- PASS: TestRemoteClient_clientMD5 (11.75s) --- PASS: TestRemoteClient_stateChecksum (10.07s) ```

radeksimko

💯 🎉

This overall looks good and I'm so thankful for this PR!
My only real concern/question is about the deprecations.

radeksimko · 2019-02-18T08:10:32Z

 				Optional:    true,
 				Description: "Skip getting the supported EC2 platforms.",
 				Default:     false,
+				Deprecated:  "This attribute is no longer used.",


Is there any more context to this? e.g. why are we deprecating this and what do we expect users which used this in the past to do? As far as I remember this also had some performance background.

Regardless: Do we need to introduce deprecations as part of this PR which is swapping a library?

Ah yes. 😄 The reason for the deprecation is that for the S3 Backend this EC2 service functionality is not needed since we are only interfacing with the S3 service and potentially the DynamoDB service. When I split off the new shared library, I specifically omitted it from the library since only the Terraform AWS Provider actually needs it. The Terraform AWS Provider will continue to utilize the functionality and provide that configuration option.

👌Removing skip_get_ec2_platforms makes sense. I'd just add the explanation there right to the deprecation message, so it's clear.

I'm not sure about the other two though:

skip_region_validation - this provider still uses region which may get validated?

skip_requesting_account_id - there's a pending PR already which is basically making this field relevant backend/s3: Add allowed_account_ids and forbidden_account_ids options #20278

skip_region_validation - this provider still uses region which may get validated?

The utility of the region validation check is questionable, in my opinion. There are valid cases where the validation gets in the way:

AWS turns on new region in Standard/GovCloud/China partition: Operators have to wait for a Terraform release with updated AWS Go SDK that captures the updated region and know to implement the workaround in the meantime. This happened recently with eu-north-1.

AWS has valid regions outside Standard/GovCloud/China partitions (e.g. AWS Secret and AWS Top Secret partitions) which are not captured by the default region validation.

I'm not heavily opposed to adding the check into the new library and still validating it with the S3 Backend though.

skip_requesting_account_id - there's a pending PR already which is basically making this field relevant backend/s3: Add allowed_account_ids and forbidden_account_ids options #20278

Previously requesting account ID did nothing useful for the S3 Backend beyond additional IAM/STS API calls which could fail, so the removal of the account ID logic from the S3 Backend provides adds stability.

In the context of the feature request mentioned and this argument, enabling the skip_requesting_account_id argument and either of the new allowed_account_ids/forbidden_account_ids arguments seems like they would represent a conflicting configuration that should error. Rather than potentially having conflicting arguments, it seems like we can tune that pull request functionality to only do the account ID lookup when the new arguments are defined.

I think this represents a useful split in functionality from the Terraform AWS Provider, where the Terraform AWS Provider actually requires the account ID lookup for various manual ARN construction, while the S3 Backend would only need it in the context of optionally validating the account ID itself.

Re skip_region_validation

Thanks a lot for describing the background here. It sounds like keeping this option would allow folks to use (and keep using) new and secret regions, which I think is desirable? In other words it is desirable that we keep the option for these people?

I don't have too strong opinion about this one though and block the PR on it, but I'd be more inclined to keep this one.

Rather than potentially having conflicting arguments, it seems like we can tune that pull request functionality to only do the account ID lookup when the new arguments are defined.

That's a great idea 👍 Feel free to deprecate it then.

radeksimko · 2019-02-18T08:16:11Z

 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
-github.com/kubernetes-sigs/aws-iam-authenticator v0.3.1-0.20181019024009-82544ec86140 h1:AtXWrgewhHlLux0IAfHINCbkxkf47euklyallWlximw=
-github.com/kubernetes-sigs/aws-iam-authenticator v0.3.1-0.20181019024009-82544ec86140/go.mod h1:ItxiN33Ho7Di8wiC4S4XqbH1NLF0DNdDWOd/5MI9gJU=


I'm sure this will make @alexsomesan happy 😄

Overall this PR makes me happy. Thank a ton, @bflad !
Also, you were super fast!

radeksimko · 2019-02-18T08:19:08Z

-github.com/terraform-providers/terraform-provider-template v1.0.0/go.mod h1:/J+B8me5DCMa0rEBH5ic2aKPjhtpWNeScmxFJWxB1EU=
-github.com/terraform-providers/terraform-provider-tls v0.1.0/go.mod h1:Mxe/v5u31LDW4m32O1z6Ursdh95dpc9Puq6otkYg7tU=
-github.com/terraform-providers/terraform-provider-tls v1.2.0 h1:wcD0InKzNh8fanUYQwim62WCd4toeD9WJnPw/RjBI4o=
-github.com/terraform-providers/terraform-provider-tls v1.2.0/go.mod h1:Mxe/v5u31LDW4m32O1z6Ursdh95dpc9Puq6otkYg7tU=


bflad · 2019-02-19T01:05:46Z

Please let me know if you have any additional questions/concerns/changes! 👍

…g_account_id argument deprecations

radeksimko · 2019-02-20T09:00:19Z

 				Optional:    true,
 				Description: "Skip requesting the account ID.",
 				Default:     false,
+				Deprecated:  "The S3 Backend no longer automatically uses IAM or STS functionality to lookup the AWS Account ID and this attribute is no longer used.",


Nitpick: IAM or STS are mostly implementation details, so we could just say "The S3 Backend no longer looks up the AWS Account ID and this attribute is no longer used." ?

No problem will update tomorrow 👍

radeksimko

@bflad I will leave the decision about skip_region_validation on you. If you do however decide to deprecate it, I think it should come with a reason in the deprecation message which answers the "why" question and ideally "what do I do now" question. 😉

Thanks again for all your efforts that went into this PR and the relevant library. 🎉 🚀

bflad · 2019-02-20T09:56:52Z

I will pull in the validation functions and keep the region checking for now. Updates tomorrow morning will be updating the copy on the account ID deprecation, updating the vendoring to include the validation functions, and add the region check back into the backend.

Thanks so much for your detailed reviews!

…esting_account_id deprecation message

bflad · 2019-02-20T16:41:59Z

Everything passes as it did previously, merging!

--- PASS: TestBackend_impl (0.00s)
--- PASS: TestBackendConfig (0.39s)
--- PASS: TestBackendConfig_invalidKey (0.00s)
--- PASS: TestBackend (2.83s)
--- PASS: TestBackendLocked (9.76s)
--- FAIL: TestBackendExtraPaths (2.47s)
--- PASS: TestBackendPrefixInWorkspace (2.39s)
--- PASS: TestKeyEnv (8.57s)
--- PASS: TestRemoteClient_impl (0.00s)
--- PASS: TestRemoteClient (1.77s)
--- PASS: TestRemoteClientLocks (9.26s)
--- PASS: TestForceUnlock (9.69s)
--- PASS: TestRemoteClient_clientMD5 (8.64s)
--- PASS: TestRemoteClient_stateChecksum (9.95s)

…ent instantiation The new library was split off from this repository to improve dependency management upstream in Terraform core. See also: hashicorp/terraform#20374

…ider-aws to github.com/hashicorp/aws-sdk-go-base References: * #20374 * #20338 * #19992 Backports the S3 backend code from master to v0.11 relating to the AWS Go SDK, which removes the circular dependency with the Terraform AWS Provider and adds retry logic to the backend to prevent temporary networking or AWS service issues from failing unnecessarily. Updated via code change then: ``` go mod tidy go mod vendor ```

ghost · 2020-03-29T02:31:53Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bflad added dependencies Auto-pinning backend/s3 labels Feb 18, 2019

bflad requested review from a team February 18, 2019 07:36

radeksimko reviewed Feb 18, 2019

View reviewed changes

backend/s3: Update copy for skip_get_ec2_platforms and skip_requestin…

43f12bb

…g_account_id argument deprecations

radeksimko reviewed Feb 20, 2019

View reviewed changes

radeksimko approved these changes Feb 20, 2019

View reviewed changes

backend/s3: Reinstate region validation and update copy for skip_requ…

a41e545

…esting_account_id deprecation message

bflad added this to the v0.12.0 milestone Feb 20, 2019

bflad merged commit 185a330 into master Feb 20, 2019

bflad deleted the td-backend-s3-dependencies branch February 20, 2019 16:42

bflad added a commit that referenced this pull request Feb 20, 2019

Update CHANGELOG for #20374

6260226

radeksimko mentioned this pull request Feb 21, 2019

Proposal: Split auth logic into its own module/repository terraform-provider-openstack/terraform-provider-openstack#686

Closed

bflad mentioned this pull request Feb 22, 2019

provider: Use github.com/hashicorp/aws-sdk-go-base for AWS Go SDK client instantiation hashicorp/terraform-provider-aws#7653

Merged

bflad mentioned this pull request Mar 12, 2019

[v0.11] backend/s3: Switch from github.com/terraform-providers/terraform-provider-aws to github.com/hashicorp/aws-sdk-go-base #20659

Merged

ghost mentioned this pull request Oct 10, 2019

backend:s3 skip_region_validation is deprecated, but there's no replacement #23039

Closed

ghost locked and limited conversation to collaborators Mar 29, 2020

Conversation

bflad commented Feb 18, 2019

Uh oh!

radeksimko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bflad commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radeksimko left a comment

Choose a reason for hiding this comment

Uh oh!

bflad commented Feb 20, 2019

Uh oh!

bflad commented Feb 20, 2019

Uh oh!

ghost commented Mar 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants