rdblue commented on code in PR #6884: URL: https://github.com/apache/iceberg/pull/6884#discussion_r1320867941
########## core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManagerFactory.java: ########## @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.encryption; + +import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT; +import static org.apache.iceberg.TableProperties.DEFAULT_FILE_FORMAT_DEFAULT; + +import java.io.IOException; +import java.util.Map; +import org.apache.iceberg.FileFormat; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.PropertyUtil; + +public class StandardEncryptionManagerFactory implements EncryptionManagerFactory { Review Comment: After looking at how this is used in context and looking at the implementation here, I don't think that we need an `EncryptionManagerFactory`. The `StandardEncryptionManager` uses 3 arguments: the key ID, DEK length, and a KMS client. The key ID should never come from the catalog config (or else you'd be using the same key across tables) and KMS client settings and instance are intended to be shared across a catalog. There is clear separation. The only setting that may overlap is if you want to set a catalog-level DEK length default or have it at the table level. I'm skeptical about the utility of allowing both, but it should be fine: the KMS client should simply accept the table setting and override it if the catalog's setting is higher. Since we have clear separation of config, I don't think that we need this factory at all. Instead, `BaseMetastoreTableOperations` should add a constructor with an optional `kmsClient` and should then override `encryption()` like this: ```java @Override public EncryptionManager encryption() { String encryptionKeyId = current().properties().get("encryption.key-id"); if (encryptionKeyId != null) { int dataKeyLength = current().propertyAsInt("encryption.data-key-length", 16); return new StandardEncryptionManager(encryptionKeyId, dataKeyLength, kmsClient); } return new PlaintextEncryptionManager(); } ``` -- 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]
