From e0553b398e80d5fd06c4c161e989e49aeb408e54 Mon Sep 17 00:00:00 2001 From: Joshua Carp Date: Wed, 2 May 2018 00:02:45 -0400 Subject: [PATCH 1/2] Always set load balancer access log bucket prefix. [Fixes #4361] --- aws/resource_aws_elb.go | 15 +++++---------- aws/resource_aws_lb.go | 14 ++++---------- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/aws/resource_aws_elb.go b/aws/resource_aws_elb.go index eb26f9626339..9058cc79a988 100644 --- a/aws/resource_aws_elb.go +++ b/aws/resource_aws_elb.go @@ -584,17 +584,12 @@ func resourceAwsElbUpdate(d *schema.ResourceData, meta interface{}) error { logs := d.Get("access_logs").([]interface{}) if len(logs) == 1 { l := logs[0].(map[string]interface{}) - accessLog := &elb.AccessLog{ - Enabled: aws.Bool(l["enabled"].(bool)), - EmitInterval: aws.Int64(int64(l["interval"].(int))), - S3BucketName: aws.String(l["bucket"].(string)), - } - - if l["bucket_prefix"] != "" { - accessLog.S3BucketPrefix = aws.String(l["bucket_prefix"].(string)) + attrs.LoadBalancerAttributes.AccessLog = &elb.AccessLog{ + Enabled: aws.Bool(l["enabled"].(bool)), + EmitInterval: aws.Int64(int64(l["interval"].(int))), + S3BucketName: aws.String(l["bucket"].(string)), + S3BucketPrefix: aws.String(l["bucket_prefix"].(string)), } - - attrs.LoadBalancerAttributes.AccessLog = accessLog } else if len(logs) == 0 { // disable access logs attrs.LoadBalancerAttributes.AccessLog = &elb.AccessLog{ diff --git a/aws/resource_aws_lb.go b/aws/resource_aws_lb.go index ff022d89ba61..a25722efbf66 100644 --- a/aws/resource_aws_lb.go +++ b/aws/resource_aws_lb.go @@ -1,14 +1,13 @@ package aws import ( + "bytes" "fmt" "log" "regexp" "strconv" "time" - "bytes" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/elbv2" @@ -139,13 +138,11 @@ func resourceAwsLb() *schema.Resource { "prefix": { Type: schema.TypeString, Optional: true, - Computed: true, DiffSuppressFunc: suppressIfLBType("network"), }, "enabled": { Type: schema.TypeBool, Optional: true, - Computed: true, DiffSuppressFunc: suppressIfLBType("network"), }, }, @@ -361,14 +358,11 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error { &elbv2.LoadBalancerAttribute{ Key: aws.String("access_logs.s3.bucket"), Value: aws.String(log["bucket"].(string)), - }) - - if prefix, ok := log["prefix"]; ok { - attributes = append(attributes, &elbv2.LoadBalancerAttribute{ + }, + &elbv2.LoadBalancerAttribute{ Key: aws.String("access_logs.s3.prefix"), - Value: aws.String(prefix.(string)), + Value: aws.String(log["prefix"].(string)), }) - } } else if len(logs) == 0 { attributes = append(attributes, &elbv2.LoadBalancerAttribute{ Key: aws.String("access_logs.s3.enabled"), From c2267c4d983fe9508cd134e7b55d94dffd63402e Mon Sep 17 00:00:00 2001 From: Joshua Carp Date: Wed, 2 May 2018 00:17:37 -0400 Subject: [PATCH 2/2] Add regression test for #4361. --- aws/resource_aws_lb_test.go | 46 +++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/aws/resource_aws_lb_test.go b/aws/resource_aws_lb_test.go index 8e72328ec752..80774a2710bc 100644 --- a/aws/resource_aws_lb_test.go +++ b/aws/resource_aws_lb_test.go @@ -508,6 +508,7 @@ func TestAccAWSLB_accesslogs(t *testing.T) { var conf elbv2.LoadBalancer bucketName := fmt.Sprintf("testaccawslbaccesslogs-%s", acctest.RandStringFromCharSet(6, acctest.CharSetAlphaNum)) lbName := fmt.Sprintf("testaccawslbaccesslog-%s", acctest.RandStringFromCharSet(4, acctest.CharSetAlpha)) + bucketPrefix := "testAccAWSALBConfig_accessLogs" resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -537,12 +538,12 @@ func TestAccAWSLB_accesslogs(t *testing.T) { ), }, { - Config: testAccAWSLBConfig_accessLogs(true, lbName, bucketName), + Config: testAccAWSLBConfig_accessLogs(true, lbName, bucketName, bucketPrefix), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckAWSLBExists("aws_lb.lb_test", &conf), testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.enabled", "true"), testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.bucket", bucketName), - testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.prefix", "testAccAWSALBConfig_accessLogs"), + testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.prefix", bucketPrefix), resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName), resource.TestCheckResourceAttr("aws_lb.lb_test", "internal", "true"), resource.TestCheckResourceAttr("aws_lb.lb_test", "subnets.#", "2"), @@ -556,18 +557,43 @@ func TestAccAWSLB_accesslogs(t *testing.T) { resource.TestCheckResourceAttrSet("aws_lb.lb_test", "dns_name"), resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.#", "1"), resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.bucket", bucketName), - resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.prefix", "testAccAWSALBConfig_accessLogs"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.prefix", bucketPrefix), resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.enabled", "true"), resource.TestCheckResourceAttrSet("aws_lb.lb_test", "arn"), ), }, { - Config: testAccAWSLBConfig_accessLogs(false, lbName, bucketName), + Config: testAccAWSLBConfig_accessLogs(true, lbName, bucketName, ""), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckAWSLBExists("aws_lb.lb_test", &conf), + testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.enabled", "true"), + testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.bucket", bucketName), + testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.prefix", ""), + resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName), + resource.TestCheckResourceAttr("aws_lb.lb_test", "internal", "true"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "subnets.#", "2"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "security_groups.#", "1"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "tags.%", "1"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "tags.Name", "TestAccAWSALB_basic1"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_deletion_protection", "false"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "idle_timeout", "50"), + resource.TestCheckResourceAttrSet("aws_lb.lb_test", "vpc_id"), + resource.TestCheckResourceAttrSet("aws_lb.lb_test", "zone_id"), + resource.TestCheckResourceAttrSet("aws_lb.lb_test", "dns_name"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.#", "1"), + resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.bucket", bucketName), + resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.prefix", ""), + resource.TestCheckResourceAttr("aws_lb.lb_test", "access_logs.0.enabled", "true"), + resource.TestCheckResourceAttrSet("aws_lb.lb_test", "arn"), + ), + }, + { + Config: testAccAWSLBConfig_accessLogs(false, lbName, bucketName, ""), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckAWSLBExists("aws_lb.lb_test", &conf), testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.enabled", "false"), testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.bucket", bucketName), - testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.prefix", "testAccAWSALBConfig_accessLogs"), + testAccCheckAWSLBAttribute("aws_lb.lb_test", "access_logs.s3.prefix", ""), resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName), resource.TestCheckResourceAttr("aws_lb.lb_test", "internal", "true"), resource.TestCheckResourceAttr("aws_lb.lb_test", "subnets.#", "2"), @@ -1727,7 +1753,7 @@ resource "aws_security_group" "alb_test" { }`, lbName) } -func testAccAWSLBConfig_accessLogs(enabled bool, lbName, bucketName string) string { +func testAccAWSLBConfig_accessLogs(enabled bool, lbName, bucketName, bucketPrefix string) string { return fmt.Sprintf(`resource "aws_lb" "lb_test" { name = "%s" internal = true @@ -1739,7 +1765,7 @@ func testAccAWSLBConfig_accessLogs(enabled bool, lbName, bucketName string) stri access_logs { bucket = "${aws_s3_bucket.logs.bucket}" - prefix = "${var.bucket_prefix}" + prefix = "${var.bucket_prefix}" enabled = "%t" } @@ -1755,7 +1781,7 @@ variable "bucket_name" { variable "bucket_prefix" { type = "string" - default = "testAccAWSALBConfig_accessLogs" + default = "%s" } resource "aws_s3_bucket" "logs" { @@ -1779,7 +1805,7 @@ data "aws_iam_policy_document" "logs_bucket" { statement { actions = ["s3:PutObject"] effect = "Allow" - resources = ["arn:${data.aws_partition.current.partition}:s3:::${var.bucket_name}/${var.bucket_prefix}/AWSLogs/${data.aws_caller_identity.current.account_id}/*"] + resources = ["arn:${data.aws_partition.current.partition}:s3:::${var.bucket_name}/${var.bucket_prefix}${var.bucket_prefix == "" ? "" : "/"}AWSLogs/${data.aws_caller_identity.current.account_id}/*"] principals = { type = "AWS" @@ -1837,7 +1863,7 @@ resource "aws_security_group" "alb_test" { tags { Name = "TestAccAWSALB_basic" } -}`, lbName, enabled, bucketName) +}`, lbName, enabled, bucketName, bucketPrefix) } func testAccAWSLBConfig_nosg(lbName string) string {