f/provider - Add validation for provider session-name

[pierre-borckmans]

This PR adds validation for the provider session-name.
The regex was found on this AWS documentation page:
https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html

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

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSProvider'

...

[github-actions]

Welcome @pierre-borckmans 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[zhelding]

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

[ewbankkit]

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccAcctestProvider_AssumeRole_' PKG_NAME=internal/acctest
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/acctest/... -v -count 1 -parallel 20 -run=TestAccAcctestProvider_AssumeRole_ -timeout 180m
=== RUN   TestAccAcctestProvider_AssumeRole_empty
=== PAUSE TestAccAcctestProvider_AssumeRole_empty
=== CONT  TestAccAcctestProvider_AssumeRole_empty
--- PASS: TestAccAcctestProvider_AssumeRole_empty (9.43s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/acctest	13.048s

[ewbankkit]

@pierre-borckmans Thanks for the contribution 🎉 👏.

[github-actions]

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

[n0rig]

@pierre-borckmans / @ewbankkit

I believe due to these validations:
https://github.com/hashicorp/terraform-provider-aws/pull/18085/files#diff-58d6a027753b50994deb7e11e4a99dde423f35844986019bd9cea5e0c94aba22R1958-R1960

I am now getting these sort of errors and I believe this is leading to our cicd pipelines to fail:

with the following terragrunt.hcl code:

locals {
  service_name = "foobar"

  # Automatically load environment-level variables
  account_vars = read_terragrunt_config(find_in_parent_folders("foobar-account.hcl"))

  # Extract the variables we need for easy access
  account_name = local.account_vars.locals.account_name
  account_id   = local.account_vars.locals.aws_account_id
  aws_region   = local.account_vars.locals.aws_region

  # Running in CICD or not
  #
  # Allows overriding of the AWS Role which is assumed to let CICD service accounts use different Roles.
  assume_role_name = get_env("ASSUME_ROLE_NAME", "")

  # Assume role as a service (atlantis)
  external_id = get_env("ASSUME_ROLE_EXTERNAL_ID", "")
}

provider "aws" {
  alias  = "us_west_2"
  region = "us-west-2"
  ignore_tags {
    key_prefixes = ["kubernetes.io/"]
  }
  assume_role {
    role_arn = "arn:aws:iam::${local.account_id}:role/${local.assume_role_name}"
    external_id = "${local.external_id}"
  }
}

Why is an empty string not allowed?

[n0rig]

I think I am okay by setting a default value which can be overridden if the env_var is present:

  external_id = get_env("ASSUME_ROLE_EXTERNAL_ID", "some-value")

[Shanshan-Qu]

This PR adds validation for the provider session-name. The regex was found on this AWS documentation page: https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html

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

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSProvider'

...

Hi @pierre-borckmans

The current issue we have is:
Previously, we leave the value empty as default for ExternalId and RoleSessionName. After this release, we have the erorr:

Error: expected length of assume_role.0.session_name to be in the range (2 - 64), got

Error: expected length of assume_role.0.external_id to be in the range (2 - 1224), got

The error above leads me to this PR. And I am thinking about what the logic should the code be rather than remove these two values from our various code repos which previously works well.

As the AWS docs mentioned, ExternalId and RoleSessionName are not required. Shall the code have the logic as AWS docs:

1. If there is no value, it is still allowed it is an empty value.
2. If there is a value, then, use the validation that has been added.

Also another suggestion:
For a new release feature, shall we give some clue in the error like feature changed in PR18085 to make troubleshooting a bit more efficient?

It will be appreciated to hear your feedback.

Thanks
shanshan

[github-actions]

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.