don't enable the rule by default, handle more cases, use lang.Referen…

…cesInExpr

rules/aws_s3_no_global_endpoint.go

@@ -1,37 +1,33 @@
 package rules
 
 import (
+	"strings"
+
 	hcl "github.com/hashicorp/hcl/v2"
+	hclsyntax "github.com/hashicorp/hcl/v2/hclsyntax"
+	lang "github.com/terraform-linters/tflint-plugin-sdk/terraform/lang"
 	"github.com/terraform-linters/tflint-plugin-sdk/tflint"
 	"github.com/terraform-linters/tflint-ruleset-aws/project"
 )
 
-// AwsS3NoGlobalEndpointRule checks whether deprecated s3 global endpoint is used instead
-// of the regional endoint
+// AwsS3NoGlobalEndpointRule checks whether deprecated s3 global endpoint is used
 type AwsS3NoGlobalEndpointRule struct {
 	tflint.DefaultRule
-
-	// TODO: do we need this, if so why
-	resourceType  string
-	attributeName string
 }
 
 // NewAwsS3NoGlobalEndpointRule returns new rule with default attributes
 func NewAwsS3NoGlobalEndpointRule() *AwsS3NoGlobalEndpointRule {
-	return &AwsS3NoGlobalEndpointRule{
-		resourceType:  "aws_s3_bucket",
-		attributeName: "",
-	}
+	return &AwsS3NoGlobalEndpointRule{}
 }
 
 // Name returns the rule name
 func (r *AwsS3NoGlobalEndpointRule) Name() string {
-	return "aws_acm_certificate_lifecycle"
+	return "aws_s3_no_global_endpoint"
 }
 
 // Enabled returns whether the rule is enabled by default
 func (r *AwsS3NoGlobalEndpointRule) Enabled() bool {
-	return true
+	return false
 }
 
 // Severity returns the rule severity
@@ -44,23 +40,47 @@ func (r *AwsS3NoGlobalEndpointRule) Link() string {
 	return project.ReferenceLink(r.Name())
 }
 
-// Check checks whether the aws_acm_certificate resource contains create_before_destroy = true in lifecycle block
+func isS3BucketResourceOrData(str string) bool {
+	return strings.HasPrefix(str, "aws_s3_bucket") || strings.HasPrefix(str, "data.aws_s3_bucket")
+}
+
+func getRealRange(srcRange hcl.Range) hcl.Range {
+	// ref.SourceRange doesn't include the bucket_domain_name attribute which is 19 characters long
+	srcRange.End.Column += 19
+	return srcRange
+}
+
+// Check checks whether the deprecated s3 global endpoint is used
 func (r *AwsS3NoGlobalEndpointRule) Check(runner tflint.Runner) error {
 	var err error
 	runner.WalkExpressions(tflint.ExprWalkFunc(func(expr hcl.Expression) hcl.Diagnostics {
-		vars := expr.Variables()
+		_, isTemplateExpr := expr.(*hclsyntax.TemplateExpr)
+		if isTemplateExpr {
+			// TODO: check sth like .s3.amazonaws.com inside template expression ?
 
-		if len(vars) == 0 {
+			// "${aws_s3_bucket.test.bucket_domain_name}" would report the same error twice if we didn't return here
 			return nil
 		}
 
-		// is this ever greater than 0
-		v := vars[0]
+		refs := lang.ReferencesInExpr(expr)
 
-		if v.RootName() == "aws_s3_bucket" && len(v) == 3 && v[2].(hcl.TraverseAttr).Name == "bucket_domain_name" {
-			err = runner.EmitIssue(r, "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", v.SourceRange())
+		if len(refs) != 1 {
+			return nil
 		}
 
+		ref := refs[0]
+
+		// lang.ReferencesInExpr(expr) is broken when the reference contains subexpression ?
+		// e.g. aws_s3_bucket.test[length(var.bucket_names) - 1]
+		// TODO: seems like a niche case, should we handle this ?
+		if isS3BucketResourceOrData(ref.Subject.String()) && len(ref.Remaining) == 1 && ref.Remaining[0].(hcl.TraverseAttr).Name == "bucket_domain_name" {
+			// ref.SourceRange doesn't include the bucket_domain_name attribute which is 19 characters long
+			srcRange := getRealRange(ref.SourceRange)
+			err = runner.EmitIssue(r, "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", srcRange)
+		}
+
+		// TODO: handle foreach, count, dynamic blocks
+
 		return nil
 	}))