[Impala-ASF-CR] IMPALA-10550: Add External Frontend service port

["Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>]

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17125 )

Change subject: IMPALA-10550: Add External Frontend service port
......................................................................


Patch Set 5:

(2 comments)

> > (2 comments)
 > >
 > > Are there any existing hs2 methods that it might make sense to
 > > block for the "external frontend" server? eg. we might want to
 > > return an error for ExecuteStatement() from it if the external
 > > frontend will never need to call it.
 > >
 > > That would give us a little more safety in case people
 > accidentally
 > > expose this port to the outside world (of course, it would still
 > be
 > > possible for bad actors to use the ExecutePlannedStatement
 > > interface, but its probably a lot harder to put together a valid
 > > TExecRequest to use it than it is to put together a SQL string
 > like
 > > ExecuteStatement takes)
 > >
 > > Also out of curiosity - what's the long run testing plan here?
 > Are
 > > we going to have an actual external FE running in the minicluster
 > > that can exercise this stuff?
 > 
 > So the current implementation of external frontend does utilize the
 > ExecuteStatement functionality (for things like COMPUTE STATS). I
 > do agree with your assessment that it would be nice to reduce the
 > surface area in the future. The long term plan would also likely
 > include enabling similar protections that intra-impalad
 > communication use between nodes (that prevent people connecting
 > easily to the backend port and pretending to be a coordinator).
 > 
 > One option I considered based on your comment was to add a 2nd flag
 > that  would be named something like: external_fe_allow_unsafe which
 > defaulted to false and disallowed ExecuteStatement via the
 > external_fe_port. So a user would have to enable external_fe_port
 > AND set external_fe_allow_unsafe to true to be able to call
 > ExecuteStatement. But if someone is enabling the external_fe_port -
 > it is somewhat assumed they know what they are doing so I'm not
 > 100% convinced this approach is worth it. I am open to suggestions
 > (or if you like the idea of the 2nd flag).

Agreed that sounds unnecessarily complicated. I think its fine as-is for now, just something to keep in mind.

 > 
 > As for testing - I do believe once the various external FE commits
 > land we should focus on:
 > 1) auditing and shoring up what we can build unit tests around
 > 2) And, yes, it is my understanding that we will eventually be
 > including an external frontend in the minicluster for more
 > end-to-end testing. Otherwise, we will need to mock up some sort of
 > "send pre-made exec request" and "check response" test framework
 > but I suspect that might be not fun to implement cleanly.

Sounds good

http://gerrit.cloudera.org:8080/#/c/17125/5/be/src/rpc/authentication.h
File be/src/rpc/authentication.h:

http://gerrit.cloudera.org:8080/#/c/17125/5/be/src/rpc/authentication.h@67
PS5, Line 67: Currently this is either null if external_fe_port <= 0 or
            :   /// NoAuthProvider.
I think you put this above the wrong function?


http://gerrit.cloudera.org:8080/#/c/17125/5/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/17125/5/bin/start-impala-cluster.py@235
PS5, Line 235:           'external_fe_port': DEFAULT_EXTERNAL_FE_PORT + instance_num,
What would you think about putting this behind a flag for now, eg. "start-impala-cluster.py --enable_external_fe_support" or similar?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I991b5b05e12e37d8739e18ed1086bbb0228acc40
Gerrit-Change-Number: 17125
Gerrit-PatchSet: 5
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 20:54:06 +0000
Gerrit-HasComments: Yes