Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24448 )
Change subject: IMPALA-14799: Add oauth_servers support and tests ...................................................................... Patch Set 8: (33 comments) http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h@110 PS8, Line 110: OAuthServersManager* GetOAuthServersManager() { return oauth_servers_mgr_.get(); } Nit: The original names GetJWTHelperInstance() and GetOAuthHelperInstance() did not exactly meet coding standards. Instead, name this function oauth_servers_mgr() to match the getters like in lines 141-151. Also, move this function down by those getters instead of being in this one-off location and have it return std::shared_ptr<OAuthServersManager> std::shared_ptr<OAuthServersManager> oauth_servers_mgr() { return oauth_servers_mgr_; } http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h@299 PS8, Line 299: std::unique_ptr< Make this a std::shared_ptr to avoid use-after-free issues. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.cc@578 PS8, Line 578: oauth_servers_mgr_.reset(new OAuthServersManager()); oauth_servers_mgr_ should be initialized as part of the constructor definition (lines 235-260). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/service/impala-server.cc@3240 PS8, Line 3240: vector<OAuthServerConfig> oauth_server_configs; : RETURN_IF_ERROR(BuildOAuthServerConfigs(&oauth_server_configs)); : RETURN_IF_ERROR(ExecEnv::GetInstance()->GetOAuthServersManager()->Init( : oauth_server_configs, /*require_jwks=*/true)); This coding structure is sub-optimal since it requires initializing a throwaway vector who's contents are all deep copied in the Init() function. A better way would be for the Init() function to declare a vector<OAuthServerConfig>, call BuildOAuthServerConfigs(), and std::move() the contents of the vector. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@771 PS8, Line 771: if (pull_timeout_secs <= 0) { : LOG(WARNING) << "Invalid JWKS pull timeout: " << pull_timeout_secs : << ", use default value 10."; : pull_timeout_secs = 10; : } The flag validators for FLAGS_jwks_pulling_timeout_s take care of ensuring its value is greater than 0. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@785 PS8, Line 785: if (update_frequency_secs <= 0) { : LOG(WARNING) << "Invalid JWKS update frequency: " << update_frequency_secs : << ", use default value 60."; : update_frequency_secs = 60; : } The flag validators for FLAGS_jwks_update_frequency_s take care of ensuring its value is greater than 0. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@812 PS8, Line 812: int32_t update_frequency_secs = jwks_update_frequency_secs_ >= 0 ? : jwks_update_frequency_secs_ : FLAGS_jwks_update_frequency_s; : if (update_frequency_secs <= 0) update_frequency_secs = 60; : int32_t pull_timeout_secs = jwks_pull_timeout_secs_ >= 0 ? : jwks_pull_timeout_secs_ : FLAGS_jwks_pulling_timeout_s; : if (pull_timeout_secs <= 0) pull_timeout_secs = 10; : int64_t timeout_millis = update_frequency_secs * 1000; The logic in JWKSMgr::Init should take care of ensuring jwks_update_frequency_secs_ and jwks_pull_timeout_secs_ are both greater than 0. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@869 PS8, Line 869: : JWTHelper* CreateJWTHelper() { : return new JWTHelper(); : } : : void DestroyJWTHelper(JWTHelper* helper) { : delete helper; : } These functions are not needed. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config-test.cc File be/src/util/oauth-server-config-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config-test.cc@164 PS8, Line 164: TEST(OAuthServerConfigTest, BuildOAuthServerConfigsMergesLegacyOAuthFlags) { Please add another test that combines all three sets of flags (oauth_servers, legacy JWT, and legacy OAuth). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@17 PS8, Line 17: : #ifndef IMPALA_UTIL_OAUTH_SERVER_CONFIG_H : #define IMPALA_UTIL_OAUTH_SERVER_CONFIG_H Use the newer #pragma once directive instead of macro guards. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@37 PS8, Line 37: std::string jwks_file_path; : std::string jwks_url; Instead of having separate fields for jwks file path and url, please consider a single field that accepts either a file://, http://, or https:// url. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@35 PS8, Line 35: std::string ca_cert_file_path; : bool verify_server_cert = true; : std::string jwks_file_path; : std::string jwks_url; : int32_t jwks_pull_timeout_secs = DEFAULT_JWKS_PULL_TIMEOUT_SECS; : int32_t jwks_update_frequency_secs = DEFAULT_JWKS_UPDATE_FREQUENCY_SECS; : std::string username_claim = "username"; Since this is security related, I would prefer for all these fields to be declared const and set by the constructor. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@48 PS8, Line 48: /// Returns the JWKS URI to use. If both file and URL are set, the file takes precedence : /// (matching legacy Impala behavior). : Status GetJwksUri(std::string* jwks_uri, bool* is_local_file) const; Since defining both the JWKS file path and url is legacy behavior, let's keep that out of this new struct and move it to oauth-server-config.cc in the BuildLegacyJwtServerConfig() and BuildLegacyOAuthServerConfig() functions. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@53 PS8, Line 53: /// Parses the --oauth_servers JSON array into 'configs_out'. : Status ParseOAuthServersJson( : const std::string& oauth_servers_json, std::vector<OAuthServerConfig>* configs_out); It's a design smell if a function is exposed only for testing purposes. C++, much more so than in other languages, does have cases where functions do have to be exposed for testing. In those cases, declaring namespace test is the preferred way as it avoids polluting the impala namespace (e.g. https://github.com/apache/impala/blob/0b8294b1a5b0ad4a817dee13b7fbb2ee53f534e2/be/src/service/query-profile-ai-analysis.h#L48). In this specific case, BuildOAuthServerConfigs() is a small wrapper around ParseOAuthServersJson(), and it would be fine to call BuildOAuthServerConfigs() from the CTest tests. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@62 PS8, Line 62: void WarnOnDeprecatedOAuthFlags(); I don't think this function needs to be in the global namespace but rather should be in the anonymous inner namespace. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.cc File be/src/util/oauth-server-config.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.cc@31 PS8, Line 31: DECLARE_string(oauth_servers); Remove, no need to DECLARE_ in the same file where the flag is defined. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager-test.cc File be/src/util/oauth-servers-manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager-test.cc@120 PS8, Line 120: TEST(OAuthServersManagerTest, VerifyFailsWhenNoServersConfigured) { Please add a test that asserts token validation fails when only the testdata/jwt/jwks_rs256.json JWKS is configured. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@17 PS8, Line 17: : #ifndef IMPALA_UTIL_OAUTH_SERVERS_MANAGER_H : #define IMPALA_UTIL_OAUTH_SERVERS_MANAGER_H Use the newer #pragma once directive instead of macro guards. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@40 PS8, Line 40: bool require_jwks Remove this function parameter since it is `true` in all the main code where this function is called. The only place I found where it was `false` was in the CTest tests. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@41 PS8, Line 41: : bool empty() const; : size_t size() const; : : const OAuthServerConfig& config(size_t index) const; Nit: these simple one-line functions can have their function bodies placed here in the header file instead of separately defining them in the .cc file. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@53 PS8, Line 53: static Why is this function declared static instead of being a class member function? http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@35 PS8, Line 35: JWTHelper* No need to use JWTHelper*, use JWTHelper instead. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@32 PS8, Line 32: struct OAuthServersManager::OAuthServersImpl { : struct OAuthServer { : OAuthServerConfig config; : JWTHelper* jwt_helper = nullptr; : }; : : std::vector<OAuthServer> servers; : }; This struct is not necessary. Instead, in the private section of the OAuthServersManager class declaration in the header file, simply put: using OAuthServersImpl = std::vector<std::pair<OAuthServerConfig, JWTHelper*>>; http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@41 PS8, Line 41: OAuthServersManager::OAuthServersManager() : : impl_(new OAuthServersImpl()) {} Delete this constructor implementation, and, in the Init() function, do this instead to make it clear when impl_ has truly been initialized: impl_ = make_unique<OAuthServersImpl>(); http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@50 PS8, Line 50: impl_.reset(); No need to reset impl_, it will be automatically reset when it goes out of scope after this destructor runs. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@55 PS8, Line 55: return impl_->servers.empty(); Add DCHECK(impl_) to ensure that Init() has been called. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@59 PS8, Line 59: return impl_->servers.size(); Add DCHECK(impl_) to ensure that Init() has been called. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@63 PS8, Line 63: return impl_->servers[index].config; Add DCHECK(impl_) to ensure that Init() has been called. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@88 PS8, Line 88: GetJwksUri This function duplicates the validation performed in HasJwksSource(). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@93 PS8, Line 93: if (!is_local_file && TestInfo::is_test()) sleep(1); Is this sleep necessary? If so, add a comment explaining why or remove the need for sleeping by a deterministic check (for example, if there is a need to wait until a certain point in the startup process). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1167 PS8, Line 1167: OAuthServersManager* Needs to be changed to std::shared_ptr<OAuthServersManager> http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1177 PS8, Line 1177: } else if (!oauth_servers_mgr->empty()) { : matched_server_idx = 0; Useless condition, matched_server_idx will already be 0 here. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1219 PS8, Line 1219: } else if (!oauth_servers_mgr->empty()) { : matched_server_idx = 0; Useless condition, matched_server_idx will already be 0 here. -- To view, visit http://gerrit.cloudera.org:8080/24448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib29ff36600406ba59c10f29d79cc632020f4a3f7 Gerrit-Change-Number: 24448 Gerrit-PatchSet: 8 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Wed, 17 Jun 2026 23:09:40 +0000 Gerrit-HasComments: Yes
