Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19156 )
Change subject: jwt: Additional test ...................................................................... Patch Set 9: (19 comments) http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@515 PS6, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) { > Since this scenario runs over 3 seconds now, consider adding SKIP_IF_SLOW_N Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@540 PS6, Line 540: CHECK(pb.SerializeToString(&creds)); : : S > nit: could use SleepFor(MonoDelta::FromMilliseconds(0)) to simplify the cod Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@572 PS6, Line 572: ASSERT_TRUE(s.IsRuntimeError()); > Here and elsewhere in this test scenario: this sort of non-OK assertion sho Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: PS6: > It's better to keep non-related changes out of the patch. Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc File src/kudu/rpc/negotiation-test.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@127 PS6, Line 127: vector<SaslMechanism::Type> sasl_mechs; > Add a comment similar to the comment for 'token' above to clarify on the se Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@242 PS6, Line 242: if (desc.server.token) { : ASSERT_OK(token_verifier.ImportKeys(token_signer.verifier().ExportKeys())); : } > nit here and elsewhere: could these be marked as constant? Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@247 PS6, Line 247: > nit for better readability: add a comment for parameter name to clarify on Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@995 PS6, Line 995: NegotiationDescriptor { > nit: looks a bit strange that the indent has changed only for this block, b Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1200 PS6, Line 1200: }, > nit: extra empty lines Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1202 PS6, Line 1202: true, > I think it's necessary to add more test sub-scenarios, at least to test how Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation.cc@178 PS6, Line 178: const auto jwt = messenger->jwt(); : ClientNegotiation client_negotiation(conn->release_sock > IIUC, JWT is this context represents primary credentials, not secondary: JW Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/rpc-test-base.h@56 PS6, Line 56: #include "kudu/util/trace.h" > add it in alphabet order Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/server_negotiation.cc@208 PS6, Line 208: are not null, si > So, where is that CHECK then? I don't see it in the code below. Sorry this was left in, the jwt_verifier actually can be null and is not necessary for the negotiation if the server doesn't support JWT, I'll update the comment. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/server/server_base.cc@703 PS6, Line 703: > Is this still needed? Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/CMakeLists.txt@369 PS6, Line 369: add_library(jwt_test_certs jwt_test_certs.cc) : target_link_libraries(jwt_test_certs) > What drives this change? Ack http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.h File src/kudu/util/jwt_test_certs.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.h@22 PS6, Line 22: > style nit: add an empty line before 'namespace kudu' Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.cc File src/kudu/util/jwt_test_certs.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.cc@373 PS6, Line 373: : return WriteStringToFile(Env::Default(), jwks_content, : > nit: could be shorten into Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.h File src/kudu/util/mini_oidc.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.h@78 PS6, Line 78: const > indent Done http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.cc File src/kudu/util/mini_oidc.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.cc@24 PS6, Line 24: #include <vector> > alphabet order Done -- To view, visit http://gerrit.cloudera.org:8080/19156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1977c80b70fd9628ac800671f6cf16e9fa96c0f0 Gerrit-Change-Number: 19156 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Andrew Wong <anj...@gmail.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Comment-Date: Fri, 31 Mar 2023 16:57:42 +0000 Gerrit-HasComments: Yes
