Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster ...................................................................... Patch Set 11: (10 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? http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@534 PS11, Line 534: jwt.set_jwt_data(encoded_token); nit: could this be written to match line 535? *jwt.mutable_jwt_data() = std::move(encoded_token); Alternatively, since encoded_token isn't used anywhere else, could it be just *jwt.mutable_jwt_data() = MiniOidc::CreateJwt(account_id, kSubject, true); http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@535 PS11, Line 535: jwt nit: could use std::move() here? http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@538 PS11, Line 538: CHECK_EQ(true, pb.has_jwt()); Is this really necessary given that the jwt field has just been set at line 535? http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@560 PS11, Line 560: ASSERT_FALSE(s.ok()) << s.ToString(); In addition to verifying non-OK result, does it make sense to check for particular type of error here? http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@563 PS11, Line 563: } Could you add a sub-case to check how it works when no JWT is provided with serialized AuthenticationCredentialsPB imported as authn creds? 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@286 PS11, Line 286: jwt_verifier Could use std::move() to avoid incrementing reference counter for jwt_verifier? 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: RETURN_NOT_OK_PREPEND(jwt_verifier->Init(), "Failed to init JWT verifier"); If we call Init() on JWT verifier here, why to call Init() on JWT verifier second time in MessengerBuilder::Build() then? 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: extern Here and below: should all these be 'extern const' since they are not modified once instantiated? http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@22 PS11, Line 22: std::string nit: using 'const char[]' or 'const char* const' instead of std::string for such constants might make the data section of the executable a bit lighter (de facto all such constants in Kudu code are char[] or char*, not std::string). -- 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: 11 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: Mon, 27 Mar 2023 22:26:45 +0000 Gerrit-HasComments: Yes
