fix(s3): replace deprecated EndpointResolverWithOptionsFunc in initConfig - #1392
fix(s3): replace deprecated EndpointResolverWithOptionsFunc in initConfig#1392pujitha24 wants to merge 2 commits into
Conversation
…nfig /kind cleanup # Motivation `initConfig` in pkg/api/server/v1alpha2/log/s3.go built the S3 client's custom-endpoint support (used for S3-compatible backends such as MinIO) with `aws.EndpointResolverWithOptionsFunc` and `config.WithEndpointResolverWithOptions`. Both are deprecated in aws-sdk-go-v2 in favor of `BaseEndpoint`, and the code already carried `//nolint:staticcheck` markers to suppress the deprecation warnings. # Approach Set `o.BaseEndpoint` directly on `s3.Options` inside the `s3.NewFromConfig` functional options, alongside the existing `o.UsePathStyle = true`, only when `S3_ENDPOINT` is configured. This is the migration path the SDK itself documents (see the deprecation comment on `s3.Options.EndpointResolver` in vendor/github.com/aws/aws-sdk-go-v2/service/s3/options.go). The `S3_HOSTNAME_IMMUTABLE` config value that used to feed the removed resolver's `aws.Endpoint.HostnameImmutable` field is no longer read. This is not a behavior change: `UsePathStyle` was hardcoded to `true` both before and after this change, and per vendor/github.com/aws/aws-sdk-go-v2/service/s3/serialize_immutable_hostname_bucket.go, when `UsePathStyle` is true the bucket is always placed in the request path regardless of `HostnameImmutable`'s value, so the flag never had an observable effect in this codebase. `S3_REGION` continues to flow into both the client region and the base-endpoint signing region via `config.WithRegion`, so signing is unaffected. # Validation - `go build ./...` - `go test ./pkg/api/server/v1alpha2/log/...` — passes, including a new `TestInitConfig` covering both the custom-endpoint and default-endpoint branches of `initConfig`. - `gofmt -l` and `go vet ./pkg/api/server/v1alpha2/log/...` — clean. ```release-note NONE ``` Fixes tektoncd#986 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
There was a problem hiding this comment.
Pull request overview
Updates Tekton Results’ S3 log backend initialization to align with current aws-sdk-go-v2 guidance by replacing deprecated endpoint resolver APIs with s3.Options.BaseEndpoint for custom S3-compatible endpoints.
Changes:
- Simplifies
initConfigto always load the default AWS config and setso.BaseEndpointwhenS3_ENDPOINTis provided. - Removes the deprecated
EndpointResolverWithOptionsFunc/WithEndpointResolverWithOptionspath and associated//nolint:staticchecksuppressions. - Adds
TestInitConfigto validate both custom-endpoint and default-endpoint branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/api/server/v1alpha2/log/s3.go | Migrates custom endpoint handling to s3.Options.BaseEndpoint and simplifies AWS config loading. |
| pkg/api/server/v1alpha2/log/s3_test.go | Adds unit coverage for initConfig behavior with/without S3_ENDPOINT. |
Comments suppressed due to low confidence (1)
pkg/api/server/v1alpha2/log/s3_test.go:200
- Same as the "with custom endpoint" case: provide dummy
S3_ACCESS_KEY_ID/S3_SECRET_ACCESS_KEYvalues so this test does not depend on when the AWS SDK chooses to validate credentials.
c := &server.Config{
S3_REGION: "us-east-1",
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return s3.NewFromConfig(awsConfig, func(o *s3.Options) { | ||
| o.UsePathStyle = true | ||
| if len(cfg.S3_ENDPOINT) > 0 { | ||
| o.BaseEndpoint = aws.String(cfg.S3_ENDPOINT) | ||
| } |
There was a problem hiding this comment.
Good catch. I went with the lighter option you suggested — documenting that it's ignored rather than removing the field — to avoid breaking existing deployments that already set S3_HOSTNAME_IMMUTABLE in their config. I added a comment on the field in pkg/api/server/config/config.go explaining it's deprecated/ignored (since UsePathStyle is always true, so the bucket is always placed in the path regardless of this value), and updated the description in cmd/api/README.md to say the same. I left config/base/env/config and test/e2e/gcs-emulator.yaml as-is since they just set it to false, which remains harmless.
| c := &server.Config{ | ||
| S3_REGION: "us-east-1", | ||
| S3_ENDPOINT: "https://minio.example.com", | ||
| } |
There was a problem hiding this comment.
You're right, thanks. I added dummy S3_ACCESS_KEY_ID/S3_SECRET_ACCESS_KEY values to the server.Config in both TestInitConfig subtests ("with custom endpoint" and "without custom endpoint") in pkg/api/server/v1alpha2/log/s3_test.go, so the test doesn't rely on the SDK not validating credentials.
Document that S3_HOSTNAME_IMMUTABLE is deprecated and ignored (the S3 client always uses path-style addressing), instead of silently dropping a config value that used to have an effect. Also set dummy S3 credentials in TestInitConfig so the test doesn't depend on the AWS SDK's credential validation timing. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
Document that S3_HOSTNAME_IMMUTABLE is deprecated and ignored (the S3 client always uses path-style addressing), instead of silently dropping a config value that used to have an effect. Also set dummy S3 credentials in TestInitConfig so the test doesn't depend on the AWS SDK's credential validation timing. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
/easycla |
|
/retest |
|
/assign @divyansh42 |
b3db972 to
fb5cdf9
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
/kind cleanup
Motivation
initConfigin pkg/api/server/v1alpha2/log/s3.go built the S3 client'scustom-endpoint support (used for S3-compatible backends such as MinIO)
with
aws.EndpointResolverWithOptionsFuncandconfig.WithEndpointResolverWithOptions. Both are deprecated inaws-sdk-go-v2 in favor of
BaseEndpoint, and the code already carried//nolint:staticcheckmarkers to suppress the deprecation warnings.Approach
Set
o.BaseEndpointdirectly ons3.Optionsinside thes3.NewFromConfigfunctional options, alongside the existingo.UsePathStyle = true, only whenS3_ENDPOINTis configured. This isthe migration path the SDK itself documents (see the deprecation
comment on
s3.Options.EndpointResolverinvendor/github.com/aws/aws-sdk-go-v2/service/s3/options.go).
The
S3_HOSTNAME_IMMUTABLEconfig value that used to feed the removedresolver's
aws.Endpoint.HostnameImmutablefield is no longer read.This is not a behavior change:
UsePathStylewas hardcoded totrueboth before and after this change, and per
vendor/github.com/aws/aws-sdk-go-v2/service/s3/serialize_immutable_hostname_bucket.go,
when
UsePathStyleis true the bucket is always placed in the requestpath regardless of
HostnameImmutable's value, so the flag never hadan observable effect in this codebase.
S3_REGIONcontinues to flowinto both the client region and the base-endpoint signing region via
config.WithRegion, so signing is unaffected.Validation
go build ./...go test ./pkg/api/server/v1alpha2/log/...— passes, including a newTestInitConfigcovering both the custom-endpoint anddefault-endpoint branches of
initConfig.gofmt -landgo vet ./pkg/api/server/v1alpha2/log/...— clean.Fixes #986
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com