Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@301 PS8, Line 301: opts.bind_mode = BindMode::LOOPBACK; nit: please add a comment to explain why it's necessary to override the default setting for the 'bind_mode' in this test. I guess that's because of the name in the service keytab, but having a comment wouldn't hurt. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@316 PS8, Line 316: MultiMasterSpnegoTest, TestAuthenticatedLeaderAccess nit for here and the rest of the scenarios: drop the 'Test' prefix for the scenario name, because the full name of the scenario right now is MultiMasterSpnegoTest.TestAuthenticatedLeaderAccess (that's output by gtest scaffolding when the test scenarios are run), and the duplicate 'Test' looks a bit funny. If that were removed, it'd become MultiMasterSpnegoTest.AuthenticatedLeaderAccess which is easier to read, at least. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@321 PS8, Line 321: static Why to have 'static' here? http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@331 PS8, Line 331: matches nit: even if regex matches (i.e. KuduRegex::Match() returns 'true'), it's a good practice to check for the number of matching components -- there might be optional components that are also reported as matching ones http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@335 PS8, Line 335: // All masters should report the same leader with authentication : ASSERT_EQ(1, leader_addresses.size()) << "Authenticated requests yielded different leaders: " : << JoinStrings(leader_addresses, ", "); This fails if leader election happens in the middle. That's possible on busy nodes and/or nodes with inferior hardware, so this is a new source of test flakiness. Either (a) switch Raft consensus into a manual election mode, so no leader elections are possible or (b) embed the fetching & comparison into ASSERT_EVENTUALLY block to avoid flakiness. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@345 PS8, Line 345: Status s = c.FetchURL(Substitute("http://$0/api/v1/tables";, : cluster_->mini_master(i)->bound_http_addr().ToString()), : &buf); : ASSERT_STR_CONTAINS(s.ToString(), "HTTP 401"); : ASSERT_STR_CONTAINS(buf.ToString(), "Must authenticate with SPNEGO"); : : s = c.FetchURL(Substitute("http://$0/api/v1/leader";, : cluster_->mini_master(i)->bound_http_addr().ToString()), : &buf); : ASSERT_STR_CONTAINS(s.ToString(), "HTTP 401"); : ASSERT_STR_CONTAINS(buf.ToString(), "Must authenticate with SPNEGO"); Consider implementing this with less boilerplate: have a cycle where the URL is just a parameter for each iteration of the cycle. BTW, what happens if trying to access an invalid URL without prior authentication with SPENGO? Do we expect to have HTTP 401 as well or that's some other error returned back (e.g., HTTP 404)? I'd expect it to be HTTP 401, but it's always good to be explicit about that by adding corresponding test scenarios. Those are quite easy to add once this is updated to have URL as a parameter. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@357 PS8, Line 357: } What happens if sending PUT or other improper types of requests to a particular URL without authentication? Do we want to add test coverage for that as well? http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@378 PS8, Line 378: ASSERT_STR_CONTAINS(buf.ToString(), kDefaultPrincipal); What is being verified here? It would be great to have a small text blurb explaining the semantics of this. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@381 PS8, Line 381: } // namespace master I'd think of adding one extra test scenario to verify the behavior of the system for the following sequence: * a client makes request to fetch information on leader master * the request is processed and the information on the leader master address is sent back * in the middle, a leader election happens between the Kudu masters * a new Kudu leader master is elected, and that's not the same as the former leader master (i.e., the address of new leader master is different from the information that the client has just received) * the client send in a request to one of the former's leader endpoints (e.g., a request to get list of tables) * what is the expected response that client is going to receive from the former leader master? -- 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: 8 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Tue, 19 Aug 2025 05:35:27 +0000 Gerrit-HasComments: Yes
