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!

src/gcp/cloudsql/connect.ts

src/gcp/cloudsql/permissions.ts

src/dataconnect/schemaMigration.ts

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.

+1 to splitting this up into multiple files - I'd love to keep the prompting logic in a separate file from the SQL command logic

I moved setupSQLPermissions to the command file dataconnect-sql-setup.ts, this way interactivity is not part of permissions and it's within the command file which makes sense to me. I do import the setupSQLPermissions in schemaMigration.ts but I also feel this make sense, it basically makes it clear that we are running the setup workflow within the migration.

commands/** should only have scaffolding for each comment.

Any re-usable logics should be defined in the dataconnect/* package.

For the sake of making it easier to map it to the backend permissions.go, can you move any logics not in backend to a separate file?

Yeah, split it into two files

src/gcp/cloudsql/permissions.ts

src/dataconnect/schemaMigration.ts

src/gcp/cloudsql/permissions.ts

joehan · 2025-02-03T23:25:06Z

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) {


+1 to splitting this up into multiple files - I'd love to keep the prompting logic in a separate file from the SQL command logic

src/gcp/cloudsql/permissions.ts

codecov-commenter · 2025-02-12T01:05:15Z

Codecov Report

Attention: Patch coverage is 23.64865% with 113 lines in your changes missing coverage. Please review.

Project coverage is 51.02%. Comparing base (774d3f0) to head (e928a6b).
Report is 51 commits behind head on master.

Files with missing lines	Patch %	Lines
src/gcp/cloudsql/permissions.ts	24.35%	59 Missing ⚠️
src/commands/dataconnect-sql-setup.ts	28.84%	37 Missing ⚠️
src/dataconnect/schemaMigration.ts	7.14%	13 Missing ⚠️
src/gcp/cloudsql/connect.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8150      +/-   ##
==========================================
- Coverage   51.10%   51.02%   -0.08%     
==========================================
  Files         423      424       +1     
  Lines       29636    30473     +837     
  Branches     6063     6322     +259     
==========================================
+ Hits        15144    15548     +404     
- Misses      13134    13555     +421     
- Partials     1358     1370      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ownfield

fredzqm · 2025-02-12T22:56:39Z

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.

It should already work in the way that you want.

sql:migrate talks to backend to determine if there are any diffs. If there are no IncompatibleSchemaError returned, it would actually skip any migration steps. Brown field customer can use firebase deploy to deploy FDC Schema & Connectors as long as there is no diff.

src/dataconnect/schemaMigration.ts

fredzqm · 2025-02-12T23:00:12Z

src/commands/dataconnect-sql-setup.ts

+      await greenFieldSchemaSetup(instanceId, databaseId, schema, options, silent);
+      logger.info(clc.green("Database setup complete."));
+      return true;
+    }


If !rerunSetup, should we abort?

Honestly, don't find this prompt that valuable.

If they run sql:setup, the intent to setup SQL is already clear. It feels redundant to ask them again.

Logging is enough here.

fredzqm · 2025-02-12T23:01:44Z

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) {


commands/** should only have scaffolding for each comment.

Any re-usable logics should be defined in the dataconnect/* package.

For the sake of making it easier to map it to the backend permissions.go, can you move any logics not in backend to a separate file?

src/commands/dataconnect-sql-setup.ts

src/dataconnect/schemaMigration.ts

…backend permissions there

…lease

…s. (#8204) * Format INTERACTIVE_ACK issues as table as well and add extra "type" column to table. * Update warning and prompt wording to reflect insecure operations as well as connector evolution issues. * Wording. * Sort issues in table by category and some formatting fixes.

* Use correct import path for data connect emulator * Actually fix it this time * fix the thing i broke and format * Update src/emulator/dataconnect/pgliteServer.ts Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com> --------- Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com>

* Don't surface connector evolution or insecure operation issues in VSCode.' * Fix * Filter by "INSECURE" substring rather than warningLevel.

…hen env vars for emulators are set (#8231) * fix bug where apphosting emulator info is not complete when env vars for other emulators are set * add proper fix and test * fix * remove async from non-async test func * address comments

* Bump FDC local toolkit to v1.8.2. * Update changelog.

…lease

* fix: #8168 - enforce webframeworks only when needed In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks. * remove console * add changelog, add tests * Add matchesHostingTarget to improve readability --------- Co-authored-by: Chalo Salvador <chalo@monogram.io>

…8237) * Added env var to magically import data connect service from console * actually check location too * formats * Formats

* Improve robustness of function discovery for python Anecdotally, python function discovery is flakey. We propose 2 change in this PR: 1. For python discovery, add a small initial delay for python's admin server to boot. 2. Add a request timeout to retry call to retrieve trigger information. Previously, the default timeout would've been set to OS-level TCP timeout, which in my laptop was between 20~30s. * Add changelog. * Remove per-req timeout to accomodate loading large/slow main.py. * Update changelog. * Revert timeout bump.

* update vscode to 0.13.1 * remove changelog line

* Apply ajv and ajv-formats overrides to shrinkwrap * Apply whatwg-url override to shrinkwrap * npm i to stabilize shrinkwrap --------- Co-authored-by: Joe Hanley <joehanley@google.com>

`--location` will be removed from apphosting commands in the next major CLI release. Before then, the CLI will print a warning about this removal whenever `--location` is used.

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

Helper functions and basic setup for dataconnect:sql:setup

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

cf4feab

Refactor setupIamUsers for better composability

Loading
Loading status checks…

37bdfa5

FDC MVP brownfield and greenfield to brownfield schema setup

Loading
Loading status checks…

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

joehan reviewed Feb 3, 2025

View reviewed changes

src/gcp/cloudsql/permissions.ts Outdated Show resolved Hide resolved

joehan requested changes Feb 3, 2025

View reviewed changes

tammam-g added 2 commits February 11, 2025 19:50

Fix small things and address comments

b1931e4

Fix bug in role grants

e928a6b

tammam-g added 6 commits February 12, 2025 12:08

Add logging that database setup completed

ff1b518

Make grant command not go through setup if roles can be granted in br…

f8e4ec1

…ownfield

bug fix from changing the getting schema owner command

fc51dae

Simplify getSchemaMetaData in permissions.ts

f53e45b

Merge branch 'master' into fdc-brownfield-setup

61e66fc

Fix log statement

0c72a2b

tammam-g requested a review from fredzqm February 12, 2025 17:44

fredzqm reviewed Feb 12, 2025

View reviewed changes

tammam-g added 4 commits February 14, 2025 13:59

Split permissions.ts into front facing permissions_setup.ts and keep …

17e3c58

…backend permissions there

No need to ask user if they want to rerun greenfield setup

8e29299

Make setupSQLPermissions return a setup status instead of a boolean

f52a6b1

Change an if statment to switch statement

7038a28

maneesht and others added 23 commits March 10, 2025 18:09

FDC Emulator Update v1.8.1(#8216)

e717aca

13.31.1

a5950df

[firebase-release] Removed change log and reset repo after 13.31.1 re…

3ec0f2a

…lease

Don't surface insecure operations errors in VSCode. (#8215)

7b07e17

* Don't surface connector evolution or insecure operation issues in VSCode.' * Fix * Filter by "INSECURE" substring rather than warningLevel.

Add path information to formatted GraphqlError. (#8228)

2052bc1

Bump FDC local toolkit to v1.8.2. (#8232)

409dde6

* Bump FDC local toolkit to v1.8.2. * Update changelog.

13.31.2

6acc6ae

[firebase-release] Removed change log and reset repo after 13.31.2 re…

e3a6e8c

…lease

Added env var to magically import data connect service from console (#…

cf4f30e

…8237) * Added env var to magically import data connect service from console * actually check location too * formats * Formats

Update vscode to 0.13.1 (#8236)

ddc0514

* update vscode to 0.13.1 * remove changelog line

Propagate overrides (#8253)

8fcccce

* Apply ajv and ajv-formats overrides to shrinkwrap * Apply whatwg-url override to shrinkwrap * npm i to stabilize shrinkwrap --------- Co-authored-by: Joe Hanley <joehanley@google.com>

Print warning about --location removal from apphosting commands. (#8229)

01bbf36

`--location` will be removed from apphosting commands in the next major CLI release. Before then, the CLI will print a warning about this removal whenever `--location` is used.

Fix issue where apps:init breaks on app creation (#8258)

f49703f

Rename MetaData to Metadata

c1aa045

Change setup to set up in firebase error

8840d77

Improve logger message

b5500fa

Merge branch 'master' into fdc-brownfield-setup

16d7144

Fix bugs in brownfield setup status checks

b962c71

tammam-g enabled auto-merge (squash) March 10, 2025 23:14

tammam-g disabled auto-merge March 10, 2025 23:15

fix lint issues

916129b

tammam-g enabled auto-merge (squash) March 10, 2025 23:18

tammam-g disabled auto-merge March 11, 2025 01:12

tammam-g enabled auto-merge (squash) March 11, 2025 01:13

tammam-g merged commit 1d45035 into master Mar 11, 2025
54 of 55 checks passed

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

joehan Feb 3, 2025

tammam-g Feb 12, 2025 •

edited

Loading

fredzqm Feb 12, 2025

tammam-g Feb 14, 2025

joehan Feb 3, 2025

codecov-commenter commented Feb 12, 2025

fredzqm commented Feb 12, 2025

fredzqm Feb 12, 2025

fredzqm Feb 12, 2025

Fdc brownfield setup #8150

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

fredzqm Feb 2, 2025

Choose a reason for hiding this comment

joehan Feb 3, 2025

Choose a reason for hiding this comment

tammam-g Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

fredzqm Feb 12, 2025

Choose a reason for hiding this comment

tammam-g Feb 14, 2025

Choose a reason for hiding this comment

joehan Feb 3, 2025

Choose a reason for hiding this comment

codecov-commenter commented Feb 12, 2025

Codecov Report

fredzqm commented Feb 12, 2025

fredzqm Feb 12, 2025

Choose a reason for hiding this comment

fredzqm Feb 12, 2025

Choose a reason for hiding this comment

tammam-g commented Jan 29, 2025 •

edited

Loading

tammam-g Feb 12, 2025 •

edited

Loading