Core, REST, AWS: Implement RemoteSigningConfig by adutra · Pull Request #17709 · apache/iceberg

adutra · 2026-08-18T12:36:39Z

This PR implements the changes introduced to the REST spec by #16822. In particular, it brings support for the newly-introduced RemoteSigningConfig object.

This PR also removes signing-related elements that were deprecated for removal in 1.12.0: the legacy signer properties in S3V4RestSignerClient, as well as the S3-specific classes that are now orphaned: S3SignRequest, S3SignResponse, S3SignRequestParser, S3SignResponseParser and S3ObjectMapper.

This PR may overlap with #17627. I suggest merging #17627 first, then I will update this one accordingly.

This PR implements the changes introduced to the REST spec by apache#16822. In particular, it brings support for the newly-introduced `RemoteSigningConfig` object. This PR also removes signing-related elements that were deprecated for removal in 1.12.0: the legacy signer properties in `S3V4RestSignerClient`, as well as the S3-specific classes that are now orphaned: `S3SignRequest`, `S3SignResponse`, `S3SignRequestParser`, `S3SignResponseParser` and `S3ObjectMapper`.

adutra · 2026-08-18T12:57:05Z

@nastra @dramaticlly FYI

danielcweeks · 2026-08-18T16:08:15Z

@adutra This doesn't look like it needs to be stacked on the other PR. Can you please split them up?

danielcweeks · 2026-08-18T16:09:10Z

 * under the License.
 */
-package org.apache.iceberg.aws.s3.signer;
+package org.apache.iceberg.rest.signing;


I don't think we need to create a new package just for two classes. This makes more sense to just put under org.apache.iceberg.rest

danielcweeks · 2026-08-18T16:24:08Z

  private final Set<Endpoint> supportedEndpoints;
  private final Map<String, String> catalogProperties;
  private final Object hadoopConf;
+  private final RemoteSigningConfig remoteSigningConfig;


Why are we adding this to the REST Table? This should only be necessary for the FileIO creation which comes through the table ops.

Because RESTTableScan creates a dedicated FileIO when FetchPlanningResultResponse.credentials() is non empty.

I assume the only reason we need that is the PlanID, though? We're not actually creating a new config, it's the same as at the time of load.

danielcweeks · 2026-08-18T16:27:21Z

  private final ParserContext parserContext;
  private final Map<String, String> catalogProperties;
  private final Object hadoopConf;
+  private final RemoteSigningConfig remoteSigningConfig;


Same here, I don't think this is necessary. The table ops/fileio is already configured for this. If you're doing remote signing, we don't need to reconfigure at this point (also it would just be applying the same configuration that was loaded from the catalog).

ok, I think I see now that this is all to plumb through the scan ID for remote scanning. Need to think about this. It looks like we're reserializing the config?

Yes, I went with the option of serializing the config. That's way easier then passing down the RemoteSigningConfig object through the various FileIO layers until it reaches the signer. Also, that would require creating a serializable impl of RemoteSigningConfig (for Kryo mostly) and also introducing a new SupportsRemoteSigning marker interface to distinguish which FileIO impls support remote signing.

danielcweeks · 2026-08-18T16:44:45Z

+  public static String toJson(RemoteSigningConfig credential) {
+    return toJson(credential, false);
+  }


Suggested change

public static String toJson(RemoteSigningConfig credential) {

return toJson(credential, false);

}

public static String toJson(RemoteSigningConfig config) {

return toJson(config, false);

}

Credential?

