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

Add validation for aws_s3_bucket_metric.name #25260

Merged
merged 4 commits into from
Jun 21, 2022

Conversation

justinretzolk
Copy link
Member

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 #25251

Output from acceptance testing:

$ make testacc TESTS=TestAccS3BucketMetric PKG=s3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/s3/... -v -count 1 -parallel 20 -run='TestAccS3BucketMetric'  -timeout 180m
=== RUN   TestAccS3BucketMetric_basic
=== PAUSE TestAccS3BucketMetric_basic
=== RUN   TestAccS3BucketMetric_withEmptyFilter
=== PAUSE TestAccS3BucketMetric_withEmptyFilter
=== RUN   TestAccS3BucketMetric_withFilterPrefix
=== PAUSE TestAccS3BucketMetric_withFilterPrefix
=== RUN   TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== PAUSE TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== RUN   TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== PAUSE TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== RUN   TestAccS3BucketMetric_withFilterMultipleTags
=== PAUSE TestAccS3BucketMetric_withFilterMultipleTags
=== RUN   TestAccS3BucketMetric_withFilterSingleTag
=== PAUSE TestAccS3BucketMetric_withFilterSingleTag
=== CONT  TestAccS3BucketMetric_basic
=== CONT  TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== CONT  TestAccS3BucketMetric_withFilterSingleTag
=== CONT  TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== CONT  TestAccS3BucketMetric_withFilterMultipleTags
=== CONT  TestAccS3BucketMetric_withFilterPrefix
=== CONT  TestAccS3BucketMetric_withEmptyFilter
--- PASS: TestAccS3BucketMetric_withEmptyFilter (1.75s)
--- PASS: TestAccS3BucketMetric_basic (18.81s)
--- PASS: TestAccS3BucketMetric_withFilterPrefix (29.34s)
--- PASS: TestAccS3BucketMetric_withFilterSingleTag (29.61s)
--- PASS: TestAccS3BucketMetric_withFilterPrefixAndSingleTag (29.69s)
--- PASS: TestAccS3BucketMetric_withFilterPrefixAndMultipleTags (30.06s)
--- PASS: TestAccS3BucketMetric_withFilterMultipleTags (30.43s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/s3 31.345s

Information

As called out in the linked issue, aws_s3_bucket_metric.name must be less than or equal to 64 characters. I've not been able to find any AWS documents that call out this requirement, but tested manually (reproduction notes below) to verify this requirement.

Notes

I have not added any additional testing around this change. This is my first code change for the provider, and in looking around, I wasn't able to find any other areas where there are tests specific to ValidateFuncs. If additional test(s) are needed, I'm happy to write them if someone could point me in the right direction for a reference 🙂

Reproductions proving this behavior

Works:

Configuration:

resource "aws_s3_bucket" "test" {
  bucket = "jretzolk-test"
}

resource "random_string" "test" {
  length  = 64
  special = false
}

resource "aws_s3_bucket_metric" "test" {
  bucket = aws_s3_bucket.test.bucket
  name   = random_string.test.result
}

Output:

Plan: 3 to add, 0 to change, 0 to destroy.
random_string.test: Creating...
random_string.test: Creation complete after 0s [id=KkG9ogxU3duvIhKxIB1fG1WlVkCOqnncU7PjoVTreEzVdlB7oMb99LNDyfgOkmpf]
aws_s3_bucket.test: Creating...
aws_s3_bucket.test: Creation complete after 2s [id=jretzolk-test]
aws_s3_bucket_metric.test: Creating...
aws_s3_bucket_metric.test: Creation complete after 0s [id=jretzolk-test:KkG9ogxU3duvIhKxIB1fG1WlVkCOqnncU7PjoVTreEzVdlB7oMb99LNDyfgOkmpf]

Apply complete! Resources: 3 added, 0 changed, 0 destroyed.

Doesn't work (>64 characters)

Configuration:

resource "aws_s3_bucket" "test" {
  bucket = "jretzolk-test"
}

resource "random_string" "test" {
  length  = 65
  special = false
}

resource "aws_s3_bucket_metric" "test" {
  bucket = aws_s3_bucket.test.bucket
  name   = random_string.test.result
}

Output:

Plan: 3 to add, 0 to change, 0 to destroy.
random_string.test: Creating...
random_string.test: Creation complete after 0s [id=Zs0CBxX27kVb2HNgF2hI5wjLrFi8ioUwiEWuDF7JUrZeSgKZPfxkBzqainal7Pkae]
aws_s3_bucket.test: Creating...
aws_s3_bucket.test: Creation complete after 1s [id=jretzolk-test]
aws_s3_bucket_metric.test: Creating...
╷
│ Error: error putting S3 Bucket Metrics Configuration: MalformedXML: The XML you provided was not well-formed or did not validate against our published schema
│       status code: 400, request id: DH90RA2ZAARJ609B, host id: PL/ee2FJ8tUhy+020Wty95uyStjltfb3K5gnLuNBq/VGGbLs/DKSp7eNc8mR0krLYN8WzjbD5nk=
│
│   with aws_s3_bucket_metric.test,
│   on main.tf line 10, in resource "aws_s3_bucket_metric" "test":
│   10: resource "aws_s3_bucket_metric" "test" {
│
╵

Doesn't work (0 characters)

Configuration:

resource "aws_s3_bucket" "test" {
  bucket = "jretzolk-test"
}

resource "aws_s3_bucket_metric" "test" {
  bucket = aws_s3_bucket.test.bucket
  name   = ""
}

Output:

Plan: 2 to add, 0 to change, 0 to destroy.
aws_s3_bucket.test: Creating...
aws_s3_bucket.test: Creation complete after 2s [id=jretzolk-test]
aws_s3_bucket_metric.test: Creating...
╷
│ Error: error putting S3 Bucket Metrics Configuration: InvalidConfigurationId: The specified configuration id is invalid.
│       status code: 400, request id: HHMMTRDG7C3CVXDZ, host id: 3Icvz8BMuk5eknAFv8PVgi6qpvAPAz08EREixcjfeaZh5I1/OT/p3faE1MUwaMAdUQgjaiVfha4=
│
│   with aws_s3_bucket_metric.test,
│   on main.tf line 12, in resource "aws_s3_bucket_metric" "test":
│   12: resource "aws_s3_bucket_metric" "test" {
│
╵

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. service/s3 Issues and PRs that pertain to the s3 service. size/XS Managed by automation to categorize the size of a PR. and removed documentation Introduces or discusses updates to documentation. service/s3 Issues and PRs that pertain to the s3 service. labels Jun 9, 2022
@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. service/s3 Issues and PRs that pertain to the s3 service. labels Jun 9, 2022
@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed documentation Introduces or discusses updates to documentation. labels Jun 9, 2022
@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. and removed enhancement Requests to existing resources that expand the functionality or scope. labels Jun 9, 2022
@github-actions github-actions bot removed the documentation Introduces or discusses updates to documentation. label Jun 21, 2022
@anGie44 anGie44 force-pushed the b-aws_s3_bucket_metric-name-validation branch from 8fc422f to b88219c Compare June 21, 2022 17:15
@github-actions github-actions bot added the documentation Introduces or discusses updates to documentation. label Jun 21, 2022
@anGie44 anGie44 force-pushed the b-aws_s3_bucket_metric-name-validation branch from b88219c to 2b1449f Compare June 21, 2022 17:16
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

I cherry-picked @meetreks changes (thanks @meetreks for your contribution as well 🎉 ), with a small update to set the validation minimum to 1 as you had done originally for bucket since validation looked to be added to bucket instead of name by mistake.

Output of acceptance tests:

$ make testacc TESTARGS='-run=TestAccS3BucketMetric_' PKG=s3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/s3/... -v -count 1 -parallel 20  -run=TestAccS3BucketMetric_ -timeout 180m
=== RUN   TestAccS3BucketMetric_basic
=== PAUSE TestAccS3BucketMetric_basic
=== RUN   TestAccS3BucketMetric_withEmptyFilter
=== PAUSE TestAccS3BucketMetric_withEmptyFilter
=== RUN   TestAccS3BucketMetric_withFilterPrefix
=== PAUSE TestAccS3BucketMetric_withFilterPrefix
=== RUN   TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== PAUSE TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== RUN   TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== PAUSE TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== RUN   TestAccS3BucketMetric_withFilterMultipleTags
=== PAUSE TestAccS3BucketMetric_withFilterMultipleTags
=== RUN   TestAccS3BucketMetric_withFilterSingleTag
=== PAUSE TestAccS3BucketMetric_withFilterSingleTag
=== CONT  TestAccS3BucketMetric_basic
=== CONT  TestAccS3BucketMetric_withFilterPrefixAndSingleTag
=== CONT  TestAccS3BucketMetric_withFilterSingleTag
=== CONT  TestAccS3BucketMetric_withFilterMultipleTags
=== CONT  TestAccS3BucketMetric_withFilterPrefixAndMultipleTags
=== CONT  TestAccS3BucketMetric_withFilterPrefix
=== CONT  TestAccS3BucketMetric_withEmptyFilter
--- PASS: TestAccS3BucketMetric_withEmptyFilter (2.04s)
--- PASS: TestAccS3BucketMetric_basic (23.90s)
--- PASS: TestAccS3BucketMetric_withFilterSingleTag (41.11s)
--- PASS: TestAccS3BucketMetric_withFilterPrefix (41.69s)
--- PASS: TestAccS3BucketMetric_withFilterPrefixAndMultipleTags (41.85s)
--- PASS: TestAccS3BucketMetric_withFilterMultipleTags (41.88s)
--- PASS: TestAccS3BucketMetric_withFilterPrefixAndSingleTag (41.88s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/s3	44.947s

@anGie44 anGie44 added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 21, 2022
@anGie44 anGie44 added this to the v4.20.0 milestone Jun 21, 2022
@anGie44 anGie44 self-assigned this Jun 21, 2022
@anGie44 anGie44 merged commit fb3807d into main Jun 21, 2022
@anGie44 anGie44 deleted the b-aws_s3_bucket_metric-name-validation branch June 21, 2022 17:55
github-actions bot pushed a commit that referenced this pull request Jun 21, 2022
@github-actions
Copy link

This functionality has been released in v4.20.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 24, 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/s3 Issues and PRs that pertain to the s3 service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing information in documentation about name in aws_s3_bucket_metric
2 participants