singhpk234 commented on code in PR #13225:
URL: https://github.com/apache/iceberg/pull/13225#discussion_r2484099336
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -152,6 +194,17 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
String.format("Update type %s is not supported", updateType));
}
+ if (base != null) {
+ Set<String> removedProps =
+ base.properties().keySet().stream()
+ .filter(key -> !metadata.properties().containsKey(key))
Review Comment:
nit: for consistency ?
```suggestion
.filter(key -> !metadataToCommit.properties().containsKey(key))
```
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -166,7 +208,67 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
@Override
public FileIO io() {
- return io;
+ if (encryptionKeyId == null) {
+ return io;
+ }
+
+ if (encryptingFileIO == null) {
+ encryptingFileIO = EncryptingFileIO.combine(io, encryption());
+ }
+
+ return encryptingFileIO;
+ }
+
+ @Override
+ public EncryptionManager encryption() {
+ if (encryptionManager != null) {
+ return encryptionManager;
+ }
+
+ if (encryptionKeyId != null) {
+ if (kmsClient == null) {
+ throw new RuntimeException(
+ "Cant create encryption manager, because key management client is
not set");
+ }
+
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.put(TableProperties.ENCRYPTION_TABLE_KEY,
encryptionKeyId);
+ tableProperties.put(
+ TableProperties.ENCRYPTION_DEK_LENGTH,
String.valueOf(encryptionDekLength));
+ encryptionManager =
+ EncryptionUtil.createEncryptionManager(
+ encryptedKeysFromMetadata, tableProperties, kmsClient);
+ } else {
+ return PlaintextEncryptionManager.instance();
+ }
+
+ return encryptionManager;
+ }
+
+ private void encryptionPropsFromMetadata(TableMetadata metadata) {
Review Comment:
Thank you everyone for great discussions !
There is one more perspective I wanna share, which IMHO we should consider
That metadata.json should be consistent to what we have done in other
catalogs, one of the reasons to have a metadata.json is to have portability
between different catalogs for example one can migrate from hive to REST and
REST to hive, but if we store part of metadata in different place and plan to
return in /config, imho it will become a concern to portability
Also please consider there are REST server implementations out there who
cache the whole metadata.json in memory / db to avoid lookup to remote store.
In rest we can add requirement for an update that we can't drop it /
meanwhile server can also double assert, if there is a concern for malicious
clients and the key update or drop, then an implicit trust relation between
catalog and client is required which means a catalog can reject the whole
update if it doesn't trust the client ? if the concern is server tampering
stuff, then a lot of things go at toss ? we requires similar trust relation for
other works as wells :
- DEFINER views
- Access Decision Exchange
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -115,14 +146,25 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
Consumer<ErrorResponse> errorHandler;
List<UpdateRequirement> requirements;
List<MetadataUpdate> updates;
+
+ TableMetadata metadataToCommit = metadata;
+ if (encryption() instanceof StandardEncryptionManager) {
Review Comment:
is it possible to add this update to the metadata before we call `commit` ?
per API contract we should try commit `metadata`
https://github.com/apache/iceberg/blob/aa14aae0111d757f8cded70b87c1d58da3fe272c/core/src/main/java/org/apache/iceberg/TableOperations.java#L45-L63
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -201,7 +254,41 @@ private boolean reconcileOnSimpleUpdate(
@Override
public FileIO io() {
- return io;
+ if (tableKeyId == null) {
+ return io;
+ }
+
+ if (encryptingFileIO == null) {
+ encryptingFileIO = EncryptingFileIO.combine(io, encryption());
+ }
+
+ return encryptingFileIO;
+ }
+
+ @Override
+ public EncryptionManager encryption() {
+ if (encryptionManager != null) {
+ return encryptionManager;
+ }
+
+ if (tableKeyId != null) {
+ if (kmsClient == null) {
+ throw new RuntimeException(
+ "Cant create encryption manager, because key management client is
not set");
+ }
+
+ Map<String, String> encryptionProperties = Maps.newHashMap();
+ encryptionProperties.put(TableProperties.ENCRYPTION_TABLE_KEY,
tableKeyId);
+ encryptionProperties.put(
+ TableProperties.ENCRYPTION_DEK_LENGTH,
String.valueOf(encryptionDekLength));
Review Comment:
any reason why we don't list these properties here :
https://iceberg.apache.org/docs/1.10.0/docs/configuration/?h=write.m#table-properties
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -241,12 +328,38 @@ private static Long
expectedSnapshotIdIfSnapshotAddOnly(List<MetadataUpdate> upd
return addedSnapshotId;
}
+ private void encryptionPropsFromMetadata(TableMetadata metadata) {
+ if (metadata == null || metadata.properties() == null) {
+ return;
+ }
+
+ encryptedKeysFromMetadata = metadata.encryptionKeys();
+
+ Map<String, String> tableProperties = metadata.properties();
+ if (tableKeyId == null) {
+ tableKeyId = tableProperties.get(TableProperties.ENCRYPTION_TABLE_KEY);
+ }
+
+ if (tableKeyId != null && encryptionDekLength <= 0) {
+ String dekLength =
tableProperties.get(TableProperties.ENCRYPTION_DEK_LENGTH);
+ encryptionDekLength =
+ (dekLength == null)
+ ? TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT
+ : Integer.parseInt(dekLength);
Review Comment:
can use PropertyUtils ?
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -152,6 +194,17 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
String.format("Update type %s is not supported", updateType));
}
+ if (base != null) {
+ Set<String> removedProps =
+ base.properties().keySet().stream()
+ .filter(key -> !metadata.properties().containsKey(key))
+ .collect(Collectors.toSet());
+
+ if (removedProps.contains(TableProperties.ENCRYPTION_TABLE_KEY)) {
+ throw new RuntimeException("Cannot remove key in encrypted table");
+ }
Review Comment:
shouldn't this be an IllegalState ? instead of RTE ?
--
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]
