From d783ff919d166a4a59cc7f93af11b4cfce0dd626 Mon Sep 17 00:00:00 2001 From: AleksaC Date: Mon, 12 Jun 2023 23:43:47 +0200 Subject: [PATCH 1/3] [WIP] no s3 global endpoint rule --- rules/aws_s3_no_global_endpoint.go | 68 ++++++++++++++++++++ rules/aws_s3_no_global_endpoint_test.go | 82 +++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 rules/aws_s3_no_global_endpoint.go create mode 100644 rules/aws_s3_no_global_endpoint_test.go diff --git a/rules/aws_s3_no_global_endpoint.go b/rules/aws_s3_no_global_endpoint.go new file mode 100644 index 00000000..2111d930 --- /dev/null +++ b/rules/aws_s3_no_global_endpoint.go @@ -0,0 +1,68 @@ +package rules + +import ( + hcl "github.com/hashicorp/hcl/v2" + "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 +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: "", + } +} + +// Name returns the rule name +func (r *AwsS3NoGlobalEndpointRule) Name() string { + return "aws_acm_certificate_lifecycle" +} + +// Enabled returns whether the rule is enabled by default +func (r *AwsS3NoGlobalEndpointRule) Enabled() bool { + return true +} + +// Severity returns the rule severity +func (r *AwsS3NoGlobalEndpointRule) Severity() tflint.Severity { + return tflint.WARNING +} + +// Link returns the rule reference link +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 (r *AwsS3NoGlobalEndpointRule) Check(runner tflint.Runner) error { + var err error + runner.WalkExpressions(tflint.ExprWalkFunc(func(expr hcl.Expression) hcl.Diagnostics { + vars := expr.Variables() + + if len(vars) == 0 { + return nil + } + + // 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" { + err = runner.EmitIssue(r, "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", v.SourceRange()) + } + + return nil + })) + + return err +} diff --git a/rules/aws_s3_no_global_endpoint_test.go b/rules/aws_s3_no_global_endpoint_test.go new file mode 100644 index 00000000..a5e71036 --- /dev/null +++ b/rules/aws_s3_no_global_endpoint_test.go @@ -0,0 +1,82 @@ +package rules + +import ( + "testing" + + hcl "github.com/hashicorp/hcl/v2" + "github.com/terraform-linters/tflint-plugin-sdk/helper" +) + +func Test_AwsS3NoGlobalEndpoint(t *testing.T) { + cases := []struct { + Name string + Content string + Expected helper.Issues + }{ + { + Name: "unrelated expression", + Content: ` +output "test" { + value = "testing" +}`, + Expected: helper.Issues{}, + }, + { + Name: "multiple unrelated expressions", + Content: ` +output "test1" { + value = var.whatever +} + +output "test2" { + value = "testing" +} + +output "test3" { + value = aws_iam_role.test.arn +} + `, + Expected: helper.Issues{}, + }, + // TODO: nested expressions ? + { + Name: "regional endpoint used", + Content: ` +output "test" { + value = aws_s3_bucket.test.bucket_regional_domain_name +}`, + Expected: helper.Issues{}, + }, + // TODO: strings of the form "bucket_name.s3.amazonaws.com" ? + { + Name: "legacy global endpoint used", + Content: ` +output "test" { + value = aws_s3_bucket.test.bucket_domain_name +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: "resource.tf", + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 48}, + }, + }, + }, + }, + } + + rule := NewAwsS3NoGlobalEndpointRule() + + for _, tc := range cases { + runner := helper.TestRunner(t, map[string]string{"resource.tf": tc.Content}) + + if err := rule.Check(runner); err != nil { + t.Fatalf("Unexpected error occurred: %s", err) + } + + helper.AssertIssues(t, tc.Expected, runner.Issues) + } +} From 1b90b9fe8770b7f9a69fec320985db21b73be5ca Mon Sep 17 00:00:00 2001 From: AleksaC Date: Mon, 17 Jul 2023 01:59:29 +0200 Subject: [PATCH 2/3] more robust tests --- rules/aws_s3_no_global_endpoint_test.go | 224 ++++++++++++++++++++++-- 1 file changed, 208 insertions(+), 16 deletions(-) diff --git a/rules/aws_s3_no_global_endpoint_test.go b/rules/aws_s3_no_global_endpoint_test.go index a5e71036..8eedd01b 100644 --- a/rules/aws_s3_no_global_endpoint_test.go +++ b/rules/aws_s3_no_global_endpoint_test.go @@ -8,21 +8,15 @@ import ( ) func Test_AwsS3NoGlobalEndpoint(t *testing.T) { + filename := "resource.tf" + cases := []struct { Name string Content string Expected helper.Issues }{ { - Name: "unrelated expression", - Content: ` -output "test" { - value = "testing" -}`, - Expected: helper.Issues{}, - }, - { - Name: "multiple unrelated expressions", + Name: "unrelated expressions", Content: ` output "test1" { value = var.whatever @@ -34,11 +28,9 @@ output "test2" { output "test3" { value = aws_iam_role.test.arn -} - `, +}`, Expected: helper.Issues{}, }, - // TODO: nested expressions ? { Name: "regional endpoint used", Content: ` @@ -47,9 +39,8 @@ output "test" { }`, Expected: helper.Issues{}, }, - // TODO: strings of the form "bucket_name.s3.amazonaws.com" ? { - Name: "legacy global endpoint used", + Name: "global endpoint used", Content: ` output "test" { value = aws_s3_bucket.test.bucket_domain_name @@ -59,19 +50,220 @@ output "test" { Rule: NewAwsS3NoGlobalEndpointRule(), Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", Range: hcl.Range{ - Filename: "resource.tf", + Filename: filename, Start: hcl.Pos{Line: 3, Column: 11}, End: hcl.Pos{Line: 3, Column: 48}, }, }, }, }, + { + Name: "global endpoint used from aws_s3_bucket data source", + Content: ` +output "test" { + value = data.aws_s3_bucket.test.bucket_domain_name +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 53}, + }, + }, + }, + }, + { + Name: "global endpoint interpolation", + Content: ` +output "test" { + value = "interpolation test: ${aws_s3_bucket.test.bucket_domain_name}" +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 34}, + End: hcl.Pos{Line: 3, Column: 71}, + }, + }, + }, + }, + { + Name: "global endpoint interpolation multiple expressions", + Content: ` +output "test" { + value = "interpolation test: ${aws_s3_bucket.test.bucket_domain_name} ${var.whatever}" +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 34}, + End: hcl.Pos{Line: 3, Column: 71}, + }, + }, + }, + }, + { + Name: "global endpoint with count", + Content: ` +output "test1" { + value = aws_s3_bucket.test[0].bucket_domain_name +} + +output "test2" { + value = aws_s3_bucket.test[length(var.bucket_names) - 1].bucket_domain_name +} +`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 11}, + End: hcl.Pos{Line: 3, Column: 51}, + }, + }, + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 7, Column: 11}, + End: hcl.Pos{Line: 7, Column: 78}, + }, + }, + }, + }, + { + Name: "global endpoint in a for expression", + Content: ` +output "test" { + value = { + for bucket_name, bucket in aws_s3_bucket.test: bucket_name => bucket.bucket_domain_name + } +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 4, Column: 67}, + End: hcl.Pos{Line: 4, Column: 92}, + }, + }, + }, + }, + { + Name: "global endpoint with foreach", + Content: ` +resource "aws_s3_bucket" "test" { + for_each = toset(var.bucket_names) + + bucket = each.key +} + +resource "aws_ssm_parameter" "test" { + for_each = aws_s3_bucket.test + + name = each.value.id + type = "String" + value = each.value.bucket_domain_name +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 13, Column: 11}, + End: hcl.Pos{Line: 13, Column: 40}, + }, + }, + }, + }, + { + Name: "global endpoint in a dynamic block", + Content: ` +resource "aws_s3_bucket" "test" { + for_each = toset(var.bucket_names) + + bucket = each.key +} + +resource "aws_cloudfront_distribution" "test" { + dynamic "origin" { + for_each = aws_s3_bucket.test + + content { + origin_id = origin.value.id + domain_name = origin.value.bucket_domain_name + } + } +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 14, Column: 11}, + End: hcl.Pos{Line: 14, Column: 40}, + }, + }, + }, + }, + { + Name: "global endpoint literal", + Content: ` +output "test" { + value = "test.s3.amazonaws.com" +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 12}, + End: hcl.Pos{Line: 3, Column: 33}, + }, + }, + }, + }, + { + Name: "global endpoint literal with interpolation", + Content: ` +output "test" { + value = "${var.bucket_name}.s3.amazonaws.com" +}`, + Expected: helper.Issues{ + { + Rule: NewAwsS3NoGlobalEndpointRule(), + Message: "`bucket_domain_name` returns the legacy s3 global endpoint, use `bucket_regional_domain_name` instead", + Range: hcl.Range{ + Filename: filename, + Start: hcl.Pos{Line: 3, Column: 12}, + End: hcl.Pos{Line: 3, Column: 47}, + }, + }, + }, + }, } rule := NewAwsS3NoGlobalEndpointRule() for _, tc := range cases { - runner := helper.TestRunner(t, map[string]string{"resource.tf": tc.Content}) + runner := helper.TestRunner(t, map[string]string{filename: tc.Content}) if err := rule.Check(runner); err != nil { t.Fatalf("Unexpected error occurred: %s", err) From 20b5d5e1686bf4109a0a2d1859c63e9ede27552d Mon Sep 17 00:00:00 2001 From: AleksaC Date: Mon, 17 Jul 2023 02:00:49 +0200 Subject: [PATCH 3/3] don't enable the rule by default, handle more cases, use lang.ReferencesInExpr --- rules/aws_s3_no_global_endpoint.go | 58 ++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/rules/aws_s3_no_global_endpoint.go b/rules/aws_s3_no_global_endpoint.go index 2111d930..fe7dcba8 100644 --- a/rules/aws_s3_no_global_endpoint.go +++ b/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 }))