Merge pull request #4383 from jmcarp/issue-4361-set-empty-bucket-prefix

Always set elb access log bucket prefix.
hashicorp · May 11, 2018 · e2b5437 · e2b5437
2 parents 65d02e4 + c2267c4
commit e2b5437
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 30 deletions.
diff --git 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
@@ -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"),

diff --git 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 {