You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/06/11 17:08:14 UTC

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13585


Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................

IMPALA-8605: clean up HS2/beeswax session management

Session/operation secrets are part of the HS2 handle
but we haven't made use of them up until now. This
patch checks the value and treats it as part of the
session key, as originally intended. I.e. if the
secret is missing, the session lookup fails.

The operation secret is the same as the session secret
to save having to generate and store extra secrets
(there's no real benefit).

A secret is added to each Beeswax session. This secret is
internal to the server and not exposed. Adds validation
that client requests accessed via Beeswax belong to the
same user as the session.

We switch uuid_generator_ to use boost::random_device, which
uses /dev/urandom as its source of randomness to be more robust -
otherwise it's hard to be sure that we won't have collisions,
although it doesn't seem to be a problem in practice.

For requests - GetRuntimeProfile() and GetExecSummary()
that provide both a session and query ID, the code already
checks that the session's user matches the query.

An exception to the validation mechanisms above is added
for Close() and Cancel() beeswax operations, because impala-shell
and some administrative tools allow cancellation of
queries on different threads and from different tools.

We skip validating the session secret when cancelling queries
from the web UI, since web UI users don't have the secret.

Testing:
* Ran exhaustive tests.
* Add tests for all HS2 RPCs that provide invalid session secrets.
* Add tests for HS2 RPCs that provide both session and query
  ID to ensure that query belongs to the session.
* Add basic test for beeswax testing accessing a query from
  different connections.

Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
---
M CMakeLists.txt
M be/src/service/child-query.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/beeswax/impala_beeswax.py
M tests/common/impala_connection.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_fetch.py
M tests/hs2/test_hs2.py
A tests/query_test/test_beeswax.py
13 files changed, 688 insertions(+), 133 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 17:08:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 22:38:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 17:39:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 17:52:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................

IMPALA-8605: clean up HS2/beeswax session management

Session/operation secrets are part of the HS2 handle
but we haven't made use of them up until now. This
patch checks the value and treats it as part of the
session key, as originally intended. I.e. if the
secret is missing, the session lookup fails.

The operation secret is the same as the session secret
to save having to generate and store extra secrets
(there's no real benefit).

A secret is added to each Beeswax session. This secret is
internal to the server and not exposed. Adds validation
that client requests accessed via Beeswax belong to the
same user as the session.

We switch uuid_generator_ to use boost::random_device, which
uses /dev/urandom as its source of randomness to be more robust -
otherwise it's hard to be sure that we won't have collisions,
although it doesn't seem to be a problem in practice.

For requests - GetRuntimeProfile() and GetExecSummary()
that provide both a session and query ID, the code already
checks that the session's user matches the query.

An exception to the validation mechanisms above is added
for Close() and Cancel() beeswax operations, because impala-shell
and some administrative tools allow cancellation of
queries on different threads and from different tools.

We skip validating the session secret when cancelling queries
from the web UI, since web UI users don't have the secret.

Testing:
* Ran exhaustive tests.
* Add tests for all HS2 RPCs that provide invalid session secrets.
* Add tests for HS2 RPCs that provide both session and query
  ID to ensure that query belongs to the session.
* Add basic test for beeswax testing accessing a query from
  different connections.

Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Reviewed-on: http://gerrit.cloudera.org:8080/13585
Reviewed-by: Lars Volker <lv...@cloudera.com>
Reviewed-by: Thomas Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M CMakeLists.txt
M be/src/service/child-query.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/beeswax/impala_beeswax.py
M tests/common/impala_connection.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_fetch.py
M tests/hs2/test_hs2.py
A tests/query_test/test_beeswax.py
13 files changed, 688 insertions(+), 133 deletions(-)

Approvals:
  Lars Volker: Looks good to me, approved
  Thomas Marshall: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-8605: clean up HS2/beeswax session management

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

Change subject: IMPALA-8605: clean up HS2/beeswax session management
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c014d1a32e273275a773f842b9ed9793dbdba6b
Gerrit-Change-Number: 13585
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2019 17:55:32 +0000
Gerrit-HasComments: No