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.

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

provider/aws: Don't set DBName on aws_db_instance from snapshot #13140

Merged
merged 1 commit into from
Mar 29, 2017

Conversation

phinze
Copy link
Contributor

@phinze phinze commented Mar 28, 2017

It turns out if you're trying to write a config to conditionally restore
an instance from a snapshot, you end up painted into a corner with the
combination of snapshot_identifier and name.

You need name to be specified for DBs you're creating, but when
snapshot_identifier is populated you need it to be blank or else the
AWS API throws an error.

The logical next step is to drop a ternary in:

resource "aws_db_instance" "foo" {
 # ...
 snapshot_identifier = "${var.snap}"
 name = "${var.snap != "" ? "" : var.dbname}"
}

BUT the above config will replace the DB on subsequent runs as the
config basically has name = "" which will trigger a ForceNew diff once
the name is populated from the snapshot restore.

SO we can get a workable solution by actively avoiding populating
DBName when we're using one of the engines the docs explicitly mention.

It does not look like there are any tests covering snapshot_identifer,
so I'll subject this to some manual tests and follow up with some
results.

It turns out if you're trying to write a config to conditionally restore
an instance from a snapshot, you end up painted into a corner with the
combination of `snapshot_identifier` and `name`.

You need `name` to be specified for DBs you're creating, but when
`snapshot_identifier` is populated you need it to be blank or else the
AWS API throws an error.

The logical next step is to drop a ternary in:

```tf
resource "aws_db_instance" "foo" {
 # ...
 snapshot_identifier = "${var.snap}"
 name = "${var.snap != "" ? "" : var.dbname}"
}
```

**BUT** the above config will _replace_ the DB on subsequent runs as the
config basically has `name = ""` which will trigger a ForceNew diff once
the `name` is populated from the snapshot restore.

**SO** we can get a workable solution by actively avoiding populating
DBName when we're using one of the engines the docs explicitly mention.

It does not look like there are any tests covering `snapshot_identifer`,
so I'll subject this to some manual tests and follow up with some
results.
@phinze
Copy link
Contributor Author

phinze commented Mar 29, 2017

Manually confirmed that using the following config:

resource "aws_db_instance" "test" {
  identifier                = "snapshot-identifier-test"
  engine                    = "postgres"
  db_subnet_group_name      = "..."
  username                  = "..."
  password                  = "..."
  instance_class            = "db.t2.small"
  vpc_security_group_ids    = ["..."]
  name                      = "(dbname from snapshot)"
  snapshot_identifier       = "( a snapshot )"
}

Master yields:

Error creating DB Instance: InvalidParameterValue: DBName must be null when Restoring for this Engine.

And this branch works correctly, with the subsequent plan being empty provided that the name parameter is set to the same name as the database in the snapshot.

Copy link
Contributor

@grubernaut grubernaut left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!!

@ghost
Copy link

ghost commented Apr 14, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants