crowdsec-firewall-bouncer: restart on config changes - #30351
Conversation
Formality Check: FailedWe checked this pull request against the contribution guidelines. Here is what needs your attention: 🛑 CRITICAL ERRORS
|
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The reload -> restart switch for the crowdsec config trigger looks correct and the diagnosis in the PR body is consistent with what the init script does; two inline notes, neither of which blocks a merge on its own — the main one is that the degraded reload path is still reachable through the other trigger and through a manual reload.
Generated by Claude Code
| service_triggers() { | ||
| procd_add_reload_trigger crowdsec-firewall-bouncer | ||
| procd_add_config_trigger "config.change" "crowdsec" /etc/init.d/crowdsec-firewall-bouncer reload | ||
| procd_add_config_trigger "config.change" "crowdsec" /etc/init.d/crowdsec-firewall-bouncer restart |
There was a problem hiding this comment.
This redirects one entry point, but the broken reload path stays reachable. The script defines no reload_service(), so with USE_PROCD=1 reload() in rc.common falls through to plain start — which re-runs init_nftables() (nft delete table ip crowdsec + recreate) without ever going through stop/service_stopped, and procd only respawns the instance if the instance parameters changed. Since the command line is identical across reloads, that is exactly the state described in the PR body: table replaced, dynamic sets gone, nothing to repopulate them.
So /etc/init.d/crowdsec-firewall-bouncer reload typed by hand, the reload_config ubus event, and the procd_add_reload_trigger on line 18 all still take that path. The PR body's "the explicit reload command remains available through the existing reload trigger" reads as if reload is a working alternative, but by this analysis it is the defect itself.
Would it make sense to also make the fallback safe, e.g. adding
reload_service() {
restart
}so every reload entry point gets the same lifecycle, rather than only the crowdsec config trigger?
Generated by Claude Code
| @@ -16,7 +16,7 @@ TABLE6="crowdsec6" | |||
|
|
|||
| service_triggers() { | |||
| procd_add_reload_trigger crowdsec-firewall-bouncer | |||
There was a problem hiding this comment.
nit: pre-existing, but since this PR is about the trigger set it is worth a look. _procd_add_reload_trigger expands to a config.change trigger on the UCI package named here, i.e. /etc/config/crowdsec-firewall-bouncer. No package in the feed installs that file — this one installs /etc/config/crowdsec and net/crowdsec installs the same path — so this trigger never fires and the line below is the only live one.
If that reading is right, this line could be dropped (or pointed at crowdsec, which would then duplicate the line below) as part of the same cleanup.
Generated by Claude Code
The current
config.changetrigger invokesreloadfor changes to theCrowdSec UCI configuration.
With the nftables backend, this can leave the bouncer running without its
dynamically populated CrowdSec sets after a configuration change. In
testing,
reloadreplaced the nftables table and rotated the bouncer childprocess, but the dynamic sets were not recreated or populated afterward.
The CAPI set remained absent across multiple update cycles.
A full
restartcorrectly tears down the previous generation and starts afresh one. The new generation recreates the per-origin sets and repopulates
the decisions from LAPI.
Change the
config.changetrigger fromreloadtorestartsoconfiguration changes follow that lifecycle.
Reproduction/verification on OpenWrt 25.12:
the procd-managed outer PID remained the same.
sets and hydrated 31,940 decisions.
sets, and authenticated LAPI access remained healthy.
This intentionally changes only the config-change lifecycle; the explicit
reload command remains available through the existing reload trigger.