adapter: Restrict the queries that can run on mz_introspection #18312

do-not-use-parker-timmerman · 2023-03-21T21:54:20Z

Motivation

This PR adds a known-desirable feature.
- Fixes Further restrict mz_introspection cluster #18075

This PR restricts what queries can get run on the mz_introspection cluster. Specifically only queries that depend on system tables can get run. As part of this change, we also update the dbt-adapter to set the auto_route_introspection_queries session var to make sure all of its introspection queries automatically get run on the mz_introspection cluster, regardless of what cluster is currently set.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

benesch · 2023-03-22T03:40:32Z

Note: This PR is stacked on top of #18310 but I couldn't figure out how to get GitHub to only recognize the changes made to this branch, and not the changes since main

Unfortunately GH has no support for stacked PRs. What you've done here is about the best you can do. Recommend just mentioning in the PR description the title of the commit at which to start reviewing. That's what most folks do.

benesch · 2023-03-22T03:45:22Z

The code here looks 👌🏽. I suspect that the reason you're still seeing this failure in the dbt tests...

[2023-03-21T22:13:34Z] 22:13:08    querying the following items "materialize.test16794367864971609909_test_basic.base" is not allowed from the "mz_introspection" cluster. Use `SET CLUSTER = <cluster-name>` to change your cluster and re-run the query.
--
  | [2023-03-21T22:13:34Z] 22:13:08    compiled Code at target/run/base/models/swappable.sql

...is because the version check is for v0.47, but what you actually need is a version check for v0.48-dev, since this PR did not make it into v0.47. Looks like we forgot to update main to v0.48-dev; I'll do that now.

benesch · 2023-03-22T03:47:27Z

Here's the PR to bump to v0.48-dev: #18319. Feel free to merge tomorrow. If you merge that PR, then rebase this PR on top of it, and then update the version check to v0.48-dev, I think the dbt tests will start passing. 🤞🏽

do-not-use-parker-timmerman · 2023-03-22T17:37:44Z

Unfortunately GH has no support for stacked PRs. What you've done here is about the best you can do. Recommend just mentioning in the PR description the title of the commit at which to start reviewing. That's what most folks do.

Darn, stacked PRs are a great feature, thanks for the heads up though! I'll update the description

do-not-use-parker-timmerman · 2023-03-22T17:41:51Z

The code here looks 👌🏽. I suspect that the reason you're still seeing this failure in the dbt tests...
[2023-03-21T22:13:34Z] 22:13:08    querying the following items "materialize.test16794367864971609909_test_basic.base" is not allowed from the "mz_introspection" cluster. Use `SET CLUSTER = <cluster-name>` to change your cluster and re-run the query.
--
  | [2023-03-21T22:13:34Z] 22:13:08    compiled Code at target/run/base/models/swappable.sql
...is because the version check is for v0.47, but what you actually need is a version check for v0.48-dev, since this PR did not make it into v0.47. Looks like we forgot to update main to v0.48-dev; I'll do that now.

First, sorry about the comment edit, I clicked too quickly and thought I was replying not editing 😅

Thanks for the heads up about the version and the PR! I'll update my branch and push 🤞

do-not-use-parker-timmerman · 2023-03-23T15:51:15Z

Note: this PR is on hold for the moment because we need #18310 to land first, and the LaunchDarkly gate to be rolled out to 100%

morsapaes · 2023-04-17T15:51:25Z

misc/dbt-materialize/dbt/adapters/materialize/connections.py

        if not versions_compatible(mz_version, SUPPORTED_MATERIALIZE_VERSIONS):
            raise dbt.exceptions.DbtRuntimeError(
                f"Detected unsupported Materialize version {mz_version}\n"
                f"  Supported versions: {SUPPORTED_MATERIALIZE_VERSIONS}"
            )
+        elif not versions_compatible(mz_version, AUTO_MZ_INTROSPECTION_VERSION):


Since all user environments are upgraded in tandem, we could just bump SUPPORTED_MATERIALIZE_VERSIONS to >=0.48.0 and avoid the extra conditional logic altogether.

Unless CI is testing a version range that goes further into the past, and we want to keep it around for that reason.

I'm not aware of anything in CI that tests the version range, let's see if anything breaks! :)

jkosh44 · 2023-04-17T18:34:06Z

src/pgwire/src/message.rs

@@ -362,6 +362,7 @@ impl ErrorResponse {
            AdapterError::SqlCatalog(_) => SqlState::INTERNAL_ERROR,
            AdapterError::SubscribeOnlyTransaction => SqlState::INVALID_TRANSACTION_STATE,
            AdapterError::Transform(_) => SqlState::INTERNAL_ERROR,
+            AdapterError::UnallowedOnCluster { .. } => SqlState::INTERNAL_ERROR,


This doesn't seem like an INTERNAL_ERROR to me. I'm not sure what the best error code to use here is since this scenario is not possible in PostgreSQL. Maybe SqlState::INSUFFICIENT_PRIVILEGE? Though that one maybe implies there's some privilege that would allow the user to execute this. ~~Maybe SqlState::S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED~~ Never mind, that's specifically for SQL Routines.

Whatever you pick it should indicate that the user did something they weren't allowed to, not that it was an internal error. (For reference: https://www.postgresql.org/docs/current/errcodes-appendix.html).

That makes sense to me! I updated it to be SqlState::S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED, nothing else seemed to be a great fit. Maaaaaybe INSUFFICIENT_RESOURCES but that's not really correct either

jkosh44 · 2023-04-17T18:46:06Z

src/pgwire/src/message.rs

+            AdapterError::UnallowedOnCluster { .. } => {
+                SqlState::S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED
+            }


Sorry, I know I suggested this one, but right after I realized that this is not correct either. These are meant for SQL routines (https://www.postgresql.org/docs/current/infoschema-routines.html), which are functions and procedures.

No worries! Looking through all of the error codes the following seem to be somewhat decent:

protocol_violation - Class 08 — Connection Exception

invalid_parameter_value - Class 22 — Data Exception (not sure what classifies as a "parameter" though)

system_error - Class 58 — System Error (errors external to PostgreSQL itself)

system_error seems to be the most generic, so that's what I'm leaning towards, what do you think?

Hmm, invalid_param_value wouldn't work because param refers to parameterized queries like SELECT * FROM $1.
I don't think system_error makes sense either because the error is not external to Materialize. protocol_violation refers to the pgwire protocol, which this query isn't in violation of.

I actually think the privilege error is the best one the more I think about it. There's nothing inherently wrong with the query, we have just made it forbidden when using a specific cluster.

Sorry to be nit-picky on this.

maddyblue · 2023-04-17T19:04:23Z

src/adapter/src/coord/introspection.rs

+    }
+
+    // Allows explain queries.
+    if let Plan::Explain(_) = plan {


Minor concern: I'm not convinced this is helpful? We'd let them run an explain and then when they do a real query it'd error? Also fine leaving as is.

The reason I opted to allow all explain queries was because I pictured a flow where we reject a query because it references non-system tables, and I wanted the user to be able to better understand why we rejected it, i.e. use EXPLAIN, without having to switch clusters. It's a minor detail and I'm totally fine with removing it if we think it'll cause more confusion because of the inconsistency

maddyblue · 2023-04-17T19:05:18Z

src/adapter/src/error.rs

+                write!(
+                    f,
+                    "querying the following items {items} is not allowed from the {} cluster. \
+                    Use `SET CLUSTER = <cluster-name>` to change your cluster and re-run the query.",


This second sentence should probably be a hint or detail. See https://github.com/MaterializeInc/materialize/blob/main/doc/developer/style.md#error-message-style

src/adapter/src/error.rs

morsapaes

Thanks for taking care of the dbt changes, @parker-timmerman! Only missing a changelog note, but I can push that in.

- check if we're allowed to run a given query on mz_introspection - update tests - update dbt connector run dbt version check on mz_introspection cluster

Co-authored-by: morsapaes <marta.paes.moreira@gmail.com> Co-authored-by: Matt Jibson <matt.jibson@gmail.com>

…on` cluster (#19041) ### Motivation #18312 restricted queries that depend on non-system objects, from being run on the `mz_introspection` cluster. The changes it introduced though were too restrictive. This PR relaxes those restrictions, allowing any query to run on mz_introspection, as long as it's not a `SELECT` or `SUBSCRIBE`. ### Tips for reviewer I think I gated against the correct `Plan`s, but two I was unsure about were `Plan::Fetch` and `Plan::Insert`. I'm not sure if we also want to prevent those from running on `mz_introspection`. ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design.  - [ ] This PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way) and therefore is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)).  - [ ] This PR includes the following [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note): -

do-not-use-parker-timmerman mentioned this pull request Mar 21, 2023

coord: restrict queries run on mz_introspection #18276

Closed

5 tasks

do-not-use-parker-timmerman mentioned this pull request Mar 22, 2023

adapter: Automatically run some queries on mz_introspection #18310

Merged

5 tasks

do-not-use-parker-timmerman force-pushed the mz_intro/restrict branch from dfd90f3 to 392460d Compare March 23, 2023 15:48

morsapaes reviewed Apr 17, 2023

View reviewed changes

do-not-use-parker-timmerman force-pushed the mz_intro/restrict branch 2 times, most recently from 7bca5bd to 6535665 Compare April 17, 2023 18:16

do-not-use-parker-timmerman marked this pull request as ready for review April 17, 2023 18:17

do-not-use-parker-timmerman requested a review from a team as a code owner April 17, 2023 18:17

do-not-use-parker-timmerman requested a review from a team April 17, 2023 18:17

jkosh44 reviewed Apr 17, 2023

View reviewed changes

maddyblue approved these changes Apr 17, 2023

View reviewed changes

morsapaes approved these changes Apr 17, 2023

View reviewed changes

morsapaes force-pushed the mz_intro/restrict branch from f230d0e to d9bca89 Compare April 17, 2023 22:14

ParkMyCar added 2 commits April 18, 2023 10:13

start

328d22d

- check if we're allowed to run a given query on mz_introspection - update tests - update dbt connector run dbt version check on mz_introspection cluster

change error code that is emitted

0230f00

do-not-use-parker-timmerman force-pushed the mz_intro/restrict branch 2 times, most recently from 31ce8a9 to da2abec Compare April 18, 2023 14:18

do-not-use-parker-timmerman enabled auto-merge (squash) April 18, 2023 14:27

update dbt-connector CHANGELOG.md, change error to hint

526b384

Co-authored-by: morsapaes <marta.paes.moreira@gmail.com> Co-authored-by: Matt Jibson <matt.jibson@gmail.com>

do-not-use-parker-timmerman force-pushed the mz_intro/restrict branch from da2abec to 526b384 Compare April 18, 2023 17:20

fix sqllitetest

2fe15fc

do-not-use-parker-timmerman force-pushed the mz_intro/restrict branch from de5c117 to 2fe15fc Compare April 18, 2023 20:06

do-not-use-parker-timmerman merged commit 6847744 into MaterializeInc:main Apr 18, 2023

do-not-use-parker-timmerman deleted the mz_intro/restrict branch April 18, 2023 20:33

materialize-bot mentioned this pull request Apr 21, 2023

release: v0.52.0 required reviews #18907

Closed

12 tasks

ParkMyCar mentioned this pull request Apr 28, 2023

adapter: Allow more kinds of queries to be run on the mz_introspection cluster #19041

Merged

5 tasks

dehume mentioned this pull request Aug 28, 2023

dbt-materialize: use connection options instead of SET #18263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter: Restrict the queries that can run on mz_introspection #18312

adapter: Restrict the queries that can run on mz_introspection #18312

do-not-use-parker-timmerman commented Mar 21, 2023 •

edited

Loading

benesch commented Mar 22, 2023

benesch commented Mar 22, 2023 •

edited by do-not-use-parker-timmerman

Loading

benesch commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 23, 2023

morsapaes Apr 17, 2023

morsapaes Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

jkosh44 Apr 17, 2023 •

edited

Loading

do-not-use-parker-timmerman Apr 17, 2023

jkosh44 Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

jkosh44 Apr 17, 2023

maddyblue Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

maddyblue Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

morsapaes left a comment

adapter: Restrict the queries that can run on mz_introspection #18312

adapter: Restrict the queries that can run on mz_introspection #18312

Conversation

do-not-use-parker-timmerman commented Mar 21, 2023 • edited Loading

Motivation

Checklist

benesch commented Mar 22, 2023

benesch commented Mar 22, 2023 • edited by do-not-use-parker-timmerman Loading

benesch commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 22, 2023

do-not-use-parker-timmerman commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkosh44 Apr 17, 2023 • edited Loading

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

morsapaes left a comment

Choose a reason for hiding this comment

do-not-use-parker-timmerman commented Mar 21, 2023 •

edited

Loading

benesch commented Mar 22, 2023 •

edited by do-not-use-parker-timmerman

Loading

jkosh44 Apr 17, 2023 •

edited

Loading