-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adapter: Restrict the queries that can run on mz_introspection #18312
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,6 +362,9 @@ impl ErrorResponse { | |
AdapterError::SqlCatalog(_) => SqlState::INTERNAL_ERROR, | ||
AdapterError::SubscribeOnlyTransaction => SqlState::INVALID_TRANSACTION_STATE, | ||
AdapterError::Transform(_) => SqlState::INTERNAL_ERROR, | ||
AdapterError::UnallowedOnCluster { .. } => { | ||
SqlState::S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED | ||
} | ||
Comment on lines
+365
to
+367
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries! Looking through all of the error codes the following seem to be somewhat decent:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, 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. |
||
AdapterError::Unauthorized(_) => SqlState::INSUFFICIENT_PRIVILEGE, | ||
AdapterError::UncallableFunction { .. } => SqlState::FEATURE_NOT_SUPPORTED, | ||
AdapterError::UnknownCursor(_) => SqlState::INVALID_CURSOR_NAME, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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