You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2020/02/26 17:50:24 UTC

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components 

Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15301


Change subject: IMPALA-9423: Add keys to cookie components
......................................................................

IMPALA-9423: Add keys to cookie components

Impala's HTTP authentication cookie format is similar, but not
identical, to the format used by other Hadoop components.

This has caused us some problems when other Hadoop systems make
assumptions about the cookie format. In particular, KNOX-2223 made a
change to Apache Knox's cookie handling that works for cookies
generated by Hive and HDFS but breaks cookie handling for Impala when
requests are proxied through Knox.

This match modifies our cookie format slightly to more closely
resemble Hive and HDFS's format by adding keys to the individual
components of the cookie value, fixing the regression caused by
KNOX-2223 and hopefully reducing the probability of another such
regression being introduced in the future.

Testing:
- Ran existing cookie auth tests. Updated one test to fix an issue
  introduced by IMPALA-8899 where we were no longer testing what we
  thought we were.
- Manualy tested in a cluster with Apache Knox.

Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
---
M be/src/rpc/cookie-util.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
2 files changed, 15 insertions(+), 6 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15301 )

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc@39
PS1, Line 39: // <signature>&u=<username>&t=<timestamp>&r=<random number>
> It'd be good to include that in the comment just so there are more breadcru
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:07:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:52:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 18:37:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................

IMPALA-9423: Add keys to cookie components

Impala's HTTP authentication cookie format is similar, but not
identical, to the format used by other Hadoop components.

This has caused us some problems when other Hadoop systems make
assumptions about the cookie format. In particular, KNOX-2223 made a
change to Apache Knox's cookie handling that works for cookies
generated by Hive and HDFS but breaks cookie handling for Impala when
requests are proxied through Knox.

This match modifies our cookie format slightly to more closely
resemble Hive and HDFS's format by adding keys to the individual
components of the cookie value, fixing the regression caused by
KNOX-2223 and hopefully reducing the probability of another such
regression being introduced in the future.

Testing:
- Ran existing cookie auth tests. Updated one test to fix an issue
  introduced by IMPALA-8899 where we were no longer testing what we
  thought we were.
- Manualy tested in a cluster with Apache Knox.

Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Reviewed-on: http://gerrit.cloudera.org:8080/15301
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/cookie-util.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
2 files changed, 18 insertions(+), 6 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:16:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:17:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................

IMPALA-9423: Add keys to cookie components

Impala's HTTP authentication cookie format is similar, but not
identical, to the format used by other Hadoop components.

This has caused us some problems when other Hadoop systems make
assumptions about the cookie format. In particular, KNOX-2223 made a
change to Apache Knox's cookie handling that works for cookies
generated by Hive and HDFS but breaks cookie handling for Impala when
requests are proxied through Knox.

This match modifies our cookie format slightly to more closely
resemble Hive and HDFS's format by adding keys to the individual
components of the cookie value, fixing the regression caused by
KNOX-2223 and hopefully reducing the probability of another such
regression being introduced in the future.

Testing:
- Ran existing cookie auth tests. Updated one test to fix an issue
  introduced by IMPALA-8899 where we were no longer testing what we
  thought we were.
- Manualy tested in a cluster with Apache Knox.

Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
---
M be/src/rpc/cookie-util.cc
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
2 files changed, 18 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:17:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc@39
PS1, Line 39: // <signature>&u=<username>&t=<timestamp>&r=<random number>
Can you reference an example of this, even if it's just in another apache codebase?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 17:56:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 23:43:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

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

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc@39
PS1, Line 39: // <signature>&u=<username>&t=<timestamp>&r=<random number>
> https://github.com/apache/hive/blob/master/service/src/java/org/apache/hive
It'd be good to include that in the comment just so there are more breadcrumbs for people to follow to understand why the format is that way, if needed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:01:26 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9423: Add keys to cookie components

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15301 )

Change subject: IMPALA-9423: Add keys to cookie components
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc
File be/src/rpc/cookie-util.cc:

http://gerrit.cloudera.org:8080/#/c/15301/1/be/src/rpc/cookie-util.cc@39
PS1, Line 39: // <signature>&u=<username>&t=<timestamp>&r=<random number>
> Can you reference an example of this, even if it's just in another apache c
https://github.com/apache/hive/blob/master/service/src/java/org/apache/hive/service/auth/HttpAuthUtils.java#L81



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73f37a4c4f28edf35f4b8195d3f03a2f178f3d0b
Gerrit-Change-Number: 15301
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 18:14:14 +0000
Gerrit-HasComments: Yes