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

backend/s3: Adds nested attribute for assume_role #33630

Merged
merged 16 commits into from
Aug 21, 2023
Merged

Conversation

gdavison
Copy link
Contributor

@gdavison gdavison commented Aug 4, 2023

Adds a nested attribute assume_role for all arguments related to assuming an IAM role. Deprecates arguments related to assuming role at top level.

Related #30443
Closes #30495

Target Release

1.6.0

Draft CHANGELOG entry

ENHANCEMENTS

  • backend/s3: Moves arguments related to assuming IAM role to nested attribute assume_role
  • backend/s3: Deprecates the arguments role_arn, session_name, external_id, assume_role_duration_seconds, assume_role_policy, assume_role_policy_arns, assume_role_tags, and assume_role_transitive_tag_keys

@gdavison gdavison requested a review from a team as a code owner August 4, 2023 00:49
Copy link
Member

@jar-b jar-b left a comment

Choose a reason for hiding this comment

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

LGTM 👍

$ TF_ACC=1 go test -count=1 ./internal/backend/remote-state/s3/...
ok      github.com/hashicorp/terraform/internal/backend/remote-state/s3 171.348s

@Nuru
Copy link

Nuru commented Aug 15, 2023

This is a breaking change. IMHO this does not provide enough benefits to be worth a breaking change. Why is this important?

@gdavison
Copy link
Contributor Author

Hi @Nuru. The top-level parameters are being deprecated, not removed, so they will still work. That isn't my understanding of a breaking change. Could you explain your perspective more, please?

The benefit in this case is to have configuration parity with the AWS Provider

@Nuru
Copy link

Nuru commented Aug 17, 2023

@gdavison "deprecated" is the step before "removed". Usually "deprecated" means "will be removed in the next major release". Additionally, people complain about deprecation warnings and want the underlying cause fixed. It ends up being a lot of work, especially on something as widely used as the S3 backend.

If you were adding a new capability or removing something that does not work anymore, that would be a good reason for such a change. Changing things to look more like the AWS provider, especially when you are not providing full feature parity, does not seem worth the disruption to me.

@gdavison
Copy link
Contributor Author

@Nuru we are planning for feature parity for authentication and configuration. The issue #33687 details the changes that we will be making.

@github-actions
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link
Contributor

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 contributions.
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 Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backend/s3: Move assume role parameters to block
3 participants