provider/aws: Allow disabled access_log in ELB

builtin/providers/aws/resource_aws_elb.go

@@ -115,6 +115,7 @@ func resourceAwsElb() *schema.Resource {
 			"access_logs": &schema.Schema{
 				Type:     schema.TypeList,
 				Optional: true,
+				MaxItems: 1,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"interval": &schema.Schema{
@@ -392,7 +393,26 @@ func resourceAwsElbRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("connection_draining_timeout", lbAttrs.ConnectionDraining.Timeout)
 	d.Set("cross_zone_load_balancing", lbAttrs.CrossZoneLoadBalancing.Enabled)
 	if lbAttrs.AccessLog != nil {
-		if err := d.Set("access_logs", flattenAccessLog(lbAttrs.AccessLog)); err != nil {
+		// The AWS API does not allow users to remove access_logs, only disable them.
+		// During creation of the ELB, Terraform sets the access_logs to disabled,
+		// so there should not be a case where lbAttrs.AccessLog above is nil.
+
+		// Here we do not record the remove value of access_log if:
+		// - there is no access_log block in the configuration
+		// - the remote access_logs are disabled
+		//
+		// This indicates there is no access_log in the configuration.
+		// - externally added access_logs will be enabled, so we'll detect the drift
+		// - locally added access_logs will be in the config, so we'll add to the
+		// API/state
+		// See https://github.com/hashicorp/terraform/issues/10138
+		_, n := d.GetChange("access_logs")
+		elbal := lbAttrs.AccessLog
+		nl := n.([]interface{})
+		if len(nl) == 0 && !*elbal.Enabled {
+			elbal = nil
+		}
+		if err := d.Set("access_logs", flattenAccessLog(elbal)); err != nil {
 			return err
 		}
 	}
@@ -533,18 +553,16 @@ func resourceAwsElbUpdate(d *schema.ResourceData, meta interface{}) error {
 		}
 
 		logs := d.Get("access_logs").([]interface{})
-		if len(logs) > 1 {
-			return fmt.Errorf("Only one access logs config per ELB is supported")
-		} else if len(logs) == 1 {
-			log := logs[0].(map[string]interface{})
+		if len(logs) == 1 {
+			l := logs[0].(map[string]interface{})
 			accessLog := &elb.AccessLog{
-				Enabled:      aws.Bool(log["enabled"].(bool)),
-				EmitInterval: aws.Int64(int64(log["interval"].(int))),
-				S3BucketName: aws.String(log["bucket"].(string)),
+				Enabled:      aws.Bool(l["enabled"].(bool)),
+				EmitInterval: aws.Int64(int64(l["interval"].(int))),
+				S3BucketName: aws.String(l["bucket"].(string)),
 			}
 
-			if log["bucket_prefix"] != "" {
-				accessLog.S3BucketPrefix = aws.String(log["bucket_prefix"].(string))
+			if l["bucket_prefix"] != "" {
+				accessLog.S3BucketPrefix = aws.String(l["bucket_prefix"].(string))
 			}
 
 			attrs.LoadBalancerAttributes.AccessLog = accessLog

builtin/providers/aws/resource_aws_elb_test.go

@@ -82,9 +82,56 @@ func TestAccAWSELB_fullCharacterRange(t *testing.T) {
 	})
 }
 
-func TestAccAWSELB_AccessLogs(t *testing.T) {
+func TestAccAWSELB_AccessLogs_enabled(t *testing.T) {
 	var conf elb.LoadBalancerDescription
 
+	rName := fmt.Sprintf("terraform-access-logs-bucket-%d", acctest.RandInt())
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:      func() { testAccPreCheck(t) },
+		IDRefreshName: "aws_elb.foo",
+		Providers:     testAccProviders,
+		CheckDestroy:  testAccCheckAWSELBDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSELBAccessLogs,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSELBExists("aws_elb.foo", &conf),
+				),
+			},
+
+			resource.TestStep{
+				Config: testAccAWSELBAccessLogsOn(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSELBExists("aws_elb.foo", &conf),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.0.bucket", rName),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.0.interval", "5"),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.0.enabled", "true"),
+				),
+			},
+
+			resource.TestStep{
+				Config: testAccAWSELBAccessLogs,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSELBExists("aws_elb.foo", &conf),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.#", "0"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccAWSELB_AccessLogs_disabled(t *testing.T) {
+	var conf elb.LoadBalancerDescription
+
+	rName := fmt.Sprintf("terraform-access-logs-bucket-%d", acctest.RandInt())
+
 	resource.Test(t, resource.TestCase{
 		PreCheck:      func() { testAccPreCheck(t) },
 		IDRefreshName: "aws_elb.foo",
@@ -99,15 +146,17 @@ func TestAccAWSELB_AccessLogs(t *testing.T) {
 			},
 
 			resource.TestStep{
-				Config: testAccAWSELBAccessLogsOn,
+				Config: testAccAWSELBAccessLogsDisabled(rName),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckAWSELBExists("aws_elb.foo", &conf),
 					resource.TestCheckResourceAttr(
 						"aws_elb.foo", "access_logs.#", "1"),
 					resource.TestCheckResourceAttr(
-						"aws_elb.foo", "access_logs.0.bucket", "terraform-access-logs-bucket"),
+						"aws_elb.foo", "access_logs.0.bucket", rName),
 					resource.TestCheckResourceAttr(
 						"aws_elb.foo", "access_logs.0.interval", "5"),
+					resource.TestCheckResourceAttr(
+						"aws_elb.foo", "access_logs.0.enabled", "false"),
 				),
 			},
 
@@ -995,12 +1044,14 @@ resource "aws_elb" "foo" {
   }
 }
 `
-const testAccAWSELBAccessLogsOn = `
+
+func testAccAWSELBAccessLogsOn(r string) string {
+	return fmt.Sprintf(`
 # an S3 bucket configured for Access logs
 # The 797873946194 is the AWS ID for us-west-2, so this test
 # must be ran in us-west-2
 resource "aws_s3_bucket" "acceslogs_bucket" {
-  bucket = "terraform-access-logs-bucket"
+  bucket = "%s"
   acl = "private"
   force_destroy = true
   policy = <<EOF
@@ -1013,7 +1064,7 @@ resource "aws_s3_bucket" "acceslogs_bucket" {
       "Principal": {
         "AWS": "arn:aws:iam::797873946194:root"
       },
-      "Resource": "arn:aws:s3:::terraform-access-logs-bucket/*",
+      "Resource": "arn:aws:s3:::%s/*",
       "Sid": "Stmt1446575236270"
     }
   ],
@@ -1037,7 +1088,55 @@ resource "aws_elb" "foo" {
 		bucket = "${aws_s3_bucket.acceslogs_bucket.bucket}"
 	}
 }
-`
+`, r, r)
+}
+
+func testAccAWSELBAccessLogsDisabled(r string) string {
+	return fmt.Sprintf(`
+# an S3 bucket configured for Access logs
+# The 797873946194 is the AWS ID for us-west-2, so this test
+# must be ran in us-west-2
+resource "aws_s3_bucket" "acceslogs_bucket" {
+  bucket = "%s"
+  acl = "private"
+  force_destroy = true
+  policy = <<EOF
+{
+  "Id": "Policy1446577137248",
+  "Statement": [
+    {
+      "Action": "s3:PutObject",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "arn:aws:iam::797873946194:root"
+      },
+      "Resource": "arn:aws:s3:::%s/*",
+      "Sid": "Stmt1446575236270"
+    }
+  ],
+  "Version": "2012-10-17"
+}
+EOF
+}
+
+resource "aws_elb" "foo" {
+  availability_zones = ["us-west-2a", "us-west-2b", "us-west-2c"]
+
+  listener {
+    instance_port = 8000
+    instance_protocol = "http"
+    lb_port = 80
+    lb_protocol = "http"
+  }
+
+	access_logs {
+		interval = 5
+		bucket = "${aws_s3_bucket.acceslogs_bucket.bucket}"
+		enabled = false
+	}
+}
+`, r, r)
+}
 
 const testAccAWSELBGeneratedName = `
 resource "aws_elb" "foo" {

builtin/providers/aws/structure.go

@@ -360,27 +360,29 @@ func expandElastiCacheParameters(configured []interface{}) ([]*elasticache.Param
 func flattenAccessLog(l *elb.AccessLog) []map[string]interface{} {
 	result := make([]map[string]interface{}, 0, 1)
 
-	if l != nil && *l.Enabled {
-		r := make(map[string]interface{})
-		if l.S3BucketName != nil {
-			r["bucket"] = *l.S3BucketName
-		}
+	if l == nil {
+		return nil
+	}
 
-		if l.S3BucketPrefix != nil {
-			r["bucket_prefix"] = *l.S3BucketPrefix
-		}
+	r := make(map[string]interface{})
+	if l.S3BucketName != nil {
+		r["bucket"] = *l.S3BucketName
+	}
 
-		if l.EmitInterval != nil {
-			r["interval"] = *l.EmitInterval
-		}
+	if l.S3BucketPrefix != nil {
+		r["bucket_prefix"] = *l.S3BucketPrefix
+	}
 
-		if l.Enabled != nil {
-			r["enabled"] = *l.Enabled
-		}
+	if l.EmitInterval != nil {
+		r["interval"] = *l.EmitInterval
+	}
 
-		result = append(result, r)
+	if l.Enabled != nil {
+		r["enabled"] = *l.Enabled
 	}
 
+	result = append(result, r)
+
 	return result
 }