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

fix: update option group to fix name vs. name_prefix and conditional creation for postgresql #302

Merged

Conversation

bryantbiggs
Copy link
Member

@bryantbiggs bryantbiggs commented Mar 8, 2021

Description

  • update option group name to use option_group_name provided by root module
  • update option group to allow users to select to use name_prefix or name depending on value of option_group_use_name_prefix
  • when engine == 'postgresql', no option group is created and a null value provided to the root module (PostgreSQL does not use option groups)
  • added an example project to verify/validate changes and also updated the README.md to show different configuration combinations for users

Motivation and Context

  • give users the ability to better control the management of option groups and to avoid having to set values for PostgreSQL where option groups are not utilized

Closes #188
Closes #230
Closes #251
Closes #259
Closes #272
Closes #282

Breaking Changes

  • Yes but albeit minor. We should add a note about upgraded to show users how they can upgrade without any infra changes (though, its just an option group so its quite safe to re-create):

Users who create an option group using the module should change the following to avoid any re-creation:

	# Add
	option_group_name = "${var.identifier}-"

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
  • Tested using the provided examples/option-groups example directory

@antonbabenko
Copy link
Member

Looks good to me once CI is happy again.

@bryantbiggs
Copy link
Member Author

ya, looks like I forgot the version block in the new example, should be all set for review now @antonbabenko

@antonbabenko antonbabenko merged commit a735414 into terraform-aws-modules:master Mar 8, 2021
@antonbabenko
Copy link
Member

Awesome work! 🚀

v2.24.0 has been just released.

@paul-pop
Copy link

I'm afraid this has now broken my Terraform run (I'm using Postgres and I have an instance running with an option group - although not supported, it still created it a long time ago).

Unfortunately, there are cases where the option group can't just be removed immediately from a Postgres instance because of snapshots being attached to the option group. Here is the plan I got:

# module.rds_app_db.module.db_instance.aws_db_instance.this[0] will be updated in-place
  ~ resource "aws_db_instance" "this" {
        id                                    = "my-dev-app-db"
        name                                  = "my"
      ~ parameter_group_name                  = "my-dev-app-db-20201027145803537200000001" -> (known after apply)

        # (48 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

  # module.rds_app_db.module.db_option_group.aws_db_option_group.this[0] will be destroyed
  - resource "aws_db_option_group" "this" {
      - arn                      = "arn:aws:rds:eu-west-2:xxxx:og:my-dev-app-db-20201027145803542000000002" -> null
      - engine_name              = "postgres" -> null
      - id                       = "my-dev-app-db-20201027145803542000000002" -> null
      - major_engine_version     = "12" -> null
      - name                     = "my-dev-app-db-20201027145803542000000002" -> null
      - name_prefix              = "my-dev-app-db-" -> null
      - option_group_description = "Option group for my-dev-app-db" -> null

      - timeouts {
          - delete = "15m" -> null
        }
    }

  # module.rds_app_db.module.db_parameter_group.aws_db_parameter_group.this[0] must be replaced
+/- resource "aws_db_parameter_group" "this" {
      ~ arn         = "arn:aws:rds:eu-west-2:xxx:pg:my-dev-app-db-20201027145803537200000001" -> (known after apply)
      ~ description = "Database parameter group for my-dev-app-db" -> "my-dev-app-db parameter group" # forces replacement
      ~ id          = "my-dev-app-db-20201027145803537200000001" -> (known after apply)
      ~ name        = "my-dev-app-db-20201027145803537200000001" -> (known after apply)

        # (2 unchanged attributes hidden)

        # (4 unchanged blocks hidden)
    }

So if you have an RDS cluster that performs automated backups, you would need to manually remove all snapshots (which is also not great because they can only be removed by reducing the backup window to 0). The error message you get when attempting to delete:

You can’t delete a system snapshot. To remove system snapshots for an active DB instance, modify the DB instance and reduce the automated backup window. To remove system snapshots for a deleted instance, delete the retained automated backup.

My workaround was to:

  1. Set backup window to 0
  2. I used the AWS CLI to update to the default option group:
aws rds modify-db-instance --db-instance-identifier my-dev-app-db --option-group-name default:postgres-12 --apply-immediately
  1. Wait ~5 mins for RDS instance to be modified
  2. Terraform apply - which will remove the option group and re-enable automated backups

@bryantbiggs @antonbabenko

@bryantbiggs
Copy link
Member Author

@paul-pop while I am deeply sorry that this has caused issues on your end, and potentially others, it is the desired effect that we want for the module. I am not sure why the AWS API allows for the creation of an option group for PostgreSQL instances while stating that its not supported in the docs, we are trying to match those same intentions here in the module

@paul-pop
Copy link

@bryantbiggs I completely understand and this is indeed a bug on the AWS side because as soon as you create an option group for Postgres it hangs when you want to add an option so this module avoids getting into that.

Luckily for us, we're ok to lose the snapshots but other people might find it hard to do so. Probably worth keeping an eye out for issues that are raised because of this and suggesting my workaround or something similar.

@bryantbiggs
Copy link
Member Author

yes, and thank you for sharing very detailed steps @paul-pop - I am sure it will be quite helpful for others who will come across this issue

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants