Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad ...................................................................... Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc@1190 PS14, Line 1190: HS2_RETURN_IF_ERROR(return_val, THandleIdentifierToTUniqueId( I think you may have misunderstood my previous comment here, sorry if I was unclear. Is there any reason for this rpc to take 'sessionHandle' at all, i.e. TGetBackendConfigReq.sessionHandle? I don't think it will ever be the case that the value of backend flags will be session-dependent, so removing TGetBackendConfigReq.sessionHandle would increase flexibility - eg. presumably this will just be getting called once at startup, when there isn't an actual user session anyways, so removing 'sessionHandle' would mean the external FE doesn't have to make a fake session just to call this. I guess one consideration is authentication - maybe we only want users that can create sessions to be able to call this - but I don't think that comes into play here since this will only be accessible over the external FE interface where authentication is always turned off. I may be missing something about how this will eventually evolve, and its not that big of a deal since like I said this will presumably only be called once at startup so perf doesn't really matter, so I'm fine if you prefer to leave it as is. http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc@258 PS14, Line 258: string saml2_private_key_password; As is, this patch will return things from GetBackendConfig that are both sensitive and (I assume) unnecessary for the external FE, such as passwords. Since we're restricting this to use by the external FE, which we implicitly trust, maybe that's fine, but it would be nice if we had some way to remove this kind of stuff from what gets returned, eg. by depending on the TAG_FLAG(sensitive)/CheckFlagAndRedact construct like we do to prevent these sorts of things from being printed in the logs or output on the debug webui, eg. see: https://github.com/apache/impala/blob/master/be/src/util/default-path-handlers.cc#L105 -- To view, visit http://gerrit.cloudera.org:8080/17116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69 Gerrit-Change-Number: 17116 Gerrit-PatchSet: 14 Gerrit-Owner: Kurt Deschler <[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: Tue, 02 Mar 2021 19:28:12 +0000 Gerrit-HasComments: Yes
