sdaberdaku opened a new pull request, #17862: URL: https://github.com/apache/iceberg/pull/17862
Closes #17860. `RemoveOrphanFiles` lists the table location through the Hadoop `FileSystem` API, with a configuration built from the Spark session: ```java // DeleteOrphanFilesSparkAction.java this.hadoopConf = new SerializableConfiguration(spark.sessionState().newHadoopConf()); ``` Everything else in the procedure reaches storage through `table.io()`, which the catalog built. When a catalog assumes a role of its own to reach its storage, those are two different principals, and the listing is performed as the cluster identity. On a multi-account setup that means `expire_snapshots` succeeds and `remove_orphan_files` gets a 403 on the same table, seconds apart, naming an IAM principal that appears nowhere in the catalog configuration. #17860 has the full report. `SparkUtil.hadoopConfCatalogOverrides` already existed to give a catalog its own Hadoop configuration, but no action or procedure used it, so there was no per-catalog knob for this path at all. This change: - derives the S3A assume-role settings from the catalog's own `client.assume-role.arn`, so the role does not have to be restated in Hadoop's namespace. Because the configuration is per catalog there is nothing to scope by bucket. - keeps the identity resolved for the session as `fs.s3a.assumed.role.credentials.provider`, so it remains what signs the `AssumeRole` call. Without that, S3A falls back to its `SimpleAWSCredentialsProvider` default and IRSA or instance-profile clusters have no credentials for the STS call. - leaves `spark.sql.catalog.<name>.hadoop.*` applied afterwards, so per-catalog S3A settings still win over the derived ones. - adds `DeleteOrphanFilesSparkAction.catalogName(...)`, set by `RemoveOrphanFilesProcedure` from `tableCatalog().name()`, so the action picks up that configuration. Catalogs that declare no assume-role arn are untouched. For catalogs that do declare one, this is a behaviour change: their listing starts going through that role rather than through the cluster identity. That is the point of the fix, and it matches what the rest of the procedure already does, but it is worth calling out for reviewers. ### Tests - `TestSparkUtil` covers the derivation, the preserved inner provider, precedence of an explicit `hadoop.*` override, and a catalog with no role. - `TestRemoveOrphanFilesAction3.catalogHadoopConfOverridesApplyToListing` registers a `FileSystem` for a scheme under that catalog's `hadoop.*` only, and lists a table location on that scheme, so it passes only when the catalog's configuration reaches the walk. Verified against `spark/v4.1`: both classes green, and reverting the derivation turns the two `TestSparkUtil` cases that assert it red. If the direction is agreed I will follow up with the v4.0 and v3.5 backports. -- 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]
