[kudu-CR] webserver: add support for Knox URL rewriting

["Andrew Wong (Code Review)" <ge...@cloudera.org>]

Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14573 )

Change subject: webserver: add support for Knox URL rewriting
......................................................................


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc
File src/kudu/integration-tests/webserver-crawl-itest.cc:

http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@93
PS5, Line 93:   // When Knox rewrites an HTTP response, it encodes query parameter values in
            :   // any URLs in the response. To impersonate Knox we must do the same.
As with regex parsing, it'd be nice if you could comment what this "matches" (and replaces)


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@119
PS5, Line 119: 
             : TEST_P(WebserverCrawlITest, TestAllWebPages) {
nit: for anyone who isn't particularly well-versed in Knox, it isn't super obvious what verifications and rewritings we're doing in this crawling. Could you summarize what we're looking for in each link, what "Knoxifying" the link accomplishes, and why that's important?


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@158
PS5, Line 158: ' WALs.
Why are the WALs important? If it's so there's a meaningful log anchoring page, maybe extend this comment to clarify?


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@219
PS5, Line 219:       NO_FATALS(KnoxifyURL(&link));
Is the point of Knoxifying the link purely to match what knox does? Does this test any Kudu codepaths?


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.h
File src/kudu/server/webserver.h:

http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.h@133
PS5, Line 133: WebRequest& req,
nit: doc how this is used?


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.cc@543
PS5, Line 543:     // Canonicalize header names to lower case.
nit: the "why" is less clear than the "what" here IMO. Could you comment why we need to do this?


http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/tserver/tserver_path_handlers.cc@347
PS5, Line 347: UrlEncodeToString(status.tablet_id()));
Why don't we need to check whether we're going through Knox here?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee92cb094b81609356acf858b7c549b6c281a7e5
Gerrit-Change-Number: 14573
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 01 Nov 2019 00:12:40 +0000
Gerrit-HasComments: Yes