Flink: Filter CatalogTable properties#16313
Conversation
| this.catalogProps = | ||
| catalogProps.entrySet().stream() | ||
| .filter(e -> !GlobalConfiguration.isSensitive(e.getKey())) | ||
| .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, Map.Entry::getValue)); |
There was a problem hiding this comment.
This seems like a reasonable short-term fix to me.
There was a problem hiding this comment.
ConfigurationUtils.hideSensitiveValues is even more simple.
There was a problem hiding this comment.
@pvary The isSensitive list doesn't cover all Iceberg configurations. For example, the credential keyword used for REST catalog OAuth2 client credentials or S3 access key was only added to Flink's list in a later release than the one we're using. See https://github.com/apache/flink/blob/release-2.1.1/flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java#L49
There was a problem hiding this comment.
On master which will be 2.4 later I've added. https://github.com/apache/flink/blob/6c60f4fee2a8da6990738dc0c5b479f7f2accba8/flink-core/src/main/java/org/apache/flink/configuration/GlobalConfiguration.java#L276
There was a problem hiding this comment.
This is true for ConfigurationUtils.hideSensitiveValues also...
gaborgsomogyi
left a comment
gaborgsomogyi
left a comment
There was a problem hiding this comment.
+1 from Flink side. We should add this redaction to all our SQL param outputs
| super(catalogName, defaultDatabase); | ||
| this.catalogLoader = catalogLoader; | ||
| this.catalogProps = catalogProps; | ||
| this.catalogProps = |
There was a problem hiding this comment.
If this property is needed in order to create Iceberg catalog, then filtering it here breaks CREATE TABLE LIKE functionality.
https://github.com/apache/iceberg/blob/main/flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/FlinkDynamicTableFactory.java#L190-L204
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
i think @stevenzwu had some concerns with this approach offline, blocking this for now so it doesnt get accidentally merged
6 participants
Filter out
CatalogTableproperties to contain only relevant infoClaude 4.7 generated the first version of the tests. Reviewed and simplified a bit.