Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Adding support for cluster engine upgrade #5010

Merged
merged 1 commit into from
Oct 5, 2018
Merged

[WIP] Adding support for cluster engine upgrade #5010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions aws/resource_aws_rds_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func resourceAwsRDSCluster() *schema.Resource {
"engine_version": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: ForceNew: false is the default and can be omitted from the schema

Computed: true,
},

Expand Down Expand Up @@ -163,7 +163,7 @@ func resourceAwsRDSCluster() *schema.Resource {
"source_engine_version": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attribute does not relate to the changes in this pull request -- will revert on merge.

},
},
},
Expand Down Expand Up @@ -901,6 +901,11 @@ func resourceAwsRDSClusterUpdate(d *schema.ResourceData, meta interface{}) error
requestUpdate = true
}

if d.HasChange("engine_version") {
req.EngineVersion = aws.String(d.Get("engine_version").(string))
requestUpdate = true
}

if d.HasChange("vpc_security_group_ids") {
if attr := d.Get("vpc_security_group_ids").(*schema.Set); attr.Len() > 0 {
req.VpcSecurityGroupIds = expandStringList(attr.List())
Expand Down Expand Up @@ -949,6 +954,11 @@ func resourceAwsRDSClusterUpdate(d *schema.ResourceData, meta interface{}) error
if isAWSErr(err, "InvalidParameterValue", "IAM role ARN value is invalid or does not include the required permissions") {
return resource.RetryableError(err)
}

if isAWSErr(err, rds.ErrCodeInvalidDBClusterStateFault, "Cannot modify engine version without a primary instance in DB cluster") {
return resource.NonRetryableError(err)
}

if isAWSErr(err, rds.ErrCodeInvalidDBClusterStateFault, "") {
return resource.RetryableError(err)
}
Expand Down Expand Up @@ -1162,4 +1172,5 @@ var resourceAwsRdsClusterUpdatePendingStates = []string{
"backing-up",
"modifying",
"resetting-master-credentials",
"upgrading",
}
2 changes: 1 addition & 1 deletion aws/resource_aws_rds_cluster_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func resourceAwsRDSClusterInstance() *schema.Resource {
"engine_version": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this change was necessary to get the test configuration (testAccAWSClusterConfig_EngineVersionWithPrimaryInstance) working where engine_version is actually set, the correct behavior is actually to leave it as ForceNew: true as its not possible to update the engine_version on its own and the correct configuration is to omit engine_version.

The challenge here is that the resource itself cannot be configured to support updates of the engine_version on its own. This can been seen by creating an acceptance test and configuration like the below:

func TestAccAWSRDSClusterInstance_EngineVersion(t *testing.T) {
	var dbInstance rds.DBInstance
	rName := acctest.RandomWithPrefix("tf-acc-test")
	resourceName := "aws_rds_cluster_instance.test"

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckAWSClusterDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccAWSRDSClusterInstanceConfig_EngineVersion(rName, "9.6.3"),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSClusterInstanceExists(resourceName, &dbInstance),
					resource.TestCheckResourceAttr(resourceName, "engine_version", "9.6.3"),
				),
			},
			{
				ResourceName:            resourceName,
				ImportState:             true,
				ImportStateVerify:       true,
				ImportStateVerifyIgnore: []string{"apply_immediately"},
			},
			{
				Config: testAccAWSRDSClusterInstanceConfig_EngineVersion(rName, "9.6.6"),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSClusterInstanceExists(resourceName, &dbInstance),
					resource.TestCheckResourceAttr(resourceName, "engine_version", "9.6.6"),
				),
			},
		},
	})
}

func testAccAWSRDSClusterInstanceConfig_EngineVersion(rName, engineVersion string) string {
	return fmt.Sprintf(`
resource "aws_rds_cluster" "test" {
  cluster_identifier  = %q
  engine              = "aurora-postgresql"
  engine_version      = "9.6.3"
  master_username     = "foo"
  master_password     = "mustbeeightcharaters"
  skip_final_snapshot = true

  lifecycle {
    ignore_changes = ["engine_version"]
  }
}

resource "aws_rds_cluster_instance" "test" {
  apply_immediately       = true
  cluster_identifier      = "${aws_rds_cluster.test.id}"
  engine                  = "${aws_rds_cluster.test.engine}"
  engine_version          = %q
  identifier              = %q
  instance_class          = "db.r4.large"
}
`, rName, engineVersion, rName)
}

In this setup without code changes to this pull request, it will create a perpetual difference of trying to perform engine_version updates since nothing is actually triggering the engine version update in the Update function:

--- FAIL: TestAccAWSRDSClusterInstance_EngineVersion (695.08s)
    testing.go:527: Step 2 error: Check failed: Check 2/2 error: aws_rds_cluster_instance.test: Attribute 'engine_version' expected "9.6.6", got "9.6.3"

If we add the following code within the Update function of resource:

