Stop self-invoking sudo in the Makefile (#36)

claude commented

2026-07-08 16:39:20 +01:00

(Migrated from gitlab.keyop.co.uk)

Closes #36.

Both check-build-requirements and install used to self-elevate via sudo, which is counter-intuitive for a caller without passwordless sudo configured.

check-build-requirements now prints the exact install command for the detected package manager and exits 1, rather than calling sudo apt-get/dnf/yum install itself.
install drops its build prerequisite and every internal sudo prefix. It guards on id -u being 0 ("try: sudo make install") and on dist/nfq_forwarder/dist/iptables already existing ("run 'make build' first, as yourself"). This also directly addresses the "sane environment" half of the ticket: sudo make install's install: build prerequisite used to run the entire recipe - including build, which needs the full dev toolchain (goreleaser, etc.) - under sudo's own environment, which commonly resets PATH/HOME. Splitting the unprivileged build step away from the privileged install(1)/setcap calls avoids needing any PATH-preservation workaround at all.

Verified end to end, not just reasoned about: non-root make install fails with guidance, root make install without a prior make build fails with guidance, and a full make build && sudo make install succeeds - binary/iptables copied, capabilities set correctly (confirmed via getcap), PAM file installed.

CI is unaffected - .gitlab-ci.yml's before_script already installs libpam0g-dev directly (no sudo, since the CI job already runs as root), so check-build-requirements never reaches its failure branch there.

Closes #36. Both `check-build-requirements` and `install` used to self-elevate via `sudo`, which is counter-intuitive for a caller without passwordless sudo configured. - `check-build-requirements` now prints the exact install command for the detected package manager and exits 1, rather than calling `sudo apt-get/dnf/yum install` itself. - `install` drops its `build` prerequisite and every internal `sudo` prefix. It guards on `id -u` being 0 ("try: sudo make install") and on `dist/nfq_forwarder`/`dist/iptables` already existing ("run 'make build' first, as yourself"). This also directly addresses the "sane environment" half of the ticket: `sudo make install`'s `install: build` prerequisite used to run the *entire* recipe - including `build`, which needs the full dev toolchain (goreleaser, etc.) - under sudo's own environment, which commonly resets `PATH`/`HOME`. Splitting the unprivileged `build` step away from the privileged `install`(1)/`setcap` calls avoids needing any PATH-preservation workaround at all. **Verified end to end**, not just reasoned about: non-root `make install` fails with guidance, root `make install` without a prior `make build` fails with guidance, and a full `make build` && `sudo make install` succeeds - binary/iptables copied, capabilities set correctly (confirmed via `getcap`), PAM file installed. CI is unaffected - `.gitlab-ci.yml`'s `before_script` already installs `libpam0g-dev` directly (no `sudo`, since the CI job already runs as root), so `check-build-requirements` never reaches its failure branch there.

claude commented

2026-07-08 16:39:20 +01:00

(Migrated from gitlab.keyop.co.uk)

requested review from @danny

claude commented

2026-07-08 16:39:20 +01:00

(Migrated from gitlab.keyop.co.uk)

assigned to @claude

danny commented

2026-07-08 19:03:36 +01:00

(Migrated from gitlab.keyop.co.uk)

I would prefer this gated similarly to the 'install' target - If we're running as root, the fixes should be applied (without 'sudo').
If we're not running as root and:

everything is installed Ok - no error.
Some things are not installed - warn about all of them and suggest re-running 'sudo make check-build-requirements' to fix.

I would prefer this gated similarly to the 'install' target - If we're running as root, the fixes should be applied (without 'sudo'). If we're not running as root and: 1) everything is installed Ok - no error. 2) Some things are not installed - warn about all of them and suggest re-running 'sudo make check-build-requirements' to fix.

danny commented

2026-07-08 19:03:49 +01:00

(Migrated from gitlab.keyop.co.uk)

See comments.

danny commented

2026-07-08 19:03:49 +01:00

(Migrated from gitlab.keyop.co.uk)

requested changes

claude commented

2026-07-08 19:08:42 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit 6c88be75e0

mentioned in commit 6c88be75e0f996b0ffe54670834541f629fc841d

claude commented

2026-07-08 19:08:43 +01:00

(Migrated from gitlab.keyop.co.uk)

added 4 commits

16e08fb1...e701ff5e - 2 commits from branch main
14f847bf - Stop self-invoking sudo in the Makefile (#36)
6c88be75 - Address review: gate check-build-requirements like install (#36)

Compare with previous version

added 4 commits <ul><li>16e08fb1...e701ff5e - 2 commits from branch <code>main</code></li><li>14f847bf - Stop self-invoking sudo in the Makefile (#36)</li><li>6c88be75 - Address review: gate check-build-requirements like install (#36)</li></ul> [Compare with previous version](/keyop/go/nfq_forwarder/-/merge_requests/38/diffs?diff_id=358&start_sha=16e08fb12a5fa24d0f3dae02a86e44c6e0ae68b0)

claude commented

2026-07-08 19:09:05 +01:00

(Migrated from gitlab.keyop.co.uk)

Done in 6c88be7 - check-build-requirements now checks id -u the same way install does: root+missing installs directly (no sudo), non-root+present is silent, non-root+missing only warns and suggests sudo make check-build-requirements rather than hard-failing (the real cgo compiler error surfaces on its own moments later if the header genuinely can't be found). Verified all four branches with a standalone harness faking id -u/header-presence, plus both reachable-on-this-host cases for real.

Done in `6c88be7` - check-build-requirements now checks `id -u` the same way install does: root+missing installs directly (no sudo), non-root+present is silent, non-root+missing only warns and suggests `sudo make check-build-requirements` rather than hard-failing (the real cgo compiler error surfaces on its own moments later if the header genuinely can't be found). Verified all four branches with a standalone harness faking id -u/header-presence, plus both reachable-on-this-host cases for real.

claude commented

2026-07-08 19:09:05 +01:00

(Migrated from gitlab.keyop.co.uk)

Rebased onto main (now includes !78's CGO_CFLAGS fix) - no conflicts remaining.

claude commented

2026-07-08 19:09:05 +01:00

(Migrated from gitlab.keyop.co.uk)

resolved all threads

claude commented

2026-07-08 19:09:05 +01:00

(Migrated from gitlab.keyop.co.uk)

resolved all threads

claude commented

2026-07-08 19:09:14 +01:00

(Migrated from gitlab.keyop.co.uk)

Rebased onto main (no more conflicts) and reworked check-build-requirements per the review comments (6c88be7): it now checks id -u the same way install does, self-heals directly when already root, and only warns (doesn't hard-fail) when not root and something's missing. make lint/make test/make build + a full sudo make install all verified again on this host. Ready for another look.

Rebased onto `main` (no more conflicts) and reworked `check-build-requirements` per the review comments (`6c88be7`): it now checks `id -u` the same way `install` does, self-heals directly when already root, and only warns (doesn't hard-fail) when not root and something's missing. `make lint`/`make test`/`make build` + a full `sudo make install` all verified again on this host. Ready for another look.

danny commented

2026-07-08 19:16:34 +01:00

(Migrated from gitlab.keyop.co.uk)

Ok, that looks better.

danny commented

2026-07-08 19:16:34 +01:00

(Migrated from gitlab.keyop.co.uk)

approved this merge request

danny commented

2026-07-08 19:16:38 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit ad5d4b4646