From f46e36c57945de7551ae1602695f19ea54fa24ae Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 29 Jan 2018 06:41:57 -0800 Subject: [PATCH 1/2] Store all config-specified DB params in state. Prior to this commit, only database parameters that AWS recognised as being user-altered were stored in state. This would be fine, except if users specified the same value for a parameter as AWS' default for that value, they would get a perpetual diff, because AWS would report it as a non-user-changed parameter. Meaning it wouldn't be set in state, no matter how many times the user applied. This fixes the problem by retrieving all parameters, regardless of how AWS reports them being set. We then select out from those parameters only the ones that report that they're user-modified _or_ those that are found in a config, if a config is available. This commit also adds a test which exhibits this behaviour, which failed before this fix was applied, and passes now that it has been applied. --- aws/resource_aws_db_parameter_group.go | 67 +++++++++++++++++++-- aws/resource_aws_db_parameter_group_test.go | 37 ++++++++++++ 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/aws/resource_aws_db_parameter_group.go b/aws/resource_aws_db_parameter_group.go index 7d2be31dcd8..8e70a6e4b29 100644 --- a/aws/resource_aws_db_parameter_group.go +++ b/aws/resource_aws_db_parameter_group.go @@ -151,18 +151,77 @@ func resourceAwsDbParameterGroupRead(d *schema.ResourceData, meta interface{}) e d.Set("family", describeResp.DBParameterGroups[0].DBParameterGroupFamily) d.Set("description", describeResp.DBParameterGroups[0].Description) - // Only include user customized parameters as there's hundreds of system/default ones + configParams := d.Get("parameter").(*schema.Set) describeParametersOpts := rds.DescribeDBParametersInput{ DBParameterGroupName: aws.String(d.Id()), - Source: aws.String("user"), + } + if configParams.Len() < 1 { + // if we don't have any params in the ResourceData already, two possibilities + // first, we don't have a config available to us. Second, we do, but it has + // no parameters. We're going to assume the first, to be safe. In this case, + // we're only going to ask for the user-modified values, because any defaults + // the user may have _also_ set are indistinguishable from the hundreds of + // defaults AWS sets. If the user hasn't set any parameters, this will return + // an empty list anyways, so we just make some unnecessary requests. But in + // the more common case (I assume) of an import, this will make fewer requests + // and "do the right thing". + describeParametersOpts.Source = aws.String("user") } - describeParametersResp, err := rdsconn.DescribeDBParameters(&describeParametersOpts) + var parameters []*rds.Parameter + err = rdsconn.DescribeDBParametersPages(&describeParametersOpts, + func(describeParametersResp *rds.DescribeDBParametersOutput, lastPage bool) bool { + parameters = append(parameters, describeParametersResp.Parameters...) + return !lastPage + }) if err != nil { return err } - d.Set("parameter", flattenParameters(describeParametersResp.Parameters)) + var userParams []*rds.Parameter + if configParams.Len() < 1 { + // if we have no config/no parameters in config, we've already asked for only + // user-modified values, so we can just use the entire response. + userParams = parameters + } else { + // if we have a config available to us, we have two possible classes of value + // in the config. On the one hand, the user could have specified a parameter + // that _actually_ changed things, in which case its Source would be set to + // user. On the other, they may have specified a parameter that coincides with + // the default value. In that case, the Source will be set to "system" or + // "engine-default". We need to set the union of all "user" Source parameters + // _and_ the "system"/"engine-default" Source parameters _that appear in the + // config_ in the state, or the user gets a perpetual diff. See + // terraform-providers/terraform-provider-aws#593 for more context and details. + confParams, err := expandParameters(configParams.List()) + if err != nil { + return err + } + for _, param := range parameters { + if param.Source == nil || param.ParameterName == nil { + continue + } + if *param.Source == "user" { + userParams = append(userParams, param) + continue + } + var paramFound bool + for _, cp := range confParams { + if cp.ParameterName == nil { + continue + } + if *cp.ParameterName == *param.ParameterName { + userParams = append(userParams, param) + break + } + } + if !paramFound { + log.Printf("[DEBUG] Not persisting %s to state, as its source is %q and it isn't in the config", *param.ParameterName, *param.Source) + } + } + } + + d.Set("parameter", flattenParameters(userParams)) paramGroup := describeResp.DBParameterGroups[0] arn, err := buildRDSPGARN(d.Id(), meta.(*AWSClient).partition, meta.(*AWSClient).accountid, meta.(*AWSClient).region) diff --git a/aws/resource_aws_db_parameter_group_test.go b/aws/resource_aws_db_parameter_group_test.go index ab100b03b65..eb2fb55b145 100644 --- a/aws/resource_aws_db_parameter_group_test.go +++ b/aws/resource_aws_db_parameter_group_test.go @@ -412,6 +412,29 @@ func TestAccAWSDBParameterGroup_Only(t *testing.T) { }) } +func TestAccAWSDBParameterGroup_MatchDefault(t *testing.T) { + var v rds.DBParameterGroup + + groupName := fmt.Sprintf("parameter-group-test-terraform-%d", acctest.RandInt()) + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSDBParameterGroupDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSDBParameterGroupIncludeDefaultConfig(groupName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSDBParameterGroupExists("aws_db_parameter_group.bar", &v), + resource.TestCheckResourceAttr( + "aws_db_parameter_group.bar", "name", groupName), + resource.TestCheckResourceAttr( + "aws_db_parameter_group.bar", "family", "postgres9.4"), + ), + }, + }, + }) +} + func TestResourceAWSDBParameterGroupName_validation(t *testing.T) { cases := []struct { Value string @@ -752,6 +775,20 @@ resource "aws_db_parameter_group" "large" { }`, n) } +func testAccAWSDBParameterGroupIncludeDefaultConfig(n string) string { + return fmt.Sprintf(` +resource "aws_db_parameter_group" "bar" { + name = "%s" + family = "postgres9.4" + + parameter { + name = "client_encoding" + value = "UTF8" + apply_method = "pending-reboot" + } +}`, n) +} + const testAccDBParameterGroupConfig_namePrefix = ` resource "aws_db_parameter_group" "test" { name_prefix = "tf-test-" From 8e0df3b7f8757e1b9e60e499d67618502d7741a2 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 29 Jan 2018 08:14:47 -0800 Subject: [PATCH 2/2] Check error when setting params in state. Because our parameters are a set, let's just check the error when setting them in state, just in case. --- aws/resource_aws_db_parameter_group.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_db_parameter_group.go b/aws/resource_aws_db_parameter_group.go index 8e70a6e4b29..cbc00b4cfcb 100644 --- a/aws/resource_aws_db_parameter_group.go +++ b/aws/resource_aws_db_parameter_group.go @@ -221,7 +221,10 @@ func resourceAwsDbParameterGroupRead(d *schema.ResourceData, meta interface{}) e } } - d.Set("parameter", flattenParameters(userParams)) + err = d.Set("parameter", flattenParameters(userParams)) + if err != nil { + return fmt.Errorf("error setting 'parameter' in state: %#v", err) + } paramGroup := describeResp.DBParameterGroups[0] arn, err := buildRDSPGARN(d.Id(), meta.(*AWSClient).partition, meta.(*AWSClient).accountid, meta.(*AWSClient).region)