Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@515 PS11, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) { > Could you also add a test to cover handling of an expired JWT token? that testcase is added in the following patch http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@526 PS11, Line 526: > nit: duplicated as line #517 Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@534 PS11, Line 534: *pb.mutable_jwt() = std::move(jw > nit: could this be written to match line 535? Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@535 PS11, Line 535: > nit: could use std::move() here? Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@538 PS11, Line 538: for (auto i = 0; i < cluster_ > Is this really necessary given that the jwt field has just been set at line Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@560 PS11, Line 560: > In addition to verifying non-OK result, does it make sense to check for par The error status returned here is a RuntimeError, I don't think that would provide more context, also we're parsing the error message in the next line to make sure it fails in the way we expect it. http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@563 PS11, Line 563: shared_ptr<KuduClient> client; > Could you add a sub-case to check how it works when no JWT is provided with Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@85 PS11, Line 85: #include "kudu/util/test_util.h > nit: put this line in front of line #75 to keep alphabet order Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc@716 PS11, Line 716: } > If we call Init() on JWT verifier here, why to call Init() on JWT verifier Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h File src/kudu/util/jwt_test_certs.h: http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@21 PS11, Line 21: // The > Here and below: should all these be 'extern const' since they are not modif Done http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@22 PS11, Line 22: to-jwk tool > nit: using 'const char[]' or 'const char* const' instead of std::string for Done -- To view, visit http://gerrit.cloudera.org:8080/18475 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653 Gerrit-Change-Number: 18475 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 28 Mar 2023 14:18:45 +0000 Gerrit-HasComments: Yes
