Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15538 )
Change subject: IMPALA-9537: Add LDAP auth to the webui ...................................................................... Patch Set 1: (3 comments) This makes sense to me as the authentication piece of the solution. I had a few readability comments but no concerns about the logic or testing. http://gerrit.cloudera.org:8080/#/c/15538/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/15538/1/be/src/util/webserver.cc@122 PS1, Line 122: DEFINE_bool(webserver_require_ldap, false, Maybe in the help briefly explain the interaction between the different kinds of auth - is it that clients need to authenticate with only one of the enabled mechanisms? edit: oh i guess setting both is disallowed http://gerrit.cloudera.org:8080/#/c/15538/1/be/src/util/webserver.cc@581 PS1, Line 581: AddCookie(request_info, &response_headers); It feels a little weird that we don't set authenticated = true here. The control flow doesn't require it, so we don't need to add unnecessary logic. Maybe it would be clearer with a comment, or if the ldap and spnego branches were made obviously mutually exclusive. E.g. if (!authenticated && FLAGS_spnego) { } else if (!authenticated && FLAGS_ldap) { } or if (!authenticated) { if (FLAGS_spnego) { } else if (FLAGS_ldap) { } } http://gerrit.cloudera.org:8080/#/c/15538/1/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java: http://gerrit.cloudera.org:8080/#/c/15538/1/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@59 PS1, Line 59: LOG.info(IOUtils.toString(p.getInputStream())); I guess this is good to aid debugging. Maybe merits a one line comment to explain what it's doing? -- To view, visit http://gerrit.cloudera.org:8080/15538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e92481929f2f06898b8496233ab4134792c9f10 Gerrit-Change-Number: 15538 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 24 Mar 2020 23:31:10 +0000 Gerrit-HasComments: Yes
