fix(s3): remove hardcoded dummy credentials, add miniotesting#593
fix(s3): remove hardcoded dummy credentials, add miniotesting#593Dav-14 wants to merge 3 commits into
Conversation
The s3AWSModule unconditionally injected dummy static credentials when an endpoint override was set, overriding any real credentials provided via IAM flags. This broke S3 operations against MinIO (or any S3-compatible store that requires auth) in integration tests and local dev. Also adds a reusable miniotesting package for spinning up MinIO containers in tests, with helpers for bucket creation and S3 client construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughRemoves conditional dummy credentials injection in S3 FX wiring and adds a new MinIO testing suite: a MinioServer helper, Docker-backed startup, Ginkgo helper, and integration tests exercising S3 client creation and object operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Pool as Docker Pool
participant Container as MinIO Container
participant SDK as AWS SDK / s3.Client
Test->>Pool: CreateMinioServer(opts)
Pool->>Container: Start minio/minio with env creds
Container->>Container: Health check /live/health returns 200
Container-->>Test: Endpoint mapped (host:port)
Test->>SDK: s3.NewClient(endpoint override, pathStyle)
Test->>SDK: CreateBucket / PutObject / GetObject
SDK->>Container: HTTP S3 requests (path-style)
Container-->>SDK: 200 OK / object data
SDK-->>Test: Operation results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
==========================================
+ Coverage 28.34% 28.76% +0.42%
==========================================
Files 180 182 +2
Lines 8493 8572 +79
==========================================
+ Hits 2407 2466 +59
- Misses 5971 5989 +18
- Partials 115 117 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/testing/platform/miniotesting/minio_test.go (1)
34-44: This test doesn't actually coverCreateMinioServerdefaults.
TestDefaultConfigconstructs aconfigliteral in-place using theDefault*constants and asserts they equal the same constants – it only verifies thatassert.Equalworks. To actually guard against regressions in the defaults applied byCreateMinioServer, assert againstsrv(or on acfgproduced by applying zero options), e.g.:-func TestDefaultConfig(t *testing.T) { - cfg := &config{ - accessKey: DefaultAccessKey, - secretKey: DefaultSecretKey, - tag: "latest", - } - - assert.Equal(t, "minioadmin", cfg.accessKey) - assert.Equal(t, "minioadmin", cfg.secretKey) - assert.Equal(t, "latest", cfg.tag) -} +func TestDefaultConfig(t *testing.T) { + assert.Equal(t, DefaultAccessKey, srv.AccessKey) + assert.Equal(t, DefaultSecretKey, srv.SecretKey) + assert.Equal(t, DefaultRegion, srv.Region) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testing/platform/miniotesting/minio_test.go` around lines 34 - 44, TestDefaultConfig currently builds a config literal and only compares constants; instead call CreateMinioServer (with no options / zero opts) to obtain the server (or the config it constructs) and assert the server's effective config fields equal DefaultAccessKey, DefaultSecretKey and "latest". Update TestDefaultConfig to invoke CreateMinioServer, extract the resulting config (or srv.cfg) and assert on those fields (accessKey, secretKey, tag) rather than comparing the constants to themselves.pkg/testing/platform/miniotesting/minio.go (2)
103-114: Use an HTTP client with a timeout and drain the body for the readiness probe.
http.Getuseshttp.DefaultClient, which has no timeout. If the MinIO container accepts the TCP connection but stalls the HTTP response (not uncommon during early startup), the check goroutine can block indefinitely and prevent dockertest's retry loop from making progress. Also, closing the body without draining it prevents keep-alive reuse on subsequent retries (minor).🛡️ Proposed fix
CheckFn: func(ctx context.Context, resource *dockertest.Resource) error { endpoint := fmt.Sprintf("http://127.0.0.1:%s", resource.GetPort("9000/tcp")) - resp, err := http.Get(endpoint + "/minio/health/live") + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint+"/minio/health/live", nil) + if err != nil { + return err + } + client := &http.Client{Timeout: 2 * time.Second} + resp, err := client.Do(req) if err != nil { return err } - _ = resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("minio health check returned %d", resp.StatusCode) } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testing/platform/miniotesting/minio.go` around lines 103 - 114, In the CheckFn readiness probe replace the blocking http.Get with a short-timeout http.Client and perform the request using http.NewRequestWithContext(ctx, ...) so the probe respects the provided context; set a client.Timeout (e.g. 5s), call client.Do(req), and on success always io.Copy(io.Discard, resp.Body) before resp.Body.Close() to drain the body and allow connection reuse; keep the same endpoint construction using resource.GetPort("9000/tcp") and return errors as before.
93-115: Consider exposing console port (9001) and guardWithTag()against empty strings.Two issues with
CreateMinioServer:
WithTag("")will pass an empty string directly toRunOptions.Tag, overriding the default"latest". Guard against empty strings by falling back to"latest"in the option.- Port exposure for
9000/tcprelies on implicit behavior; if you plan to expose the console port (9001/tcp), add explicit port configuration toHostConfigOptionsor document that only the API port is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testing/platform/miniotesting/minio.go` around lines 93 - 115, Change CreateMinioServer so WithTag("") cannot overwrite the default tag: when building RunOptions.Tag (the value passed from WithTag) treat empty string as "latest" and set RunOptions.Tag = "latest" if the option value is empty; also explicitly expose the MinIO console port by adding port configuration to RunOptions.HostConfigOptions (or equivalent HostConfig/PortBindings) to include "9001/tcp" in addition to "9000/tcp" so both the API and console are available, leaving the existing CheckFn/health check intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/testing/platform/miniotesting/minio_test.go`:
- Around line 34-44: TestDefaultConfig currently builds a config literal and
only compares constants; instead call CreateMinioServer (with no options / zero
opts) to obtain the server (or the config it constructs) and assert the server's
effective config fields equal DefaultAccessKey, DefaultSecretKey and "latest".
Update TestDefaultConfig to invoke CreateMinioServer, extract the resulting
config (or srv.cfg) and assert on those fields (accessKey, secretKey, tag)
rather than comparing the constants to themselves.
In `@pkg/testing/platform/miniotesting/minio.go`:
- Around line 103-114: In the CheckFn readiness probe replace the blocking
http.Get with a short-timeout http.Client and perform the request using
http.NewRequestWithContext(ctx, ...) so the probe respects the provided context;
set a client.Timeout (e.g. 5s), call client.Do(req), and on success always
io.Copy(io.Discard, resp.Body) before resp.Body.Close() to drain the body and
allow connection reuse; keep the same endpoint construction using
resource.GetPort("9000/tcp") and return errors as before.
- Around line 93-115: Change CreateMinioServer so WithTag("") cannot overwrite
the default tag: when building RunOptions.Tag (the value passed from WithTag)
treat empty string as "latest" and set RunOptions.Tag = "latest" if the option
value is empty; also explicitly expose the MinIO console port by adding port
configuration to RunOptions.HostConfigOptions (or equivalent
HostConfig/PortBindings) to include "9001/tcp" in addition to "9000/tcp" so both
the API and console are available, leaving the existing CheckFn/health check
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c08424ef-7c40-4e82-ab80-afbf73930787
📒 Files selected for processing (4)
pkg/fx/storagefx/s3.gopkg/testing/platform/miniotesting/ginkgo.gopkg/testing/platform/miniotesting/minio.gopkg/testing/platform/miniotesting/minio_test.go
| )) | ||
| } | ||
| return loadOptions | ||
| return []func(*config.LoadOptions) error{optFn} |
There was a problem hiding this comment.
if we do that we break the regular dev env for banking bridge right?
minIO is only s3 isn't it?
There was a problem hiding this comment.
Good catch. Here's how this is addressed:
Credentials: The dummy credentials (dummy/dummy/dummy) were removed because they silently overrode real IAM credentials passed via --aws-access-key-id/--aws-secret-access-key flags. For dev with MinIO, credentials now need to be provided explicitly — either via CLI flags or env vars (AWS_ACCESS_KEY_ID=minioadmin, etc.). The config.LoadDefaultConfig() won't crash without credentials, but S3 calls will fail at runtime if none are resolved through the AWS default chain.
Path style: Was hardcoded to true whenever an endpoint override was set. Now it's split:
- Real AWS S3 (no endpoint override): always
UsePathStyle = true - S3-compatible services (endpoint override like MinIO): configurable via new
--s3-bucket-path-styleflag (defaultfalse)
So for banking bridge dev env with MinIO, the fix is to pass --aws-access-key-id=minioadmin --aws-secret-access-key=minioadmin --s3-bucket-path-style=true (or equivalent env vars).
Path style is always enabled for real AWS S3 (no endpoint override). For S3-compatible services (endpoint override), it is now configurable via the new --s3-bucket-path-style flag (default: false). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/testing/platform/miniotesting/minio.go`:
- Around line 3-7: The readiness probe uses http.Get without honoring the
provided ctx or any timeout (causing potential CI hangs); change the probe to
build the request with http.NewRequestWithContext(ctx, "GET", url, nil) and
execute it via an http.Client that either respects ctx or has a reasonable
Timeout, ensure you call client.Do(req) instead of http.Get, handle errors and
close resp.Body, and use the existing dockertest context variable (ctx) used by
the surrounding function (e.g., the MinIO startup/wait helper) so dockertest
retries/cancels properly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b32865e-fcb7-436f-abf3-21660b94fb21
📒 Files selected for processing (4)
pkg/fx/storagefx/s3.gopkg/storage/s3/module.gopkg/testing/platform/miniotesting/minio.gopkg/testing/platform/miniotesting/minio_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/storage/s3/module.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/fx/storagefx/s3.go
- pkg/testing/platform/miniotesting/minio_test.go
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify readiness probes are not using context-free http.Get calls.
rg -n -C3 '\bhttp\.Get\s*\(' --iglob 'minio.go'
rg -n -C5 'NewRequestWithContext|WithTimeout|http\.Client\{.*Timeout' --iglob 'minio.go'Repository: formancehq/go-libs
Length of output: 648
🏁 Script executed:
head -20 pkg/testing/platform/miniotesting/minio.goRepository: formancehq/go-libs
Length of output: 507
🏁 Script executed:
sed -n '100,115p' pkg/testing/platform/miniotesting/minio.goRepository: formancehq/go-libs
Length of output: 585
Bind the readiness probe to the dockertest context.
Line 107 uses http.Get without a timeout or the provided ctx. If MinIO's mapped port accepts the connection but stalls before responding, this can hang CI indefinitely instead of allowing dockertest to retry or cancel cleanly.
🛠️ Proposed fix
import (
"context"
"fmt"
"net/http"
+ "time"
"github.com/aws/aws-sdk-go-v2/aws" CheckFn: func(ctx context.Context, resource *dockertest.Resource) error {
endpoint := fmt.Sprintf("http://127.0.0.1:%s", resource.GetPort("9000/tcp"))
- resp, err := http.Get(endpoint + "/minio/health/live")
+ reqCtx, cancel := context.WithTimeout(ctx, 2*time.Second)
+ defer cancel()
+
+ req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, endpoint+"/minio/health/live", nil)
+ if err != nil {
+ return err
+ }
+
+ resp, err := http.DefaultClient.Do(req)
if err != nil {
return err
}
- _ = resp.Body.Close()
+ defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
return fmt.Errorf("minio health check returned %d", resp.StatusCode)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/testing/platform/miniotesting/minio.go` around lines 3 - 7, The readiness
probe uses http.Get without honoring the provided ctx or any timeout (causing
potential CI hangs); change the probe to build the request with
http.NewRequestWithContext(ctx, "GET", url, nil) and execute it via an
http.Client that either respects ctx or has a reasonable Timeout, ensure you
call client.Do(req) instead of http.Get, handle errors and close resp.Body, and
use the existing dockertest context variable (ctx) used by the surrounding
function (e.g., the MinIO startup/wait helper) so dockertest retries/cancels
properly.
2 participants
Summary
pkg/fx/storagefx/s3.go— thes3AWSModuleunconditionally injectedStaticCredentialsProvider("dummy", "dummy", "dummy")when an endpoint override was set, overriding real credentials from IAM flags. This broke S3 operations against MinIO or any auth-requiring S3-compatible store.--s3-bucket-path-styleflag. Path style is always enabled for real AWS S3 (no endpoint override). For S3-compatible services (endpoint override), it's now explicitly configurable instead of being hardcoded.pkg/testing/platform/miniotesting— reusable package for spinning up MinIO containers in tests, following the same pattern aslocalstacktestingandpgtesting.Why
The dummy credentials were intended for LocalStack (which doesn't need auth), but they silently overwrote real IAM credentials when used with MinIO. This caused
InvalidAccessKeyIderrors in integration tests of services liketerraform-hcp-proxy.Credential configuration
Credentials are no longer magically injected. They must be provided explicitly via:
--aws-access-key-id/--aws-secret-access-keyAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYPath-style addressing
UsePathStyle = true--s3-bucket-path-styleflag (default:false)Test plan
S3ModuleFromFlagswith real MinIO credentials and path-style flag🤖 Generated with Claude Code