Skip to content

AWS: Remove deprecated S3 signer classes and properties - #17627

Open
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:1.12deprecation-aws-signer
Open

AWS: Remove deprecated S3 signer classes and properties#17627
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:1.12deprecation-aws-signer

Conversation

@dramaticlly

@dramaticlly dramaticlly commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

1. Remove five orphaned classes

S3SignRequest, S3SignResponse, S3SignRequestParser, S3SignResponseParser, S3ObjectMapper.

AI Disclosure

Model: Claude Opus 5 (1M context)
Platform/Tool: Claude Code
Human Oversight: reviewed
Prompt Summary: split #16449 into smaller self-contained PRs; verify each group compiles and tests green standalone; revise per reviewer feedback to drop the required-endpoint change

@dramaticlly
AWS: Remove deprecated S3 signer request/response classes
31afe33 
These five classes were deprecated for removal in 1.12.0 and are now
orphaned: they reference only each other and have no remaining callers
in main or test sources. S3V4RestSignerClient uses the RemoteSign*
equivalents.

- S3SignRequest, S3SignResponse
- S3SignRequestParser, S3SignResponseParser
- S3ObjectMapper
@github-actions github-actions Bot added the AWS label Aug 13, 2026
@dramaticlly

dramaticlly commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@nastra and @adutra can you help review if this align with your expectation in #15451?

uros-b
uros-b reviewed Aug 13, 2026
View reviewed changes
Comment thread aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java
properties().containsKey(RESTCatalogProperties.SIGNER_ENDPOINT),
"S3 signer endpoint (%s) is required",
RESTCatalogProperties.SIGNER_ENDPOINT);
}

@uros-b uros-b Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in this file is stricter than a pure deprecation-alias removal. Beyond stripping the s3.signer.uri/s3.signer.endpoint aliases, it also removes the working "v1/aws/s3/sign" implicit default (S3_SIGNER_DEFAULT_ENDPOINT), meaning users who set s3.remote-signing-enabled=true without ever configuring an explicit endpoint now fail at construction with IllegalArgumentException. The 1.11.0 LOG.warn, the @deprecated "no replacement, remove in 1.12.0" on S3_SIGNER_DEFAULT_ENDPOINT, and the updated SIGNER_ENDPOINT Javadoc ("If remote signing has been requested, this must be set") all point toward requiring the endpoint in 1.12.0

@uros-b uros-b Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leaving a note here, although I see this has already been noted in the description - thank you!

@nssalian nssalian added this to the Iceberg 1.12.0 milestone Aug 15, 2026
@adutra

adutra commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Hi @dramaticlly thanks for tackling this!

Unfortunately things have changed since #15451 :

When I last modified S3V4RestSignerClient in #15451, the consensus back then was that the signer endpoint could be dynamically generated by the server. One of the reasons for this idea was that servers could wish to encode authorization data in the signer endpoint URL. Therefore, we agreed back then that the endpoint path must be provided by the server, as there is no good default.

However in #16822, things changed in the opposite direction. Following this feedback, we decided instead that the signer endpoint would be "just" a regular endpoint, and its URL could be deterministically inferred as follows:

<catalog base uri>/v1/{prefix}/namespaces/{namespace}/tables/{table}/sign

(The same PR decided that server-specific data would from now on be communicated separately to signer clients via the new RemoteSigningConfig object.)

As a consequence, signer.endpoint and signer.uri were themselves deprecated in the spec, as now the client is capable of inferring the endpoint from the catalog URI + prefix + table identifier.

That's why I think this PR should not require signer.endpoint to be set. I'm OK with removing the deprecated classes and deprecated fields, but I'd prefer that you leave the signer.endpoint changes out of it. I am planning to provide a PR very shortly to introduce support for RemoteSigningConfig in the Java code, and that will also take care of signer.endpoint.

Would that work for you?

Thanks!

