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 <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 01 Mar 2021 20:54:06 +0000 Gerrit-HasComments: Yes