danielcweeks · 2026-08-18T16:52:42Z

  private FileIO scanFileIO(List<Credential> storageCredentials) {
    ImmutableMap.Builder<String, String> builder =
-        ImmutableMap.<String, String>builder().putAll(catalogProperties);
+        ImmutableMap.<String, String>builder()


This is a little awkward because we're configuring the remote signing even when it's not configured. Can't we just add the configuration from the provided operations (e.g. `putAll(operations.io().properties()))?

Then I don't think we need to push the config all the way down.

I asked myself the same question. We could even get rid of the catalogProperties field.

But that would be a semantic change: operations.io().properties() contains 3 layers of properties: catalog properties + table properties (from the load-table-result config) + remote signing properties (set by the catalog).

However, the current code (inadvertently?) constructs the scan FileIO using just the catalog properties, and nothing else.

So, operations.io().properties() indeed contains the remote signing properties that we want, but it also contains the table properties.

IOW, your suggestion would make table properties available to the scan FileIO, while today they aren't.

If we just want to include the remote signing properties, we can check if endpoint is set (required) and then just extract the signing properties.

You can then ignore the rest of the config, though I didn't think we included all the table properties in the IO config (need to look into that).

danielcweeks · 2026-08-18T16:57:11Z

+ * <p>They are not intended for user-facing configuration, and may be removed or changed in future
+ * releases without notice.
+ */
+public final class RemoteSigningProperties {


I don't think we need a whole new properties class for three properties. Seems like we should just put these in RESTCatalog properties (we'd be removing two? there anyway)

But they are not meant to be user-facing (as in: they cannot appear in catalog or table configs). Are you OK with that?

If we want to protect them, we shouldn't expose them as public. I assume the reason we don't is because of the package boundaries. However, documenting it is sufficient, creating a new class doesn't make them any less accessible and still relies on documentation to tell people not to use them.

adutra · 2026-08-18T17:03:48Z

@adutra This doesn't look like it needs to be stacked on the other PR. Can you please split them up?

Hmm in this case we'd need @dramaticlly to change their PR #17627. They'd need to revert the changes to S3V4RestSignerClient.

Are you OK with that @dramaticlly ?

In this case we could split as follows:

AWS: Remove deprecated S3 signer classes and properties #17627 removes orphaned classes only
Core, REST, AWS: Implement RemoteSigningConfig #17709 (this PR) updates S3V4RestSignerClient

UPDATE: I already reverted the removal of orphaned classes in this PR.

dramaticlly · 2026-08-18T20:18:42Z

@adutra This doesn't look like it needs to be stacked on the other PR. Can you please split them up?

Hmm in this case we'd need @dramaticlly to change their PR #17627. They'd need to revert the changes to S3V4RestSignerClient.

Are you OK with that @dramaticlly ?

In this case we could split as follows:

AWS: Remove deprecated S3 signer classes and properties #17627 removes orphaned classes only

Core, REST, AWS: Implement RemoteSigningConfig #17709 (this PR) updates S3V4RestSignerClient

UPDATE: I already reverted the removal of orphaned classes in this PR.

Thanks @adutra for the coordination of the PR. I think it make sense for my #17627 to only focus on removing of deprecated class and leave S3V4RestSignerClient and its unit test unchanged. Updated and please take another look.

github-actions Bot added core AWS labels Aug 18, 2026

Simplified design

60ed16e

adutra force-pushed the remote-signing-config-java branch from ff9f453 to 60ed16e Compare August 18, 2026 12:49

adutra mentioned this pull request Aug 18, 2026

AWS: Remove deprecated S3 signer classes and properties #17627

Open

danielcweeks self-requested a review August 18, 2026 16:04

danielcweeks reviewed Aug 18, 2026

View reviewed changes

review

4c62d5d

danielcweeks reviewed Aug 18, 2026

View reviewed changes

put back orphaned classes (they are removed by apache#17627)

54914fc

nssalian added this to the Iceberg 1.12.0 milestone Aug 18, 2026

Uh oh!

Conversation

adutra commented Aug 18, 2026

Uh oh!

adutra commented Aug 18, 2026

Uh oh!

danielcweeks commented Aug 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dramaticlly commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielcweeks Aug 18, 2026 •

edited

Loading

danielcweeks Aug 18, 2026 •

edited

Loading

adutra commented Aug 18, 2026 •

edited

Loading