You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "radford nguyen (Code Review)" <ge...@cloudera.org> on 2019/04/01 05:22:34 UTC

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag 

radford nguyen has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12901


Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
14 files changed, 201 insertions(+), 57 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/1
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 1
Gerrit-Owner: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#4).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
M tests/authorization/test_ranger.py
15 files changed, 229 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/4
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 18:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2917/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 18
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 19:40:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#9).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
15 files changed, 331 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/9
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 9
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 21:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2943/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 23:02:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2636/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 04:33:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23:

> Patch Set 23: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4094/

Failure in unrelated test, possibly flaky test: ImpalaBeeswaxException: ImpalaBeeswaxException:  INNER EXCEPTION: <class 'beeswaxd.ttypes.BeeswaxException'>  MESSAGE: TableLoadingException: Loading file and block metadata for 1 paths for table cachedb.cached_tbl_part: failed to load 1 paths. Check the catalog server log for more details.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 13:32:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 19: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4068/


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 19
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 00:28:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 2:

(13 comments)

You also need to add E2E tests in the python testing.

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@41
PS2, Line 41:     "sentry",
nit: move default to line above like other DEFINE


http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@45
PS2, Line 45:     "",
nit: move default to line above like other DEFINE.


http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@46
PS2, Line 46: "Specifies the class name that implements the authorization provider. "
            :     "This will override the authorization_provider flag if both are specified."
nit: Wrap at the character limit not at the end of a sentence.


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@31
PS2, Line 31: factoryClass
Should this be `factoryClassName`?


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@67
PS2, Line 67:       public String getProviderName() { return "none"; }
Can we make "none" a public static final String?


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java@45
PS2, Line 45:   public String getProviderName() { return "ranger"; }
Can we make "ranger" a public static final String?


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java@130
PS2, Line 130:   public String getProviderName() { return "sentry"; }
Can we make "sentry" a public static final String?


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141
PS2, Line 141:   public @Nullable String getAuthorizationFactoryClassOrNull() {
> Not sure if there's a standard for handling optional flags... at least I di
nit: I'm not opposed to adding the @Nullable annotation and adding `orNull` in the class name but it is inconsistent with the rest of the code base.


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@773
PS2, Line 773: * @param beCfg
             :    * @return
             :    * @throws InternalException
Finish the documentation


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@778
PS2, Line 778:   throws InternalException {
nit: indent 4


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@794
PS2, Line 794: +authzProvider
nit: add space around the `+`


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@812
PS2, Line 812:   throws InternalException {
nit: indent 4


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@822
PS2, Line 822: +authzFactoryClassName
nit: add space around the `+`



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 2
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 17:05:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 19:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4068/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 19
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:59:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2816/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 5
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:13:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java@113
PS1, Line 113:     final AuthorizationFactory authzFactory = JniFrontend.authzFactoryFrom(BackendConfig.INSTANCE);
line too long (99 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@92
PS1, Line 92:       Class<? extends AuthorizationFactory> authorization_factory_class, String authorization_provider) {
line too long (105 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@93
PS1, Line 93:     return authCfg(authorization_factory_class.getCanonicalName(), authorization_provider);
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@110
PS1, Line 110:   public void testAuthorizationPolicySelection_FactoryTakesPrecedenceOverProvider() throws Exception {
line too long (102 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@113
PS1, Line 113:         JniFrontend.authzFactoryClassNameFrom(authCfg(SentryAuthorizationFactory.class, "ranger"))
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@118
PS1, Line 118:         JniFrontend.authzFactoryClassNameFrom(authCfg(RangerAuthorizationFactory.class, "sentry"))
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@123
PS1, Line 123:         JniFrontend.authzFactoryClassNameFrom(authCfg(SentryAuthorizationFactory.class, "sentry"))
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@128
PS1, Line 128:         JniFrontend.authzFactoryClassNameFrom(authCfg(RangerAuthorizationFactory.class, "ranger"))
line too long (98 > 90)



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 1
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 05:23:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 16:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2898/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 16
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:40:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has removed Todd Lipcon from this change.  ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Removed reviewer Todd Lipcon.
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 19
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2817/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 6
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:21:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 18: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 18
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:59:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2601/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 1
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 06:04:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 1:

Hi guys, relatively simply ticket here to add a more human-friendly flag for choosing authorization provider(s)


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 1
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 05:24:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java@130
PS2, Line 130:   public String getProviderName() { return "sentry"; }
> I dunno, in this case the magic string is returned as the implementation of
IOW, if we provide a constant, that just means that there will be multiple paths to getting the same information and they could potentially and unintentionally diverge



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Apr 2019 04:45:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2819/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 8
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:33:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2852/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 10
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 22:14:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4094/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 04:42:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Reviewed-on: http://gerrit.cloudera.org:8080/12901
Reviewed-by: Fredy Wijaya <fw...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 340 insertions(+), 62 deletions(-)

Approvals:
  Fredy Wijaya: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 24
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#2).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
14 files changed, 217 insertions(+), 57 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/2
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 2
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2818/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 7
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:29:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@41
PS2, Line 41:     "sentry",
> Honest question: why?
It's a nit so it is not something that needs to be fixed for the change to be merged. I just noticed all the other DEFINE_string in this file had the default value on the same line so I pointed it out.


http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:

http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py@38
PS4, Line 38:                   "--authorization_provider=ranger")
> I changed these to use the new flag as I imagine this is the preferred way 
Would prefer if there was an e2e test with an invalid as well as valid `--authorization_factory_class`. These tests could be put in a new, separate e2e test file.



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Apr 2019 05:33:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 17:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4061/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 17
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:20:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2602/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 2
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 10:02:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#5).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 315 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/5
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 5
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 23:26:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#8).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 306 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/8
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 8
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#16) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 335 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/16
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 16
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 10:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/12901/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12901/10//COMMIT_MSG@23
PS10, Line 23: 
             : Manually started minicluster with each of following flags
             : and verified correct authorization strategy chosen:
             : 
             : - provider='' factory='' => sentry
             : - provider=sentry factory='' => sentry
             : - provider=ranger factory='' => ranger
             : - provider='' factory=sentry => sentry
             : - provider='' factory=ranger => ranger
             : - provider=sentry factory=sentry => sentry
             : - provider=ranger factory=sentry => sentry
             : - provider=sentry factory=ranger => ranger
             : - provider=ranger factory=ranger => ranger
             : 
             : Wrote unit tests to capture above assertions
             : 
             : Ran fe unit and e2e tests
             : 
             : Wrote e2e test to verify new flag behavior
nit: add bullet points for each item?


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@30
PS10, Line 30:   This is
             :    * typically used only for logging.
this comment is not necessary


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@35
PS10, Line 35: factoryClassName
nit: the naming convention is factoryClassName_


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@37
PS10, Line 37:   AuthorizationProvider(String factoryClassName) {
             :     this.factoryClassName = factoryClassName;
             :   }
             : 
             :   /**
             :    * Returns the canonical name of the {@link AuthorizationFactory} implementation
             :    * associated with this provider.
             :    *
             :    * @return the canonical name of the {@link AuthorizationFactory} impl for `this`
             :    */
             :   public String getAuthorizationFactoryClassName() {
             :     return this.factoryClassName;
             :   }
nit: remove this.


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/BackendConfig.java@31
PS10, Line 31: 
nit: remove extra new line


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniCatalog.java@116
PS10, Line 116:     final AuthorizationFactory authzFactory
              :         = JniFrontend.authzFactoryFrom(BackendConfig.INSTANCE);
sharing code between JniCatalog and JniFrontend is weird. Maybe create a helper class or helper function elsewhere?


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@791
PS10, Line 791: );
nit: move to L790


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@796
PS10, Line 796: );
nit: move to L795


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/main/java/org/apache/impala/service/JniFrontend.java@824
PS10, Line 824: );
nit: move to L823


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@326
PS10, Line 326:           public String getProviderName() {
              :             return "noop";
              :           }
nit: can be put in a single line


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@81
PS10, Line 81:   private static BackendConfig authCfg(
the whole thing here should be in a separate test class


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@102
PS10, Line 102: _onlyProviderSpecified
nit: don't use underscore


http://gerrit.cloudera.org:8080/#/c/12901/10/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@115
PS10, Line 115: _FactoryTakesPrecedenceOverProvider
nit: don't use underscore


http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@28
PS10, Line 28: class TestProviderFails(CustomClusterTestSuite):
I think we can just put the test in test_authorization.py instead.


http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@43
PS10, Line 43: 
nit: remove new line


http://gerrit.cloudera.org:8080/#/c/12901/10/tests/authorization/test_provider.py@48
PS10, Line 48: 
nit: remove new line



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 10
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 16:55:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#7).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 305 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/7
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 7
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2850/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 9
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 04:45:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 21:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12901/21/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/21/tests/authorization/test_provider.py@60
PS21, Line 60: )
flake8: E124 closing bracket does not match visual indentation



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 22:15:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#6).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 313 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/6
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 6
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 16: Code-Review+2

Thanks Radford and Austin for the patch!


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 16
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:20:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 15:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2897/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 15
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:29:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 9:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141
PS9, Line 141: getAuthorizationFactoryClassOrNull
> nit: remove the `orNull` in the function name
because the annotation is already present or because you don't like the name?  i think there should be some indication that the return value can be null, if we're not using Optional


http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java@114
PS9, Line 114: //<<<<<<< 67f77d41d40523074385b8dbccfa6ef6ef81dd57
> Why is this commented out?
ugh was sanity checking a merge conflict and forgot to delete


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@21
PS5, Line 21: import os
> flake8: F401 'time' imported but unused
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@25
PS5, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite
> flake8: F401 'tests.util.filesystem_utils.IS_LOCAL' imported but unused
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@28
PS5, Line 28: class TestProviderFails(CustomClusterTestSuite):
> flake8: F401 'tests.common.impala_test_suite.ImpalaTestSuite' imported but 
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@38
PS5, Line 38:   LOG_DIR = tempfile.mkdtemp(prefix="test_provider_", dir=os.getenv("LOG_DIR"))
> line has trailing whitespace
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@39
PS5, Line 39: 
> flake8: E302 expected 2 blank lines, found 1
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@53
PS5, Line 53: e
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@54
PS5, Line 54: 
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@55
PS5, Line 55: a
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@58
PS5, Line 58: 
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@59
PS5, Line 59: 
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@60
PS5, Line 60: 
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@66
PS5, Line 66:  
> flake8: E502 the backslash is redundant between brackets
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@67
PS5, Line 67: 
> flake8: E501 line too long (92 > 90 characters)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@68
PS5, Line 68:  
> flake8: E124 closing bracket does not match visual indentation
Ack


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@60
PS6, Line 60: 
not the happiest with this approach; open to suggestions


http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py@27
PS7, Line 27: 
> flake8: E302 expected 2 blank lines, found 1
Ack


http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:

http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py@38
PS4, Line 38:   """
> Would prefer if there was an e2e test with an invalid as well as valid `--a
Ack



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 9
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 20:19:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 22:

(1 comment)

Running a dry-run first.

http://gerrit.cloudera.org:8080/#/c/12901/21/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/21/tests/authorization/test_provider.py@60
PS21, Line 60: 
> flake8: E124 closing bracket does not match visual indentation
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 22
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 22:18:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 5:

(24 comments)

http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@21
PS5, Line 21: import time
flake8: F401 'time' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@25
PS5, Line 25: from tests.util.filesystem_utils import IS_LOCAL
flake8: F401 'tests.util.filesystem_utils.IS_LOCAL' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@28
PS5, Line 28: from tests.common.impala_test_suite import ImpalaTestSuite
flake8: F401 'tests.common.impala_test_suite.ImpalaTestSuite' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.CATALOGD_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.IMPALA_LOG_DIR' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.DEFAULT_QUERY_OPTIONS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.IMPALAD_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.SENTRY_CONFIG' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.STATESTORED_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.CLUSTER_SIZE' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@29
PS5, Line 29: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.DEFAULT_CLUSTER_SIZE' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@30
PS5, Line 30: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@38
PS5, Line 38:  
flake8: W293 blank line contains whitespace


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@38
PS5, Line 38:  
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@39
PS5, Line 39: class TestProviderFails(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@53
PS5, Line 53: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@54
PS5, Line 54: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@55
PS5, Line 55: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@58
PS5, Line 58: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@59
PS5, Line 59: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@60
PS5, Line 60: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@66
PS5, Line 66: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@67
PS5, Line 67: G
flake8: E501 line too long (92 > 90 characters)


http://gerrit.cloudera.org:8080/#/c/12901/5/tests/authorization/test_provider.py@68
PS5, Line 68: )
flake8: E124 closing bracket does not match visual indentation



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 5
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:36:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 17: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4061/


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 17
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 02:57:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4094/


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 10:05:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#10).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
15 files changed, 304 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/10
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 10
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2635/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 3
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 02:52:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 2:

(8 comments)

Addressed nits; adding e2e tests next

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/12901/2/be/src/service/frontend.cc@41
PS2, Line 41:     "sentry",
> nit: move default to line above like other DEFINE
Honest question: why?

I don't really see consistent usage in this file; for example `DEFINE_string(ranger_app_id` begins the description string on a newline but `DEFINE_string(server_name` begins the description on the same line... even though both description strings end up wrapping.

I think the current approach is perfectly readable, so I don't understand the point of these 3 nits.


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java@31
PS2, Line 31: factoryClass
> Should this be `factoryClassName`?
Yeah that's a better name.  I was waffling back and forth between using the actual `Class` object vs. the canonical name.


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141
PS2, Line 141:   public @Nullable String getAuthorizationFactoryClassOrNull() {
> nit: I'm not opposed to adding the @Nullable annotation and adding `orNull`
That's the thing, I wasn't able to find another example in the codebase for handling optional flags.  If you could point one out I'll make sure this conforms


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@773
PS2, Line 773: * @param beCfg
             :    * @return
             :    * @throws InternalException
> Finish the documentation
Done


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@778
PS2, Line 778:   throws InternalException {
> nit: indent 4
Done


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@794
PS2, Line 794: +authzProvider
> nit: add space around the `+`
Done


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@812
PS2, Line 812:   throws InternalException {
> nit: indent 4
Done


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@822
PS2, Line 822: +authzFactoryClassName
> nit: add space around the `+`
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 2
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 02:08:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#18) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 338 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/18
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 18
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 13:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2895/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 13
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:17:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java@130
PS2, Line 130:   public String getProviderName() { return "sentry"; }
> Can we make "sentry" a public static final String?
I dunno, in this case the magic string is returned as the implementation of the abstract `getProviderName` method.  IOW, the method itself kind of acts as the public contract for getting the name, and any other entity which wants to get that value should be calling the method (as opposed to accessing a public static constant).



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 20:24:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#14) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
17 files changed, 337 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/14
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 14
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#15) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 318 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/15
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 15
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 21:

> Patch Set 20: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4082/

Failed because of the test generates a minidump (intentional) but finalize.sh treats it as an error. Updating the test to generate a minidump in a location that's not read by finalize.sh.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 22:16:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#22) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 340 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/22
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 22
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 18:16:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 20: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4082/


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 20
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 21:13:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/7/tests/authorization/test_provider.py@27
PS7, Line 27: class TestProviderFails(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 7
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:49:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 22:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2944/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 22
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 23:14:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 20: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 20
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 15:21:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 19: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 19
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:59:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 20:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4082/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 20
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 15:21:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#21) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
16 files changed, 341 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/21
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 21:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4089/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 22:16:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#13) to the change originally created by radford nguyen. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

- Manually started minicluster with each of following flags
  and verified correct authorization strategy chosen:
  - provider='' factory='' => sentry
  - provider=sentry factory='' => sentry
  - provider=ranger factory='' => ranger
  - provider='' factory=sentry => sentry
  - provider='' factory=ranger => ranger
  - provider=sentry factory=sentry => sentry
  - provider=ranger factory=sentry => sentry
  - provider=sentry factory=ranger => ranger
  - provider=ranger factory=ranger => ranger
- Wrote unit tests to capture above assertions
- Ran fe unit and e2e tests
- Wrote e2e test to verify new flag behavior

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
A fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
A tests/authorization/test_provider.py
M tests/authorization/test_ranger.py
17 files changed, 335 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/13
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 13
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 14:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2896/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 14
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:38:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 2:

(10 comments)

> Patch Set 1:
> 
> (8 comments)

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141
PS2, Line 141:   public @Nullable String getAuthorizationFactoryClassOrNull() {
Not sure if there's a standard for handling optional flags... at least I didn't really see one.

We could alternatively return an `Optional` (generally my preference) or just return the raw value that the be populates and let callers decide what is "present" or not.


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java@113
PS1, Line 113:     final AuthorizationFactory authzFactory
> line too long (99 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@92
PS1, Line 92:       Class<? extends AuthorizationFactory> authorization_factory_class,
> line too long (105 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@93
PS1, Line 93:       String authorization_provider) {
> line too long (91 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@110
PS1, Line 110:         JniFrontend.authzFactoryClassNameFrom(authCfg("", "ranger"))
> line too long (102 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@113
PS1, Line 113: 
> line too long (98 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@118
PS1, Line 118:         SentryAuthorizationFactory.class.getCanonicalName(),
> line too long (98 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@123
PS1, Line 123: 
> line too long (98 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/1/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@128
PS1, Line 128:         )
> line too long (98 > 90)
Ack


http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
File fe/src/test/java/org/apache/impala/service/JniFrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/12901/2/fe/src/test/java/org/apache/impala/service/JniFrontendTest.java@47
PS2, Line 47:     origFlags = BackendConfig.INSTANCE.getBackendCfg();
This seems to be what other tests do



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 2
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 01 Apr 2019 09:38:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 4:

(1 comment)

Modified existing e2e tests to use new `authorization_provider` flag

http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:

http://gerrit.cloudera.org:8080/#/c/12901/4/tests/authorization/test_ranger.py@38
PS4, Line 38:                   "--authorization_provider=ranger")
I changed these to use the new flag as I imagine this is the preferred way to select internally-supported auth providers.

I didn't see any existing e2e tests for the `authorization_factory_class` flag, otherwise I would have duplicated them using the `authorization_provider` flag.

Up to this point, looks like we only have full test coverage of authorization selection behavior in the fe, and I'm wondering if this is enough



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 4
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 04:30:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 13:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12901/13/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/13/tests/authorization/test_provider.py@53
PS13, Line 53: T
flake8: F821 undefined name 'TestProviderFails'


http://gerrit.cloudera.org:8080/#/c/12901/13/tests/authorization/test_provider.py@56
PS13, Line 56: T
flake8: F821 undefined name 'TestProviderFails'


http://gerrit.cloudera.org:8080/#/c/12901/13/tests/authorization/test_provider.py@64
PS13, Line 64: T
flake8: F821 undefined name 'TestProviderFails'


http://gerrit.cloudera.org:8080/#/c/12901/13/tests/authorization/test_provider.py@73
PS13, Line 73: T
flake8: F821 undefined name 'TestProviderFails'



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 13
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 20:35:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 17: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 17
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:20:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 10: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 10
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 21:33:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
Hello Austin Nobis, Fredy Wijaya, Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12901

to look at the new patch set (#3).

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................

IMPALA-8309: add user authorization_provider flag

This commit adds a `authorization_provider` user-facing flag
in order to provide a more human-readable alternative to the
`authorization_factory_class` for internally-provided
authorization strategies.

The `authorization_factory_class` flag is retained, but no
longer takes a default value if not specified.  The default
for `authorization_provider` is "sentry" in order to retain
backwards-compatibility.

If specified, `authorization_factory_class` will take
precedence.

Testing:

Manually started minicluster with each of following flags
and verified correct authorization strategy chosen:

- provider='' factory='' => sentry
- provider=sentry factory='' => sentry
- provider=ranger factory='' => ranger
- provider='' factory=sentry => sentry
- provider='' factory=ranger => ranger
- provider=sentry factory=sentry => sentry
- provider=ranger factory=sentry => sentry
- provider=sentry factory=ranger => ranger
- provider=ranger factory=ranger => ranger

Wrote unit tests to capture above assertions

Ran fe unit and e2e tests

Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/service/JniFrontendTest.java
14 files changed, 227 insertions(+), 57 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/12901/3
-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 3
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 6:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py
File tests/authorization/test_provider.py:

http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.CATALOGD_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.IMPALA_LOG_DIR' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.DEFAULT_QUERY_OPTIONS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.IMPALAD_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.SENTRY_CONFIG' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.STATESTORED_ARGS' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.CLUSTER_SIZE' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@25
PS6, Line 25: from tests.common.custom_cluster_test_suite import CustomClusterTestSuite, \
flake8: F401 'tests.common.custom_cluster_test_suite.DEFAULT_CLUSTER_SIZE' imported but unused


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@26
PS6, Line 26: I
flake8: E126 continuation line over-indented for hanging indent


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@35
PS6, Line 35: class TestProviderFails(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@51
PS6, Line 51: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@52
PS6, Line 52: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@53
PS6, Line 53: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@56
PS6, Line 56: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@57
PS6, Line 57: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@58
PS6, Line 58: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/12901/6/tests/authorization/test_provider.py@63
PS6, Line 63: \
flake8: E502 the backslash is redundant between brackets



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 6
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:43:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 21: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4089/


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 21
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 03:14:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/BackendConfig.java@141
PS9, Line 141: getAuthorizationFactoryClassOrNull
nit: remove the `orNull` in the function name


http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12901/9/fe/src/main/java/org/apache/impala/service/JniCatalog.java@114
PS9, Line 114: //<<<<<<< 67f77d41d40523074385b8dbccfa6ef6ef81dd57
Why is this commented out?



-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 9
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 19:58:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 23:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4098/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 23
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Apr 2019 18:15:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8309: add user authorization provider flag

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12901 )

Change subject: IMPALA-8309: add user authorization_provider flag
......................................................................


Patch Set 19: Code-Review+2

> Patch Set 19: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4068/

No tests failed and towards the end after jenkins/finalize.sh, it got this error in Jenkins:
00:26:46 Process leaked file descriptors. See https://jenkins.io/redirect/troubleshooting/process-leaked-file-descriptors for more information

I suspect something unstable with Jenkins. Will retry.


-- 
To view, visit http://gerrit.cloudera.org:8080/12901
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I001c20505ba4f0562b60fdef73d15308e8500c19
Gerrit-Change-Number: 12901
Gerrit-PatchSet: 19
Gerrit-Owner: radford nguyen <ra...@gmail.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 15:20:41 +0000
Gerrit-HasComments: No