Fdc brownfield setup #8150

tammam-g · 2025-01-29T21:38:45Z

Description

Support brownfield database setup. When we detect the database schema already have other tables (brownfield or greenfield but then a table was added manually) we ask the user if they want to setup the database as greenfield.
1- They say yes -> We transfer schema and table ownerships to firebase roles, and grant previous roles writer access only.
2- They say no -> We setup the database in brownfield mode which means that deployed connectors have writer permissions but SQL migration will stop working through cli.

Note: We should find a way to automatically turn on compat mode when database is setup as greenfield from brownfield to avoid accidentally dropping important tables.

Note 2: I think I can improve the brownfield experience by allowing firebase deploy to run if the database diff isn't incompatible (say only drops tables and remove fields). Right now sql:migrate will fail if database is found as brownfield which will fail firebase deploy.

Scenarios Tested

Fresh greenfield setup

Setup greenfield database from scratch using firebase deploy/sql:migrate -> no change

Brownfield to greenfield setup

I created one database with one table (not owned by firebase) and then user agrees to set it up as greenfield.

Brownfield setup test

I create a new database with one table not owned by firebase and firebase isn't setup.

User runs firebase deploy, we then detect it's brownfield and ask the user if they want to setup the database as greenfield. -> User chooses to keep project as brownfield which sets up firebase writer/reader roles and p4sa access but deployment fails.

Confirming the user which has firebase writer role now can edit and select but can't create new tables.

BrownField Default Permissions Test

Finally, I test default permissions when database is setup. Firebase writer/reader should be able to see and edit tables created by other users in the schema ("public") after the setup is done in brownfield mode.

Additional flags test

Trying --force when migrating a brownfield database fails.

Sample Commands

firebase dataconnect:sql:setup

…gration success

fredzqm

Great work! Excited to have brown field support very soon!

fredzqm · 2025-02-02T06:46:44Z

src/gcp/cloudsql/connect.ts

@@ -200,18 +201,6 @@ export async function setupIAMUsers(
  );


Should we update jsdoc now that permissions part are moved out?

Like this clean up.

fredzqm · 2025-02-02T06:55:43Z

src/gcp/cloudsql/permissions.ts

+        ELSE FALSE
+    END AS schema_exists,
+    (SELECT pg_get_userbyid(nspowner) FROM pg_namespace WHERE nspname = '${schema}') AS schema_owner;`,
+    ],


SELECT pg_get_userbyid(nspowner) AS schema_owner FROM pg_namespace WHERE nspname = '${schema}'

Would this work for the purpose? A bit simpler

fredzqm · 2025-02-02T07:15:39Z

src/gcp/cloudsql/permissions.ts

+      owner: row.tableowner,
+    };
+  });
+


After tables, the below lines are to find setupSchema.

nit: Can we extract a helper caller getupSetupSchema? To make it clear that other fields are the same in 3 branches below.

fredzqm · 2025-02-02T07:24:22Z

src/dataconnect/schemaMigration.ts

@@ -237,10 +237,19 @@ export async function grantRoleToUserInSchema(options: Options, schema: Schema)
  }

  // Run the database roles setup. This should be idempotent.
-  await setupIAMUsers(instanceId, databaseId, options);
+  const schemaInfo = await getSchemaMetaData(instanceId, databaseId, DEFAULT_SCHEMA, options);
+  if (schemaInfo.setupStatus === SchemaSetupStatus.GreenField) {


The condition here looks wrong.

Green field database would always return early without grant any permissions.

fredzqm · 2025-02-02T20:04:41Z

src/gcp/cloudsql/permissions.ts

-    `ALTER DEFAULT PRIVILEGES IN SCHEMA "${schema}" GRANT SELECT, USAGE ON SEQUENCES TO "${firebaseReaderRole}";`,
-    `ALTER DEFAULT PRIVILEGES IN SCHEMA "${schema}" GRANT EXECUTE ON FUNCTIONS TO "${firebaseReaderRole}"`,
-    `SET ROLE = cloudsqlsuperuser`,
+function defaultPermissions(databaseId: string, schema: string, ownerRole: string) {


Nice cleanup.

Would it make sense to port those changes to backend code as well to keep them reasonably aligned?

I know they should do the same thing for green field.

To make this sync easier in the future, it might make sense to split caller logics to separate files.

We can split logics that aren't copied to the backend permissions.go into permissions_setup.ts. It can have getSchemaMetaData‎ , greenFieldSchemaSetup, brownfieldSqlSetup, etc.

fredzqm · 2025-02-02T20:08:55Z

src/gcp/cloudsql/permissions.ts

+  };
+}
+
+export async function getUserAccessLevel(


Is it used?

fredzqm · 2025-02-02T20:24:29Z

src/gcp/cloudsql/permissions.ts

+  // Step 1: Grant firebasesuperuser access to the original owner
+  const uniqueTablesOwners = [...new Set(schemaInfo.tables.map((t) => t.owner))];
+  const grants = uniqueTablesOwners.map((owner) => `GRANT ${owner} TO ${FIREBASE_SUPER_USER}`);
+  await executeSqlCmdsAsSuperUser(options, instanceId, databaseId, grants, silent);


nit: any reason, this couldn't be executed in the same executeSqlCmdsAsSuperUser as below.

Each executeSqlCmdsAsSuperUser would call UpsertUser with random password. Adds a few seconds of delay.

fredzqm · 2025-02-02T20:26:59Z

src/gcp/cloudsql/permissions.ts

+      return true;
+    }
+  } else {
+    logger.info(`Detected schema "${schema}" isn't fully setup or setup as brownfield project.`);


nit: Shall we log schemaInfo.setupStatus here to help debugging?

fredzqm · 2025-02-02T20:29:44Z

src/gcp/cloudsql/permissions.ts

+    logger.info(
+      clc.yellow(
+        "Setting up database in brownfield mode.\n" +
+          "Note: SQL migrations can't be done through FDC in this mode.",


done through clc.bold("firebase dataconnect:sql:migrate")

Be specific about what command we are referring to.

fredzqm · 2025-02-02T20:35:37Z

src/dataconnect/schemaMigration.ts

-    // until we confirm stability it's ok to run it on every migration by admin user.
-    if (userIsCSQLAdmin) {
-      await setupIAMUsers(instanceId, databaseId, options);
+    const schemaInfo = await getSchemaMetaData(instanceId, databaseId, DEFAULT_SCHEMA, options);


Should we check setupStatus the first thing in handleIncompatibleSchemaError?

When handleIncompatibleSchemaError is called, we have detected some diffs in sql:migrate. We should surface any permission setup error before any SQL migration specific errors.

tammam-g added 9 commits January 14, 2025 17:15

Helper functions and basic setup for dataconnect:sql:setup

cf4feab

Refactor setupIamUsers for better composability

37bdfa5

FDC MVP brownfield and greenfield to brownfield schema setup

830e010

Add required logic inside schemaMigration for handling brownfield

2f445be

Cleanup and fix bugs in brownfield setup

ee20d66

Use firebasesuperuser instead of cloudsqlsuper user for brownfield mi…

f7edd16

…gration success

Add default permissions for brownfield

bf5c904

Fix lint/format

d4d80e0

Refactor to allow setup reruns

4fce5d3

tammam-g requested review from fredzqm and joehan January 31, 2025 21:52

fredzqm requested changes Feb 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fdc brownfield setup #8150

Fdc brownfield setup #8150

tammam-g commented Jan 29, 2025 •

edited

Loading

fredzqm left a comment

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

fredzqm Feb 2, 2025

Fdc brownfield setup #8150

Are you sure you want to change the base?

Fdc brownfield setup #8150

Conversation

tammam-g commented Jan 29, 2025 • edited Loading

Description

Scenarios Tested

Fresh greenfield setup

Brownfield to greenfield setup

Brownfield setup test

BrownField Default Permissions Test

Additional flags test

Sample Commands

fredzqm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammam-g commented Jan 29, 2025 •

edited

Loading