Skip to content

feat: switch S3 client back to aws-sdk-go-v2 - #74

Open
cgrard wants to merge 2 commits into
siderolabs:mainfrom
cgrard:feat/aws-sdk-s3
Open

cgrard wants to merge 2 commits into
siderolabs:mainfrom
cgrard:feat/aws-sdk-s3

Conversation

@cgrard

@cgrard cgrard commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Moves the S3 client back to aws-sdk-go-v2 and fixes the root cause that led to minio-go in 0b3984d: since service/s3 v1.73 the SDK sends flexible checksums by default, and many S3-compatible providers reject them.

 CreateClientWithCustomEndpoint
-  minio.New(endpoint, creds: env | IAM | ~/.aws/credentials)
-  BucketLookupPath if USE_PATH_STYLE=true, otherwise auto (path-style for non-AWS hosts anyway)
+  config.LoadDefaultConfig            # full AWS credential chain
+  if CUSTOM_S3_ENDPOINT
+    BaseEndpoint, UsePathStyle
+    checksums only when required
 PushSnapshot
-  minio PutObject
+  transfermanager.UploadObject       # same checksum setting, not inherited from the client

USE_PATH_STYLE with a custom endpoint:

value before (minio-go) after
unset auto, so path-style for non-AWS hosts path-style
true path-style path-style
false auto, so path-style for non-AWS hosts virtual-hosted-style

It has no effect on AWS S3, where there is no custom endpoint.

Also included:

  • Dependencies: all bumped (talos v1.14.5, cosi runtime v1.16.3, age v1.3.2, compress v1.20.3); minio-go is dropped entirely.
  • Age keys: blank recipient keys are no longer produced. GetServiceConfig returned ["", ""] when no key was set, and "age1a, age1b" failed to parse because of the space.
  • Integration test: minio/minio is no longer pullable from Docker Hub, so the test is switched to versity/versitygw (Apache-2.0). It is also fixed for Talos 1.14 (dropped the hostDNS patch) and for recent Docker (container IP read from the bridge network).

Evidence

  • Before: TestPushSnapshotCustomEndpoint fails with unexpected checksum header X-Amz-Checksum-Crc32 (SDK defaults applied to the upload).
    After: passes, request goes to /bucket/prefix/snap.db with no x-amz-checksum-* header and no trailing checksum.
  • Before: TestGetServiceConfig/defaults fails with AgeRecipientPublicKey: []string{"", ""}.
    After: all 5 cases pass.
  • Before: integration test fails at setup: pull access denied for minio/minio.
    After: TZ=UTC go test -tags integration ./internal/integration/ passes (snapshot taken from a Talos 1.14 node, compressed, encrypted, uploaded, listed back).
  • make lint-golangci-lint lint-gofumpt lint-govulncheck lint-markdown: clean, no vulnerabilities found.
  • Unit coverage goes from 0 % (no unit tests) to about 17 %.

Merge Danger

Door: two-way

The only behaviour change for existing users is USE_PATH_STYLE=false, which now really selects virtual-hosted-style. Unset and true behave as before. One edge case: a custom endpoint pointing at a Google or Aliyun host used virtual-hosted-style under minio-go's auto mode; it now needs USE_PATH_STYLE=false.

Blast Radius: uploads

Every backup goes through this client. Exercised here against versitygw and an httptest server, not against real AWS, Backblaze, R2 or Ceph.

Notes for maintainers:

  • go.mod now requires go 1.26.8 (pulled in by dependencies) while the kres Makefile pins GO_VERSION 1.26.1. A make rekres is probably needed.
  • feature/s3/transfermanager is the replacement for the deprecated feature/s3/manager, but it is still v0.x.
  • The integration test only passes with TZ=UTC: the key regex expects a Z suffix. This predates this PR and is left unchanged.

@github-project-automation github-project-automation Bot moved this to To Do in Planning Oct 27, 2026
@talos-bot talos-bot moved this from To Do to In Review in Planning Oct 27, 2026
Replace minio-go with aws-sdk-go-v2 for S3 uploads and fix the issue that originally motivated the move to minio-go: since service/s3 v1.73 the SDK sends flexible checksums (CRC32, trailing checksums) by default, which many S3-compatible providers reject. With a custom endpoint, checksums are now only sent when the operation requires them, including in the transfer manager which does not inherit the client setting.

- restore the full AWS credential chain (profiles, assume role, SSO, credential_process), as documented in the README
- upload with feature/s3/transfermanager (multipart for large files)
- USE_PATH_STYLE now defaults to path-style with a custom endpoint, matching what minio-go did for non-AWS hosts, and "false" selects virtual-hosted-style, which was not possible before
- ignore blank and space-padded age recipient keys
- bump dependencies (talos v1.14.5, cosi runtime v1.16.3, age v1.3.2, compress v1.20.3) and drop minio-go
- integration test: use versitygw instead of the minio/minio image, which is no longer available on Docker Hub, drop the hostDNS patch rejected by Talos 1.14 and read the container IP from the bridge network (empty NetworkSettings.IPAddress on recent Docker)
- add unit tests for the S3 client and GetServiceConfig

Signed-off-by: Cedric Grard <cedric.grard@eurofiber.com>
minio-go silently fell back to us-east-1 when no region was set, the AWS
SDK does not: without AWS_REGION, uploads to an S3-compatible endpoint
failed with "A region must be set when sending requests to S3", after the
snapshot was taken.

Default to us-east-1 with a custom endpoint, as most S3-compatible
providers ignore the region, and return an explicit error for AWS S3.

Signed-off-by: Cedric Grard <cedric.grard@eurofiber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

1 participant