Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix/msp/postgresqlroles: wait for databases to be provisioned#63362

Merged
bobheadxi merged 1 commit into
mainfrom
msp-wait-for-databases
Jun 20, 2024
Merged

fix/msp/postgresqlroles: wait for databases to be provisioned#63362
bobheadxi merged 1 commit into
mainfrom
msp-wait-for-databases

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Jun 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Wait for databases to be provisioned before granting database-specific roles to the operator access user.

Test plan

Re-apply fixed https://sourcegraph.slack.com/archives/C05E2LHPQLX/p1718850688397579, indicating a race condition on database creation. Diff looks good:

@@ -1447,10 +1472,15 @@
             "path": "cloudrun/cloudrun-postgresqlroles-msp_iam-operator_access_service_account_table_grant",
             "uniqueId": "cloudrun-postgresqlroles-msp_iam-operator_access_service_account_table_grant"
           }
         },
         "database": "msp_iam",
+        "depends_on": [
+          "google_sql_database.postgresql-database-enterprise-portal",
+          "google_sql_database.postgresql-database-enterprise_portal",
+          "google_sql_database.postgresql-database-msp_iam"
+        ],
         "object_type": "table",
         "objects": [
         ],
         "privileges": [
           "SELECT"

Changelog

  • MSP Cloud SQL: Fix race condition between database creation and role grants for the read-only operator access user

@bobheadxi bobheadxi requested review from a team and unknwon June 20, 2024 04:22
@cla-bot cla-bot Bot added the cla-signed label Jun 20, 2024
jac
jac approved these changes Jun 20, 2024
View reviewed changes

@jac jac left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Kinda a weird one because there is already an implicit dependency.
The grants use the Operator SqlUser which explicitly depends on the databases.

Looking at the terraform apply for the run which didn't work the ordering already looks correct. I wouldn't be surprised if this PR doesn't actually fix the issue (I could be wrong though)

unknwon
unknwon approved these changes Jun 20, 2024
View reviewed changes

@unknwon unknwon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@bobheadxi

bobheadxi commented Jun 20, 2024

Copy link
Copy Markdown
Member Author

The grants use the Operator SqlUser which explicitly depends on the databases.

@jac For the second+ database, if added later, the operator SqlUser will already be available, so while that dependency is there, it is fulfilled right away I think.

But, we'll find out for sure when @unknwon adds a second DB for Cody Analytics 😛

@bobheadxi bobheadxi merged commit 2958abc into main Jun 20, 2024
@bobheadxi bobheadxi deleted the msp-wait-for-databases branch June 20, 2024 14:43
@unknwon

unknwon commented Jun 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

But, we'll find out for sure when @unknwon adds a second DB for Cody Analytics

Is Cody Analytics going to have a database at all? 😂 (it has 0 right now, a stateless service haha)

@bobheadxi

bobheadxi commented Jun 20, 2024

Copy link
Copy Markdown
Member Author

Doh, you're right. Well, guess we'll find out... another time

@jac

jac commented Jun 20, 2024

Copy link
Copy Markdown
Member

For the second+ database, if added later, the operator SqlUser will already be available, so while that dependency is there, it is fulfilled right away I think.

Terraform should still respect the ordering of the dependency graph for changes meaning database creation should still happen first

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@unknwon unknwon unknwon approved these changes

@jac jac jac approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bobheadxi @unknwon @jac