Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala ...................................................................... Patch Set 4: (21 comments) http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h File src/kudu/util/jwt-util-internal.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49 PS2, Line 49: > missed std::move()? Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49 PS2, Line 49: > missed std::move()? Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@79 PS2, Line 79: > style nit for here and below: per Kudu code style, the indent is wrong (see Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@370 PS2, Line 370: std::unique_ptr<WritableFile > IIRC, in Kudu we do have env::NewTempWritableFile() for this. Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@372 PS2, Line 372: WritableFileOptions opts; > Throughout this code: Ack http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@542 PS2, Line 542: . > What is this for? Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@1141 PS2, Line 1141: : > Here and elsewhere: we do have ASSERT_STR_CONTAINS() for this in Kudu. Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h@71 PS2, Line 71: const JWTDecodedToken* decoded_token > In Kudu we do pass input parameters as constant references, not constant po As discussed, we'll keep the signatures for now. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@20 PS2, Line 20: #include <cstddef> > nit: use C++ style includes instead (i.e. this should be <cstring>) Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@90 PS2, Line 90: tatus Parse(const Document& rules_doc) { > This can be a static method. Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@118 PS2, Line 118: ret > nit: remove 'else' for readability Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@341 PS2, Line 341: return > Why not just return the status from here? Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@346 PS2, Line 346: return Status::InvalidArgument > There is RETURN_NOT_OK() for this Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@358 PS2, Line 358: 2JWTPublicKey(algorith > Use C++ casts instead Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@372 PS2, Line 372: status = Status::NotAuthorized( : Substitute("Failed to initial > We do have safer ssl_make_unique() for these in Kudu. Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@616 PS2, Line 616: status = Status::InvalidArgument("root element must be a JSON Object"); : } else if (!jwks_doc.HasMember("keys")) { : status = Status::InvalidArgument("keys is required"); : } else { : // Load and initialize public keys. : JWKSetParser jwks_parser( > In Kudu we have FindPtrOrNull() for this. Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@663 PS2, Line 663: : // : // JWKSMgr member functions. : // : : JWKSMgr::~JWKSMgr() { : shut_down_promise_.Set(true); : if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join(); : } : > This is a wrong place to check -- that should be done in flag validators in Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@793 PS2, Line 793: rn status; > nit: create a reference decoded_token->decoded_jwt_ and use it here and bel Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@51 PS2, Line 51: bool IsSet() const { > I think the overall semantics of this method is somewhat bogus due to the s Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@53 PS2, Line 53: } : > It seems 'return' is misplaced here? Done http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@56 PS2, Line 56: // > It seems 'return' is missed here? Done -- To view, visit http://gerrit.cloudera.org:8080/18467 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 4 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: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 03 Nov 2022 14:38:41 +0000 Gerrit-HasComments: Yes
