Skip to content

Add PersistentKeepalive, consolidate resource rendering, fix docs (#4, #1) - #39

Merged
jacaudi merged 3 commits into
mainfrom
claude/init-container-support-review-b953sx
Aug 29, 2026
Merged

Add PersistentKeepalive, consolidate resource rendering, fix docs (#4, #1)#39
jacaudi merged 3 commits into
mainfrom
claude/init-container-support-review-b953sx

Conversation

@jacaudi

@jacaudi jacaudi commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Implements #4 and #1, the two Phase 0 foundations, plus a documentation pass. Three independent commits, reviewable separately.

Rebased onto a4c7b78 and no longer touches CI. This PR originally carried a fourth commit adopting an orchestrator CI pipeline; #43 landed that work instead, so it has been dropped wholesale. The diff now contains no file under .github/, no release-please config, and no .releaserc.json. Commit subjects were rewritten to conventional-commit form so release-please — live on main since #43 — reads them; authors, dates and bodies are unchanged.

feat: — Add PersistentKeepalive to generated peer configs (#4)

Peer configs carried no PersistentKeepalive, so an idle client would not retry a handshake until it had traffic of its own, and its NAT/conntrack entry would expire leaving the server unable to initiate toward it. That matters most right after the server pod is rescheduled, which is this operator's primary recovery path.

Adds spec.persistentKeepalive as *int32 with a CRD default of 25. The pointer is load-bearing: WireGuard reads 0 as disabled, and a plain int32 with omitempty could not distinguish "user asked for 0" from "user said nothing". The Go constant is only the nil fallback for objects stored before the field existed.

The line is rendered once and interpolated into all three config flavors — tunnel, dual-mode direct, and plain. When disabled the interpolated string is empty, leaving output byte-identical to before.

release.yaml is regenerated in this commit. It is the documented install path and a generated artifact, so the new CRD property would otherwise fail hack/release-file-drift.sh. Regenerated at the committed image pins — bumping those is release-time work.

refactor: — Consolidate resource rendering onto internal/resources builders (#1)

internal/resources was entirely dead code: all four builders were constructed in SetupWithManager but no method was ever called, because each resource was rendered by an inline function on the reconciler instead. The two copies had already drifted.

Repoints all twelve call sites, deletes the six inline functions, and removes the duplicated constants (plus labelsForWireguard, which only the deleted functions called).

Three deliberate differences in rendered output:

resource change effect
Deployment metrics container port now emitted declarative only; a metrics Service already targets it
Deployment http container port was 51820 while both probes target 8080 declarative only; wrong in both copies, so not drift
ConfigMap builder sets empty Data map vs nil omitempty omits a zero-length map — serialized object unchanged

None of these reach existing deployments: the reconciler only re-renders on four triggers (agent image, userspace flag, wstunnel sidecar, scheduling settings) and none compares container ports. Declarative reconciliation is #2.

docs: — Correct and extend project documentation

  • README support link pointed at the upstream issue tracker; this fork has its own.
  • README features list omitted IPv6 support and per-peer egress network policies, both implemented.
  • README install instructions silently install the upstream operator, which is diverging from this fork. The URLs are left working, since this fork has no release to point at, but the divergence is now called out with a pointer to [P6] Fork housekeeping: Go module path #36.
  • ROADMAP 0.1 still described consolidating only the two Deployment builders, from before that scope was widened to all four. Now matches the issue and the implementation.
  • CONTRIBUTING was a single placeholder line. It now documents the build and codegen entry points, which generated files must never be hand-edited, and the engineering practices this work is held to.

Verification

The original version of this PR shipped unverified — nothing had been compiled or tested, because the Go toolchain and module proxy were unreachable in that environment. That is no longer true. Everything below was run locally against this branch:

gate result
task ci exit 0 — golangci-lint 0 issues, govulncheck 0 affecting, go mod tidy -diff clean, fmt clean
go test ./... -race pass
task go:integration (envtest, -race, -tags=integration) pass, internal/controller 78.8s
make manifests generate no diff under config / api
sh hack/release-file-drift.sh release.yaml matches config/

Four things worth calling out, since they are exactly what the earlier version could not check:

  • The hand-written generated files were correct. make manifests generate produces no diff, so the by-hand persistentKeepalive CRD schema and the zz_generated.deepcopy.go pointer copy match what controller-gen v0.16.5 emits.
  • The new tests actually run. internal/controller is behind //go:build integration as of ci: adopt the fleet taskfile and lint baseline, and clear the debt it surfaces #42; the new specs carry the tag and were proven to execute rather than to be silently skipped — a focused run reports Will run 4 of 23 Specs … 4 Passed, and the suite takes ~78s rather than seconds.
  • The lint ratchet was live. new-from-merge-base: origin/main resolved its merge base; had it failed open it would have reported the 113 pre-existing findings instead of 0.
  • release.yaml drift is a new gate this PR predates. Adding a CRD property changes the rendered manifest, so it is regenerated in the feat: commit.

Closes #4
Closes #1

🤖 Generated with Claude Code

https://claude.ai/code/session_01XcHdnmhuAbyBYG2VfwuPDD

claude added 3 commits August 29, 2026 10:20
Peer configs carried no PersistentKeepalive, so an idle client would not retry
a handshake until it had traffic of its own, and its NAT/conntrack entry would
expire leaving the server unable to initiate toward it. This matters most after
the server pod is rescheduled, which is the operator's primary recovery path.

Adds spec.persistentKeepalive as a pointer with a CRD default of 25, so an
explicit 0 can disable it — WireGuard reads zero as "off", which a non-pointer
field could not distinguish from unset. The Go constant is only the nil
fallback for objects stored before the field existed.

The line is rendered once and interpolated into all three config flavors
(tunnel, dual-mode direct, and plain). When disabled the interpolated string is
empty, leaving output byte-identical to before.

release.yaml is regenerated for the new CRD property. It is the documented
install path and a generated artifact, so hack/release-file-drift.sh fails if it
falls behind config/. Regenerated at the committed image pins; bumping those is
release-time work, not gate work.

Closes #4

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XEBcAdEeo5Kp8L6ns3yzRs
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcHdnmhuAbyBYG2VfwuPDD
…ders

The internal/resources package was entirely dead: all four builders were
constructed in SetupWithManager but no method was ever called, because every
resource was rendered by an inline function on the reconciler instead. The two
copies had already drifted, and every planned feature touches this rendering —
so changes made in the wrong copy would have had no runtime effect.

Repoints all twelve call sites at the builders, adding error handling since the
builders return (T, error) where the inline functions returned bare pointers,
and deletes the six inline functions. The duplicated port and image constants
in the controller are removed in favour of the ones in internal/resources, as
is labelsForWireguard, which only the deleted functions called.

Three differences in rendered output, all deliberate:

- Deployment: the metrics container port the builder declared is now emitted.
  The agent serves metrics on it and a metrics Service already targets it.
- Deployment: the http container port was declared as the WireGuard port while
  both probes target the health port. Wrong in both copies, so not drift;
  corrected here since containerPort is purely declarative.
- ConfigMap: the builder sets an empty Data map where the inline version left
  it nil. Data is omitempty and a zero-length map is omitted, so the serialized
  object is unchanged.

None of these reach existing deployments. The reconciler only re-renders on
four triggers — agent image, userspace flag, wstunnel sidecar presence, and
scheduling settings — and none of them compares container ports, so a running
Deployment keeps its current spec until an unrelated trigger fires. Declarative
reconciliation is #2.

The Service, Secret and ConfigMap builders were verified equivalent to the
inline versions before switching, and are otherwise unchanged.

Closes #1

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XEBcAdEeo5Kp8L6ns3yzRs
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcHdnmhuAbyBYG2VfwuPDD
README:
- Support link pointed at the upstream issue tracker; this fork has its own.
- Features list omitted IPv6 support and per-peer egress network policies, both
  of which are implemented.
- Install instructions silently install the upstream operator, which is now
  diverging from this fork. The URLs are left working — this fork has no release
  to point at yet — but the divergence is now called out, with a pointer to #36.

CONTRIBUTING was a single placeholder line. It now covers the build and codegen
entry points, the files that must be regenerated rather than hand-edited, and the
engineering practices the roadmap work is held to. These previously existed only
as a GitHub issue comment, so nobody reading the repository could find them.

ROADMAP 0.1 still described consolidating only the two Deployment builders. That
scope was widened to all four resource types, so the section now matches the
issue and what was implemented.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XEBcAdEeo5Kp8L6ns3yzRs
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XcHdnmhuAbyBYG2VfwuPDD
@jacaudi
jacaudi force-pushed the claude/init-container-support-review-b953sx branch from d60e365 to 9320283 Compare August 29, 2026 17:29
@jacaudi
jacaudi merged commit 3a8fb1b into main Aug 29, 2026
9 checks passed
@jacaudi
jacaudi deleted the claude/init-container-support-review-b953sx branch August 29, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P0] Add PersistentKeepalive to generated peer configs [P0] Consolidate resource rendering onto internal/resources builders

2 participants