yuqi1129 commented on code in PR #12555: URL: https://github.com/apache/gravitino/pull/12555#discussion_r3893171479
########## docs/trino-connector/catalog-iceberg.md: ########## @@ -20,6 +20,122 @@ To use Iceberg, you need: - ORC - Parquet (default) +## How Trino Reaches the Catalog + +The Gravitino Trino connector loads every `lakehouse-iceberg` catalog through the Gravitino Iceberg +REST server (IRC), regardless of the catalog's `catalog-backend`. `catalog-backend` describes how +Gravitino stores the catalog's metadata; it does not decide how the query engine reaches the data. + +This is what makes [credential vending](../security/credential-vending.md) work. Trino only consumes +vended credentials in its `rest` Iceberg catalog type — the `jdbc` and `hive_metastore` types have +nowhere to put the session token of an STS temporary credential — so a catalog with +`credential-providers=s3-token` produces no usable credential on those paths. Routing through the +IRC means every table access gets a freshly issued temporary credential over the Iceberg REST +protocol. + +The connector already connects to the Gravitino server (it is how catalogs are discovered in the +first place), so it also asks that server whether it has an Iceberg REST server running as an +[auxiliary service](../iceberg-rest-service.md) for the connector's metalake. By default, a +non-REST `lakehouse-iceberg` catalog is not registered until an endpoint is discovered or configured +explicitly. It is retried during every metadata refresh rather than silently falling back and +disabling credential vending. To retain the behavior from older connector versions, set +`gravitino.iceberg.rest-routing-enabled=false`; this skips discovery and translates the catalog's +`catalog-backend` into the corresponding native Trino Iceberg configuration. + +Only the coordinator polls the Gravitino server, so the coordinator resolves the endpoint when a +catalog is registered or refreshed and hands it to every node (coordinator and workers alike) +as part of that catalog's own definition — the same way Trino replicates any other catalog property +cluster-wide. A catalog that could not be registered before the IRC started is registered +automatically after a later discovery poll succeeds; no Trino restart is required. + +Set `gravitino.iceberg.rest-uri` to override the discovered endpoint, and it is required — not just +an override — for a standalone IRC (its own process, not the Gravitino server's auxiliary service): +the Gravitino server has no way to know a standalone IRC exists, so discovery never finds one. See +[Limitations](#limitations). + +```properties +connector.name=gravitino +gravitino.metalake=test +gravitino.uri=http://gravitino-host:8090 + +gravitino.iceberg.rest-uri=http://gravitino-host:9001/iceberg +``` + +The connector derives everything else from the catalog itself. The Gravitino catalog name is passed +as both `iceberg.rest-catalog.warehouse` and `iceberg.rest-catalog.prefix` — the Iceberg client +selects the catalog twice over, first as the query parameter of the `GET /v1/config` call that +discovers it, then as the path segment of every request after that. + +The Trino native file system is derived from the catalog's `warehouse` scheme, because vended +credentials are only consumed by Trino's native file systems: + +| Warehouse scheme | Derived properties | +|:------------------------------------------|:------------------------------------------------------------------------------------------------------| +| `s3://`, `s3a://`, `s3n://` | `fs.native-s3.enabled`, plus `s3.region`, `s3.endpoint` and `s3.path-style-access` where the catalog defines `s3-region`, `s3-endpoint` and `s3-path-style-access` | Review Comment: Format the table ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergCatalogPropertyConverter.java: ########## @@ -152,6 +248,99 @@ private Map<String, String> buildJDBCBackendProperties(Map<String, String> prope return jdbcProperties; } + private void warnOnReservedOverrides(GravitinoCatalog catalog, Map<String, String> config) { Review Comment: Can you add some comments about this method? I can't get the meaning from the method name. ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogRegister.java: ########## @@ -57,6 +60,18 @@ public class CatalogRegister { private static final String SSL_VERIFICATION_NONE = "NONE"; private static final Set<String> SSL_VERIFICATION_MODES = ImmutableSet.of(SSL_VERIFICATION_FULL, SSL_VERIFICATION_CA, SSL_VERIFICATION_NONE); + private static final Pattern SECRET_PROPERTY_PATTERN = + Pattern.compile( + "\"([^\"]*(?:credential|token|secret|password)[^\"]*)\"\\s*=\\s*'([^']*)'", + Pattern.CASE_INSENSITIVE); + + // Matches "key":"value" style secret assignments inside the serialized GravitinoCatalog JSON + // that GRAVITINO_DYNAMIC_CONNECTOR_CATALOG_CONFIG carries (e.g. "jdbc-password":"..." or + // "s3-secret-key":"..."), which SECRET_PROPERTY_PATTERN's SQL-assignment shape does not match. + private static final Pattern SECRET_JSON_PROPERTY_PATTERN = + Pattern.compile( + "\"([^\"]*(?:credential|token|secret|password)[^\"]*)\"\\s*:\\s*\"([^\"]*)\"", Review Comment: Other words like `passcode` should also be considered. Is there a better way to detect it? It's not so elegant and will always suffer from conner cases. -- 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]
