SEPE-1165: Fix three bugs that stop the agent working under a monitoring system - #8
Closed
eschoeller wants to merge 3 commits into
Closed
SEPE-1165: Fix three bugs that stop the agent working under a monitoring system#8eschoeller wants to merge 3 commits into
eschoeller wants to merge 3 commits into
Conversation
init() discarded the error from zap's Build() and then called Sugar() on
the result, so any failure to construct the logger surfaced as a nil
pointer dereference during package initialisation -- before main() runs,
producing a stack trace rather than a diagnostic.
This is reachable in ordinary use. The daemon runs as pdagent and owns
/var/log/pdagent, but the CLI runs as whoever invoked it. A monitoring
system calling `pdagent nagios enqueue` does so as its own user, which
has no reason to be able to write the daemon's log, so opening it fails
and the command crashes:
panic: runtime error: invalid memory address or nil pointer dereference
go.uber.org/zap.(*Logger).Sugar(...)
github.com/PagerDuty/go-pdagent/pkg/common.init.2()
pkg/common/logging.go:22
Fall back to stderr when the log file cannot be opened, and to a no-op
logger if even that fails, so Sugar() always has a receiver. Being unable
to write a log file should not stop an event being enqueued.
Extracted into newBaseLogger() with the path as a variable so the failure
can be tested. Verified on RHEL 8: `pdagent health` as the naemon user
with APP_ENV=production returned a segfault before and returns OK now.
/var/run/pdagent was shipped in the package. /run is tmpfs, so it does not survive a reboot, and a package operation recreates it owned by root while the daemon runs as User=pdagent. Unlinking a file needs write permission on the containing directory, so the daemon could not remove its own pidfile on shutdown: pdagent[10769]: remove /var/run/pdagent/pidfile: permission denied pdagent[10865]: pidfile already exists Every subsequent start then failed, leaving the service dead after any restart. Drop the directory from the package and declare RuntimeDirectory= in the unit instead. systemd creates /run/pdagent with User/Group ownership before ExecStart and removes it on stop, which takes the pidfile with it and makes a stale pidfile impossible. Verified on RHEL 8: three consecutive restarts, all active, with /run/pdagent owned pdagent:pdagent.
`pdagent init` refuses to overwrite an existing config and exits non-zero. Package managers preserve /etc/pdagent/config.yaml across removal and upgrade, so on every upgrade the scriptlet aborted under `set -e`: Error writing config: Config File "/etc/pdagent/config.yaml" Already Exists Error in POSTIN scriptlet in rpm package oit-sepe-go-pdagent That happens before the service is enabled and started, so upgrading the package left the daemon down. Only initialise when no config is present. Verified on RHEL 8: upgrading over an existing install now leaves the service active.
Contributor
Author
|
Superseded by the recovery branch. Merging the stack bottom-up with squash merges deleted the base branches these PRs were stacked on, and GitHub auto-closes a PR whose base disappears. #5's content survived by riding along in #6's rebase; #7's and #8's did not. Both branches were intact, so the missing five commits were cherry-picked onto main unchanged and reopened as a single PR. |
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
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.
Stacked on #7. Found by installing the CI-built RPM on a real RHEL 8 cluster and driving it the way Naemon will — which is the only reason any of these surfaced. All three are inherited from upstream.
1. The agent segfaults when run as a non-root user
The whole point of this integration is Naemon invoking
pdagent nagios enqueueas thenaemonuser. That crashed:init()discarded the error fromBuild()and calledSugar()on the result:The daemon owns
/var/log/pdagent, but the CLI runs as whoever invoked it — a monitoring user has no business writing the daemon's log, soBuild()fails and the binary dies during package init, beforemain(), with a stack trace instead of a diagnostic.Now falls back to stderr, then to a no-op logger, so
Sugar()always has a receiver. Failing to open a log file shouldn't stop an event being enqueued. Extracted tonewBaseLogger()with the path as a variable so the failure is testable; four tests added.2. The daemon can't restart
/var/run/pdagentwas shipped in the package./runis tmpfs, so it doesn't survive a reboot and a package operation recreates it root-owned while the daemon runs asUser=pdagent. Unlinking needs write permission on the directory, so the daemon couldn't remove its own pidfile — and every subsequent start failed.Dropped from the package;
RuntimeDirectory=pdagentin the unit instead. systemd creates it with the right ownership beforeExecStartand removes it on stop, taking the pidfile with it. A stale pidfile becomes structurally impossible.3. Every upgrade leaves the service down
pdagent initrefuses to overwrite an existing config and exits non-zero. Package managers preserve/etc/pdagent/config.yamlacross removal and upgrade, so underset -ethe scriptlet aborted — before the service was enabled and started. Only initialise when no config is present.Verification
On both RHEL 8 peers:
pdagent healthasnaemon, production/run/pdagentownershiproot:rootpdagent:pdagentFull Go suite passes.
Upstream
All three exist upstream and are worth offering back. #1 in particular means the agent cannot be driven by any unprivileged process in production mode, which is its primary use case.