Skip to content

docs+refactor(cni): fix stale package doc, promote hostconf/crdnames/nadpatch - #315

Merged
privateip merged 1 commit into
mainfrom
fix/cni-review-followups-doc-placement
Aug 10, 2026
Merged

docs+refactor(cni): fix stale package doc, promote hostconf/crdnames/nadpatch#315
privateip merged 1 commit into
mainfrom
fix/cni-review-followups-doc-placement

Conversation

@privateip

Copy link
Copy Markdown
Contributor

Follow-up to mattdjenkinson's review on #303 — 2 of the review's 5 points. The other 3 are handled separately:

  • The isTapMode()/CNI_NETNS_OVERRIDE dead-code point was already resolved upstream in this stack (725ef87, this branch's own parent).
  • The missing nil-tracker.k8s guard point is now moot — the BGP publish call it was about no longer lives in ops_add.go at all (moved to its own galactic-bgp chain plugin in e33092c).
  • The internal/cnitap vs internal/cni duplication point is a separate, larger follow-up: #(fix/cni-review-followups-dedup), stacked on top of this branch.

I'll leave a comment on #303 summarizing all 5.

This PR

internal/cni/doc.go staleness. The package doc still said ADD creates "a veth or tap interface" and listed tap as one of this package's subpackages — both stale since step 0 of this stack made galactic-cni veth-only and moved tap to galactic-tap-cni/internal/cnitap. Rewrote the doc to describe what internal/cni actually does now (VRF + veth pair + NAD patch only) and to explain why internal/cni/ipam, internal/cni/route, and internal/cni/tap are still physically nested there despite no longer being used by this package itself.

Package placement inconsistency. hostconf, crdnames, and nadpatch were nested under internal/cni/* while cnibgp/cniipam/cnitap/cniroute — doing the same cross-binary-sharing job — got promoted to top-level internal/*. That made it look like internal/cnitap (and cnibgp/cniroute) were reaching into cni's own internals, which is exactly what extracting them in the first place was meant to avoid. Promoted all three to internal/hostconf, internal/crdnames, internal/nadpatch. Package names are unchanged — every call site is a mechanical import-path update (15 files) plus two doc references (ARCHITECTURE.md, a comment in internal/cniroute/config.go).

internal/cni/ipam, internal/cni/route, and internal/cni/tap are a different category (kernel-primitive packages, like veth) and weren't part of this review point, so they're untouched.

Verification

  • task lint
  • task build ✅ (all 8 binaries)
  • go vet ./...
  • task test:unit ✅ (33/33 packages passing)
  • task test:e2e not run (same caveat as every step in this stack — needs a Kind cluster bring-up not attempted here)

🤖 Generated with Claude Code

@privateip
privateip requested a review from a team as a code owner August 8, 2026 18:55
@privateip
privateip requested review from fr6nco and removed request for a team August 8, 2026 18:55
@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch from 725ef87 to c690b5b Compare August 8, 2026 19:04
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 5f1d26b to 6cd671f Compare August 8, 2026 19:06
@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch from c690b5b to 6fdfbda Compare August 9, 2026 20:45
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 6cd671f to 49bc55f Compare August 9, 2026 20:56
privateip added a commit that referenced this pull request Aug 9, 2026
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
#307 moved out from under it) after that branch's history moved out
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch from 29fc68c to 70be97d Compare August 10, 2026 12:35
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 49bc55f to 553c6e1 Compare August 10, 2026 12:57
privateip added a commit that referenced this pull request Aug 10, 2026
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
#307 moved out from under it) after that branch's history moved out
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch from 70be97d to 46b4238 Compare August 10, 2026 13:02
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 553c6e1 to d9c08ec Compare August 10, 2026 13:31
privateip added a commit that referenced this pull request Aug 10, 2026
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
#307 moved out from under it) after that branch's history moved out
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fr6nco
fr6nco previously approved these changes Aug 10, 2026

@fr6nco fr6nco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm;

mostly just package reorganization as per PR description.

@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch from 64ef03d to afc568b Compare August 10, 2026 17:51
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from d9c08ec to 244f8df Compare August 10, 2026 17:54
privateip added a commit that referenced this pull request Aug 10, 2026
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
#307 moved out from under it) after that branch's history moved out
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip force-pushed the refactor/cni-chain-4-installer-docs branch 4 times, most recently from 2acb50a to 9e24f70 Compare August 10, 2026 18:53
Base automatically changed from refactor/cni-chain-4-installer-docs to main August 10, 2026 18:54
@privateip
privateip dismissed fr6nco’s stale review August 10, 2026 18:54

The base branch was changed.

@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 244f8df to 2572386 Compare August 10, 2026 20:11
@privateip
privateip requested a review from ecv August 10, 2026 20:33
…nadpatch

Rebased onto the updated refactor/cni-chain-4-installer-docs (which
picked up PR #307's own review-feedback fix commit) after that
branch's history moved out from under this one -- same ripple as

Conflicts resolved: internal/cnibgp/bgp.go, ops_check.go, and
resource.go each had an import-block conflict from this PR's
internal/cni/crdnames -> internal/crdnames promotion landing on
lines the current tree had already changed independently (PR #305's
own fix commit moved cnibgp's eBPF pin-dir handling behind a package-
level ebpfPinDir var in cnibgp.go, so bgp.go/ops_check.go/resource.go
no longer import internal/plumbing/ebpf/attach directly the way this
PR's diff -- authored before that -- expected). Resolved by applying
just the crdnames rename to each file and leaving the attach import
out, matching how the current tree already gets ebpfPinDir.

Also fixed one file this PR's diff never touched: internal/cnibgp/
resource_test.go didn't exist yet at this PR's original base -- it
was added later by #305's own fix commit -- so its own
"go.datum.net/galactic/internal/cni/crdnames" import needed the same
promotion this PR already applied everywhere else, or the package
wouldn't build.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the three promoted packages
(internal/crdnames, internal/hostconf, internal/nadpatch). tests/e2e
not run in this sandbox (no Kind cluster / root), same caveat as
every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 2572386 to 4517b0c Compare August 10, 2026 20:38
@privateip
privateip enabled auto-merge August 10, 2026 20:42

@ecv ecv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The placement argument is right — cnitap importing internal/cni/hostconf read as reaching into another plugin's tree, which is the thing the extraction was supposed to end. Package names unchanged keeps it a pure import-path change, and the rebase carried RejectMovedIPAMKeys through the move intact.

The doc rewrite is more useful than the usual staleness fix, because it explains why ipam, route and tap are still nested rather than pretending they aren't.

One gap, filed rather than blocking: #344. internal/cni/hostgw is the same shape as the three you moved — imported by internal/cnitap and internal/cnibgp as well as by internal/cni itself — and it stayed behind. It's arguably the strongest case of the set, since it configures kernel state and its path claims an ownership that isn't true. doc.go doesn't mention it in either category either.

Nit: docs/plans/854-vpc-http-ingress-endpointslice.md still names internal/cni/crdnames and internal/cni/nadpatch. Plan docs are point-in-time, so ignore if that's the convention here.

CI green including E2E.

@privateip
privateip merged commit 519c85b into main Aug 10, 2026
10 checks passed
@privateip
privateip deleted the fix/cni-review-followups-doc-placement branch August 10, 2026 20:50
privateip added a commit that referenced this pull request Aug 10, 2026
…nimaster

Rebased onto the updated fix/cni-review-followups-doc-placement
(which picked up my own rebase-and-reconcile of that branch after PR
from under this one -- same ripple as #306 -> #307 -> #315 -> #316.

Conflicts resolved: internal/cni/resource.go and internal/cnitap/
resource.go each had two independent changes touching the same
resourceTracker/cleanup() region -- this PR's own extraction of the
shared k8s-client-construction (newK8sClient/cniScheme) and interface+
VRF rollback (veth.Delete/tap.Delete + vrf.Delete) into
internal/cnimaster's NewK8sClient/CleanupAttachment, and PR #305/#306's
unrelated addition of ipamDelegated/ipamType/ipamStdin fields plus an
IPAM-release rollback step (this PR's diff predates that feature
entirely, same gap #311/#312/#315 each hit one level up). Kept both:
call cnimaster.CleanupAttachment for the interface+VRF half (this PR's
whole point), and kept the IPAM release step ahead of it, unchanged.
internal/cnitap/ops_check.go had one similar import-only conflict
(this PR drops the netlink/rest/ctrl/vrf imports cnimaster.
CheckNodeLevelState/ProbeAPIServer/RunStatus now cover internally) --
its own IPAM CHECK delegation step (ipam.ExecCheck, same predates-this-
PR gap) sat entirely outside the conflicted hunk and needed no
resolution beyond keeping the "github.com/containernetworking/plugins/
pkg/ipam" import alive.

Verification: task lint, task build (all 8 binaries), task test:unit
all pass on the rebased tree, including the new internal/cnimaster
package. tests/e2e not run in this sandbox (no Kind cluster / root),
same caveat as every PR in this stack.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

3 participants