r/s3: add public access block resource

[acburdine]

fixes #6489

• add new s3_bucket_public_access_block_resource and tests

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAvailabilityZones'

...

TODO:

• add docs
• figure out test issues

[acburdine]

Getting some inconsistencies when running acceptance tests - it appears there's a delay between when the PutPublicAccessBlock API endpoint is called, and when GetPublicAccessBlock will actually return that the public access block is created.

Gonna see if there's something I can do on the read end, or if I need to add a StateChangeConf. Given that the acceptance tests sometimes succeed and sometimes fail, it seems that the delay is fairly short (maybe a couple of seconds).

@bflad is there some way I'm not familiar with to handle the delay between create and read? Or is the StateChangeConf the right way to go about handling it?

[FireballDWF]

Very interested in an update on status and if need any AWS help on this, I'm an AWS employee in the ProServe org and willing to help in any way, including testing, working with AWS support or service team, etc.

[acburdine]

@FireballDWF I think i've got the code correct, I just need to figure out why the tests don't seem to work right.

[bflad]

@acburdine sorry for not replying sooner, I wanted to get a fresh take on handling eventual consistency issues. You might find some inspiration in the resource code and acceptance testing of #6851 -- resource.Retry() is definitely helpful in pretty much all over and S3 is one of the few services I think we would allow explicit d.Set() during Update and not calling Read after Update. You can separate out your Create from Update to better handle that implementation.

[bflad]

Oh! One thing, its not possible to workaround this acceptance test failure at the moment: After applying this step and refreshing, the plan was not empty

That will require an update upstream in the provider SDK acceptance testing framework to allow refresh retries. As long as the acceptance testing for this resource includes TestCheck like resource.TestCheckResourceAttr() after updates for the correct attribute values and those pass, we can "ignore" the refresh failures for now since those would not represent real world failures beyond a separately configured data source immediately trying to read an updated resource (not likely IMO).

[acburdine]

@bflad went through and re-did this resource based on what you'd done in #6851. Still getting some acceptance test errors related to the state not updating correctly though 😕

$ make testacc TESTARGS='-run TestAccAWSS3BucketPublicAccessBlock'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run TestAccAWSS3BucketPublicAccessBlock -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3BucketPublicAccessBlock_basic
=== PAUSE TestAccAWSS3BucketPublicAccessBlock_basic
=== RUN   TestAccAWSS3BucketPublicAccessBlock_disappears
=== PAUSE TestAccAWSS3BucketPublicAccessBlock_disappears
=== RUN   TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls
=== PAUSE TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls
=== CONT  TestAccAWSS3BucketPublicAccessBlock_basic
=== CONT  TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls
=== CONT  TestAccAWSS3BucketPublicAccessBlock_disappears
--- PASS: TestAccAWSS3BucketPublicAccessBlock_disappears (15.42s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_basic (32.12s)
--- FAIL: TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls (37.72s)
    testing.go:538: Step 3 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: aws_s3_bucket_public_access_block.bucket
          block_public_acls: "false" => "true"

        STATE:

        aws_s3_bucket.bucket:
          ID = tf-test-bucket-5199215422396962737
          provider = provider.aws
          acceleration_status =
          acl = private
          arn = arn:aws:s3:::tf-test-bucket-5199215422396962737
          bucket = tf-test-bucket-5199215422396962737
          bucket_domain_name = tf-test-bucket-5199215422396962737.s3.amazonaws.com
          bucket_regional_domain_name = tf-test-bucket-5199215422396962737.s3.amazonaws.com
          cors_rule.# = 0
          force_destroy = false
          hosted_zone_id = Z3AQBSTGFYJSTF
          lifecycle_rule.# = 0
          logging.# = 0
          region = us-east-1
          replication_configuration.# = 0
          request_payer = BucketOwner
          server_side_encryption_configuration.# = 0
          tags.% = 1
          tags.TestName = TestACCAWSS3BucketPublicAccessBlock_basic
          versioning.# = 1
          versioning.0.enabled = false
          versioning.0.mfa_delete = false
          website.# = 0
        aws_s3_bucket_public_access_block.bucket:
          ID = tf-test-bucket-5199215422396962737
          provider = provider.aws
          block_public_acls = false
          block_public_policy = false
          bucket = tf-test-bucket-5199215422396962737
          ignore_public_acls = false
          restrict_public_buckets = false

          Dependencies:
            aws_s3_bucket.bucket
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	48.781s
make: *** [testacc] Error 1
exit status 2

[bflad]

Ah shucks, I really wish I saw your note yesterday as this likely could have gone out. That error is "acceptable" for now: #6607 (comment)

The only bits missing from this pull request are around retrying for s3.ErrCodeNoSuchBucket on Create and d.IsNewResource() in Read because S3 eventual consistency is a pain. I'd like to get this out before break if there's enough items to warrant another release today out-of-band, so I might just post a quick followup commit here and verify the After applying this step and refreshing, the plan was not empty is actually okay from looking at the acceptance testing debug logs.

[acburdine]

@bflad I can get this finished up then, will work on it now unless you've already got something in the works.

[bflad]

@acburdine go for it!

[acburdine]

The only bits missing from this pull request are around retrying for s3.ErrCodeNoSuchBucket on Create and d.IsNewResource() in Read because S3 eventual consistency is a pain.

@bflad was this what you meant?

[bflad]

Yes! And in the Create:

	log.Printf("[DEBUG] S3 bucket: %s, public access block: %v", bucket, input.PublicAccessBlockConfiguration)
	err := resource.Retry(1*time.Minute, func() *resource.RetryError {
		_, err := s3conn.PutPublicAccessBlock(input)

		if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") {
			return resource.RetryableError(err)
		}

		if err != nil {
			return resource.NonRetryableError(err)
		}

		return nil
	})

	if err != nil {
		return fmt.Errorf("error creating public access block policy for S3 bucket (%s): %s", bucket, err)
	}

💯

[acburdine]

@bflad should be good now - let me know if it needs anything else 😄

[bflad]

Awesome work, @acburdine! LGTM 🚀

--- PASS: TestAccAWSS3BucketPublicAccessBlock_basic (28.71s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicAcls (68.06s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_BlockPublicPolicy (62.44s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_disappears (35.51s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_IgnorePublicAcls (63.38s)
--- PASS: TestAccAWSS3BucketPublicAccessBlock_RestrictPublicBuckets (61.18s)

[bflad]

This has been released in version 1.54.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!