dimas-b commented on code in PR #3826:
URL: https://github.com/apache/polaris/pull/3826#discussion_r2829624929


##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +262,55 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessCredential:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
credential is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
credentials of the same type are available.
+        config:
+          type: object
+          description: |
+            Credential configurations for AWS S3, GCP GCS, and Azure ADLS are 
supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## AWS Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3
+              - `s3.access-key-id`: id for credentials that provide access to 
the data in S3
+              - `s3.secret-access-key`: secret for credentials that provide 
access to data in S3
+              - `s3.session-token`: if present, this value should be used for 
as the session token
+              - `s3.session-token-expires-at-ms`: the time the aws session 
token expires, in milliseconds
+            Extra properties:
+              - `s3.endpoint`: the S3 endpoint to use for requests

Review Comment:
   `endpoint` is technically not a "credential", which is implied by 
`StorageAccessCredential`. (also allies to other properties below).
   
   Could we structure the API in a way that clearly separates credentials from 
other configuration?



##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +262,55 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessCredential:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
credential is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
credentials of the same type are available.
+        config:
+          type: object
+          description: |
+            Credential configurations for AWS S3, GCP GCS, and Azure ADLS are 
supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## AWS Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3

Review Comment:
   Is "AWS" critical here? There are many S3-compatible implementations.



##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +262,55 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessCredential:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
credential is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
credentials of the same type are available.
+        config:
+          type: object
+          description: |
+            Credential configurations for AWS S3, GCP GCS, and Azure ADLS are 
supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## AWS Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3
+              - `s3.access-key-id`: id for credentials that provide access to 
the data in S3
+              - `s3.secret-access-key`: secret for credentials that provide 
access to data in S3
+              - `s3.session-token`: if present, this value should be used for 
as the session token
+              - `s3.session-token-expires-at-ms`: the time the aws session 
token expires, in milliseconds
+            Extra properties:
+              - `s3.endpoint`: the S3 endpoint to use for requests
+              - `s3.path-style-access`: whether to use S3 path style access
+              - `client.region`: region to configure client for making 
requests to AWS
+              - `client.refresh-credentials-endpoint`: the endpoint to load 
vended credentials for a table from the catalog
+              
+            ## GCP Configurations
+            
+            The following configurations should be respected when working with 
tables stored in GCP GCS
+              - `gcs.oauth2.token`: the gcs scoped access token
+              - `gcs.oauth2.token-expires-at`: the time the gcs access token 
expires, in milliseconds
+            Extra properties:
+              - `gcs.oauth2.refresh-credentials-endpoint`: the endpoint to 
load vended credentials for a table from the catalog
+            
+            # AZURE Configuration
+            
+            The following configurations should be respected when working with 
tables stored in AZURE ADLS
+              - `adls.sas-token.<hostname>`: an azure shared access signature 
token
+              - `adls.sas-token-expires-at-ms.<hostname>`: the expiration time 
for the access token, in milliseconds
+            Extra properties:
+              - `adls.refresh-credentials-endpoint`: the endpoint to load 
vended credentials for a table from the catalog
+
+          additionalProperties:

Review Comment:
   What is the purpose of `additionalProperties`?



##########
spec/polaris-catalog-apis/generic-tables-api.yaml:
##########
@@ -256,6 +262,55 @@ components:
           items:
             $ref: 
'../iceberg-rest-catalog-open-api.yaml#/components/schemas/TableIdentifier'
 
+
+    StorageAccessCredential:
+      type: object
+      required:
+        - prefix
+        - config
+      properties:
+        prefix:
+          type: string
+          description: Indicates a storage location prefix where the 
credential is relevant. Clients should choose the most 
+            specific prefix (by selecting the longest prefix) if several 
credentials of the same type are available.
+        config:
+          type: object
+          description: |
+            Credential configurations for AWS S3, GCP GCS, and Azure ADLS are 
supported. The following outlines 
+            the currently supported configuration options:
+              
+            ## AWS Configurations
+              
+            The following configurations should be respected when working with 
tables stored in AWS S3
+              - `s3.access-key-id`: id for credentials that provide access to 
the data in S3
+              - `s3.secret-access-key`: secret for credentials that provide 
access to data in S3

Review Comment:
   Would it be possible to use declared OpenAPI objec properties for this data, 
as opposed to relying of a naming convention? I mean something like the 
`base-location` property on line 230.



-- 
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]

Reply via email to