dimas-b commented on code in PR #3729:
URL: https://github.com/apache/polaris/pull/3729#discussion_r2853773783
##########
spec/polaris-management-service.yml:
##########
@@ -987,6 +988,23 @@ components:
BEARER: "#/components/schemas/BearerAuthenticationParameters"
SIGV4: "#/components/schemas/SigV4AuthenticationParameters"
IMPLICIT: "#/components/schemas/ImplicitAuthenticationParameters"
+ GCP: "#/components/schemas/GcpAuthenticationParameters"
+
+ GcpAuthenticationParameters:
+ type: object
+ description: Uses Iceberg GoogleAuthManager for authentication.
Review Comment:
Is the "Iceberg GoogleAuthManager" just a tool for implementing this feature
or does it actually define a particular authentication mechanism? In the first
case, it might be preferable to just refer to Google in the spec and leave the
Iceberg part to the impl. code.
Is there a Google doc that describes this auth mechanism? It would be great
to mention it.
##########
spec/polaris-management-service.yml:
##########
@@ -987,6 +988,23 @@ components:
BEARER: "#/components/schemas/BearerAuthenticationParameters"
SIGV4: "#/components/schemas/SigV4AuthenticationParameters"
IMPLICIT: "#/components/schemas/ImplicitAuthenticationParameters"
+ GCP: "#/components/schemas/GcpAuthenticationParameters"
+
+ GcpAuthenticationParameters:
+ type: object
+ description: Uses Iceberg GoogleAuthManager for authentication.
+ allOf:
+ - $ref: '#/components/schemas/AuthenticationParameters'
+ properties:
+ quotaProject:
+ type: string
+ description: The project to which this call is billed. Avoids "The
biglake.googleapis.com API requires a quota project, which is not set by
default." error
Review Comment:
The `Avoids [...]` part looks odd in the spec. We could mention it in docs /
README. WDYT about keeping only `The project to which authentication calls are
billed`?
##########
polaris-core/src/main/java/org/apache/polaris/core/connection/ConnectionConfigInfoDpo.java:
##########
@@ -117,7 +122,7 @@ public AuthenticationParametersDpo
getAuthenticationParameters() {
return serviceIdentity;
}
- private static final ObjectMapper DEFAULT_MAPPER;
+ @VisibleForTesting static final ObjectMapper DEFAULT_MAPPER;
Review Comment:
Could we keep this `private` (for isolation) and use some other mapper in
tests?
##########
polaris-core/src/main/java/org/apache/polaris/core/connection/AuthenticationParametersDpo.java:
##########
@@ -85,8 +94,6 @@ public static AuthenticationParametersDpo
fromAuthenticationParametersModelWithS
oauthClientCredentialsModel.getScopes());
break;
case BEARER:
- BearerAuthenticationParameters bearerAuthenticationParametersModel =
- (BearerAuthenticationParameters) authenticationParameters;
Review Comment:
This is a valid change, but it seems irrelevant to the purpose of this PR...
Would you mind moving it to a separate PR?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRESTExternalCatalogFactory.java:
##########
@@ -18,6 +18,8 @@
*/
package org.apache.polaris.service.catalog.iceberg;
+import static org.apache.iceberg.CatalogProperties.URI;
Review Comment:
+1 to @snazy 's point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
