SEPE-1165: Add CI, document the fork, and fix three agent bugs - #9
Merged
Conversation
The repo had no working CI. Upstream's .circleci config does nothing on GitHub, so nothing verified build, vet, tests, vulnerabilities or the resulting packages on a pull request. Add build.yml, following the pipeline shape used in oit-sepe-naemon-build: runs on every push, with a publish job gated on tag pushes that attaches artifacts to a GitHub Release. test ── build-packages ── verify-package ── publish (tags only) The govulncheck gate is deliberate. Clearing 25 reachable CVEs is the reason this fork moved off the original dependency set, so a regression there should fail the build rather than be discovered later. verify-package asserts the packaged binary path matches the path the systemd unit and wrapper scripts invoke. That mismatch is not hypothetical -- porting to goreleaser v2 silently moved the binary to /usr/bin while everything referencing it expects /usr/local/bin, which installs cleanly and then fails to start. The check was confirmed to exit non-zero against a simulated regression, not just to pass today. It is driven from the runner rather than via `container:` so the actions run against the runner's Node rather than RHEL 8's older glibc. Drop the signs block. It expected a GPG key that does not exist, and oit-sepe-naemon-build does not sign its packages either. Document the fork in the README, including the changes made relative to upstream as Apache 2.0 section 4(b) asks for, the pipeline, and the release procedure. (cherry picked from commit 60f76f2)
Pin govulncheck to v1.6.0 via an env var so a tool release cannot change the gate's behaviour without a visible commit. This does not weaken coverage: the vulnerability database is fetched from vuln.go.dev at run time, so a pinned binary still picks up newly published advisories. Replace `rpm=$(ls /dist/*x86_64.rpm)` in the layout check. The glob silently produced an empty value when nothing matched, which would have turned a missing package into a confusing rpm error rather than a clear failure. Now every built RPM is checked rather than one representative arch, and an empty dist/ fails loudly. Verified: the check reports OK across all three RPMs, and exits non-zero against an empty dist/. (cherry picked from commit cff8809)
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.
(cherry picked from commit c908e52)
/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. (cherry picked from commit 70afed8)
`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. (cherry picked from commit ee65bcb)
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
1 participant
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.
Recovery PR. Combines the work from #7 and #8, which were auto-closed without merging.
What happened
Merging the five-deep stack bottom-up with squash merges went wrong.
gh pr merge --delete-branchremoved each base branch as it merged, and GitHub auto-closes any PR whose base branch is deleted. #5's content survived because it had been rebased into #6's branch and rode along; #7's and #8's did not.Nothing was lost — both branches were intact. The five genuinely-missing commits are cherry-picked here onto current main, unchanged and in original order:
All five applied without conflict. Lesson for next time: with a stack this deep, either merge without
--delete-branchor retarget each child before merging its parent.Contents
CI —
build.ymlfollowing theoit-sepe-naemon-buildpipeline shape: runs on every push, publish job gated on tags.govulncheckis a deliberate gate — clearing 25 reachable CVEs is why this fork moved off the original dependency set.verify-packageasserts the packaged binary path matches what the systemd unit invokes, verified to exit non-zero against a simulated regression rather than merely passing today.Three agent bugs, all found by installing the CI-built RPM on a real RHEL 8 cluster and driving it the way Naemon does:
init()discarded zap's error then calledSugar()/var/run/pdagentshipped in the package/runis tmpfs; recreated root-owned, daemon can't unlink its own pidfile, every restart failsRuntimeDirectory=pdagentpdagent initexits non-zero when config existsset -eaborts postinstall before the service starts — every upgrade leaves the daemon downVerification
Build, vet and the full Go suite pass. Verified on both RHEL 8 peers:
pdagent healthasnaemon, production/run/pdagentownershiproot:rootpdagent:pdagentContent re-verified after cherry-pick: CI workflow, pinned govulncheck, README fork docs,
signsremoved, logger fix,RuntimeDirectory,/var/rununpackaged, postinstall guard — all present.