snazy commented on code in PR #11281:
URL: https://github.com/apache/iceberg/pull/11281#discussion_r1796573772
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3141,32 @@ components:
uuid:
type: string
+ Credential:
+ 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
Review Comment:
I suspect, it's worth to mention that both the URI's `authority` and `path`
_elements_ must be URL escaped.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3142,6 +3211,10 @@ components:
type: object
additionalProperties:
type: string
+ storage-credentials:
+ type: array
Review Comment:
Need a definition which set of credentials a client shall pick, in case
multiple credentials match.
Either we make it easy for clients and specify that a client must use the
credentials with first matching prefix (the server can sort the credentials by
prefix-length (descending), or we specify that a client must pick the
credentials with the longest matching prefix.
I have a slight preference to the first option to reduce complexity and
effort from "all the client implementations".
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3141,32 @@ components:
uuid:
type: string
+ Credential:
+ 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 if several credentials of the same type are
available.
+ config:
Review Comment:
I think that there should be a "top level" `expiresAt` field, so
implementations do not have to peal out this common piece of information from
the `config` property bag. If `expiresAt` is missing, that would mean the
credentials do not expire.
Maybe also an optional `refreshAt` or `refreshNotBefore` timestamp "top
level" field as well, so the server can tell clients when a refresh should
happen and not have clients refresh too early (or too late).
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3141,32 @@ components:
uuid:
type: string
+ Credential:
+ 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
Review Comment:
Should have a definition here how prefixes _must_ be described.
I can think of:
* `schema:` (including the colon `:`) to indicate that the credentials are
valid for all FileIOs that use this schema
* `schema://bucket/` (including the trailing slash `/`) for credentials for
a specific bucket
* `schema://bucket/some/path/` (including the trailing slash `/`) for
credentials for a specific path in a specific bucket
I think it's important to define whether a trailing slash `/` must, should,
or must not be included in the prefix to have a common definition across all
implementations and behavior on the client side.
Mandating a trailing slash makes paths non-ambiguous (e.g. `s3://foo/bar/`
vs `s3://foo/bar` in case there are paths like `s3://foo/bar/meep` and
`s3://foo/barbaz/meep`). WDYT?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3141,32 @@ components:
uuid:
type: string
+ Credential:
+ 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 if several credentials of the same type are
available.
+ config:
+ type: object
+ additionalProperties:
+ type: string
+
+
+ LoadCredentialsResponse:
+ type: object
+ required:
+ - storage-credentials
+ properties:
+ storage-credentials:
+ type: array
Review Comment:
(Same comment as below for `LoadTableResult.array`)
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3141,32 @@ components:
uuid:
type: string
+ Credential:
+ 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 if several credentials of the same type are
available.
+ config:
Review Comment:
```suggestion
expiresAt:
type: integer
format: int64
description: Timestamp in milliseconds since epoch
(1970-01-01-00:00:00Z) when the credentials expire. If credentials do not
expire, this field is absent.
refreshNotBefore:
type: integer
format: int64
description: If this field is present, clients must not refresh
credentials before this timestamp in milliseconds since epoch
(1970-01-01-00:00:00Z). This field must not be present for credentials that do
not expire.
config:
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
