New Resource: aws_s3_bucket_metric

[bflad]

Ref: #6

make testacc TESTARGS='-run=TestAccAWSS3BucketMetric'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3BucketMetric -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSS3BucketMetric_importBasic
--- PASS: TestAccAWSS3BucketMetric_importBasic (70.02s)
=== RUN   TestAccAWSS3BucketMetric_WithoutFilter
--- PASS: TestAccAWSS3BucketMetric_WithoutFilter (33.98s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefix
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefix (69.32s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndTag (48.61s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterTagMultiple
--- PASS: TestAccAWSS3BucketMetric_WithFilterTagMultiple (47.89s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterTagSingle
--- PASS: TestAccAWSS3BucketMetric_WithFilterTagSingle (46.32s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws   316.168s

[bflad]

What needs to happen to get this looked at/reviewed/merged?

[bflad]

Hey @Ninir, sorry to bother you directly, but could you or the team take a peek at this sometime soon?

[Ninir]

Hi @bflad,

First of all, a big thank you for the work here! Then, sorry for taking so long to get back to you 😅

We discussed a bit of this with people in the team and think this should not be a new resource but part of the existing one.
Historically, people are used to look for this s3_bucket resource to ensure whether something is present or not in the provider, so we believe it should stay like this.
Having everything related to a given object in one resource seems the way to go for us, and we avoid a documentation where people would get either lost or confused... 😕

The discussion is not close on this so tell us what your thoughts are. In fact, this is just us trying to keep things consistent between providers & resources! :)

The drawback is that the resource has a crazy amount of LOC, which makes it difficult to review & changes things :/

That being said, would you be able to add your work to the existing resource? would be awesome! 😄

Thanks!

[bflad]

I appreciate the reply and if it winds up being that it should be moved into the aws_s3_bucket resource, that's fine, I can do that.

There are a few points to consider when deciding here between the monolithic aws_s3_bucket resource and separating this out. Ideally we could support a monolithic resource configuration for the easy use cases and split resource for the more advanced use cases (both served by a single provider codebase), but as seen with the multiple issues filed for troubles with aws_security_group and aws_security_group_rule this often leads to confusion. 🙁

Here is some food for thought for keeping this separate. There have been other requests for splitting even simpler things like the S3 replication configuration in #716.

Shared and Custom Filters

For ease of configuration/visibility, an organization might opt to have a standard set of filters applied to all S3 buckets while additional custom filters might be necessary. Short of un-DRY configuration across buckets, this would be a hassle to maintain those shared filters across every bucket configuration.

Separation of Access/Concerns

In a similar vein to the above, a centralized team may create the S3 bucket itself (to manage its core configuration such as access logging for security) while another team is responsible for its metrics configuration to fulfill specific business/financial requirements. It would difficult to support the metrics configuration without granting the second team IAM access to manage the other components of the bucket itself (if they were expected to keep it up to date with other bucket configurations for compliance) or potentially having the central team unexpectedly unconverge/break the second teams configuration every time its updated.

Conditional Configurations

Say I only want this in production (since it costs money) or only for a specific bucket in a standard S3 bucket module. There is no current support for "count" within fields of a resource (hashicorp/terraform#7034) so its not possible to conditionally include these configurations in a monolithic resource. An "enabled" type field is not in metrics configuration API. We would have to implement this field ourselves in the provider, which is not ideal for the provider as a whole.

Terraform Configuration LOC

S3 currently supports up to 1000 metrics configuration filters. In this extreme case I personally would not want to be stuck with a single Terraform configuration file with a few thousand lines of Terraform configuration. As far as I know, Terraform/HCL does not support splitting a single resource into multiple configuration files. Even with a few dozen filters, I think this would start to get unwieldy as a configuration maintainer.

[bflad]

More related S3 configuration split requests: #283, #1402, #1704

[bflad]

@Ninir its been almost 2 months since we've heard anything on this PR. Can you please provide any guidance, suggestions, changes, etc to move forward here? If you want me to move the code, that's fine, I just need to know before I spend the effort.

[Ninir]

Hi @bflad

Sorry for the silence here! (I've been really busy at work, and Terraform is done on my free time :) )

So we discussed about this internally, and here what we came to. The current S3 bucket resource is a lot complex, but we do think it should encapsulate the overall logic.
The S3 API is heavily impacted by the eventual consistency "issue" for multiple features, which makes it very difficult to split into multiple parts.

We do think that splitting this would make it worse. However, we would love to reach a point where it would be safe to do that, but for now, it seems to be better in one place.

If you could move this to the s3_bucket resource, it would be really awesome and valuable for Terraform.

Thanks, and again, sorry for the silence 😓 🚀

[mr-olson]

As another data point, I'd certainly like to be able to skip metrics in non-production, as well as lifecycle and replication rules as @bflad noted in the links in his comment last month.

Any guidance on how to make individual properties like these conditional and/or contain conditional clauses or sections?

[Ninir]

@mr-olson I'm not sure how one could deal with that 🤔
The actual design makes it impossible to do such thing sharing the same template. While I can understand that having a separate resource would help on that, it would still bring eventual consistency issues.

What I can tell you is that HCL (HashiCorp Configuration Languag) is being improved by the amazing core team members, so this would be better handled in the upcoming Terraform releases 🚀

[bflad]

I don't mean to harp on this, but I'm personally of the opinion that overall usability is more important than one-time eventual consistency issues. That's a fairly weak argument for something that can be handled in this resource's creation code or at worst simply applied a second time if it errors, which can be used to influence fixing the creation code error handling.

If the argument was "we want everything in one resource", the existence of aws_s3_bucket_notification already sets a precedent of splitting out lengthy pieces of functionality from the parent resource. If the goal really is to migrate everything under aws_s3_bucket then I would expect to see provider issues outlining that resource's migration and deprecation. The aws_s3_bucket documentation is already pretty large and can be hard to read depending on your use case(s).

Until HCL supports conditional nested configurations, the alternative implementation inside the parent resource costs people real money for little benefit other than having all the configuration in one single resource and therefore reduces the usefulness of the provider. Is there a roadmap timeline for that core implementation?

If I have time this weekend, I will create the alternate implementation inside aws_s3_bucket.

[Ninir]

I don't mean to harp on this, but I'm personally of the opinion that overall usability is more important than one-time eventual consistency issues.

I'm not saying the discussion is closed here, nor that it MUST be done in the AWS S3 Bucket, especially if it brings UX issues. We just try to keep things working, without bringing back previous eventual consistency issues we dealt with & resolved in the past.

That's a fairly weak argument for something that can be handled in this resource's creation code or at worst simply applied a second time if it errors, which can be used to influence fixing the creation code error handling.

I think people would find your point of view legit and valid, some other would consider a second apply being a bug (which is).

If the argument was "we want everything in one resource", the existence of aws_s3_bucket_notification already sets a precedent of splitting out lengthy pieces of functionality from the parent resource.

This is not the argument we try to bring to people (we want everything in one resource). We do try to normalize designs over resources, using a line that's the same for everyone. The S3 resources have been suffering issues regarding eventual consistency. Splitting complexity over multiple resources is a good idea

Is there a roadmap timeline for that core implementation?

Unfortunately, I can't provide such information because I'm not aware of that: just saw some public commits made, nothing more... 🤷‍♂️

Let me get back to you on that very quickly if you don't mind, so that you don't waste time doing the migration. It's just me trying to get the ideal solution for as many use-cases as possible without adding overflow nor more issues 😓

[radeksimko]

Hey @bflad
I don't enjoy looking at the aws_s3_bucket resource code either, due to its current complexity & LOC, but the main reason we'd like to see it in there is to address the eventual consistency of S3 in a single place and keep all API calls related to the bucket sequential.

Take a look at a5d6536 to see what I mean.

The other option is to add retryOnAwsCode to every API call here, but that increases the risk of bumping into errors related to eventual consistency which are already hard to tackle within the context of one or two resources due to increased no of API calls that we make in parallel to the same bucket.

See #2008

As we discussed privately with @Ninir it would make sense to break down s3_bucket to more resources, but we need to find a way to address the eventual consistency somehow.

I hope it makes sense.

[bflad]

Thanks @radeksimko and @Ninir for the followups. I really appreciate all your doing for this project, its quite complex and tough to manage. @Ninir I'm really sorry if I came off abrasive the other day. You didn't deserve that.

Possibly a bad suggestion: I'm not too familiar with all the features of Golang but is it possible to create a new s3conn object at the provider level that overrides the SDK methods (e.g. CreateS3MetricsConfiguration) of the AWS S3 class with your "wrapper" methods? Then we could adjust just a few places (top of a few methods in each data source/resource) instead of all the method call implementations across all data sources/resources? I'm thinking that centralizes the complexity of the error handing in one place, rather than spreading it out everywhere.

[radeksimko]

is it possible to create a new s3conn object at the provider level that overrides the SDK methods

we already do something like that for Kinesis: https://github.com/terraform-providers/terraform-provider-aws/blob/95ef25747edc04b532be113b9a4efc2e3c1371fb/aws/config.go#L394-L406

The situation differs slightly because the error code there is one we can safely retry on. The API clearly says "you're being throttled, please retry later". In case of S3 we need to retry on a very vague error code, like NoSuchBucket which may or may not mean the bucket genuinely doesn't exist. 🤷‍♂️

In principle this shouldn't be a problem though. The only worry I have is that AWS SDK controls the backoff & retry logic https://github.com/aws/aws-sdk-go/blob/7e79586ad292b9bbd4daf21eb73deedef74e08d4/aws/client/default_retryer.go#L35-L53 so we'd need to assess whether that's appropriate in this context. e.g. there's no point of retrying on NoSuchBucket for more than ~ 1 minute as we're more likely wasting user's time by not showing them the error message as the bucket is more likely genuinely gone.

[Ninir]

Hi @bflad

Do you think you could review what we said earlier so that this can be merged?
Would be awesome! ⭐️ 🚀

Thanks!

[bflad]

@Ninir absolutely! I'll update this PR over the weekend to include additional error checks/retries and clean up the testing.

[martinssipenko]

Hi, are there any updates on this?

[bflad]

Sorry, this PR is waiting for code updates from me. I will not be able to get to them likely until next week as we are working on crash and bug fixes this week.

[bflad]

I have refactored this PR with many of the things I've picked up the last few months. Ready for official review. 😄

go test -v ./aws -run='Test((Expand|Flatten)S3MetricsFilter|ResourceAwsS3BucketMetricParseID)' 2> /dev/null
=== RUN   TestExpandS3MetricsFilter
--- PASS: TestExpandS3MetricsFilter (0.00s)
=== RUN   TestFlattenS3MetricsFilter
--- PASS: TestFlattenS3MetricsFilter (0.00s)
=== RUN   TestResourceAwsS3BucketMetricParseID
--- PASS: TestResourceAwsS3BucketMetricParseID (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.035s

make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3BucketMetric'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSS3BucketMetric -timeout 120m
=== RUN   TestAccAWSS3BucketMetric_basic
--- PASS: TestAccAWSS3BucketMetric_basic (34.76s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefix
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefix (51.59s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags (46.95s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag (61.45s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterMultipleTags (57.16s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterSingleTag (50.61s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	302.564s

[tombuildsstuff]

one minor nit around making crash logs more useful - otherwise LGTM 👍

[tombuildsstuff]

it might be worth splitting these casts out to be clearer if there's a crash?

[bflad]

Sure! I'll split it out like this:

if v, ok := d.GetOk("filter"); ok {
	filterList := v.([]interface{})
	filterMap := filterList[0].(map[string]interface{})
	metricsConfiguration.Filter = expandS3MetricsFilter(filterMap)
}

[paultyng]

drop this space

[bflad]

👍 absolutely, thanks!

[bflad]

This has been released in version 1.10.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!