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 "invalid count argument" error #476

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Fix "invalid count argument" error #476

merged 1 commit into from
Nov 21, 2023

Conversation

lorenyu
Copy link
Contributor

@lorenyu lorenyu commented Nov 20, 2023

Ticket

Resolves #475

Changes

  • Fix "invalid count argument" error

Additional changes

  • Remove "password_ts" event from role manager lambda
  • Merge redundant IAM policies for role manager ssm access
  • Clean up DEBUG log level configuration in role manager

Context for reviewers

In #475, @rocketnova discovered a bug that prevents terraform from creating a plan for the database layer. The module sets the count for the db password secret to be length(aws_rds_cluster.db.master_user_secret), but this is unnecessary since aws_rds_cluster.db.master_user_secret will always be available as long as the rds_cluster's manage_master_user_password is set to true which will always be the case since it is hardcoded to true (see

manage_master_user_password = true
).

This changeset removes the unnecessary count which fixes the terraform plan.

This changeset also includes a number of minor cleanup changes:

  • Remove the "password_ts" event from the role manager lambda that was introduced in PR 461 and isn't needed.
  • Merge the IAM policy that was newly created in PR 469 with the existing one that is conceptually identical.
  • Clean up the DEBUG log level configuration in the role manager that was introduced in PR 469

Testing and Migration notes

Developed and tested on platform-test in this PR: navapbc/platform-test#66

Copied here for convenience:

  1. created new workspace lyfxcnt ("lorenyu fix count")
    image
  2. in new workspace, create db layer. screenshots of plan and results below
    image
    image
  3. Created db roles with make infra-update-app-database-roles APP_NAME=app ENVIRONMENT=dev
    image
  4. Checked roles (checking that we can connect with IAM auth and that the roles have proper permissions) with make infra-check-app-database-roles APP_NAME=app ENVIRONMENT=dev
    image
  5. Cleaned up by remove deletion protection, deleting the db cluster and workspace
    image
    image
    image
    image
    image
    image

Migration notes

If the rds database cluster already exists and has manage_master_user_password set to false, the terraform plan will fail with the following error:

image

thus, in order to migrate, we'll need to follow the following steps:

  1. first do a targeted apply of the aws_rds_cluster by running the following command (replace ENVIRONMENT_NAME with the correct environment)

    TF_CLI_ARGS_apply='-target="module.database.aws_rds_cluster.db"' make infra-update-app-database APP_NAME=app ENVIRONMENT=<ENVIRONMENT_NAME>
    
    image image image
  2. Then you can apply the rest of the changes normally with make infra-update-app-database APP_NAME=app ENVIRONMENT=<ENVIRONMENT_NAME>

    image image

@lorenyu
Copy link
Contributor Author

lorenyu commented Nov 20, 2023

@shawnvanderjagt @jamesbursa tagged you two since you reviewed the related PRs, and @charles-nava tagged you since you worked on those PRs.

@rocketnova FYI this PR should address the issue you ran into

Copy link
Contributor

@rocketnova rocketnova left a comment

Choose a reason for hiding this comment

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

Looks great to me! Verified by doing a deploy of our set up steps up through database role update to my AWS instance.

@lorenyu lorenyu merged commit 5dedfb0 into main Nov 21, 2023
8 checks passed
@lorenyu lorenyu deleted the lorenyu/fixdbpass branch November 21, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split database role manager into separate module
2 participants