Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/23052 )
Change subject: [WIP] SPNEGO authentication in ExternalMiniCluster ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/23052/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23052/2//COMMIT_MSG@7 PS2, Line 7: [WIP] nit: I think the patch is out of the WIP state. :) http://gerrit.cloudera.org:8080/#/c/23052/2//COMMIT_MSG@9 PS2, Line 9: REST API calls nit: calls to the webserver http://gerrit.cloudera.org:8080/#/c/23052/2//COMMIT_MSG@18 PS2, Line 18: --spnego_keytab_file As per the latest patchset I dont think we set --spnego_keytab_file, right? We use one keytab file per: --keytab_file. http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster-test.cc@348 PS2, Line 348: TEST_P(ExternalMiniClusterTest, TestRestApiSpnegoConnectionThroughCurl) { Please add a positive and negative API call inside here: first call curl without kinit and assert the result, then kinit and assert the result. http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster-test.cc@351 PS2, Line 351: opts.extra_master_flags.emplace_back("--enable_rest_api"); I think it would make sense to add a bool option to ExternalMiniClusterOptions called "enable_rest_api". By default it could be set to false. Then you could set the "--enable_rest_api" flag in ExternalMiniCluster::CreateMaster accordingly. http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc@1476 PS2, Line 1476: // --------------------------------------------------------------------------- nit: here and below: I think we dont need the ascii frame around the comments. Just simple // comments are enough. http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc@1490 PS2, Line 1490: Web UI nit: webserver http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc@1498 PS2, Line 1498: const auto kdc_env = kdc->GetEnvVars(); : env_vars->insert(kdc_env.begin(), kdc_env.end()); nit: in the end we did not end up modifying the kdc_env vars. therefore we can leave this line unchanged: *env_vars = kdc->GetEnvVars(); http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc@1505 PS2, Line 1505: flags->emplace_back(Substitute("--keytab_file=$0", kt_path)); : flags->emplace_back(Substitute("--principal=$0", spn)); : flags->emplace_back("--rpc_authentication=required"); : flags->emplace_back("--superuser_acl=test-admin"); : flags->emplace_back("--user_acl=test-user"); nit: here + the webserver flag below: You can use the same list initializer assignment we had before: *flags = {} You can still add the below comments between lines. http://gerrit.cloudera.org:8080/#/c/23052/2/src/kudu/mini-cluster/external_mini_cluster.cc@1511 PS2, Line 1511: Web UI nit: webserver you can also add a bit more details: The embedded webserver looks for its HTTP service credentials in the keytab file referenced by KRB5_KTNAME. InitKerberosForServer sets that env-var automatically from --keytab_file, so the single keytab we created above is picked up transparently. -- To view, visit http://gerrit.cloudera.org:8080/23052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6035bdf7f2f0f08f53485a7a1aa82ed26148cc4 Gerrit-Change-Number: 23052 Gerrit-PatchSet: 2 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 23 Jun 2025 19:11:35 +0000 Gerrit-HasComments: Yes
