Skip to content
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

Runtime: Prevent introspective queries at compile (SL only) #5926

Merged
merged 20 commits into from Sep 27, 2022

Conversation

racheldaniel
Copy link
Contributor

@racheldaniel racheldaniel commented Sep 23, 2022

resolves #5936

RUNTIME-443

Description

This PR conditionally no-ops warehouse connection at compile depending on an env var. This is a temporary solution to more complex permissions requirements for the semantic layer.

Checklist

@cla-bot cla-bot bot added the cla:yes label Sep 23, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@leahwicz
Copy link
Contributor

@racheldaniel for we need this to get into the 1.3 release?

@racheldaniel
Copy link
Contributor Author

@racheldaniel for we need this to get into the 1.3 release?

@leahwicz yes, and back-ported as well if possible 😬 . What do I need to do to ensure that goes smoothly?

@racheldaniel racheldaniel marked this pull request as ready for review September 26, 2022 17:31
@racheldaniel racheldaniel requested a review from a team September 26, 2022 17:31
@racheldaniel racheldaniel requested a review from a team as a code owner September 26, 2022 17:31
@@ -13,6 +17,46 @@ class RuntimeArgs:
target: str


class SqlCompileRunnerNoIntrospection(SqlCompileRunner):
def compile_and_execute(self, manifest, ctx):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this function is the same as compile_and_execute here with line 313 with self.adapter.connection_for(self.node): removed. Looks good now but I would recommend add a TODO to refactor here when doing runtime(maybe?) so we are not duplicating majority of this function in two places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChenyuLInx Roger that! This entire solution is very temporary-- we actually don't want to disable this functionality long-term, we just need time to find a way to use multiple sets of credentials (deployment for parsing, user-specific for compiling). I will leave a comment to that effect 👍

@leahwicz leahwicz merged commit f1326f5 into main Sep 27, 2022
@leahwicz leahwicz deleted the racheld/RUNTIME-443/prevent-introspection-on-compile branch September 27, 2022 13:49
@leahwicz leahwicz added the backport 1.2.latest This PR will be backported to the 1.2.latest branch label Sep 27, 2022
github-actions bot pushed a commit that referenced this pull request Sep 27, 2022
* Preliminary changes to keep compile from connecting to the warehouse for runtime calls

* Adds option to lib to skip connecting to warehouse for compile; adds prelim tests

* Removes unused imports

* Simplifies test and renames to SqlCompileRunnerNoIntrospection

* Updates name in tests

* Spacing

* Updates test to check for adapter connection call instead of compile and execute

* Removes commented line

* Fixes test names

* Updates plugin to postgres type as snowflake isn't available

* Fixes docstring

* Fixes formatting

* Moves conditional logic out of class

* Fixes formatting

* Removes commented line

* Moves import

* Unmoves import

* Updates changelog

* Adds further info to method docstring

(cherry picked from commit f1326f5)
jtcohen6 pushed a commit that referenced this pull request Sep 29, 2022
…5943)

* Preliminary changes to keep compile from connecting to the warehouse for runtime calls

* Adds option to lib to skip connecting to warehouse for compile; adds prelim tests

* Removes unused imports

* Simplifies test and renames to SqlCompileRunnerNoIntrospection

* Updates name in tests

* Spacing

* Updates test to check for adapter connection call instead of compile and execute

* Removes commented line

* Fixes test names

* Updates plugin to postgres type as snowflake isn't available

* Fixes docstring

* Fixes formatting

* Moves conditional logic out of class

* Fixes formatting

* Removes commented line

* Moves import

* Unmoves import

* Updates changelog

* Adds further info to method docstring

(cherry picked from commit f1326f5)

Co-authored-by: Rachel <41338402+racheldaniel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.2.latest This PR will be backported to the 1.2.latest branch cla:yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1242] [Feature] Conditionally prevent warehouse connection at compile time-- semantic layer only
3 participants