smaheshwar-pltr commented on code in PR #18081:
URL: https://github.com/apache/iceberg/pull/18081#discussion_r4025111533
##########
docs/docs/rest-catalog.md:
##########
@@ -83,6 +83,7 @@ at connection time through
| `rest-page-size` | null | The page size to
use when listing namespaces, tables, or other paginated resources.
|
| `namespace-separator` | `%1F` | The separator
character used for namespace levels when communicating with the REST server.
|
| `scan-planning-mode` | `client` | Controls where
scan planning is performed. Supported values: `client` (client-side planning),
`server` (server-side planning). Can be overridden per-table by the server in
LoadTableResponse. |
+| `rest.encryption.use-client-kms-creds` | `false` | Whether
encrypted REST tables may use client-side KMS credentials together with
REST-provided storage access, including vended storage credentials and remote
signing. When `false`, encrypted tables reject REST-provided storage access. |
Review Comment:
For when we do this properly, this name feels misleading. Client KMS is used
when set to true and when set to false. Maybe
`rest.encryption.allow-hybrid-credentials` is more accurate
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1246,10 +1273,14 @@ private FileIO newFileIO(
private FileIO tableFileIO(
TableIdentifier tableIdentifier,
+ TableMetadata tableMetadata,
SessionContext context,
Map<String, String> tableConf,
List<Credential> storageCredentials,
RemoteSigningConfig remoteSigningConfig) {
+ if (useClientSideStorageAccessForEncryptedTable(tableMetadata)) {
+ validateNoServerSideStorageAccess(tableIdentifier, storageCredentials,
remoteSigningConfig);
Review Comment:
As per https://github.com/apache/iceberg/pull/13225#issuecomment-5642253575,
what about when create / register table throws because of this but the creation
has succeeded in the catalog given we can only do the checks after? Have we
thought about this case?
At the least, I think the client should know about the side-effect. Trying
to "undo" the creation sounds questionable to me
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -4121,11 +4201,78 @@ protected RESTSessionCatalog newSessionCatalog(
};
catalog.initialize(
"test",
- ImmutableMap.of(
- CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.inmemory.InMemoryFileIO"));
+ ImmutableMap.<String, String>builder()
+ .put(CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.inmemory.InMemoryFileIO")
+ .putAll(additionalProperties)
+ .buildKeepingLast());
return catalog;
}
+ private static Credential storageCredential() {
+ return ImmutableCredential.builder()
+ .prefix("s3://test-bucket/")
+ .putConfig("s3.access-key-id", "test-access-key")
+ .putConfig("s3.secret-access-key", "test-secret-key")
+ .build();
+ }
+
+ private static void assertStorageCredential(List<StorageCredential>
credentials) {
+ assertThat(credentials).hasSize(1);
+ assertThat(credentials.get(0).prefix()).isEqualTo("s3://test-bucket/");
+ assertThat(credentials.get(0).config())
+ .containsEntry("s3.access-key-id", "test-access-key")
+ .containsEntry("s3.secret-access-key", "test-secret-key");
+ }
+
+ private RESTCatalogAdapter adapterWithStorageCredential() {
+ Credential credential = storageCredential();
+ return new RESTCatalogAdapter(backendCatalog) {
+ @SuppressWarnings("unchecked")
+ @Override
+ public <T extends RESTResponse> T handleRequest(
+ Route route,
+ Map<String, String> vars,
+ HTTPRequest httpRequest,
+ Class<T> responseType,
+ Consumer<Map<String, String>> responseHeaders) {
+ T response = super.handleRequest(route, vars, httpRequest,
responseType, responseHeaders);
+ if (route == Route.LOAD_TABLE && response instanceof LoadTableResponse
loadResponse) {
+ return (T)
+ LoadTableResponse.builder()
+ .withTableMetadata(loadResponse.tableMetadata())
+ .addAllConfig(loadResponse.config())
+ .addCredential(credential)
+ .withRemoteSigningConfig(TEST_REMOTE_SIGNING_CONFIG)
Review Comment:
Nit for when we do this properly, probably worth testing cred-only +
signing-only independently
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -276,6 +287,12 @@ public void initialize(String name, Map<String, String>
unresolved) {
RESTCatalogProperties.METRICS_REPORTING_ENABLED,
RESTCatalogProperties.METRICS_REPORTING_ENABLED_DEFAULT);
+ if (props.containsKey(CatalogProperties.ENCRYPTION_KMS_TYPE)
+ || props.containsKey(CatalogProperties.ENCRYPTION_KMS_IMPL)) {
+ this.keyManagementClient = EncryptionUtil.createKmsClient(props);
Review Comment:
This PR now changes to not use the merged props, but just props if I'm
understanding right. I think there is something to be discussed here on
server-returned configs (defaults + overrides) which are would be ignored with
this change e.g.
https://docs.google.com/document/d/1VSewbVmjukU5eTiZruCJVmUZcRZvcAsd4JOeC6Lt3fo/edit?tab=t.0#heading=h.huszjf7jg3fb
mentions
```
"defaults": {
"encryption.kms-type": "aws"
},
```
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableScan.java:
##########
@@ -316,12 +321,30 @@ private CloseableIterable<FileScanTask>
fetchPlanningResult() {
FetchPlanningResultResponse response = result.get();
- this.scanFileIO =
- !response.credentials().isEmpty() ? scanFileIO(response.credentials())
: table().io();
+ this.scanFileIO = fileIOForPlanningResponse(response.credentials());
return scanTasksIterable(response.planTasks(), response.fileScanTasks());
}
+ private FileIO fileIOForPlanningResponse(List<Credential>
storageCredentials) {
+ if (storageCredentials.isEmpty()) {
+ return table().io();
+ }
+
+ Preconditions.checkState(
Review Comment:
We probably want to clean up plan resources if we throw this. Otherwise with
the current implementation, clean up won't happen
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1276,6 +1307,27 @@ private FileIO tableFileIO(
return newFileIO(context, fullConf.buildKeepingLast(), storageCredentials);
}
+ private boolean useClientSideStorageAccessForEncryptedTable(TableMetadata
tableMetadata) {
+ return useClientSideStorageAccessForEncryptedTables()
+ && tableMetadata != null
+ &&
tableMetadata.properties().containsKey(TableProperties.ENCRYPTION_TABLE_KEY);
+ }
+
+ private void validateNoServerSideStorageAccess(
Review Comment:
As per https://github.com/apache/iceberg/pull/13225#issuecomment-5642253575,
this PR checks storage credentials + signing config, but what about
1. returned `LoadTableResponse` `config`? Those configs configure IO too
from the server, from before we had the `storage-credentials` field
2. server `/config` defaults/overrides also still flow through
Also, remote signing can be configured through IO properties too for both
these cases. Is it intentional to only cover the `storageCredentials` and
`remoteSigningConfig` case?
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1276,6 +1307,27 @@ private FileIO tableFileIO(
return newFileIO(context, fullConf.buildKeepingLast(), storageCredentials);
}
+ private boolean useClientSideStorageAccessForEncryptedTable(TableMetadata
tableMetadata) {
+ return useClientSideStorageAccessForEncryptedTables()
+ && tableMetadata != null
+ &&
tableMetadata.properties().containsKey(TableProperties.ENCRYPTION_TABLE_KEY);
+ }
+
+ private void validateNoServerSideStorageAccess(
+ TableIdentifier tableIdentifier,
+ List<Credential> storageCredentials,
+ RemoteSigningConfig remoteSigningConfig) {
+ Preconditions.checkState(
Review Comment:
(Noting that users will likely run into this given current docs, maybe worth
updating the docs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