dramaticlly added a commit to dramaticlly/iceberg that referenced this pull request Aug 17, 2026
@dramaticlly
AWS: Keep the default S3 signer endpoint
35e9e02 
Per review feedback on apache#17627, restores S3_SIGNER_DEFAULT_ENDPOINT and the
default it provides for RESTCatalogProperties.SIGNER_ENDPOINT, reverting the
"TODO change to required in 1.12.0" half of the previous commit.

Since apache#16822, the signer endpoint is a regular endpoint whose URL the client
can infer from the catalog URI, prefix and table identifier, so requiring it
to be configured is the wrong direction. Server-specific data is now carried
by RemoteSigningConfig instead, and support for that in the Java client will
handle the signer endpoint separately.

The removal of the legacy s3.signer.uri and s3.signer.endpoint properties is
unaffected. This drops signerEndpointIsRequired and restores the
default-endpoint case in signerUriResolution, and reverts the SIGNER_ENDPOINT
additions to the TestS3FileIOProperties tests, which no longer need it.
@dramaticlly dramaticlly changed the title AWS: Remove deprecated class and require SIGNER_ENDPOINT AWS: Remove deprecated S3 signer classes and properties Aug 17, 2026
@dramaticlly

dramaticlly commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Hi @dramaticlly thanks for tackling this!

Unfortunately things have changed since #15451 :

When I last modified S3V4RestSignerClient in #15451, the consensus back then was that the signer endpoint could be dynamically generated by the server. One of the reasons for this idea was that servers could wish to encode authorization data in the signer endpoint URL. Therefore, we agreed back then that the endpoint path must be provided by the server, as there is no good default.

However in #16822, things changed in the opposite direction. Following this feedback, we decided instead that the signer endpoint would be "just" a regular endpoint, and its URL could be deterministically inferred as follows:

<catalog base uri>/v1/{prefix}/namespaces/{namespace}/tables/{table}/sign

(The same PR decided that server-specific data would from now on be communicated separately to signer clients via the new RemoteSigningConfig object.)

As a consequence, signer.endpoint and signer.uri were themselves deprecated in the spec, as now the client is capable of inferring the endpoint from the catalog URI + prefix + table identifier.

That's why I think this PR should not require signer.endpoint to be set. I'm OK with removing the deprecated classes and deprecated fields, but I'd prefer that you leave the signer.endpoint changes out of it. I am planning to provide a PR very shortly to introduce support for RemoteSigningConfig in the Java code, and that will also take care of signer.endpoint.

Would that work for you?

Thanks!

Thanks @adutra for your pointer and context. I updated this PR accordingly to remove deprecated class and switch S3_SIGNER_URI and S3_SIGNER_ENDPOINT to its counterpart in RESTCatalogProperties. Can you please take another look?

adutra
adutra approved these changes Aug 17, 2026
View reviewed changes

@adutra adutra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changing the PR scope @dramaticlly ! It looks good to me, let's see what @nastra or @danielcweeks think.

@nastra
nastra requested a review from danielcweeks August 18, 2026 05:52
@adutra adutra mentioned this pull request Aug 18, 2026
Open
@adutra

adutra commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

FYI my PR is here: #17709
It will overlap with this one; I suggest we merge this one first, since it's approved already.

adutra added a commit to adutra/iceberg that referenced this pull request Aug 18, 2026
@adutra
put back orphaned classes (they are removed by apache#17627)
54914fc
@dramaticlly
dramaticlly force-pushed the 1.12deprecation-aws-signer branch from 35e9e02 to 31afe33 Compare August 18, 2026 20:16
@dramaticlly
dramaticlly requested review from adutra and nastra August 18, 2026 20:24
@manuzhang manuzhang mentioned this pull request Aug 19, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielcweeks danielcweeks Awaiting requested review from danielcweeks
@adutra adutra Awaiting requested review from adutra
@nastra nastra Awaiting requested review from nastra
1 more reviewer
@uros-b uros-b uros-b left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AWS

Projects

None yet

Milestone

Iceberg 1.12.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dramaticlly @adutra @nastra @uros-b @nssalian