-
Notifications
You must be signed in to change notification settings - Fork 12
Do not bail off if LAPI is unreachable - rather retry! #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this PR is needed.
If crowdsec is not available (either during nginx startup or at runtime), the bouncer will simply log an error and keep retrying every UPDATE_INTERVAL
, and existing decisions will still be applied.
Did you observe a different behavior ?
From my testing, the only way the bouncer can make nginx fail at startup is if the configuration file does not exist: I'd rather keep this as a fatal error, as this is not something that can be recovered from automatically.
Also, crowdsec_nginx.conf
should be kept as simple as possible: any retrying logic should be implemented in https://github.com/crowdsecurity/lua-cs-bouncer: the library is common to both nginx and openresty, while this file is specific to nginx.
-- ## FIX 1: Use pcall and a global flag for safer initialization ## | ||
-- This ensures that if CrowdSec fails to start, the other blocks will not run and cause errors. | ||
cs = require "crowdsec" | ||
local config_path = "/data/crowdsec/crowdsec.conf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path is specific to your installation.
-- This ensures that if CrowdSec fails to start, the other blocks will not run and cause errors. | ||
cs = require "crowdsec" | ||
local config_path = "/data/crowdsec/crowdsec.conf" | ||
local user_agent = "crowdsec-npmplus-bouncer/v1.1.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the user agent ?
local config_path = "/data/crowdsec/crowdsec.conf" | ||
local user_agent = "crowdsec-npmplus-bouncer/v1.1.3" | ||
|
||
local ok, err = pcall(cs.init, config_path, user_agent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way init can return an error is if the configuration file does not exist, which needs to be kept as a fatal error, as it's not recoverable.
else | ||
cs.Allow(ngx.var.remote_addr) | ||
end | ||
if _G.CROWDSEC_INIT_FAILED then return end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WAF should be allowed to run, even if LAPI is down.
ngx.log(ngx.INFO, "Initializing stream mode for worker " .. tostring(ngx.worker.id())) | ||
cs.SetupStream() | ||
if _G.CROWDSEC_INIT_FAILED then | ||
ngx.log(ngx.WARN, "[Crowdsec] Bouncer is disabled due to initialization failure.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a default configuration, nginx does not write warning to the error log, so users are very unlikely to see this message.
I have a remote LAPI accessible at https://crowdsec.local.example.com. Whenever I am tinkering and the proxy settings made it unreachable etc., the bouncer crashes and bails, and never retries even if the LAPI was momentarily unreachable.
With this code, it will not bail out and live with stale decisions on transient connection loss to a remote LAPI