Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL ...................................................................... Patch Set 18: (6 comments) http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@410 PS16, Line 410: void TempTestDataFile::Delete() { : if (deleted_) return; : deleted_ = true; : if (remove(name_.c_str()) != 0) { : std::cout << "Error deleting temp file; " << strerror(errno) << std::endl; : abort(); : } : } > I'm not sure this is needed since the file created with NewTempWritableFile This function is copied from Impala, which did not use NewTempWritableFile() to create the file. It's not needed. http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc@410 PS18, Line 410: void TempTestDataFile::Delete() { : if (deleted_) return; : deleted_ = true; : if (remove(name_.c_str()) != 0) { : std::cout << "Error deleting temp file; " << strerror(errno) << std::endl; : abort(); : } : } > I guess this isn't needed once switched to proper usage of NewTempWritableF Right, we can also remove destructor ~TempTestDataFile() http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611 PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F > I'd suggest to address this in a separate changelist: the context of this p We should add a flag to turn on verification of the peer's certificate in following patch. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615 PS16, Line 615: jwks_checksum_ = > I guess we can discuss that with Wenzhe and clarify using docs and other in We should return "is_changed" as true if the server doesn't have any keys in its response so that the caller will call SetJWKSSnapshot() with an empty JWKSSnapshot. http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620 PS18, Line 620: *is_changed = true; > It seems I missed to point at this earlier, but ideally should be moved to We can move line #617 (*is_changed = false) to the beginning of the function, and move #620 to #643 before calling "return Status::OK()". With this change, *is_changed return true for dst.size() as 0. The caller will call SetJWKSSnapshot() with empty JWKS. http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739 PS18, Line 739: if (new_jwks->IsEmpty()) { : // new_jwks-> : } > nit: is it some left-over from the editing? If not, maybe add a TODO inste Add a warning log message? -- To view, visit http://gerrit.cloudera.org:8080/18468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8 Gerrit-Change-Number: 18468 Gerrit-PatchSet: 18 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: Sat, 19 Nov 2022 00:07:50 +0000 Gerrit-HasComments: Yes
