tuantran0910 commented on code in PR #62988:
URL: https://github.com/apache/doris/pull/62988#discussion_r4403520619
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergCatalogFactory.java:
##########
@@ -533,10 +534,21 @@ public static void appendRestProperties(Map<String,
String> opts, IcebergRestMet
// Timeouts: legacy fields default non-blank, so they are effectively
always emitted.
opts.put(REST_CONNECTION_TIMEOUT_MS_KEY,
rest.getConnectionTimeoutMs());
opts.put(REST_SOCKET_TIMEOUT_MS_KEY, rest.getSocketTimeoutMs());
+ putIfNotBlank(opts, CatalogProperties.FILE_IO_IMPL, rest.getIoImpl());
+ putIfNotBlank(opts, "header.x-goog-user-project",
rest.getGoogleUserProject());
+ putIfNotBlank(opts, "gcs.oauth2.token", rest.getGcsOauth2Token());
Review Comment:
Thanks, both points check out. I walked P1 along the same paths before
replying:
- FE side GCS config is HMAC only. `GcsFileSystemProperties` says the Google
JSON API is intentionally not used because it only accepts OAuth2, and its
Hadoop config falls back to `AnonymousAWSCredentialsProvider` when no keys are
set.
- BE has no OAuth2 consumer at all. `StorageAdapter.alignS3FamilyBackendMap`
emits `AWS_ACCESS_KEY`/`AWS_SECRET_KEY` and sets
`AWS_CREDENTIALS_PROVIDER_TYPE=ANONYMOUS` when both are blank, and
`S3ClientFactory` then builds `AnonymousAWSCredentialsProvider`. There is no
`gcs`/`oauth` code under `be/src`.
- Vended credentials do not close it, as you said: `CredentialUtils` keeps
the `gs.` prefix but not `gcs.`, and even a forwarded `gcs.oauth2.token` would
have nothing to consume it on BE.
So I took your second option and scoped this PR, rather than folding a BE
auth path in. The PR description now has a "Supported scope and limitations"
section and the example requires `gs.access_key` / `gs.secret_key` (GCS interop
keys). While checking this I also found the metadata-table case is not purely
FE-side: `$files`/`$manifests` tasks are deserialized and executed on BE, where
`GCSFileIO` builds its own client, so those need `iceberg.gcs.oauth2.token` or
ADC on the BE hosts. That is documented too.
One question on the follow-up you suggested: for the "google auth on gs://
without HMAC keys" case, would you prefer a hard error at CREATE CATALOG or a
warning? Doris convention looks like hard validation (Glue throws when neither
`glue.access_key` nor `glue.role_arn` is set, for the same "no default
credential chain" reason), but a hard error would also reject public-bucket and
metadata-only catalogs, so I did not add either yet. Happy to add whichever you
prefer, and a BE-native Google read path (token minting/refresh on FE plus a
Bearer path in BE) is clearly a separate change.
##########
fe/fe-connector/fe-connector-iceberg/pom.xml:
##########
@@ -112,6 +112,20 @@ under the License.
<version>${iceberg.version}</version>
</dependency>
+ <!-- GoogleAuthManager and GCSFileIO are loaded by name inside the
connector plugin. -->
+ <dependency>
+ <groupId>org.apache.iceberg</groupId>
+ <artifactId>iceberg-gcp</artifactId>
Review Comment:
Fixed in d301da585e. The plugin now carries `iceberg-gcp` and
`google-cloud-storage` at runtime scope, with `google-cloud-storage` pinned to
the 2.67.0 the FE connector bundles. `iceberg-gcp` names no Google SDK
dependency of its own (`google-cloud-storage` is compileOnly upstream and
absent from its pom), so the client `GCSFileIO` builds from the task properties
has to be named as well.
Coverage added:
- `scansAFilesMetadataTableFromASerializedTask`: a `$files` scan over a
local table, which is the shape that carries `table.io()` across the wire
(previously only `$snapshots`, a StaticDataTask, was covered).
- `deserializesTheGcsFileIoATaskCarries`: serialize an initialized
`GCSFileIO`, deserialize it through the real `IcebergSerializationCompat` path,
then build the `Storage` client from the task properties (`gcs.no-auth` so the
test does not depend on the machine's ADC). This fails at compile time if
either dependency is removed, and with a ClassNotFoundException before the fix.
Verification: 14 plugin unit tests pass; the built plugin directory (218
jars) passes `tools/be-java-plugins/check_plugin_layout.py`, and on the plugin
classpath alone the GCSFileIO deserializes and its client builds as
`StorageImpl`.
What is still not covered, same caveat as your findings: a `$files` query
against a real GCS bucket with real credentials. I do not have a GCP
environment here. I also left the "reported as a different Iceberg version"
message in `IcebergSerializationCompat` untouched, since that predates this
change and deserves its own fix; say the word if you want it in this PR.
--
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]
