Skip to content

Ignore owner if translating Hive view in INVOKER mode#12403

Closed
joechu1 wants to merge 2 commits intotrinodb:masterfrom
joechu1:hive-view-invoker-owner
Closed

Ignore owner if translating Hive view in INVOKER mode#12403
joechu1 wants to merge 2 commits intotrinodb:masterfrom
joechu1:hive-view-invoker-owner

Conversation

@joechu1
Copy link
Copy Markdown
Member

@joechu1 joechu1 commented May 16, 2022

Description

#12221 added the option to set the security mode for querying Hive views to either DEFINER OR INVOKER, where previously Hive views were hardcoded to run in DEFINER mode only. This option was needed because the way Hive views are defined for many organizations did not require setting an owner for the view, which prevents the DEFINER security mode from working.

This PR addresses a bug that appears as a result of that change for a specific code path, where if using the legacy Hive view logic in INVOKER mode with a Hive view that has an owner, queries will fail with the error:
Query 20220919_184230_00013_9ndxr failed: owner cannot be present with runAsInvoker

The bug comes from not taking into account that a ConnectorViewDefinition cannot be created if running as invoker and an owner is set.

This change will avoid passing along the Hive view owner (if there is one) when creating a ConnectorViewDefinition to access a Hive view in INVOKER mode with the legacy code.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Hive connector

How would you describe this change to a non-technical end user or system administrator?

This change prevents an error when querying a Hive view using a specific set of configuration values

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@findepi
Copy link
Copy Markdown
Member

findepi commented May 16, 2022

This suggests #12221 was merged without sufficient end-to-end tests.
@joechu1 would you be able to add a regression test case showing the problem?

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 16, 2022

@joechu1 i see you pushed changes here
also @skrzypo987 is working on #14077.
do we need both, or only one of these?

@joechu1
Copy link
Copy Markdown
Member Author

joechu1 commented Sep 16, 2022

@joechu1 i see you pushed changes here also @skrzypo987 is working on #14077. do we need both, or only one of these?

Only one is needed. I'm happy to move forward with #14077 given that it includes the test cases. I added the new commit based on the previous discussion, but should not be a hard requirement. I'm just making the streamlined code available in case there was interest.

@dain
Copy link
Copy Markdown
Member

dain commented Sep 19, 2022

What does this actually do? The diff doesn't give a lot of context, so when I read it I think the config flag might be overriding the view type in the view definition.... I don't think that is what this code is doing, but I can't tell from the diff. Can you describe the problem and the fix in more detail?

@joechu1
Copy link
Copy Markdown
Member Author

joechu1 commented Sep 19, 2022

@dain I've updated the description for the PR to describe the problem and fix in more detail.

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 20, 2022

Superseded by #14077 which has the changes here + tests.

@findepi findepi closed this Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants