fix: DBTP-1304 - manage users lambda does not drop tables #207
Conversation
postgres/manage_users.py
Outdated
|
|
||
| def create_db_user(conn, cursor, username, password, permissions): | ||
| MASTER_USERNAME = "postgres" |
There was a problem hiding this comment.
Just a suggestion but I think MAIN_USERNAME rather than MASTER_USERNAME would read better, and maybe the value as "postgres_user" (if it doesn't affect anything else) I find it confusing so many places that have "postgres" as the value for the username and database name
There was a problem hiding this comment.
It is confusing, but I'm not sure we should change this in this PR - it would possibly break things for existing projects using "postgres".
There was a problem hiding this comment.
postgres is the name of the master user as provisioned by the terraform postgres module. I don't think we can change this at the moment at least. I took 'master' as the term used by aws for this user.
postgres/manage_users.py
Outdated
|
|
||
| def create_db_user(conn, cursor, username, password, permissions): | ||
| MASTER_USERNAME = "postgres" |
There was a problem hiding this comment.
Why do we need these vars?
There was a problem hiding this comment.
I guess it makes it clearer what those values represent in the code. They should be lowercased though if not global constants by convention.
There was a problem hiding this comment.
I didn't like these values being hidden within the SQL statements. First time I read it I didn't notice that application_user was being referenced there. I extracted them as constants (by convention in caps) to clarify which users the statements are being executed against.
|
Out of scope for this ticket probably, but when these lived in |
There was a problem hiding this comment.
Perhaps split the ticket into 2 subtasks, one for this so it get be released and one for the integration tests that can follow it up.
I'd really like to see some unit tests for these python lambdas as part of this subtask though. I know there are none currently (I think the python files were extracted from being inlined in the terraform), but I think we should be writing them for new or changed functionality. Just for the functions created in this PR.
postgres/manage_users.py
Outdated
|
|
||
| def create_db_user(conn, cursor, username, password, permissions): | ||
| MASTER_USERNAME = "postgres" |
There was a problem hiding this comment.
I guess it makes it clearer what those values represent in the code. They should be lowercased though if not global constants by convention.
postgres/manage_users.py
Outdated
|
|
||
| def create_db_user(conn, cursor, username, password, permissions): | ||
| MASTER_USERNAME = "postgres" |
There was a problem hiding this comment.
It is confusing, but I'm not sure we should change this in this PR - it would possibly break things for existing projects using "postgres".
ksugden
changed the title
Context:
When we do
terraform apply, a lambda is executed which creates a user for the application database. The logic drops the user and tables owned by the user before recreating the user. This means that a point in time recovery where the application user already exists results in all the recovered tables being dropped.The change
Modifies the function executed by the lambda so that when the user exists already, only the password and secrets are regenerated. For point in time recovery, the user should otherwise stay exactly as it was.
Testing:
I tested the code against integration tests in DBTP-1298-postgres-docker-tests. These tests use docker and do not have some best practices for production code (setup, teardown, docker username and other checkov errors). Due to the urgency of the ticket, I will not commit the tests to main at this time, however they have provided some assurance.
For manual E2E testing, I followed the steps in the docs. I first reproduced the issue, then I applied this branch and did it again. I verified that the user was updated and tables and data could still be accessed.
Unit and integration tests will be added to main in subtask DBTP-1305