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

No s3 global endpoint rule #499

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AleksaC
Copy link
Member

@AleksaC AleksaC commented Jun 16, 2023

Hi, I have an idea for a new rule. The idea is to disallow usage of s3 global endpoints which have been deprecated. The most notable issue caused by this is cloudfront redirecting to the regional endpoint for some time after the bucket has been created, as described here.

The aws_s3_bucket resource returns the global endpoint in bucket_domain_name output which is what my current implementation of the rule catches. However I'd also look for strings that contain something like bucketname.s3.amazonaws.com

This is a rough but working implementation of the rule, so I'm opening a draft PR to get your feedback before doing additional work. If you think this is a useful rule, I'll cover additional cases, clean up the code and add documentation.

Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable rule. Happy to give this another pass when it's closer to ready. For now I've made some high level notes. I'd encourage you to think of test cases that will break your parser. Things like using count/for_each.


// Enabled returns whether the rule is enabled by default
func (r *AwsS3NoGlobalEndpointRule) Enabled() bool {
return true
Copy link
Member

Choose a reason for hiding this comment

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

Given that the attribute isn't deprecated in the provider itself, this definitely should not be enabled by default. Terraform will automatically print warnings when deprecated attributes are referenced so if there's truly no need for this today except for exceptional backwards compatibility cases it should probably be deprecated upstream too. Not a requirement for merging this rule but worth kicking off given that it's a 1 line change.

// is this ever greater than 0
v := vars[0]

if v.RootName() == "aws_s3_bucket" && len(v) == 3 && v[2].(hcl.TraverseAttr).Name == "bucket_domain_name" {
Copy link
Member

Choose a reason for hiding this comment

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

@bendrucker
Copy link
Member

Are you interested in finishing this?

@AleksaC
Copy link
Member Author

AleksaC commented Jul 13, 2023

Yes I am. This turned out to be harder to implement than I initially thought and things kept coming up for me so I didn't have time to take a proper look at it. I should have more time in the following few days so will try to pick it up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants