Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 6: (8 comments) Overall looks good, a couple things here and there. http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/integration-tests/master_authz-itest.cc@336 PS6, Line 336: opts.extra_master_flags.emplace_back("--enable_rest_api"); nit: you can now use opts.enable_rest_api = true; http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/integration-tests/master_authz-itest.cc@990 PS6, Line 990: // REST API Authorization Integration Tests What do you think about creating a dedicated test class for the rest api authz tests? (It could be similar to MasterAuthzITest) Then you could also put into the test class all the utility functions. http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/integration-tests/master_authz-itest.cc@1084 PS6, Line 1084: // Create a fresh EasyCurl object after changing the Kerberos context : EasyCurl c_new; nit: maybe you can scope the code parts (just to avoid c, and c_new etc. more readable) ... { Kinit(secondUser); ... } { Kinit(kTestUser); ... } ... http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/integration-tests/master_authz-itest.cc@1207 PS6, Line 1207: // User A should see the table in list tables nit: same as above, you can scope user A and user B to avoid EasyCurl c; EasyCurl c_b; http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/master/spnego_rest_catalog-test.cc@317 PS6, Line 317: // Test authenticated access to leader endpoint across all masters is this test correct? I'm not sure if I understand it correctly, but here we just check one master's /api/v1/leader endpoint, not all of the masters. http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/master/spnego_rest_catalog-test.cc@338 PS6, Line 338: TEST_F(MultiMasterSpnegoTest, TestUnauthenticatedRequestsRejected) { maybe you can also test the /leader endpoint that it is getting 401. http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/master/spnego_rest_catalog-test.cc@352 PS6, Line 352: // Test table operations with proper authentication across all masters nit: is this comment correct? Isn't this testing table operations on the leader master? http://gerrit.cloudera.org:8080/#/c/23045/6/src/kudu/master/spnego_rest_catalog-test.cc@358 PS6, Line 358: // First identify the leader master I see that here we have this block to identify leader master in multiple places. How about: 1. add coverage that the leader endpoint works as expected with auth. (i think you will cover that with your existing test: TestAuthenticatedLeaderAccess 2. from then on in other tests you can just use cluster_->GetLeaderMasterIndex() to make the tests a bit more leaner. -- To view, visit http://gerrit.cloudera.org:8080/23045 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd3ff0dfd67cbc2b5ed0454372dd2bcea71e2ba3 Gerrit-Change-Number: 23045 Gerrit-PatchSet: 6 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 31 Jul 2025 17:29:01 +0000 Gerrit-HasComments: Yes
