[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Reviewed-on: http://gerrit.cloudera.org:8080/18467 Reviewed-by: Alexey Serbin Reviewed-by: Wenzhe Zhou Tested-by: Kudu Jenkins --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,562 insertions(+), 2 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Wenzhe Zhou: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 12: Code-Review+1 -- 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: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 09 Nov 2022 19:06:48 + Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 12: Code-Review+2 -- 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: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 09 Nov 2022 18:56:32 + Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#12) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,562 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/12 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/18467/11/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/11/src/kudu/util/promise.h@51 PS11, Line 51: bool IsSet() const { : return latch_.count() == 0; : } Is it needed as of PS11? I could not find its usage, but I might be missing something. -- 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: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 09 Nov 2022 18:50:26 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 11: Code-Review+1 -- 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: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 09 Nov 2022 17:07:38 + Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@718 PS10, Line 718: the file which U > nit: the file which the URI Done http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@728 PS10, Line 728: const MonoDelta &timeout = MonoDelta::FromSeconds(FLAGS_jwks_update_frequency_s); : > nit: this could be just Done http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732 PS10, Line 732: // Shutdown has happened, stop updating JWKS. > Good point. I'd vote for 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: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 09 Nov 2022 16:20:13 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#11) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,566 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/11 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@728 PS10, Line 728: int64_t timeout_millis = static_cast(FLAGS_jwks_update_frequency_s) * 1000; : const MonoDelta &timeout = MonoDelta::FromMilliseconds(timeout_millis); nit: this could be just const MonoDelta timeout = MonoDelta::FromSeconds(FLAGS_jwks_update_frequency_s); http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732 PS10, Line 732: bool timed_out = shut_down_promise_.WaitFor(timeout); > WaitFor() returns nullptr if the timeout elapses so timed_out should be tru Good point. I'd vote for if (shut_down_promise_.WaitFor(timeout) != nullptr) { // Shutdown has happened, stop updating JWKS. break; } since it doesn't introduce useless variables. Also, adding a comment helps to increase readability. -- 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: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Tue, 08 Nov 2022 19:59:29 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 10: (2 comments) Overall looks good to me. Only two minor comments. http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@718 PS10, Line 718: the file the URI nit: the file which the URI http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732 PS10, Line 732: bool timed_out = shut_down_promise_.WaitFor(timeout); WaitFor() returns nullptr if the timeout elapses so timed_out should be true if WaitFor() returns nullptr. We should break the loop if it's NOT timed out. bool timed_out = (shut_down_promise_.WaitFor(timeout) == nullptr). if (!timed_out) break; or if (shut_down_promise_.WaitFor(timeout) != nullptr) break; -- 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: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Tue, 08 Nov 2022 19:34:01 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#10) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,564 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/10 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 9: (3 comments) 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@272 PS2, Line 272: otAut > Yep -- I agree: that can be taken care of separately, if needed. So, I'm O Ack http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join() > Thank you for the analysis. It would be great if you could add a blurb to Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: << status.ToString(); > Yup, that's what I was trying to recommend as well, but failed to articulat 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: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Tue, 08 Nov 2022 08:28:25 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 9: Code-Review+1 (3 comments) Overall looks good to me, but please address Wenzhe's comment athttps://gerrit.cloudera.org/#/c/18467/7/src/kudu/util/jwt-util.cc@734 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@272 PS2, Line 272: otAut > This is a bit weird, I think it's okay if we move these to constant, but th Yep -- I agree: that can be taken care of separately, if needed. So, I'm OK with leaving this as-is for now. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join() > Sorry I haven't replied to this. The only reason why this may be okay as a Thank you for the analysis. It would be great if you could add a blurb to put that information as a code's comment. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: << status.ToString(); > define a variable for MonoDelta::FromMilliseconds(timeout_millis) out of th Yup, that's what I was trying to recommend as well, but failed to articulate properly. Thanks for the clarification, Wenzhe! -- 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: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Tue, 08 Nov 2022 02:22:55 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: << status.ToString(); > Done define a variable for MonoDelta::FromMilliseconds(timeout_millis) out of the loop. But shut_down_promise_.WaitFor() should be left in the loop. -- 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: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Mon, 07 Nov 2022 21:33:49 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#9) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,558 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/9 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 8: (7 comments) 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@272 PS2, Line 272: otAut > Did you miss to address this comment or there is some other reason behind? This is a bit weird, I think it's okay if we move these to constant, but they are not used anywhere else. At the end these only indicate which struct to pass to the verifier's allow_algorithm method as a parameter. Unfortunately the allow_algorithm's parameter is a struct (jwt::algorithm::*) from the jwt-cpp thirdparty library. E.g https://github.com/Thalhammer/jwt-cpp/blob/master/include/jwt-cpp/jwt.h#L971 So we can leave these as they are single use, or move them to constants, alternatively we can re-work the constructors for the public key variants, but that might take some serious refactoring. What would you recommend? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join() > Did you find the reason behind having this as a warning instead of an error Sorry I haven't replied to this. The only reason why this may be okay as a warning is that the JWKS information might be changing over time, if for some reason, the file the URI is pointing at becomes empty, it'll still be downloaded, but no keys will be verified successfully (due to no public keys in the jwks to do so). Since the UpdateJWKSThread is still alive, if the jwks file/endpoint is fixed, then the verification will be successful again. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@84 PS7, Line 84: DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero); : : using std::string; : using strings::Substitute; : : namespace kudu { : > nit: instead of duplicating the code, consider introducing a function and t Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474 PS7, Line 474: // ECDSA using P-521 and SHA-512 (OBJ_txt2nid("secp521r1")). : eccgrp = NID_secp521r1; : } else { : return Status::NotSupported(Substitute("Unsupported alg: '$0'", algorithm)); : } : } : : auto it_x = kv_map.find("x"); : auto it_y = kv_map.find("y"); > Since there isn't any return statement involved in three if() scopes below, Ack http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: << status.ToString(); > nit: this could be moved out of the cycle and saved to be reused in this cy Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804 PS7, Line 804: } catch (const std::invalid_argument& e) { : status = Status::NotAuthorized( : Substitute("Token is not in correct format, error: $0", e.what())); : } catch (const std::runtime_error& e) { : status = Status::NotAuthorized( : Substitute("Base64 decoding failed or invalid json, error: $0", e.what())); : } > nit: could you structured binding here instead Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h@64 PS7, Line 64: null > nit: since you are making changes here, maybe change NULL to nullptr as wel 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: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Mon, 07 Nov 2022 10:00:17 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#8) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,558 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/8 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 7: Code-Review+1 (7 comments) Looks better now: a few nits 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@272 PS2, Line 272: > Should these strings be constants and used as such elsewhere in the code? Did you miss to address this comment or there is some other reason behind? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: return FindPointeeOrNull(ec_pub_key_map_, kid); > That's a bit funny to see as a warning since in other places it's considere Did you find the reason behind having this as a warning instead of an error? Would be great adding a comment on that since it's treated as error in other part of the JWT code. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@84 PS7, Line 84: [](const char* name, const int32_t val) { : if (val <= 0) { :LOG(ERROR) << Substitute("Invalid value for $0 flag: $1", name, val); :return false; : } : return true; : }); nit: instead of duplicating the code, consider introducing a function and then refer it in both validators http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474 PS7, Line 474: if (algorithm == "es256") { : // ECDSA using P-256 and SHA-256 (OBJ_txt2nid("prime256v1")). : eccgrp = NID_X9_62_prime256v1; : } : if (algorithm == "es384") { : // ECDSA using P-384 and SHA-384 (OBJ_txt2nid("secp384r1")). : eccgrp = NID_secp384r1; : } : if (algorithm == "es512") { Since there isn't any return statement involved in three if() scopes below, keeping the original 'else if' would make the code more efficient? http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: MonoDelta::FromMilliseconds(timeout_millis) nit: this could be moved out of the cycle and saved to be reused in this cycle over and over again. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804 PS7, Line 804: for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_header_claims()) { : msg << claim.first << "=" << claim.second.to_json().serialize() << ";"; : } : msg << " JWT token payload: "; : for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_payload_claims()) { : msg << claim.first << "=" << claim.second.to_json().serialize() << ";"; : } nit: could you structured binding here instead for (auto& [key, val] : ...) { msg << key << ...; } https://en.cppreference.com/w/cpp/language/structured_binding http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h@64 PS7, Line 64: NULL nit: since you are making changes here, maybe change NULL to nullptr as well -- 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: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Sun, 06 Nov 2022 16:56:30 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#7) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,567 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/7 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#6) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,567 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/6 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#5) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,567 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/5 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
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 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 > nit: use C++ style includes instead (i.e. this should be ) 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
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#4) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,528 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/4 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Zoltan Chovan has uploaded a new patch set (#3) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,528 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/3 -- 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: newpatchset Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 2: (2 comments) 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@663 PS2, Line 663: if (FLAGS_jwks_update_frequency_s <= 0) { : LOG(WARNING) << "Invalid value for flag jwks_update_frequency_s: " :<< FLAGS_jwks_update_frequency_s << ", use default value 60."; : FLAGS_jwks_update_frequency_s = 60; : } : if (FLAGS_jwks_pulling_timeout_s <= 0) { : LOG(WARNING) << "Invalid value for flag jwks_pulling_timeout_s: " :<< FLAGS_jwks_pulling_timeout_s << ", use default value 10."; : FLAGS_jwks_pulling_timeout_s = 10; : } This is a wrong place to check -- that should be done in flag validators instead. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: if (new_jwks->IsEmpty()) LOG(WARNING) << "JWKS file is empty."; That's a bit funny to see as a warning since in other places it's considered as an error, e.g. in JWTHelper::Verify() method. -- 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: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Wed, 19 Oct 2022 20:06:04 + Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 ) Change subject: util: pull jwt code from Impala .. Patch Set 2: (24 comments) Both LINT and IWYU fail -- please make sure those pass. From my assessment, this code is of not high enough quality to meet standards we use for Kudu. I'd suggest to try improving that right away, but we can discuss other options. Also, did you check recent updates on this functionality in the Impala repo? 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: algorithm missed std::move()? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49 PS2, Line 49: pub_key missed std::move()? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@65 PS2, Line 65: const std::string algorithm_; This looks a bit lame to use arbitrary string representation for an enumerated entity. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@78 PS2, Line 78: std::string algorithm, here and below: I'm not sure I understand why to have this 'algorithm' as a parameter when it's already clear this is a key for a particular algorithm (here it's HS256 algorithm)? Am I missing something? 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 https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists) 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@35 PS2, Line 35: std::string rsa_priv_key_pem = R"(-BEGIN PRIVATE KEY- Should all these be constant strings? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@370 PS2, Line 370: int fd = mkstemp(&name_[0]); IIRC, in Kudu we do have env::NewTempWritableFile() for this. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@372 PS2, Line 372: std::cout << "Error creating temp file; " << strerror(errno) << std::endl; Throughout this code: * errors are usually written into std:cerr, not std::cout * before doing anything that might change errno (like memory allocation or other activity in outputting to streams) it's necessary to store errno into a temporary variable, and only then work with it. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@542 PS2, Line 542: EXPECT_OK(status); What is this for? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@1141 PS2, Line 1141: ASSERT_TRUE(status.ToString().find("Verification failed, error: token expired") : != std::string::npos) Here and elsewhere: we do have ASSERT_STR_CONTAINS() for this in Kudu. 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 pointers. 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 nit: use C++ style includes instead (i.e. this should be ) http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@90 PS2, Line 90: tring NameOfTypeOfJsonValue(const Value& value) { This can be a static method. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@118 PS2, Line 118: else nit: remove 'else' for readability http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272 PS2, Line 272: hs256 Should these strings be constants and used as such elsewhere in the code? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@341 PS2, Line 341: status Why not just return the status from here? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@346 PS2, Line 346: if (!status.ok()) return status; There is RETURN_NOT_OK() for this http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@358 PS2, Line 358: (const unsigned char*) Use C++ casts instead http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@372 PS2, Line 372: BIO* bio = BIO_new(BIO_s_mem()); : PEM_write_bio_RSA_PUBKEY(bio, rsa); We do have safer ssl_make_unique() for these in Kudu. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@616 PS2, Line 616: auto find_it = hs_key_map_.find(kid); : if (find_it == hs_key_map_.
[kudu-CR] util: pull jwt code from Impala
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18467 Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,541 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/1 -- 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: newchange Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong