Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL ...................................................................... Patch Set 3: (3 comments) Initial round of comments http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc@585 PS3, Line 585: curl_global_init(CURL_GLOBAL_DEFAULT); : : // Initialize curl session. : curl_handle = curl_easy_init(); : if (curl_handle == nullptr) { : status = Status("Error to initialize curl session"); : goto cleanup; : } : curl_easy_setopt(curl_handle, CURLOPT_URL, jwks_url.c_str()); : curl_easy_setopt(curl_handle, CURLOPT_WRITEFUNCTION, CurlWriteDownloadBufferCallback); : curl_easy_setopt(curl_handle, CURLOPT_WRITEDATA, (void*)&chunk); : curl_easy_setopt(curl_handle, CURLOPT_USERAGENT, "libcurl-agent/1.0"); It's worth looking at Kudu's EasyCurl wrapper to see if it would work for us: https://github.com/apache/impala/blob/master/be/src/kudu/util/curl_util.h If it does what we need, it would be less code for us to maintain. http://gerrit.cloudera.org:8080/#/c/17802/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java: http://gerrit.cloudera.org:8080/#/c/17802/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@227 PS3, Line 227: String webserverArgs = "--webserver_port=25010"; It would be good to make it very clear that the statestore is serving up the JWKS. One way is to be very explicit with the variable names here. For "webserverArgs", we could use "statestoreWebserverArgs". For jwksHttpUrl, we could use "statestoreWebserverJwksUrl" or something like that. For "jwtAargs", we could use "impaladJwtArgs". http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json File www/jwt/jwks_rs256.json: http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json@1 PS3, Line 1: : { : "keys": [ : { "use": "sig", "kty": "RSA", "kid": "public:c424b67b-fe28-45d7-b015-f79da50b5b21", "alg": "RS256", "n": "uGbXWiK3dQTyCbX5xdE4yCuYp0AF2d15Qq1JSXT_lx8CEcXb9RbDddl8jGDv-spi5qPa8qEHiK7FwV2KpRE983wGPnYsAm9BxLFb4YrLYcDFOIGULuk2FtrPS512Qea1bXASuvYXEpQNpGbnTGVsWXI9C-yjHztqyL2h8P6mlThPY9E9ue2fCqdgixfTFIF9Dm4SLHbphUS2iw7w1JgT69s7of9-I9l5lsJ9cozf1rxrXX4V1u_SotUuNB3Fp8oB4C1fLBEhSlMcUJirz1E8AziMCxS-VrRPDM-zfvpIJg3JljAh3PJHDiLu902v9w-Iplu1WyoB2aPfitxEhRN0Yw", "e": "AQAB" }, : { "use": "sig", "kty": "RSA", "kid": "public:9b9d0b47-b9ed-4ba6-9180-52fc5b161a3a", "alg": "RS256", "n": "xzYuc22QSst_dS7geYYK5l5kLxU0tayNdixkEQ17ix-CUcUbKIsnyftZxaCYT46rQtXgCaYRdJcbB3hmyrOavkhTpX79xJZnQmfuamMbZBqitvscxW9zRR9tBUL6vdi_0rpoUwPMEh8-Bw7CgYR0FK0DhWYBNDfe9HKcyZEv3max8Cdq18htxjEsdYO0iwzhtKRXomBWTdhD5ykd_fACVTr4-KEY-IeLvubHVmLUhbE5NgWXxrRpGasDqzKhCTmsa2Ysf712rl57SlH0Wz_Mr3F7aM9YpErzeYLrl0GhQr9BVJxOvXcVd4kmY-XkiCcrkyS1cnghnllh-LCwQu1sYw", "e": "AQAB" } : ] : } Everything in the www/ directory gets shipped as part of the docker image build and is likely to be shipped by other packaging systems. We don't want to ship this file. I think it would be better for the test that needs it to copy it in temporarily and delete it at the end. Then this file can live in some other location. -- To view, visit http://gerrit.cloudera.org:8080/17802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df Gerrit-Change-Number: 17802 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Thu, 16 Sep 2021 23:25:18 +0000 Gerrit-HasComments: Yes
