You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2024/07/19 11:05:54 UTC
[PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
nastra opened a new pull request, #10722:
URL: https://github.com/apache/iceberg/pull/10722
(no comment)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1706467120
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
+ type: object
+ discriminator:
+ propertyName: type
+ mapping:
+ adls: '#/components/schemas/ADLSCredentials'
+ gcs: '#/components/schemas/GCSCredentials'
+ s3: '#/components/schemas/S3Credentials'
+ oneOf:
Review Comment:
I think it would make sense to add S3RemoteSigning as another form of Configuration.
Currently it is initiated via "s3.remote-signing-enabled" config attribute for Table endpoints with the "s3.signer.uri" only being passed at the initial config endpoint. It's strictly speaking not a Credential, but it serves the same purpose and makes sense to standardize.
The selection of S3 Configuration can then be made by the server based on what is specified via the `X-Iceberg-Access-Delegation` header. https://github.com/apache/iceberg/blob/257b1d7b18f638b5925de32bcd9bbcbe5a4416c2/open-api/rest-catalog-open-api.yaml#L1493
One problem in almost all client implementations today is that this header is not implemented according to spec - i.e. even clients supporting both forms of S3 access only supply a single attribute and not a list of supported methods. This is a problem as the client can't know if an STS Endpoint is available or not for this specific S3 storage. Thus clients should supply all supported forms and leave the decision to the server. I think it would make sense making this more specific in the spec.
Currently with tabular.io, we get a 400 error if the client specifies `remote-signing,vended-credentials` as `X-Iceberg-Access-Delegation` - which shows that no client follows the desired behavior.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756796345
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
> A table may have metadata and data files spread out across multiple locations (because ultimately clients can append data from from any location). It would be nice to support those use cases too.
I don't think this should be the scope of this PR and rather warrants a separate proposal
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1761288119
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
> we most likely don't want to push the mapping to the client
How can the client deal with multiple credentials without any mapping? Even sending one credential per provider already implies mapping by provider type... Does it not?
A credential per provider would be a good start.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1767315994
##########
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:
Hi @nastra , sorry for the delay. Yes, we could handle it in a separated PR. There is one minor point, that the structure I proposed can reuse the field `type`. For example, this is the current PR:
```
credential
- type(s3/adsl/gcs)
```
This is the proposal with storage, in which we don't need to specify the storage type.
```
storage
- type(s3/adsl/gcs)
- credentials
- config <- we can work on this a bit later
```
WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773613570
##########
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:
Sorry a bit late to this, I was in general neutral about the proposal so did not pay too much attention.
But the AWS client thing seems worth discussion. Ultimately, it sounds like we want the freedom to configure the FileIO in the way we want **per table**, that includes credentials and region and maybe other things in the future.
Given that we already want to formally standardize all the FileIO properties across languages, and also we are using config map to vend the configurations of those FileIO, then why do we still need to have dedicated credentials fields? It does seem a bit redundant to me just for the sake of not using config properties for a selective subset of fields...
Yufei's proposal of having a dedicated storage field could be a more formalized way to say that these configs are only for FileIO, I am neutral on that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773432500
##########
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:
> I didn’t quite get that. Clients in other languages should stick to the names defined in the spec. I can’t imagine the server responding with different config keys, like the S3 region, just because the client is implemented in a different language.
I remember that this was the case for pyiceberg a while ago, but looks like this was addressed by https://github.com/apache/iceberg-python/pull/922.
> I'm OK with non-stardadrized schema-less for configs as they may change, but I still think it is more organized with one more layer like the following. As I said, this is a minor point to make the overall schema a bit cleaner, esp. for new storage type adding in the future.
@flyrain do we have a concrete use case for this or is this rather a nice-to-have? I'm thinking that we most likely don't need this right now. Also in the last Iceberg sync we were talking about v2 version of loadTable, so maybe that fits better with a v2 API when we have a concrete use case for introducing this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1804849870
##########
open-api/rest-catalog-open-api.py:
##########
@@ -1311,11 +1327,19 @@ class LoadViewResult(BaseModel):
- `token`: Authorization bearer token to use for view requests if OAuth2 security is enabled
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
+ Clients should first check whether the respective credentials exist in the `storage-credentials` field before checking the `config` for credentials.
Review Comment:
Nit: Should we replace the use of "should" with "must" throughout this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1685191973
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
Review Comment:
Yeah these are ultimately storage credentials that are being spec'd out so I think it makes sense to associate them with the storage service service's name
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768596344
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
ok. thx!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768131177
##########
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:
@flyrain I gave this some more thought and I don't think we should be standardizing any of the storage-related configuration in the REST spec for the following reasons:
* those are all Java-specific and other languages might use other property names for the respective FileIO implementation
* I think the issue is rather with documentation, since most storage-related properties aren't well documented. That means we should document those on the website and maybe in the REST spec (similar to what https://github.com/apache/iceberg/pull/10576 attempted). Documenting something in the REST spec is still different than actually defining a schema + properties for that configuration.
Due to that I don't think it makes sense to introduce `storage` right now in order to duplicate and provide a subset of the `config` that's already defined in `LoadTableResponse`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775507976
##########
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:
@nastra, thanks for the context. The suggest I provide is a minor structure change, which isn't needed in the new endpoint. I am OK with the current one if that's more align with the new endpoint.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684588913
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
Review Comment:
Is there no expiration for Azure credentials?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1754694285
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
+ type: object
+ discriminator:
+ propertyName: type
+ mapping:
+ adls: '#/components/schemas/ADLSCredentials'
+ gcs: '#/components/schemas/GCSCredentials'
+ s3: '#/components/schemas/S3Credentials'
+ oneOf:
Review Comment:
I don't think it makes sense to add remote signing here. The scope of the PR is focusing on defining a clear structure for credentials. This will be later then used for having a standardized way of refreshing vended credentials (which will be follow-up work)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1758243809
##########
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:
> the region is already sent back in the config of LoadTableResult/ LoadViewResult itself
> I don't think the region is bound to the credential. I believe it's possible to build a policy that allows access to buckets in multiple regions, so region should be associated with the bucket, not the credentials. Therefore, we shouldn't include it here.
I agree that the region is not bound to the credentials. I thought we had moved everything out of the config, didn’t we? I’m okay with placing it in different locations, but currently, the region in config lacks a defined schema. For consistency, perhaps we should add a schema for it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1758534281
##########
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:
> I thought we are moving everything out of the config, didn’t we?
@flyrain we're only standardizing credentials and not everything that we currently pass via the `config`. Standardizing all configuration is a bigger task and warrants a separate proposal. Besides that it's worth mentioning that a server would still have to sent credentials via the `config` for older clients.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757292842
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
Credential policies can be created to accommodate multiple locations/buckets, so that is a server-side issue, not related to the credentials. If I have multiple buckets, the policy just needs to be crafted such that appropriate access is granted for the specified locations. There's no need for a mapping.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1686287333
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
Review Comment:
there is, I didn't add it in the first iteration because I haven't double-checked the ADLS credentials
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1760951014
##########
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:
@flyrain are you suggesting to standardize all storage-related configuration into their own properties inside `config`? Wouldn't that require a spec change whenever a new configuration option would be added?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1841531249
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3142,6 +3162,10 @@ components:
type: object
additionalProperties:
type: string
+ storage-credentials:
Review Comment:
Just curious why should return multiple credentials for one table? Any use cases? The current TableOps just support one fileIO and also noticed the AWS and GCS credential refresh handler just pick the first credential from the server.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1798841576
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
yes exactly, we're trying to move the docs to this new credential section
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773708587
##########
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:
> ... then why do we still need to have dedicated credentials fields
@jackye1995 we want to standardize credentials because of a follow-up proposal that will focus on refreshing vended credentials
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1754708847
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2905,6 +2987,10 @@ components:
- `token`: Authorization bearer token to use for view requests if OAuth2 security is enabled
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
the spec is designed in a way that would easily allow to add new type(s) of credentials, so this can be done in a follow-up if there's interest from the community but it's not in the scope of this PR
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756810918
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
Is SAS the only supported credentials type of ADLS? Would it be worth adding sub-types (future-proofing)?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1754684549
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
The scope of this PR is very narrow and focuses on standardizing credentials (which are then included in the `config` when a table/view is loaded), so I don't think we should be adding more general properties here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1759207112
##########
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:
Thanks for clarification, @nastra !
Does it make sense to introduce a `storage` property within LoadTableResult? By doing that, you can group all storage-related settings—such as type, config (e.g., region), and credentials—into a cohesive structure. This approach is especially beneficial if we plan to support multiple storage types or need to extend configurations in the future.
Here's how the updated schema might look:
```
LoadTableResult:
properties:
storage:
$ref: '#/components/schemas/Storage'
Storage:
type: object
required:
- type
- config
- credentials
properties:
type:
type: string
description: The type of storage service (e.g., ADLS, S3, GCS)
config:
type: object
description: Configuration settings specific to the storage type
properties:
region:
type: string
description: The region where the storage is located
# Add other config properties as needed
credentials:
$ref: '#/components/schemas/Credentials'
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684549213
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
+ - access-key-id
+ - secret-access-key
+ properties:
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ GcsCredentials:
+ type: object
+ required:
+ - token
+ - expires-at
+ properties:
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
+ type: object
+ properties:
+ aws:
+ $ref: '#/components/schemas/AwsCredentials'
+ azure:
+ $ref: '#/components/schemas/AzureCredentials'
+ gcs:
+ $ref: '#/components/schemas/GcsCredentials'
Review Comment:
good point, that makes sense
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756199614
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
> Would it make sense to take it one step further and have clients request credentials for narrower use cases?.. For example separating read and write access.
>
> To be clear, I mean adding a separate REST endpoint for obtaining storage credentials.
@dimas-b yes I'll be working on a separate endpoint for credentials in a follow-up proposal that would allow refreshing vended credentials. The goal of this PR is to standardize the credentials themselves
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757382280
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
That does currently and I don't think multiple would be supported by the implementation as of today. We could possibly allow multiple credentials (one per provider?), but I think we should require that the provided credential be scoped for all table locations within a provider and not push that mapping onto the client since all of the policies I'm aware of allow for that.
@nastra I think multiple provider is an interesting question, but also complicates this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756790932
##########
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:
@flyrain the region is already sent back in the `config` of `LoadTableResult`/ `LoadViewResult` itself, so I'm not sure why we would want to make it part of `S3Credentials? The region is for configuring the S3 client but the credentials are for configuring `S3FileIO`, so it's a different layer where the region is being used
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1754684549
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
I don't think we should be adding more properties here. The scope of this PR is very narrow and focuses on standardizing credentials (which are then included in the `config` when a table/view is loaded)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755210069
##########
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:
Could we add the `region`? Without it, clients might run into issues. Here’s why it would be helpful:
1. Setting a default region on the client side doesn’t work well since clients could be accessing s3 files from different regions.
2. Clients could try to figure out the region using `get-bucket-location`, but this means an extra API call. Plus, if the vended credentials don’t allow this call, it could cause more problems.
Adding the `region` would simplify things and enhance overall client reliability.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755464604
##########
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:
Despite the [other comment below](https://github.com/apache/iceberg/pull/10722/files#r1755423596) - the region should be the same as the one for the S3FileIO configuration?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
Good point!
Obtaining credentials every time a table's loaded is often just overhead. Just inspecting table-metadata contents doesn't need any object-store credentials.
Also if there's finally a way to refresh credentials, the lifetime of those can be shortened.
There's a lot of room for improvement here
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
A valid point. However, that could not just span e.g. 2 S3 buckets in the same object store, but across different object stores - even two S3 - one on Minio and one on Ceph - or GCS and S3 or....
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
Review Comment:
The session-token is part of the vended credentials - omitting it can mean that the credentials are incomplete and in turn not accepted.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755859795
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
Re: refresh: it should even be possible to support very long client sessions too, where credentials are rotated during the same query: initial access is based on old credentials, overlap in the middle (both sets of credentials valid) and using new credentials at the end of processing.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757287616
##########
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:
I don't think the region is bound to the credential. I believe it's possible to build a policy that allows access to bucket in multiple regions, so region should be associated with the bucket, not the credentials.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1761451799
##########
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:
That's right. We can standardize some of them, like aws region, while still having a flexible map to hold abitrary key-value pairs, like this:
```
extraConfig:
type: object
additionalProperties:
type: string
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796543659
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
I agree with @jackye1995 that `session-token` is not part of the S3 API, and users might not have IAM or similar setups in their environment. I understand the motivation for requiring `session-token` to exist, but I'm concerned it could be too restrictive and prevent users from implementing the rest spec.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684548371
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
+ access_key_id: str = Field(..., alias='access-key-id')
+ secret_access_key: str = Field(..., alias='secret-access-key')
+ token: Optional[str] = None
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
+
+class GcsCredentials(BaseModel):
+ token: str
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
Review Comment:
I don't think we want to include those, because we only want to standardize the credentials themselves and bucket / endpoint isn't credential-related
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796090787
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3142,6 +3163,10 @@ components:
type: object
additionalProperties:
type: string
+ storage-credentials:
+ type: array
+ items:
+ $ref: '#/components/schemas/Credential'
Review Comment:
nit: should this object be called `StorageCredential` in this case?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796691807
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
@Xuanwo the PR has been reworked to not include properties specific to storage providers, so this shouldn't be an issue anymore. See also https://github.com/apache/iceberg/pull/10722#discussion_r1795062381
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796705687
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
Hi, @nastra. Sorry for my outdated comment, and thank you for this change. This should work.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775515425
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
Yes we require those specifically to make the credentials temporary and not accidentally leak non-expiring credentials (see also https://docs.google.com/document/d/1lySd_5hMZNtISLKsOvAq7xiNzdXU6TAoHF_yrOXWQvM/edit#heading=h.hs6r9d26w1y2 where this is mentioned)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756843123
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
E.g. `Account Key`? I'm not saying we should support it right away, but alternatives to SAS apparently exist.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756797341
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
@nastra ; Great to hear that this kind of improvement is in the works :) I assume a dev list discussion will start as soon as that change can be shared.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756854643
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
I believe vended credentials should have a set expiration on them and that's only possible for a SAS token, hence why there's no accountKey / accountName here as it's risky to leak such credentials and not have expiration on them. Let's move this discussion to the design doc (once I share it on the ML)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756815382
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
If a more feature-rich credentials endpoint spec is in the works (as discussed in another comment), I agree, it would be preferable to move this discussion there. However, I think it makes sense to tackle this problem during together with the dedicated credentials endpoint.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757382280
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
That does currently and I don't think multiple would be supported by the implementation as of today. We could possibly allow multiple credentials (one per provider?), but I think we should require that the provided credential be scoped for all table locations within a provider and not push that mapping onto the client since all of the policies I'm aware of allow for that.
@nastra I think multiple provider is an interesting question, but also complicates this.
We can currently support different tables across providers in the same catalog, but not tables that span multiple providers (not sure if that's scope we want to take on right now).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "aniket-s-kulkarni (via GitHub)" <gi...@apache.org>.
aniket-s-kulkarni commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755358145
##########
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:
+1 for including `region`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684545916
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
Review Comment:
yeah that might make sense to name them more closely to the underlying FileIO impl
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1686170249
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
Review Comment:
I've renamed all of them
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1705687319
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
Should we generalize this to:
```suggestion
Configuration:
```
There are some obvious properties missing, such as `region` and `endpoint`, that I think we should supply with the same object.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773438348
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
it's probably ok to rename `scheme` to `prefix` here and make it optional
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1797085223
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
#10576 appears to define credential properties in the REST spec, which is fine from my POV :) I assume after rebasing it will be talking about credential properties under the new config section introduced in this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796970338
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
@dimas-b does the documentation of the storage related properties address your comment about "additional guidelines"?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1754684549
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
I don't think we should be adding more properties here. The scope of this PR is very narrow and focuses on standardizing credentials
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755461632
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
It is tricky that the arbitrary file locations can easily break the boundary of credentials.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757301811
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
Both `region` and `endpoint` are related to the client connection, not to the credential. I would not include them here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1758243809
##########
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:
> the region is already sent back in the config of LoadTableResult/ LoadViewResult itself
> I don't think the region is bound to the credential. I believe it's possible to build a policy that allows access to buckets in multiple regions, so region should be associated with the bucket, not the credentials. Therefore, we shouldn't include it here.
I agree that the region is not bound to the credentials. I thought we are moving everything out of the config, didn’t we? I’m okay with placing it in different locations, but currently, the region in config lacks a defined schema. For consistency, perhaps we should add a schema for it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1758556272
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
I mean technically we could design the spec/impl in a way that would allow a server to sent back credentials for multiple providers (one credential per provider) in a single response. So rather than having
```
// inside LoadTableResult / LoadViewResult
"credentials" : {
"type" : "gcs",
"token" : "gcsToken",
"expires-at-ms" : 1000
}
```
we could have
```
// inside LoadTableResult / LoadViewResult
"credentials" : {
"s3": {
"access-key-id" : "keyId",
"secret-access-key" : "accessKey",
"session-token" : "sessionToken",
"expires-at-ms" : 1000
},
"gcs": {
"token" : "gcsToken",
"expires-at-ms" : 1000
},
// no adls credentials as that provider isn't used for that table
}
```
Also I agree that we most likely don't want to push the mapping to the client. Is that what you guys had in mind?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1799977130
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,22 @@ components:
uuid:
type: string
+ StorageCredential:
+ 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
+
+
Review Comment:
nit: extra line (looks like we're generally single line-separator)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10722:
URL: https://github.com/apache/iceberg/pull/10722
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796948974
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
the documentation of the respective FileIO properties is handled outside this PR and is in the scope of https://github.com/apache/iceberg/pull/10576. We'll be updating/rebasing https://github.com/apache/iceberg/pull/10576 once this PR here is in and that should make it clearer for servers which properties are available for FileIO implementations
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773717401
##########
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:
Is that published? If not, could you give a short overview if possible?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1767315994
##########
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:
Hi @nastra , sorry for the delay. Yes, we could handle it in a separated PR. There is one minor point, that the structure I proposed can reuse the field `type`. For example, this is the current PR:
```
credentials
- type(s3/adsl/gcs)
```
This is the proposal with storage, in which we don't need to specify the storage type in credentials.
```
storage
- type(s3/adsl/gcs)
- credentials
- config <- we can work on this a bit later
```
WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768581323
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
I've added some wording that says all credentials need an expiration
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768257923
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
we could make `credentials` return a list of any type of credential and each credential could have a `scheme` or `prefix` property, which can then be used to map that credential, wdyt?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1762478329
##########
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:
Where would we draw the line between which should be standardized and which shouldn't?
What happens if certain storage configurations become obsolete and get removed from the actual impl? My concern here is that we would have to support those properties forever in the REST spec for older clients.
I'm ok introducing `Storage` as shown below to provide more flexibility with the configuration but I believe I mentioned somewhere else that it's probably better to handle configuration standardization in a separate proposal. @flyrain would you be interested in working on that proposal?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684583057
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
Review Comment:
I think we might want to include `session-token` and `expires-at-ms` as required because we don't want static credentials passed through this interface. That could be the source of leaked credentials if implemented without expiration.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684584222
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
+ - access-key-id
+ - secret-access-key
+ properties:
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ token:
Review Comment:
AWS refers to this as `session token`, so I think we should use that terminology here: `session-token`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757299795
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
Review Comment:
I'm not aware of any solutions that don't require `session-token` for temporary credentials (I think Minio and R2 follow AWS here). If there are examples, we can consider those (or make a separate credential object in the future), but think we should be very strict here so someone doesn't inadvertently build an insecure solution that hands out static credentials.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756810918
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
Is SAS the only supported credentials type for ADLS? Would it be worth adding sub-types (future-proofing)?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775675415
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
That makes sense. In that case I think we can keep `expires-at-ms` required. But what about `session-token`? I think not all storage systems that implements the S3 API supports that.
I know we want to explore defining "credentials" as a map, but assuming we go with this current approach, I feel it is better to at least make that optional.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775504602
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
do we need to require `session-token` and `expires-at-ms`? This forces the credentials to be temporary, which is probably the case most of the time, but I don't think we need to exclude the case where people want to do differently. I think some systems using S3 as a proxy might be able to appreciate this.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,95 @@ components:
uuid:
type: string
+ ADLSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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.
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+
+ GCSCredential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "gcs" ]
+ 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.
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis since 1970-01-01T00:00:00Z at which the given token expires
+
+ S3Credential:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credential'
+ required:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
Review Comment:
do we need to require `session-token` and `expires-at-ms`? This forces the credentials to be temporary, which is probably the case most of the time, but I don't think we need to exclude the case where people want to do differently. I think some storage systems using S3 as a proxy might be able to appreciate this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773760835
##########
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:
> It does seem a bit redundant to me just for the sake of not using config properties for a selective subset of fields...
I think this PR is to apply schema to file IO related fields mainly for credentials, so that, both server impl. and clients can follow without worrying too much about the compatibility, like key names changes.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1705592814
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
Review Comment:
Hi, I believe it's acceptable for the rest catalog implementation to return the temporarily generated scoped AK/SK without session token, similar to what's supported in some third-party storage services. The inclusion of a `session-token` seems like an unnecessary requirement to me.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684546941
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
+ access_key_id: str = Field(..., alias='access-key-id')
+ secret_access_key: str = Field(..., alias='secret-access-key')
+ token: Optional[str] = None
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
Review Comment:
`region` / `endpoint` isn't really credential-related so I don't think these should be included here
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1684242591
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
+ access_key_id: str = Field(..., alias='access-key-id')
+ secret_access_key: str = Field(..., alias='secret-access-key')
+ token: Optional[str] = None
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
+
+class GcsCredentials(BaseModel):
+ token: str
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
Review Comment:
```suggestion
default_bucket_location: Optional[str] = Field(None, alias='default-bucket-location')
endpoint: Optional[str] = None
```
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
+ access_key_id: str = Field(..., alias='access-key-id')
+ secret_access_key: str = Field(..., alias='secret-access-key')
+ token: Optional[str] = None
+ expires_at_ms: Optional[int] = Field(None, alias='expires-at-ms')
+
Review Comment:
```suggestion
region: Optional[str] = None
endpoint: Optional[str] = None
```
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,54 @@ components:
uuid:
type: string
+ AzureCredentials:
+ type: object
+ properties:
+ account-name:
+ type: string
+ account-key:
+ type: string
+ token:
+ type: string
+
+ AwsCredentials:
+ type: object
+ required:
+ - access-key-id
+ - secret-access-key
+ properties:
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ GcsCredentials:
+ type: object
+ required:
+ - token
+ - expires-at
+ properties:
+ token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
+ type: object
+ properties:
+ aws:
+ $ref: '#/components/schemas/AwsCredentials'
+ azure:
+ $ref: '#/components/schemas/AzureCredentials'
+ gcs:
+ $ref: '#/components/schemas/GcsCredentials'
Review Comment:
We might also add a type to it, this is a bit more flexible to extend I believe (you could add a custom credential in there that will be picked up by a custom FileIO):
```suggestion
Credentials:
discriminator:
propertyName: type
mapping:
aws: '#/components/schemas/AwsCredentials'
azure: '#/components/schemas/AzureCredentials'
gcs: '#/components/schemas/GcsCredentials'
type: object
required:
- type
properties:
type:
type: string
```
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
+ access_key_id: str = Field(..., alias='access-key-id')
+ secret_access_key: str = Field(..., alias='secret-access-key')
+ token: Optional[str] = None
Review Comment:
```suggestion
session_token: Optional[str] = Field(..., alias='session-token')
```
##########
open-api/rest-catalog-open-api.py:
##########
@@ -441,6 +441,30 @@ class AssertViewUUID(BaseModel):
uuid: str
+class AzureCredentials(BaseModel):
+ account_name: Optional[str] = Field(None, alias='account-name')
+ account_key: Optional[str] = Field(None, alias='account-key')
+ token: Optional[str] = None
+
+
+class AwsCredentials(BaseModel):
Review Comment:
Do we want to call it AWS, or generalize it to S3? Same with Azure to ADLS.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796920466
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
Servers have to provide concrete properties for vendor-specific credentials. Since this spec does not define them, how will servers know what properties to provide?
If servesr adapt to the java REST client impl. then the java client will become the "spec"... I'd prefer for Iceberg to provide additional guidelines (which do not have to be part of the REST Catalog spec) to clarify credential properties for known use cases. It might be necessary for this spec to define a sub-type property since some vendors support multiple auth methods for the same URI scheme.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,22 @@ components:
uuid:
type: string
+ StorageCredential:
+ 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:
Review Comment:
I understand there was a general agreement to treat vendor-specific credentials as simple property bags. However, would it make sense to define some common attributes (like expiry time) in this spec? I suppose those would be relevant to the follow-up effort of refreshing credentials on the fly. WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768913951
##########
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:
> those are all Java-specific and other languages might use other property names for the respective FileIO implementation
I didn’t quite get that. Clients in other languages should stick to the names defined in the spec. I can’t imagine the server responding with different config keys, like the S3 region, just because the client is implemented in a different language.
Thanks for sharing the PR to add more comments. I'm OK with **non-stardadrized** **schema-less** for configs as they may change, but I still think it is more organized with one more layer like the following. As I said, this is a minor point to make the overall schema a bit cleaner, esp. for new storage type adding in the future.
```
LoadTableResponse:
storages:
type: array
items:
$ref: '#/components/schemas/Storage'
storage:
type: object
discriminator:
propertyName: type
mapping:
adls: '#/components/schemas/adls'
gcs: '#/components/schemas/gcs'
s3: '#/components/schemas/s3'
oneOf:
...
s3
- type(s3/adsl/gcs)
- credentials
- config <- schema-less key-value pairs, add comments here for storage related configs
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756802537
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ 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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
Review Comment:
nit: the `-ms` suffix alone does not seem very meaningful without specifying the reference point in time. If the reference point is in documentation, then we might as well document the unit of measurement there too.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756817303
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
could you provide an example of a sub-type here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757287616
##########
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:
I don't think the region is bound to the credential. I believe it's possible to build a policy that allows access to buckets in multiple regions, so region should be associated with the bucket, not the credentials. Therefore, we shouldn't include it here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757295238
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,79 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
Review Comment:
I would agree with this and also state that we should never pass non-expiring credentials through this interface as it would be very dangerous from a security posture to expose unbounded credentials. This means we should also require session token for AWS.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755855988
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
Not too tricky, I hope :) WDYT about mapping base table URIs to credentials? The server could return this mapping in response to the client's request for credentials, then the client would choose the appropriate credential for each file it needs to access.
If the client provided some more information about the time of operation it needs to perform (e.g. read the latest snapshot or append new files to the latest snapshot) the server should be able to return a fairly narrow set (1 in most cases) of credentials.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796961823
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,22 @@ components:
uuid:
type: string
+ StorageCredential:
+ 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:
Review Comment:
I understand that there could be some common attributes that we could extract, but that would lead to a mixture of having to look up things in a config map vs other properties.
Also what if storage providers have different semantics for how expiration is defined (expires-at-ms vs expires-in-ms vs ....). This would require additional calculations to convert from one expiration format to the other. Therefore I think it's better to keep everything in the config for now until we have a clear use case where it makes sense to extract this into a common attribute.
This isn't set in stone and we can always revisit this once we implement refreshing vended credentials for the different providers and see how helpful having a separate field for expiration would be. I'd like to unblock this proposal so that we can make progress on refreshing vended credentials
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796490776
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3142,6 +3163,10 @@ components:
type: object
additionalProperties:
type: string
+ storage-credentials:
+ type: array
+ items:
+ $ref: '#/components/schemas/Credential'
Review Comment:
renamed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768604160
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
That sounds reasonable to me :)
nit: I'd rename `scheme` to `base` or something like that... and maybe make it optional, because `scheme == gs` is essentially the same as `type == gcs`, I guess.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768583976
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
I'll close this thread since this will be handled in a separate proposal
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1773760835
##########
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:
> It does seem a bit redundant to me just for the sake of not using config properties for a selective subset of fields...
I think the purpose is to apply schema to file IO related fields (mainly for credentials in this PR), so that, both server impl. and clients can follow without worrying too much about the compatibility, like key names changes.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1758556272
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
I mean technically we could design the spec/impl in a way that would allow a server to sent back credentials for multiple providers (one credential per provider) in a single response. So rather than having
```
"credentials" : {
"type" : "gcs",
"token" : "gcsToken",
"expires-at-ms" : 1000
}
```
we could have
```
"credentials" : {
"s3": {
"access-key-id" : "keyId",
"secret-access-key" : "accessKey",
"session-token" : "sessionToken",
"expires-at-ms" : 1000
},
"gcs": {
"token" : "gcsToken",
"expires-at-ms" : 1000
},
// no adls credentials as that provider isn't used for that table
}
```
Also I agree that we most likely don't want to push the mapping to the client. Is that what you guys had in mind?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756815382
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
If a more feature-rich credentials endpoint spec is in the works (as discussed in another comment), I agree, it would be preferable to move this discussion there. However, I think it makes sense to tackle this problem together with the dedicated credentials endpoint.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756815382
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
If a more feature-rich credentials endpoint spec is in the works (as discussed in another comment), I agree, it would be preferable to move this discussion there. I think it makes sense to tackle this problem together with the dedicated credentials endpoint.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1733824283
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2747,6 +2747,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:
+ - type
+ - access-key-id
+ - secret-access-key
+ - session-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "s3" ]
+ access-key-id:
+ type: string
+ secret-access-key:
+ type: string
+ session-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+
+ Credentials:
Review Comment:
+1 for providing more properties to facility end users.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1761277139
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,82 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis at which the given token expires
Review Comment:
nit: sorry, but "the epoch" is still not precise :) Would you mind mentioning a specific point in time (as in javadoc of `System.currentTimeMillis()`?... i.e. `Milliseconds since January 1, 1970 UTC`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#issuecomment-2352770228
@nastra can you post the proposal document on the dev-ML?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1775110341
##########
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:
@jackye1995 it's not published yet because I wanted to get this proposal in first and otherwise it would deviate the discussion.
Here's a quick summary of how refreshing vended credentials would look like:
Refreshing vended credentials would entail adding a new `/v1/{prefix}/namespaces/{namespace}/tables/{table}/credentials` endpoint that returns a `LoadCredentialsResponse`, which carries
```
storage-credentials:
type: array
items:
$ref: '#/components/schemas/Credential'
```
This new endpoint would be called by the mechanism that is offered by the respective storage provider. For example, GCS offers a `OAuth2CredentialsWithRefresh` which then effectively would call this new endpoint to refresh a vended credential.
Achieving the same thing for S3 would look different but effectively comes down to calling this credential endpoint too.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1733826935
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2905,6 +2987,10 @@ components:
- `token`: Authorization bearer token to use for view requests if OAuth2 security is enabled
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
Should we consider other storage like OSS?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768575926
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3103,6 +3103,82 @@ components:
uuid:
type: string
+ ADLSCredentials:
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/Credentials'
+ required:
+ - type
+ - sas-token
+ - expires-at-ms
+ properties:
+ type:
+ type: string
+ enum: [ "adls" ]
+ sas-token:
+ type: string
+ expires-at-ms:
+ type: integer
+ format: int64
+ description: The epoch millis at which the given token expires
Review Comment:
updated
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1767315994
##########
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:
Hi @nastra , sorry for the delay. Yes, we could handle it in a separated PR. There is one minor point, that the structure I proposed can reuse the field `type`. For example, this is the current PR:
```
credential
- type(s3/adsl/gcs)
```
This is the proposal with storage, in which we don't need to specify the storage type in credentials.
```
storage
- type(s3/adsl/gcs)
- credentials
- config <- we can work on this a bit later
```
WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1768257923
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
we could make `credentials` return a list of any type of credential and each credential could have a `scheme` property, which can then be used to map that credential.
I was thinking this would look like this:
```
"storage-credentials" : [
{
"type" : "gcs",
"scheme" : "gs",
"token" : "gcsToken",
"expires-at-ms" : 1000
}, {
"type" : "gcs",
"scheme" : "gs://path/to/file/x",
"token" : "token",
"expires-at-ms" : 1000
}, {
"type" : "adls",
"scheme" : "afbs",
"sas-token" : "sasToken",
"expires-at-ms" : 1000
}
]
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1795062381
##########
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:
based on recent discussions the feedback was that we don't want to have anything storage-specific in the OpenAPI spec (other than documenting the different storage configurations, which is handled by https://github.com/apache/iceberg/pull/10576). Therefore I've updated the PR and made it flexible enough so that we could still pass different credentials based on a given `prefix`. That should hopefully work for everyone.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755423596
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
A table may have metadata and data files spread out across multiple locations (because ultimately clients can append data from from any location). It would be nice to support those use cases too.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1755413798
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
Review Comment:
Would it make sense to take it one step further and have clients request credentials for narrower use cases?.. For example separating read and write access.
To be clear, I mean adding a separate REST endpoint for obtaining storage credentials.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1796952921
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3145,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
Review Comment:
thx!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1756790932
##########
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:
@flyrain the region is already sent back in the `config` itself, so I'm not sure why we would want to make it part of `S3Credentials? The region is for configuring the S3 client but the credentials are for configuring `S3FileIO`, so it's a different layer where the region is being used
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757296953
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
@danielcweeks : does your comment assume all locations are within with same storage provider?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1757401297
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3129,6 +3204,11 @@ components:
- `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.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
+
+ ## Credentials
+
+ Credentials for ADLS / GCS / S3 are provided through the `credentials` field. Clients should first check whether the
+ respective credentials exist in the `credentials` field before checking the `config` for credentials.
Review Comment:
I did not mean to overload this PR with tables split across diverse locations, but it would be nice to consider that use case in the follow-up spec revision with a dedicated credentials endpoint (as @nastra mentioned in another comment on this PR).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
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. In that case, 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1804989952
##########
open-api/rest-catalog-open-api.py:
##########
@@ -1311,11 +1327,19 @@ class LoadViewResult(BaseModel):
- `token`: Authorization bearer token to use for view requests if OAuth2 security is enabled
+ ## Storage Credentials
+
+ Credentials for ADLS / GCS / S3 / ... are provided through the `storage-credentials` field.
+ Clients should first check whether the respective credentials exist in the `storage-credentials` field before checking the `config` for credentials.
Review Comment:
I'm fine either way and it seems we have a bunch of other places that use `should` where `must` could be used as well, so I updated this to `must`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] OpenAPI: Standardize credentials in loadTable/loadView responses [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10722:
URL: https://github.com/apache/iceberg/pull/10722#discussion_r1841704909
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3142,6 +3162,10 @@ components:
type: object
additionalProperties:
type: string
+ storage-credentials:
Review Comment:
The use case for this is outlined in https://github.com/apache/iceberg/pull/10722#discussion_r1755423596.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org