chore: Skip the creation of secondary perms during catalog migrations

[Vitor-Avila]

SUMMARY

The migrations to add catalog permissions can take considerably long, as it would get all catalogs exposed to the credentials, then get all schemas for each of these catalogs and create these permissions accordingly.

This requires longer migrations that typically run with downtime.

This PR adds a flag to the config that allows to skip these permissions, as these can be created later once the app is up by just editing the DB connection.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No UI changes.

TESTING INSTRUCTIONS

Unit tests added.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/migrations/shared/catalogs.py	✅
superset/config.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

​

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Issue Categories

​

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

[michael-s-molina]

This flag allows to skip the
creation of these secondary perms, and focus only on permissions for the default
catalog. These secondary permissions can be created later by editing the DB
connection via the UI (without downtime).

@Vitor-Avila if these migrations are optional, why are they executed by default in the first place? Isn't better to just remove them from the upgrade script instead of create a new feature flag? If they are not optional, what happens when they are skipped because of the feature flag?

[betodealmeida]

Nice!!

[Vitor-Avila]

@Vitor-Avila if these migrations are optional, why are they executed by default in the first place? Isn't better to just remove them from the upgrade script instead of create a new feature flag? If they are not optional, what happens when they are skipped because of the feature flag?

Hey @michael-s-molina,

Before catalogs, to query something from another database in the DB instance/host users would have to do it via a virtual dataset using select * from <other_catalog>.<schema>.<table>. These "secondary permissions" are created so that after the migration, limited users need access on these other catalogs in order to query them in SQL Lab.

For existing datasets and charts, they'll all be associated with the default_catalog in their permission setting (even if they're querying other catalog) as we won't parse the query to determine (the same happens today if you create a dataset with schema_a selected in the dropdown and use select * from schema_b.table).

That said, ideally creating these permissions during the migration is a good thing, but we've seen cases where this process takes too long, and that causes a very long migration/downtime. For example, there used to be a Postgres Cloud service named ElephantSQL, and your credentials would be able to list all DBs in the instance (even though you actually don't have access to these other DBs).

These permissions are going to be added by the time you edit the DB configuration (either to enable the ability to change catalogs, or any other change actually).

I thought about adding it as a config because if you're able to run the migration with the config disabled, then it's great! All perms created. But if you start facing issues, a long downtime, then enabling the setting might be ideal.

Let me know your thoughts.

[betodealmeida]

@Vitor-Avila if these migrations are optional, why are they executed by default in the first place? Isn't better to just remove them from the upgrade script instead of create a new feature flag? If they are not optional, what happens when they are skipped because of the feature flag?

@michael-s-molina we want to apply these migrations as soon as possible, because without them the data in non-default catalogs is vulnerable — users with access to schema foo in the default catalog will be able to access schemas with the same name in the non-default catalogs, which is probably not what the administrator wants.

In most cases the migration is quick and painless, but of course there are people with huge databases where this can take considerable time. So this is a compromise between security and performance.

[michael-s-molina]

Got it. Thanks for the additional context @Vitor-Avila @betodealmeida!