func resourceAwsRDSClusterInstanceUpdate(d *schema.ResourceData, meta interface{}) error {
// ...
	if d.HasChange("engine_version") {
		req.EngineVersion = aws.String(d.Get("engine_version").(string))
		requestUpdate = true
	}

We then receive an error attempting the update:

--- FAIL: TestAccAWSRDSClusterInstance_EngineVersion (684.60s)
    testing.go:527: Step 2 error: Error applying: 1 error occurred:
        	* aws_rds_cluster_instance.test: 1 error occurred:
        	* aws_rds_cluster_instance.test: Error modifying DB Instance tf-acc-test-4654433485176023154: InvalidParameterCombination: The specified DB Instance is a member of a cluster. Modify the DB engine version for the DB Cluster using the ModifyDbCluster API

To verify that the engine_version parameter should not be configurable at all, we can then just remove the first two TestStep to attempt to create a 9.6.6 instance on a 9.6.3 cluster:

func TestAccAWSRDSClusterInstance_EngineVersion(t *testing.T) {
	var dbInstance rds.DBInstance
	rName := acctest.RandomWithPrefix("tf-acc-test")
	resourceName := "aws_rds_cluster_instance.test"

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckAWSClusterDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccAWSRDSClusterInstanceConfig_EngineVersion(rName, "9.6.6"),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSClusterInstanceExists(resourceName, &dbInstance),
					resource.TestCheckResourceAttr(resourceName, "engine_version", "9.6.6"),
				),
			},
		},
	})
}

This also returns an error:

--- FAIL: TestAccAWSRDSClusterInstance_EngineVersion (114.07s)
    testing.go:527: Step 0 error: Error applying: 1 error occurred:
        	* aws_rds_cluster_instance.test: 1 error occurred:
        	* aws_rds_cluster_instance.test: error creating RDS DB Instance: InvalidParameterCombination: The engine version that you requested for your DB instance (9.6.6) does not match the engine version of your DB cluster (9.6.3).

I will agree that I think we should perform deprecation for configuring both engine and engine_version attributes within the aws_rds_cluster_instance resource.

Computed: true,
},

Expand Down
76 changes: 75 additions & 1 deletion aws/resource_aws_rds_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,41 @@ func TestAccAWSRDSCluster_EngineVersion(t *testing.T) {
CheckDestroy: testAccCheckAWSClusterDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSClusterConfig_EngineVersion(rInt, "aurora-postgresql", "9.6.6"),
Config: testAccAWSClusterConfig_EngineVersion(rInt, "aurora-postgresql", "9.6.3"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSClusterExists(resourceName, &dbCluster),
resource.TestCheckResourceAttr(resourceName, "engine", "aurora-postgresql"),
resource.TestCheckResourceAttr(resourceName, "engine_version", "9.6.3"),
),
},
{
Config: testAccAWSClusterConfig_EngineVersion(rInt, "aurora-postgresql", "9.6.6"),
ExpectError: regexp.MustCompile(`Cannot modify engine version without a primary instance in DB cluster`),
},
},
})
}

func TestAccAWSRDSCluster_EngineVersionWithPrimaryInstance(t *testing.T) {
var dbCluster rds.DBCluster
rInt := acctest.RandInt()
resourceName := "aws_rds_cluster.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSClusterDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSClusterConfig_EngineVersionWithPrimaryInstance(rInt, "aurora-postgresql", "9.6.3"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSClusterExists(resourceName, &dbCluster),
resource.TestCheckResourceAttr(resourceName, "engine", "aurora-postgresql"),
resource.TestCheckResourceAttr(resourceName, "engine_version", "9.6.3"),
),
},
{
Config: testAccAWSClusterConfig_EngineVersionWithPrimaryInstance(rInt, "aurora-postgresql", "9.6.6"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSClusterExists(resourceName, &dbCluster),
resource.TestCheckResourceAttr(resourceName, "engine", "aurora-postgresql"),
Expand Down Expand Up @@ -995,8 +1029,48 @@ resource "aws_rds_cluster" "test" {
engine = "%s"
engine_version = "%s"
master_password = "mustbeeightcharaters"
master_username = "foo"
skip_final_snapshot = true
apply_immediately = true
}`, rInt, engine, engineVersion)
}

func testAccAWSClusterConfig_EngineVersionWithPrimaryInstance(rInt int, engine, engineVersion string) string {
return fmt.Sprintf(`

data "aws_availability_zones" "available" {}

variable "cluster_identifier" {
default = "tf-acc-test-%d"
}

variable "engine" {
default = "%s"
}

variable "engine_version" {
default = "%s"
}

resource "aws_rds_cluster" "test" {
availability_zones = ["${data.aws_availability_zones.available.names}"]
cluster_identifier = "${var.cluster_identifier}"
database_name = "mydb"
db_cluster_parameter_group_name = "default.aurora-postgresql9.6"
engine = "${var.engine}"
engine_version = "${var.engine_version}"
master_password = "mustbeeightcharaters"
master_username = "foo"
skip_final_snapshot = true
apply_immediately = true
}

resource "aws_rds_cluster_instance" "test" {
identifier = "${var.cluster_identifier}"
cluster_identifier = "${aws_rds_cluster.test.cluster_identifier}"
engine = "${var.engine}"
engine_version = "${var.engine_version}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, configuring engine_version in the aws_rds_cluster_instance resource is extraneous and will cause problems performing updates. Instead, it is better to omit the Optional argument and it may soon be deprecated. I will update this on merge. 👍

instance_class = "db.r4.large"
}`, rInt, engine, engineVersion)
}

Expand Down