Conversation
|
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:
📝 WalkthroughWalkthroughAdds optional shared-jail support to the fail2ban middleware: a package-level shared jail registry with mutex, a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant H1 as Handler A
participant Global as globalJails (map + globalMu)
participant Jail as Fail2Ban Jail
participant H2 as Handler B
Client->>H1: HTTP request
H1->>Global: lock globalMu / lookup or create jail (key: name+config hash)
Global-->>H1: return Jail
H1->>Jail: check ban / record failure
Jail-->>H1: banned? / allow or block
H1->>Client: response
Note over Client,H2: Subsequent request via another middleware instance
Client->>H2: HTTP request
H2->>Global: lock globalMu / lookup jail
Global-->>H2: return same Jail (if SharedJail)
H2->>Jail: check ban
Jail-->>H2: banned? / allow or block
H2->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fail2ban_test.go`:
- Around line 519-537: Two table-driven test cases share the same "name" value
which makes test failures ambiguous; update the second test case's name field to
a unique, descriptive string (e.g., differentiate by "shared jail" vs "separate
jail" or include an index) so subtest names are distinct. Locate the duplicate
"name" fields in the table of test cases (entries that include cfg: &Config{...}
and rules.Rules) and rename the second occurrence to a different string, keeping
the rest of the test data (cfg, handlerStatus, expectStatus, expectStatusSecond)
unchanged.
- Around line 554-565: The subtest must use a unique jail key per run and
fail-fast on setup errors: replace the fixed string "fail2ban_test" passed to
New(...) with a deterministic per-subtest key (e.g., include test.name or
t.Name()) to avoid shared-jail state leakage, and check the returned error from
both calls to New (handler, err := New(...)) and (handler2, err := New(...)) and
call t.Fatalf (or t.Fatal) if err != nil so the test stops immediately on setup
failure.
In `@fail2ban.go`:
- Around line 148-157: The current SharedJail logic reuses an existing f2b from
globalJails keyed only by `name`, which silently ignores new
`rules`/`allowNetIPs`; modify the logic in the block that references
`SharedJail`, `jailKey`, `globalMu`, `globalJails`, and `fail2ban.New` to detect
config drift or include the config in the key: either compute a stable
fingerprint (hash) of `rules` and `allowNetIPs` and use `name + ":" +
fingerprint` as the `jailKey`, or load the existing `f2b` and compare its stored
config to the incoming `rules`/`allowNetIPs` and return an error if they differ;
ensure access is still protected by `globalMu` and that creation still calls
`fail2ban.New(rules, allowNetIPs)` when no matching keyed jail exists.
In `@README.md`:
- Around line 55-56: The markdown heading "### Shared Jail" is not followed by a
blank line which triggers markdownlint rule MD022; fix it by inserting a single
empty line immediately after the "### Shared Jail" heading in README.md so the
heading is separated from the following paragraph content and the lint rule
passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 931bb141-7ec2-44ba-a0b0-a1ac529e51ce
📒 Files selected for processing (3)
README.mdfail2ban.gofail2ban_test.go
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 `@fail2ban.go`:
- Around line 152-153: The json.Marshal error is ignored when creating jailKey
(keyBytes, _ := json.Marshal(config)); instead capture the error and handle it:
call json.Marshal into (keyBytes, err), and if err != nil either return the
error up from the function or log it and fail fast so you don't compute md5.Sum
on nil bytes; then proceed to compute jailKey := fmt.Sprintf("%s-%x", name,
md5.Sum(keyBytes)) only when keyBytes is valid. Ensure you reference the
existing names json.Marshal, keyBytes, err, jailKey, md5.Sum, name, and config
when implementing the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db6e6350-b6b9-4e20-951b-af811c0788b8
📒 Files selected for processing (3)
README.mdfail2ban.gofail2ban_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fail2ban_test.go`:
- Around line 560-562: The cleanup is deleting the wrong key: tests currently
call delete(globalJails, jailKey) but shared jails are stored under a
"name-hash" key. While holding globalMu, replace the deletion to remove the
actual shared-jail key (e.g., compute and delete fmt.Sprintf("%s-%s", name,
hash) or call the existing helper used to build the shared key) instead of
jailKey so globalJails is truly cleaned up; ensure you use the same name and
hash values that were used to create the shared jail.
In `@fail2ban.go`:
- Around line 150-163: The shared-jail map (globalJails) grows unbounded because
every distinct config hash creates a new entry in the block inside the
config.SharedJail branch; change this to use a bounded-retention strategy: when
inserting a new jailKey in the else branch (where fail2ban.New is called and
globalJails[jailKey] = f2b) maintain eviction metadata (e.g., lastUsed timestamp
or an LRU list) and evict old/stale entries when the map exceeds a configured
max size or when entries age out (implement eviction under globalMu to preserve
concurrency safety); ensure accesses (lookup, update of lastUsed, insert, and
eviction) are all protected by globalMu and consider a background cleanup
goroutine to remove expired entries if using TTLs so stale jails are reclaimed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6121fed7-e7ce-43c6-90ce-c9dc88875b90
📒 Files selected for processing (2)
fail2ban.gofail2ban_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fail2ban_test.go`:
- Around line 559-563: The cleanup deletes the wrong key because the test builds
jailKey as t.Name()-hash(config) but New(name) calls getOrCreateSharedJail which
stores a key as name-hash(config) again, so the stored map key is
name-hash(config)-hash(config) (i.e. double-hashed); fix by making the test use
the same key-generation logic as getOrCreateSharedJail: either call
New(t.Name()) (so stored key matches computed jailKey) or compute the full
stored key for deletion by calling the same formatting and sha256.Sum256 on the
config used by New(); update references to jailKey, New(), getOrCreateSharedJail
and globalJails accordingly so the delete removes the actual map entry.
In `@README.md`:
- Around line 57-64: The README incorrectly says routers "using the same
middleware name" share a jail; update the docs to state that sharedJail requires
both the same middleware name and identical middleware configuration because
fail2ban constructs the key as jailKey := fmt.Sprintf("%s-%x", name,
sha256.Sum256([]byte(fmt.Sprintf("%v", config)))) in fail2ban.go, so only
routers with the same name and an identical config hash will share the jail;
mention the use of sha256.Sum256 and the config hashing so readers know
configuration must match exactly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b4a9c65a-dd38-4485-a4c3-ad4611e166d9
📒 Files selected for processing (3)
README.mdfail2ban.gofail2ban_test.go
| jailKey := fmt.Sprintf("%s-%x", t.Name(), sha256.Sum256([]byte(fmt.Sprintf("%v", test.cfg)))) | ||
|
|
||
| globalMu.Lock() | ||
| delete(globalJails, jailKey) | ||
| globalMu.Unlock() |
There was a problem hiding this comment.
Test cleanup is ineffective - deletes wrong key from globalJails.
The test computes jailKey as t.Name()-<configHash> and tries to delete it from globalJails. However, when New() is called with jailKey as the name parameter, getOrCreateSharedJail computes the actual stored key as:
// fail2ban.go line 36
jailKey := fmt.Sprintf("%s-%x", name, sha256.Sum256([]byte(fmt.Sprintf("%v", config))))This means the actual key in globalJails is jailKey-<anotherHash>, not jailKey. The delete operation is a no-op.
🛠️ Proposed fix
Either pass t.Name() directly to New() and compute the full shared key for cleanup:
- jailKey := fmt.Sprintf("%s-%x", t.Name(), sha256.Sum256([]byte(fmt.Sprintf("%v", test.cfg))))
+ middlewareName := t.Name()
+ sharedKey := fmt.Sprintf("%s-%x", middlewareName, sha256.Sum256([]byte(fmt.Sprintf("%v", test.cfg))))
globalMu.Lock()
- delete(globalJails, jailKey)
+ delete(globalJails, sharedKey)
globalMu.Unlock()
@@
- handler, err := New(t.Context(), next, test.cfg, jailKey)
+ handler, err := New(t.Context(), next, test.cfg, middlewareName)
require.NoError(t, err)
- handler2, err := New(t.Context(), next, test.cfg, jailKey)
+ handler2, err := New(t.Context(), next, test.cfg, middlewareName)
require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fail2ban_test.go` around lines 559 - 563, The cleanup deletes the wrong key
because the test builds jailKey as t.Name()-hash(config) but New(name) calls
getOrCreateSharedJail which stores a key as name-hash(config) again, so the
stored map key is name-hash(config)-hash(config) (i.e. double-hashed); fix by
making the test use the same key-generation logic as getOrCreateSharedJail:
either call New(t.Name()) (so stored key matches computed jailKey) or compute
the full stored key for deletion by calling the same formatting and
sha256.Sum256 on the config used by New(); update references to jailKey, New(),
getOrCreateSharedJail and globalJails accordingly so the delete removes the
actual map entry.
| By default, each middleware instance has its own jail. To share a jail across multiple routers using the same middleware name, set `sharedJail` to `true`: | ||
|
|
||
| ```yml | ||
| testData: | ||
| sharedJail: true | ||
| ``` | ||
|
|
||
| When enabled, all routers using the same named middleware will share the same jail, allowing bans to propagate across subdomains. |
There was a problem hiding this comment.
Documentation understates the shared jail matching criteria.
The documentation states that routers "using the same middleware name" will share a jail. However, the implementation in fail2ban.go (line 36) uses both the middleware name and a hash of the entire config as the key:
jailKey := fmt.Sprintf("%s-%x", name, sha256.Sum256([]byte(fmt.Sprintf("%v", config))))This means routers must have both the same name and identical configuration to share a jail. Consider updating the documentation to clarify this behavior.
📝 Suggested documentation fix
-By default, each middleware instance has its own jail. To share a jail across multiple routers using the same middleware name, set `sharedJail` to `true`:
+By default, each middleware instance has its own jail. To share a jail across multiple routers using the same middleware name and identical configuration, set `sharedJail` to `true`:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 57 - 64, The README incorrectly says routers "using
the same middleware name" share a jail; update the docs to state that sharedJail
requires both the same middleware name and identical middleware configuration
because fail2ban constructs the key as jailKey := fmt.Sprintf("%s-%x", name,
sha256.Sum256([]byte(fmt.Sprintf("%v", config)))) in fail2ban.go, so only
routers with the same name and an identical config hash will share the jail;
mention the use of sha256.Sum256 and the config hashing so readers know
configuration must match exactly.
tomMoulard
left a comment
tomMoulard
left a comment
There was a problem hiding this comment.
Hello @xavernitsch,
Thanks for your contribution!
I am a bit concerned that your solution might cover a small section of it's potential.
For me, the best way to implement this feature would be to implement a mechanism to store the jail on a backend (local, redis, ...), and be able to load the jail content from those backend.
For your use case, it would mean that you setup all your middlewares to use the same backend (e.g., a local file), therefore all middlewares use the same jail.
For other use cases, it would allow sharing jails, loading jails from other providers (e.g., CrowdSec) (#39), or even get jail update notifications (#50, #174, #238).
|
Hi @tomMoulard, Thank you for the feedback! I completely see your point. Moving towards a pluggable storage backend (Redis, File, etc.) makes total sense for horizontal scaling and integration with tools like CrowdSec. For my use case (single Traefik instance, low traffic), a full Redis setup would be overkill, but I agree that the architecture should support it from the start. How about we compromise on an extensible configuration structure? I could refactor the PR to introduce a "Storage Provider" abstraction. The configuration could look like this: # Proposed config structure
sharedJail:
provider: "inMemory" # Default for now
key: "my-shared-jail"
This way:
What do you think about this approach? If you agree, I'll refactor the implementation to follow this provider pattern. |
|
Sure, I would rather go with this kind of configuration: jail:
- type: "inMemory" # only implement this jail type in this PR
configuration:
path: "./jail.csv" # Could be optional with a good default |
2 participants
With the help of AI I was able to implement the shared jail feature #256
I hope you and some others like it
Summary by CodeRabbit
New Features
Documentation
Tests
Chores