feat(rest)!: make SigV4 backend optional - #2060
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for this. The split achieves its main goal: go list -deps ./catalog/rest no longer includes any aws-sdk package, and go build ./..., go vet, and go test -race ./catalog/rest/... all pass. Requesting changes because sigv4.WithAwsConfig changes how signing region/service are resolved, and the most natural migration compiles but signs with the wrong scope.
Blocking
1. sigv4.WithAwsConfig bypasses WithSigV4RegionSvc and server-provided signing overrides
WithAwsConfig(cfg, region, service) builds the signer with region/service fixed at option-construction time and installs it via WithSigner. resolveSigner returns opts.signer before it ever consults opts.sigv4Region / opts.sigv4Service. On main, createSession applied both to the explicit aws.Config.
Observed with a scratch test against this branch (credential scope from the Authorization header):
| Setup | Scope signed | main |
|---|---|---|
rest.WithSigV4RegionSvc("us-west-2","s3tables") + sigv4.WithAwsConfig(cfg{Region: eu-central-1}, "", "") |
eu-central-1/execute-api |
us-west-2/s3tables |
sigv4.WithAwsConfig(cfg, "us-east-1", "execute-api") + /v1/config override rest.signing-region=ap-south-1, rest.signing-name=s3tables |
us-east-1/execute-api |
ap-south-1/s3tables |
The first row is what existing callers will write after migrating: keep WithSigV4RegionSvc, swap rest.WithAwsConfig(cfg) for sigv4.WithAwsConfig(cfg, "", ""). It compiles, and the server then rejects the requests (403) because they are signed for the wrong service.
2. The client.region hint for S3 is lost (from reading the code, not run)
toProps (rest.go ~763-777) only emits rest.sigv4-enabled, rest.signing-*, and the client.region fallback when o.enableSigv4 is set. Using sigv4.WithAwsConfig(cfg, "us-east-1", "s3tables") alone never sets it. r.props is cloned into every loaded table's config, so S3 Tables users lose the region hint for table FileIO.
Suggested fix
Hand the backend a factory rather than a pre-built signer:
- Add
rest.WithSignerFactory(SignerFactory). - In
resolveSigner, whenenableSigv4is set, useopts.signerFactoryif present, else the registry, and build withSignerConfig{Region: opts.sigv4Region, Service: opts.sigv4Service}. Server overrides are already merged intooptsbyfetchConfigat that point. - Keep the original one-argument
sigv4.WithAwsConfig(cfg aws.Config), returningrest.WithSignerFactory(...)that usescfginstead ofLoadDefaultConfig.
Migration then really is just changing the import path. WithSigV4 / WithSigV4RegionSvc stay the single source of region/service, and toProps behaves as it does today. Please also decide (and document) whether a raw WithSigner should bypass those settings.
Non-blocking
- Behavior change: on main,
WithAwsConfigalone did not sign;WithSigV4was also required.sigv4.WithAwsConfignow always signs. The fix above removes the difference; if the current design stays, please call it out in the description andconfiguration.md. - Runtime break for
catalog.Loadusers: programs loading a REST catalog withrest.sigv4-enabled=true(orsigv4-enabledin.iceberg-go.yaml) now fail at runtime until they add the blank import. The error message is good, but this belongs in the release notes next to theWithAwsConfigremoval. - Weaker concurrency coverage: the removed
TestSigv4ConcurrentSignerssent 1 KiB POSTs through one shared catalog from many goroutines.TestConcurrentSignedCatalogRequestsbuilds a catalog per goroutine, so the signer/transport are never shared, andTestSignRequestConcurrentonly signs body-less GETs, so body hashing never runs concurrently. Suggest makingTestSignRequestConcurrentsign POSTs with bodies on one sharedsigner. - Low-value tests in
rest_internal_test.go:TestSessionTransportNoSignerDoesNotSignasserts that core doesn't setx-amz-content-sha256, but core has no code that could set it, so it can't fail; suggest deleting it.TestSessionTransportConcurrentRoundTriponly counts calls on a fake signer. TestSigV4WithoutBackendReturnsHelpfulError: the registry save/restore is a no-op. The internalresttest binary can't importsigv4(import cycle), so the registry is always empty there. Dropping it lets the test run witht.Parallel().RegisterSigner(name, nil)is accepted and only panics later inresolveSigner; suggest rejecting nil at registration.- Unclosed catalogs: catalogs created in
sigv4_internal_test.goare never closed; addt.Cleanup(func() { _ = cat.Close() }). - Doc nit:
configuration.md: "wires anrest.WithSigner" → "a".
Looks good
catalog/restno longer links the AWS SDK.- The new
GetBody == nilguard turns a nil-func panic into a clear error. - The logic that closes the cloned body and reports close errors was carried over correctly, with its tests.
- The CLI blank import adds no weight, since
catalog/gluealready pulls in the SDK. - The
execute-apifallback for the properties-only path matchesWithSigV4.
Addresses review feedback on apache#2060.
zeroshade
left a comment
There was a problem hiding this comment.
Both blocking items from my 09-28 review are addressed. sigv4.WithAwsConfig now goes through WithSignerFactory, resolveSigner builds SignerConfig from the opts after fetchConfig, and toProps behaves as on main again. TestWithAwsConfigUsesConfiguredRegionService and TestSignerFactoryReceivesResolvedRegionService pin this.
I merged the branch into current main locally: go build ./..., go vet, and the catalog/rest, catalog/rest/sigv4 (-race) and cmd/iceberg tests pass, and go list -deps ./catalog/rest has no aws-sdk packages (595 -> 510).
Before merge:
- Retitle to
feat(rest)!:and rewrite the description as release-note migration text:rest.WithAwsConfig(cfg)->sigv4.WithAwsConfig(cfg), still paired withWithSigV4*. Code usingWithSigV4/WithSigV4RegionSvc/rest.sigv4-enabledstill compiles, butNewCatalog/catalog.Loadnow return an error untilcatalog/rest/sigv4is imported. - Sequencing: #1999 merges first, then this PR rebases on it. Add
Props iceberg.Properties(the post-overrideopts.additionalProps) toSignerConfig, movestaticCredsFromPropsintocatalog/rest/sigv4, and keep thesigningOriginguard in coreRoundTrip. It needs no SDK and will then coverWithSignertoo. Port the #1999 tests along with it. cc @iremcaginyurtturk - Hit Update branch so CI runs with the modernize linter (#2072) and the Go 1.26 floor (#2077); the last run predates both.
The inline comments are nits.
| func resolveSigner(ctx context.Context, opts *options) (RequestSigner, error) { | ||
| if opts.signer != nil { | ||
| return opts.signer, nil | ||
| } | ||
|
|
||
| if !opts.enableSigv4 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Nothing pins this precedence. No test calls WithSigner, so this first branch is never exercised through NewCatalog. Nothing asserts that WithSignerFactory on its own leaves requests unsigned either, and that is the main-parity property the factory design relies on (on main, WithAwsConfig without WithSigV4 did not sign). Please add one table-driven test over resolveSigner:
WithSigner, SigV4 off: returns that signer- factory, SigV4 off: returns nil
- factory, SigV4 on: calls the factory with the opts region/service
- no factory, SigV4 on, no backend registered: returns the import error
| // SignerNameSigV4 is the scheme under which the AWS SigV4 backend registers | ||
| // itself (see catalog/rest/sigv4). It is also the value carried by the | ||
| // rest.sigv4-enabled property path. It is exported so the backend can register | ||
| // under the exact same name the core looks up, rather than a duplicated string | ||
| // literal that could silently drift. | ||
| const SignerNameSigV4 = "sigv4" |
There was a problem hiding this comment.
resolveSigner only ever looks up SignerNameSigV4 (line 113), so RegisterSigner("anything-else", f) is accepted and never consulted. The name parameter plus an exported constant read like a multi-scheme registry that does not exist. The doc is also off: rest.sigv4-enabled carries "true", not this name. Since this becomes public API, either drop the name (e.g. RegisterSigV4(factory SignerFactory)) and unexport the constant, or keep the shape and state on RegisterSigner that only SignerNameSigV4 is consulted.
| // own region. Before WithAwsConfig became a signer factory it froze the scope at | ||
| // option-construction time, so the natural migration (keep WithSigV4RegionSvc, | ||
| // swap rest.WithAwsConfig for sigv4.WithAwsConfig) silently signed for the wrong | ||
| // scope and the server rejected it with a 403. |
There was a problem hiding this comment.
These lines describe an intermediate state of this PR (a pre-built signer / three-argument WithAwsConfig) that never shipped, so they will not mean anything to readers on main. Please drop them, and the matching sentence in rest_internal_test.go:1890-1892; keep only what the test asserts. Also lines 266-267 below: TestConcurrentSignedCatalogRequests does not restore the old TestSigv4ConcurrentSigners coverage. It builds one catalog per goroutine and sends only body-less GET /v1/config. Shared-signer body hashing is now covered by TestSignRequestConcurrent, so reword or drop that claim.
| |---|---| | ||
| | Authentication | `WithCredential`, `WithOAuthToken`, `WithAuthManager`, `WithAuthURI`, `WithScope`, `WithAudience`, `WithResource` | | ||
| | AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithAwsConfig` | | ||
| | AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSigner` | |
There was a problem hiding this comment.
WithSignerFactory is missing here. It is the hook sigv4.WithAwsConfig is built on, and the one to use for a custom signer that should keep following the signing region/service.
| | AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSigner` | | |
| | AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSignerFactory`, `WithSigner` | |
Move AWS SigV4 signing out of `catalog/rest` into `catalog/rest/sigv4`, so plain REST users no longer pull in the AWS SDK. Breaking change: `rest.WithAwsConfig` is removed. Use `sigv4.WithAwsConfig(...)` and add a blank import of `catalog/rest/sigv4` to enable SigV4 signing. Follows the driver pattern established for cloud IO in apache#696.
Addresses review feedback on apache#2060.
Co-authored-by: Matt Topol <zotthewizard@gmail.com>
Carry the property-credential signing from apache#1999 into the optional sigv4 backend, now that catalog/rest no longer links the AWS SDK. SignerConfig gains a Props field holding the post-override catalog properties, and resolveSigner passes opts.additionalProps through it. staticCredsFromProps and the rest.* alias keys move into catalog/rest/sigv4, where the registered signer factory builds a static credentials provider from the s3.* / rest.* properties and falls back to the AWS default chain. An explicit sigv4.WithAwsConfig fully specifies the credentials and never consults the properties. The cross-origin signing guard stays in core RoundTrip: it is signer-agnostic, so it now also covers a signer installed verbatim via WithSigner. Tests move with the code - the staticCredsFromProps table and the props-credential end-to-end test live in the sigv4 package, while the cross-origin redirect test stays in core and drives the guard through a marking signer.
07c23d4 to
1100d3c
Compare
Moves AWS SigV4 signing out of
catalog/restand into an optionalcatalog/rest/sigv4backend, so plain REST users no longer pull in the AWS SDK.go list -deps ./catalog/restdrops from 595 packages to 510 and links noaws-sdk-go-v2.This is a breaking change. Code that signs with SigV4 still compiles, but it now needs the backend to be imported, and the explicit-config entry point moved packages:
WithSigV4/WithSigV4RegionSvc(or therest.sigv4-enabledproperty) and add a blank import of the backend,import _ "github.com/apache/iceberg-go/catalog/rest/sigv4". Without it,NewCatalog/catalog.Loadreturn an error naming the import to add, rather than silently not signing.rest.WithAwsConfig(cfg)withsigv4.WithAwsConfig(cfg), still paired withWithSigV4/WithSigV4RegionSvc; no blank import is needed in that case. The one-argumentrest.WithAwsConfig(aws.Config)is removed.WithSigV4/WithSigV4RegionSvcstay the single source of the signing region and service, including any/v1/configoverrides, so swapping the import path is the whole migration.Rebased on #1999, whose property-credential signing is carried into the backend: the registered signer builds a static credentials provider from the
s3.*keys (falling back to therest.*aliases) and otherwise uses the AWS default chain. The cross-origin signing guard stays in core, where it is signer-agnostic and now also covers a signer installed viaWithSigner.Follows the driver pattern established for cloud IO in #696, and is somewhat related to #2044.