You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2019/02/01 19:32:03 UTC

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure 

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12334


Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................

IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
  config file.
- Calling AuthorizationConfig.validateConfig() was missing.

I also fixed a bug in AuthorizationConfig.validateConfig(), which
skipped the Sentry config validation when the Sentry config file path is
empty. As a result, I updated other tests that considered empty Sentry
config file path to be valid.

Testing:
- Ran AuditingTest individually
- Ran all FE tests

Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
3 files changed, 25 insertions(+), 18 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12334 )

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................

IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
  config file.
- Calling AuthorizationConfig.validateConfig() was missing.

I also fixed a bug in AuthorizationConfig.validateConfig(), which
skipped the Sentry config validation when the Sentry config file path is
empty. As a result, I updated other tests that considered empty Sentry
config file path to be valid.

Testing:
- Ran AuditingTest individually
- Ran all FE tests

Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
6 files changed, 48 insertions(+), 54 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 07:44:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 6: Code-Review+2

So apparently, to enable authorization --sentry_config is optional in impalad, which is why it can take an empty string (due to --sentry_config in gflags that defaults to an empty string) but it's required in catalogd. Since, I don't want to break the compatibility, I'm keeping the existing behavior.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 03:49:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5: Code-Review+1

The changes look sound. But, I'm not super familiar with the authorization code, so would be good or Bharath to give the final review.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 19:38:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 03:50:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 03:50:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12334 )

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................

IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
  config file.
- Calling AuthorizationConfig.validateConfig() was missing.

This patch ensures validateConfig() is always called when
AuthorizationConfig instance is created.

Testing:
- Ran AuditingTest individually
- Ran all FE tests
- Ran all E2E authorization tests

Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
6 files changed, 47 insertions(+), 53 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12334/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12334/4//COMMIT_MSG@10
PS4, Line 10:  which may be due
            : to the test depends on the state from other tests
I'm also wondering about this and why it showed up now.


http://gerrit.cloudera.org:8080/#/c/12334/4/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/12334/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@83
PS4, Line 83:     if (sentryConfig_.getConfigFile() != null) {
> You've found that something prevents the file name from ever being ""? Or, 
I think loadConfig() fails in that case.  My understanding is we wouldn't want to load only when it is null. All other error checking seems to be in loadConfig().

public void loadConfig() {
    if (Strings.isNullOrEmpty(configFile_)) {
      throw new IllegalArgumentException("A valid path to a sentry-site.xml config " +
          "file must be set using --sentry_config to enable authorization.");
    }


http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@373
PS4, Line 373:     config.validateConfig();
Wondering if we should force this somehow? All callers seem to be explicitly calling this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 07:44:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 4:

Rebased to fix gerrit-code-review-checks failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Feb 2019 22:46:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 4:

(2 comments)

Thanks for cleaning this up, and for fixing the "polarity" of the assertEquals statements. A few minor comments.

http://gerrit.cloudera.org:8080/#/c/12334/4/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/12334/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@83
PS4, Line 83:     if (sentryConfig_.getConfigFile() != null) {
You've found that something prevents the file name from ever being ""? Or, that we want to fail the load in this case?


http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@962
PS4, Line 962:           "Authorization is enabled but the server name is null or empty. Set the " +
Error message says null or empty, but the code was changed to just check null...



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Sat, 02 Feb 2019 03:19:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1976/ : 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/12334
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 19:11:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 4:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/1966/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Feb 2019 23:15:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 20:00:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 00:35:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1983/ : 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/12334
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 04:14:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 20:00:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 2:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/1963/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Feb 2019 20:08:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12334 )

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................

IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
  config file.
- Calling AuthorizationConfig.validateConfig() was missing.

I also fixed a bug in AuthorizationConfig.validateConfig(), which
skipped the Sentry config validation when the Sentry config file path is
empty. As a result, I updated other tests that considered empty Sentry
config file path to be valid.

Testing:
- Ran AuditingTest individually
- Ran all FE tests

Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
3 files changed, 25 insertions(+), 18 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12334/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12334/4//COMMIT_MSG@10
PS4, Line 10:  which may be due
            : to the test depends on the state from other tests
> I'm also wondering about this and why it showed up now.
I think we rarely run this test individually, that's why we don't really notice.


http://gerrit.cloudera.org:8080/#/c/12334/4/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/12334/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@83
PS4, Line 83:     // specified. It is optional for impalad.
> I think loadConfig() fails in that case.  My understanding is we wouldn't w
Yup, Bharath is correct. The existing logic is a bit inconsistent. loadConfig() will fail when Sentry config is an empty string but validateConfig() skips loading the config (ignores the empty string check).


http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@373
PS4, Line 373:     try (ImpaladCatalog catalog = new ImpaladTestCatalog(config)) {
> Wondering if we should force this somehow? All callers seem to be explicitl
Yeah, that's a good idea. Done.


http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/12334/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@962
PS4, Line 962:     try {
> Error message says null or empty, but the code was changed to just check nu
The empty string passed here is serverName and not sentryConfig, so the error message is correct.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Feb 2019 18:35:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

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/12334 )

Change subject: IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
......................................................................

IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure

I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
  config file.
- Calling AuthorizationConfig.validateConfig() was missing.

This patch ensures validateConfig() is always called when
AuthorizationConfig instance is created.

Testing:
- Ran AuditingTest individually
- Ran all FE tests
- Ran all E2E authorization tests

Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Reviewed-on: http://gerrit.cloudera.org:8080/12334
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
6 files changed, 47 insertions(+), 53 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Gerrit-Change-Number: 12334
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>