provider/aws: Fix issue updating ElasticBeanstalk Environment Settings (#7777)

* provider/aws: Fix issue updating ElasticBeanstalk Environment Settings

Fixes the logic that updated settings for Elastic Beanstalk Environments.
Because the update is done in the same API call, we need to split removals /
additions.

Fixes #6890

* add acc test that fails on master

builtin/providers/aws/resource_aws_elastic_beanstalk_environment.go

@@ -97,7 +97,6 @@ func resourceAwsElasticBeanstalkEnvironment() *schema.Resource {
 			"setting": &schema.Schema{
 				Type:     schema.TypeSet,
 				Optional: true,
-				Computed: true,
 				Elem:     resourceAwsElasticBeanstalkOptionSetting(),
 				Set:      optionSettingValueHash,
 			},
@@ -329,7 +328,50 @@ func resourceAwsElasticBeanstalkEnvironmentUpdate(d *schema.ResourceData, meta i
 		os := o.(*schema.Set)
 		ns := n.(*schema.Set)
 
-		updateOpts.OptionSettings = extractOptionSettings(ns.Difference(os))
+		rm := extractOptionSettings(os.Difference(ns))
+		add := extractOptionSettings(ns.Difference(os))
+
+		// Additions and removals of options are done in a single API call, so we
+		// can't do our normal "remove these" and then later "add these", re-adding
+		// any updated settings.
+		// Because of this, we need to remove any settings in the "removable"
+		// settings that are also found in the "add" settings, otherwise they
+		// conflict. Here we loop through all the initial removables from the set
+		// difference, and we build up a slice of settings not found in the "add"
+		// set
+		var remove []*elasticbeanstalk.ConfigurationOptionSetting
+		if len(add) > 0 {
+			for _, r := range rm {
+				for _, a := range add {
+					// ResourceNames are optional. Some defaults come with it, some do
+					// not. We need to guard against nil/empty in state as well as
+					// nil/empty from the API
+					if a.ResourceName != nil {
+						if r.ResourceName == nil {
+							continue
+						}
+						if *r.ResourceName != *a.ResourceName {
+							continue
+						}
+					}
+					if *r.Namespace == *a.Namespace && *r.OptionName == *a.OptionName {
+						continue
+					}
+					remove = append(remove, r)
+				}
+			}
+		} else {
+			remove = rm
+		}
+
+		for _, elem := range remove {
+			updateOpts.OptionsToRemove = append(updateOpts.OptionsToRemove, &elasticbeanstalk.OptionSpecification{
+				Namespace:  elem.Namespace,
+				OptionName: elem.OptionName,
+			})
+		}
+
+		updateOpts.OptionSettings = add
 	}
 
 	if d.HasChange("template_name") {

builtin/providers/aws/resource_aws_elastic_beanstalk_environment_test.go

@@ -3,7 +3,9 @@ package aws
 import (
 	"fmt"
 	"log"
+	"reflect"
 	"regexp"
+	"sort"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -212,6 +214,93 @@ func TestAccAWSBeanstalkEnv_template_change(t *testing.T) {
 	})
 }
 
+func TestAccAWSBeanstalkEnv_basic_settings_update(t *testing.T) {
+	var app elasticbeanstalk.EnvironmentDescription
+
+	rInt := acctest.RandInt()
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckBeanstalkEnvDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccBeanstalkEnvConfig_empty_settings(rInt),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBeanstalkEnvExists("aws_elastic_beanstalk_environment.tfenvtest", &app),
+					testAccVerifyBeanstalkConfig(&app, []string{}),
+				),
+			},
+			resource.TestStep{
+				Config: testAccBeanstalkEnvConfig_settings(rInt),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBeanstalkEnvExists("aws_elastic_beanstalk_environment.tfenvtest", &app),
+					testAccVerifyBeanstalkConfig(&app, []string{"TF_LOG", "TF_SOME_VAR"}),
+				),
+			},
+			resource.TestStep{
+				Config: testAccBeanstalkEnvConfig_empty_settings(rInt),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBeanstalkEnvExists("aws_elastic_beanstalk_environment.tfenvtest", &app),
+					testAccVerifyBeanstalkConfig(&app, []string{}),
+				),
+			},
+		},
+	})
+}
+
+func testAccVerifyBeanstalkConfig(env *elasticbeanstalk.EnvironmentDescription, expected []string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		if env == nil {
+			return fmt.Errorf("Nil environment in testAccVerifyBeanstalkConfig")
+		}
+		conn := testAccProvider.Meta().(*AWSClient).elasticbeanstalkconn
+
+		resp, err := conn.DescribeConfigurationSettings(&elasticbeanstalk.DescribeConfigurationSettingsInput{
+			ApplicationName: env.ApplicationName,
+			EnvironmentName: env.EnvironmentName,
+		})
+
+		if err != nil {
+			return fmt.Errorf("Error describing config settings in testAccVerifyBeanstalkConfig: %s", err)
+		}
+
+		// should only be 1 environment
+		if len(resp.ConfigurationSettings) != 1 {
+			return fmt.Errorf("Expected only 1 set of Configuration Settings in testAccVerifyBeanstalkConfig, got (%d)", len(resp.ConfigurationSettings))
+		}
+
+		cs := resp.ConfigurationSettings[0]
+
+		var foundEnvs []string
+		testStrings := []string{"TF_LOG", "TF_SOME_VAR"}
+		for _, os := range cs.OptionSettings {
+			for _, k := range testStrings {
+				if *os.OptionName == k {
+					foundEnvs = append(foundEnvs, k)
+				}
+			}
+		}
+
+		// if expected is zero, then we should not have found any of the predefined
+		// env vars
+		if len(expected) == 0 {
+			if len(foundEnvs) > 0 {
+				return fmt.Errorf("Found configs we should not have: %#v", foundEnvs)
+			}
+			return nil
+		}
+
+		sort.Strings(testStrings)
+		sort.Strings(expected)
+		if !reflect.DeepEqual(testStrings, expected) {
+			return fmt.Errorf("Error matching strings, expected:\n\n%#v\n\ngot:\n\n%#v\n", testStrings, foundEnvs)
+		}
+
+		return nil
+	}
+}
+
 func testAccCheckBeanstalkEnvDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).elasticbeanstalkconn
 
@@ -376,6 +465,71 @@ resource "aws_elastic_beanstalk_environment" "tfenvtest" {
 }
 `
 
+func testAccBeanstalkEnvConfig_empty_settings(r int) string {
+	return fmt.Sprintf(`
+resource "aws_elastic_beanstalk_application" "tftest" {
+  name = "tf-test-name-%d"
+  description = "tf-test-desc"
+}
+
+resource "aws_elastic_beanstalk_environment" "tfenvtest" {
+  name = "tf-test-name-%d"
+  application = "${aws_elastic_beanstalk_application.tftest.name}"
+  solution_stack_name = "64bit Amazon Linux running Python"
+
+        wait_for_ready_timeout = "15m"
+}`, r, r)
+}
+
+func testAccBeanstalkEnvConfig_settings(r int) string {
+	return fmt.Sprintf(`
+resource "aws_elastic_beanstalk_application" "tftest" {
+  name = "tf-test-name-%d"
+  description = "tf-test-desc"
+}
+
+resource "aws_elastic_beanstalk_environment" "tfenvtest" {
+  name                = "tf-test-name-%d"
+  application         = "${aws_elastic_beanstalk_application.tftest.name}"
+  solution_stack_name = "64bit Amazon Linux running Python"
+
+        wait_for_ready_timeout = "15m"
+
+  setting {
+    namespace = "aws:elasticbeanstalk:application:environment"
+    name      = "TF_LOG"
+    value     = "true"
+  }
+
+  setting {
+    namespace = "aws:elasticbeanstalk:application:environment"
+    name      = "TF_SOME_VAR"
+    value     = "true"
+  }
+
+  setting {
+    namespace = "aws:autoscaling:scheduledaction"
+    resource  = "ScheduledAction01"
+    name      = "MinSize"
+    value     = 2
+  }
+
+  setting {
+    namespace = "aws:autoscaling:scheduledaction"
+    resource  = "ScheduledAction01"
+    name      = "MaxSize"
+    value     = 3
+  }
+
+  setting {
+    namespace = "aws:autoscaling:scheduledaction"
+    resource  = "ScheduledAction01"
+    name      = "StartTime"
+    value     = "2016-07-28T04:07:02Z"
+  }
+}`, r, r)
+}
+
 const testAccBeanstalkWorkerEnvConfig = `
 resource "aws_iam_instance_profile" "tftest" {
   name = "tftest_profile"