jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775492974


##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,81 @@ components:
         uuid:
           type: string
 
+    ADLSCredentials:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Credentials'
+      required:
+        - type
+      properties:
+        type:
+          type: string
+          enum: [ "adls" ]
+        account-name:
+          type: string
+        account-key:
+          type: string
+        sas-token:
+          type: string
+        expires-at-ms:
+          type: integer
+          format: int64
+
+    GCSCredentials:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Credentials'
+      required:
+        - type
+        - token
+        - expires-at-ms
+      properties:
+        type:
+          type: string
+          enum: [ "gcs" ]
+        token:
+          type: string
+        expires-at-ms:
+          type: integer
+          format: int64
+
+    S3Credentials:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Credentials'
+      required:

Review Comment:
   Cool, in that case I think this is a great step, it does make a lot of sense 
to have dedicated credentials refresh API, we were also thinking about 
proposing it later. We have implemented credentials vending in LakeFormation 
for a long time now, and last year S3 also offered access grant with similar 
feature. The user pattern has been proven that having a dedicated API for that 
makes a lot of sense since refreshing credentials is a much more frequent 
operation than loading table and can be done very quickly with a dedicated API, 
and from security perspective it is also a better posture. In that case I think 
what is proposed here makes sense, and I am more in favor of the current design 
than what Yufei suggests, because it makes it clear that these fields are 
related to that new API, and existing config map continues to work for 
configuring the FileIO, with the additional note that the new credentials field 
would override the credentials config in the config map.



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to