[misc] Replace discontinued LocalStack with MinIO in S3 test#5680
Merged
[misc] Replace discontinued LocalStack with MinIO in S3 test#5680
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
lixmal
changed the title
lixmal
force-pushed
the
fix-s3-test-minio
branch
2 times, most recently
from
274eb15 to
79a25a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
upload-server/server/s3_test.go (1)
31-31: Consider pinning the MinIO image version for reproducible tests.Using
minio/minio:latestmay lead to flaky tests if a breaking change is introduced in a new MinIO release. Pinning to a specific version (e.g.,minio/minio:RELEASE.2025-03-12T18-04-18Z) ensures test stability across CI runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upload-server/server/s3_test.go` at line 31, The test uses an unpinned MinIO image ("Image: \"minio/minio:latest\"") which can cause flaky CI; update the Image value to a specific tagged release (e.g., "minio/minio:RELEASE.2025-03-12T18-04-18Z") in the test setup so the container used by the s3 tests is deterministic and reproducible (replace the "minio/minio:latest" literal wherever Image is set in the s3 test initialization).proxy/internal/accesslog/logger.go (1)
163-174: Consider whether a shallow copy ofMetadatais safe.
entry.Metadatais assigned directly tole.Metadatawithout copying. If the caller modifies the map after callingLogL4, the background goroutine inlog()could see inconsistent data.In practice this is likely safe because:
- Callers (e.g.,
logL4Denyintcp/router.go) create fresh maps per call- The gRPC send happens quickly in a background goroutine
If this is intentional for performance, the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/accesslog/logger.go` around lines 163 - 174, The logEntry construction assigns entry.Metadata directly to le.Metadata which can cause racey/consistency issues if the caller mutates the map after LogL4 returns; update the LogL4 path that builds logEntry (the le variable in the logEntry struct creation) to defensively copy entry.Metadata into a new map before assigning to le.Metadata (preserve keys/values), so the background goroutine in log() sees an immutable snapshot; reference functions/types: Metadata, le, logEntry, LogL4, log(), and callers like logL4Deny in tcp/router.go when making the change.shared/management/proto/proxy_service.proto (1)
109-110: Consider using an enum forcrowdsec_modeinstead of a string.The field documents specific allowed values (
"","off","enforce","observe"), but as a string it allows any value. The codebase similarly uses a string for themodefield in ProxyMapping (with values"http","tcp","udp","tls"), and no server-side validation exists for either field invalidateAccessRestrictions.If this flexibility is intentional or validation is deferred to the proxy layer, this is consistent with the codebase design. Otherwise, consider using a protobuf enum for type safety or adding validation in the management service.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/proto/proxy_service.proto` around lines 109 - 110, The crowdsec_mode field is currently an unconstrained string allowing invalid values; change it to a protobuf enum (e.g., enum CrowdSecMode { CROWDSEC_MODE_UNSPECIFIED=0; OFF=1; ENFORCE=2; OBSERVE=3; }) and update the message field to use CrowdSecMode crowdsec_mode = 5; then update all generated usages and any serialization/deserialization call sites to handle the enum, and add server-side validation in validateAccessRestrictions to explicitly accept only CROWDSEC_MODE_UNSPECIFIED/OFF/ENFORCE/OBSERVE (and likewise validate ProxyMapping.mode values if needed) so invalid values are rejected early; ensure unit tests and any clients are updated to use the new enum symbols.proxy/internal/crowdsec/registry_test.go (2)
59-66: Direct struct initialization bypasses constructor validation.
newTestRegistry()creates a*Registryby directly initializing struct fields instead of usingNewRegistry(). While this is acceptable for unit tests that need to inspect internal state, consider whetherNewRegistry()could be used for at least some tests to ensure the constructor remains exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/crowdsec/registry_test.go` around lines 59 - 66, newTestRegistry() constructs a *Registry by directly initializing fields which bypasses constructor logic; update tests to use NewRegistry() where possible so constructor validation is exercised (keep direct struct init only for cases needing internal state control). Replace calls to newTestRegistry() with NewRegistry() in tests that don't require manipulating internal refs/logger, or add a small helper that calls NewRegistry() and then adjusts internals when necessary; reference the newTestRegistry, NewRegistry, and Registry symbols to locate and change the tests.
29-42: Consider adding a test for multiple distinct service IDs.The idempotency test validates that re-acquiring the same service ID doesn't increment refs, but it doesn't test that acquiring different service IDs does increment correctly. This would strengthen coverage of the ref-counting logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/crowdsec/registry_test.go` around lines 29 - 42, The test TestRegistry_Acquire_Idempotent only checks idempotency for the same service ID; add an assertion that acquiring different IDs increases r.refs accordingly by calling r.Acquire("svc-2") (and optionally "svc-3") after the initial acquire and asserting assert.Len(t, r.refs, 2) (and 3) to verify distinct service IDs are tracked separately; use the same test registry helper newTestRegistry() and r.Acquire function and keep the existing idempotency checks.management/internals/modules/reverseproxy/service/service.go (1)
116-116: GORM serializer may be unnecessary for a plain string field.
CrowdSecModeis astringtype, but it hasgorm:"serializer:json"which is typically used for complex types like slices/maps. For a simple string, GORM handles it natively without serialization. This won't cause issues but adds unnecessary overhead.Consider removing the serializer tag
- CrowdSecMode string `json:"crowdsec_mode,omitempty" gorm:"serializer:json"` + CrowdSecMode string `json:"crowdsec_mode,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/service.go` at line 116, The struct field CrowdSecMode is a plain string but has an unnecessary GORM serializer tag; remove `gorm:"serializer:json"` from the CrowdSecMode field tag so it reads simply `json:"crowdsec_mode,omitempty"` (or omit the gorm tag entirely) to let GORM handle the string natively and avoid unneeded JSON serialization overhead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 44-45: The goxmldsig vulnerability (GHSA-479m-364c-43vc) is
introduced transitively via github.com/dexidp/dex which you currently fork as
github.com/netbirdio/dex; update the fork or add an explicit replace in go.mod
to pin github.com/russellhaering/goxmldsig to v1.6.0 or later so the vulnerable
v1.5.0 is not used: either update the netbirdio/dex dependency to a commit/tag
that references goxmldsig v1.6.0+ or add a replace directive for
github.com/russellhaering/goxmldsig => github.com/russellhaering/goxmldsig
v1.6.0 (or newer) to force the safe version.
In `@proxy/internal/crowdsec/bouncer.go`:
- Around line 150-157: Update the debug log messages that mistakenly spell
"unparseable" to "unparsable": change the format strings used in b.logger.Debugf
calls where parse failures occur (both the range deletion branch that calls
b.removePrefix(prefix) and the IP deletion branch that calls netip.ParseAddr) so
they read "skip unparsable CrowdSec ..." instead of "skip unparseable CrowdSec
..."; ensure you update all occurrences (including the other block referenced
around the b.logger.Debugf lines near the IP parsing) so codespell no longer
flags them.
- Around line 115-119: CheckIP currently returns the first matching entry from
b.prefixes which is non-deterministic because removePrefix uses swap-delete;
make range matching deterministic by ensuring prefixes are ordered by
specificity (longest prefix/mask length first) and by avoiding order-changing
swap-deletes. Concretely: when adding/updating entries for the prefix type used
in CheckIP (and the similar loops at the other locations referenced), ensure the
prefixes slice is kept sorted by prefix length (or compute a stable sorted view)
so iteration always tests most-specific prefixes first, and change removePrefix
to remove while preserving order (or maintain a stable map keyed by prefix and
derive a sorted slice for matches). Update CheckIP(), removePrefix(), and the
other matching loops (the blocks around lines 173-180 and 192-199) to iterate
the deterministic, specificity-sorted collection instead of raw b.prefixes.
In `@proxy/internal/crowdsec/registry.go`:
- Around line 51-59: The current Acquire flow inserts svcID into r.refs before
calling r.startLocked(), so if startLocked() fails the svcID remains and
subsequent Acquire() calls take the fast-path and never retry; change the logic
in Acquire (the block that checks r.refs, r.bouncer and calls startLocked()) to
only add r.refs[svcID] after startLocked() has successfully completed (or,
alternatively, call startLocked() first and on success set r.bouncer and then
add svcID), and if startLocked() returns an error ensure you do not leave svcID
in r.refs (i.e., roll back/remove the ref on error) so transient startup
failures will be retried on the next Acquire().
In `@proxy/internal/restrict/restrict.go`:
- Around line 65-79: ParseFilter currently only treats CrowdSec as active when
cfg.CrowdSec != nil, which lets an unavailable checker silently fall back to
Allow; change the hasCS logic to consider CrowdSec active whenever
cfg.CrowdSecMode is CrowdSecEnforce or CrowdSecObserve regardless of
cfg.CrowdSec being non-nil (e.g., compute hasCS := (cfg.CrowdSecMode ==
CrowdSecEnforce || cfg.CrowdSecMode == CrowdSecObserve)), then always assign
f.CrowdSecMode = cfg.CrowdSecMode and f.CrowdSec = cfg.CrowdSec so that
downstream checkCrowdSec/Registry.Acquire handling can emit crowdsec_unavailable
instead of failing open; apply the same pattern in the other similar blocks
referenced (around lines 249-258 and 277-283) where CrowdSec presence is
computed.
---
Nitpick comments:
In `@management/internals/modules/reverseproxy/service/service.go`:
- Line 116: The struct field CrowdSecMode is a plain string but has an
unnecessary GORM serializer tag; remove `gorm:"serializer:json"` from the
CrowdSecMode field tag so it reads simply `json:"crowdsec_mode,omitempty"` (or
omit the gorm tag entirely) to let GORM handle the string natively and avoid
unneeded JSON serialization overhead.
In `@proxy/internal/accesslog/logger.go`:
- Around line 163-174: The logEntry construction assigns entry.Metadata directly
to le.Metadata which can cause racey/consistency issues if the caller mutates
the map after LogL4 returns; update the LogL4 path that builds logEntry (the le
variable in the logEntry struct creation) to defensively copy entry.Metadata
into a new map before assigning to le.Metadata (preserve keys/values), so the
background goroutine in log() sees an immutable snapshot; reference
functions/types: Metadata, le, logEntry, LogL4, log(), and callers like
logL4Deny in tcp/router.go when making the change.
In `@proxy/internal/crowdsec/registry_test.go`:
- Around line 59-66: newTestRegistry() constructs a *Registry by directly
initializing fields which bypasses constructor logic; update tests to use
NewRegistry() where possible so constructor validation is exercised (keep direct
struct init only for cases needing internal state control). Replace calls to
newTestRegistry() with NewRegistry() in tests that don't require manipulating
internal refs/logger, or add a small helper that calls NewRegistry() and then
adjusts internals when necessary; reference the newTestRegistry, NewRegistry,
and Registry symbols to locate and change the tests.
- Around line 29-42: The test TestRegistry_Acquire_Idempotent only checks
idempotency for the same service ID; add an assertion that acquiring different
IDs increases r.refs accordingly by calling r.Acquire("svc-2") (and optionally
"svc-3") after the initial acquire and asserting assert.Len(t, r.refs, 2) (and
3) to verify distinct service IDs are tracked separately; use the same test
registry helper newTestRegistry() and r.Acquire function and keep the existing
idempotency checks.
In `@shared/management/proto/proxy_service.proto`:
- Around line 109-110: The crowdsec_mode field is currently an unconstrained
string allowing invalid values; change it to a protobuf enum (e.g., enum
CrowdSecMode { CROWDSEC_MODE_UNSPECIFIED=0; OFF=1; ENFORCE=2; OBSERVE=3; }) and
update the message field to use CrowdSecMode crowdsec_mode = 5; then update all
generated usages and any serialization/deserialization call sites to handle the
enum, and add server-side validation in validateAccessRestrictions to explicitly
accept only CROWDSEC_MODE_UNSPECIFIED/OFF/ENFORCE/OBSERVE (and likewise validate
ProxyMapping.mode values if needed) so invalid values are rejected early; ensure
unit tests and any clients are updated to use the new enum symbols.
In `@upload-server/server/s3_test.go`:
- Line 31: The test uses an unpinned MinIO image ("Image:
\"minio/minio:latest\"") which can cause flaky CI; update the Image value to a
specific tagged release (e.g., "minio/minio:RELEASE.2025-03-12T18-04-18Z") in
the test setup so the container used by the s3 tests is deterministic and
reproducible (replace the "minio/minio:latest" literal wherever Image is set in
the s3 test initialization).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f5fc298-9235-4e17-8e7a-36722742b656
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumshared/management/proto/proxy_service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (33)
go.modmanagement/internals/modules/reverseproxy/accesslogs/accesslogentry.gomanagement/internals/modules/reverseproxy/domain/domain.gomanagement/internals/modules/reverseproxy/domain/manager/api.gomanagement/internals/modules/reverseproxy/domain/manager/manager.gomanagement/internals/modules/reverseproxy/proxy/manager.gomanagement/internals/modules/reverseproxy/proxy/manager/controller.gomanagement/internals/modules/reverseproxy/proxy/manager_mock.gomanagement/internals/modules/reverseproxy/service/manager/l4_port_test.gomanagement/internals/modules/reverseproxy/service/service.gomanagement/internals/shared/grpc/proxy.gomanagement/internals/shared/grpc/proxy_test.goproxy/cmd/proxy/cmd/root.goproxy/internal/accesslog/logger.goproxy/internal/accesslog/middleware.goproxy/internal/auth/middleware.goproxy/internal/auth/middleware_test.goproxy/internal/crowdsec/bouncer.goproxy/internal/crowdsec/bouncer_test.goproxy/internal/crowdsec/registry.goproxy/internal/crowdsec/registry_test.goproxy/internal/proxy/context.goproxy/internal/restrict/restrict.goproxy/internal/restrict/restrict_test.goproxy/internal/tcp/router.goproxy/internal/tcp/router_test.goproxy/internal/udp/relay.goproxy/management_integration_test.goproxy/server.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/proxy_service.protoupload-server/server/s3_test.go
lixmal
force-pushed
the
fix-s3-test-minio
branch
2 times, most recently
from
dc5bb1f to
61b748a
Compare
lixmal
force-pushed
the
fix-s3-test-minio
branch
from
61b748a to
7fcd273
Compare
|
bcmmbaga
approved these changes
Mar 25, 2026
|
@lixmal It might be relevant to mention that minio has also discontinued support for their free offering |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
LocalStack discontinued the free
localstack/localstack:s3-latestimage on March 23, 2026, causing theTest_S3HandlerGetUploadURLtest to fail across all PRs and main.This replaces LocalStack with MinIO, which provides the same S3-compatible API
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Test-only change, no user-facing behavior affected.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Chores
Tests