Skip to content

refactor(cni): dedupe internal/cni vs internal/cnitap into internal/cnimaster - #316

Open
privateip wants to merge 1 commit into
fix/cni-review-followups-doc-placementfrom
fix/cni-review-followups-dedup
Open

refactor(cni): dedupe internal/cni vs internal/cnitap into internal/cnimaster#316
privateip wants to merge 1 commit into
fix/cni-review-followups-doc-placementfrom
fix/cni-review-followups-dedup

Conversation

@privateip

Copy link
Copy Markdown
Contributor

Follow-up to mattdjenkinson's review on #303 — the last of the review's 5 points (the other 4: 2 already resolved upstream in the stack, 1 now moot, 2 landed in #(fix/cni-review-followups-doc-placement), which this branch stacks on).

Problem

internal/cnitap reimplemented internal/cni's config.go (parseConf's validation, log setup, status parsing, prevResult validation, base62/error sanitizing), resource.go (newK8sClient, the tracker/cleanup rollback sequence), and ops_check.go (checkNodeLevelState, cmdStatus, probeAPIServer, validateHostInterface) almost verbatim — none of it is interface-specific. A fix to one wouldn't have propagated to the other. internal/cnitap's own tests were implicitly relying on internal/cni's tests to cover the logic it silently duplicated, which is exactly the risk the review comment flagged.

Fix

Extracted the interface-agnostic pieces into a new internal/cnimaster package — the same kind of shared-library extraction this stack already did for hostconf/crdnames/nadpatch/cniipam/cnibgp:

  • PluginConf moves to cnimaster; internal/cni and internal/cnitap each keep type PluginConf = cnimaster.PluginConf (mirroring the existing HostConf = hostconf.HostConf alias pattern).
  • ParseConf, LoadHostConf, ParseStatusConf, ValidatePrevResult(Add), IsValidBase62, SanitizeForError, UnwrapPathError, ParseLogLevel, SetupLogging move to cnimaster/config.go. Each package's own parseConf becomes a two-line wrapper binding ParseConf to its own cniConfig/ConfFile (these stay per-binary — each resolves its own GALACTIC_CNI_* environment independently).
  • NewK8sClient (+ the backing scheme) and a new CleanupAttachment (parameterized by interface kind and a Delete func, replacing the copy-pasted rollback sequence) move to cnimaster/resource.go.
  • CheckNodeLevelState, ValidateHostInterface, ProbeAPIServer(Fn), and a new RunStatus (the full cmdStatus body, since it wasn't interface-specific either) move to cnimaster/check.go. Each package's own cmdStatus becomes a one-line wrapper.
  • cmdCheck's guest-side validation (checkGuestInterface, validateGuestInterface, validateIPOnInterface) stays in internal/cni only — tap has no guest netns to check, so this piece is genuinely interface-specific and wasn't touched.

Test coverage moved with the code: the generic-logic unit tests (TestParseConf, TestIsValidBase62, TestSanitizeForError, TestValidatePrevResult(Add), TestProbeAPIServer*, TestLoadHostConf, TestParseLogLevel, TestLoggingSetup*, TestIPAMBlockPresenceIsTheOnlyTrigger) moved from internal/cni/cni_test.go into internal/cnimaster/cnimaster_test.go. internal/cnitap's own TestLoadHostConfMissingFile (testing a function that no longer exists in that package) is removed as now fully redundant with cnimaster's own coverage. Remaining tests in both cni_test.go/cnitap_test.go are either interface-specific (cmdAdd/cmdDel/cmdCheck/buildResult) or thin wiring smoke tests over the shared functions.

Net effect: internal/cni and internal/cnitap together lose ~750 lines of near-duplicate implementation+test code; internal/cnimaster adds ~460 (including its own full test suite) — about 250 fewer lines overall, in one place instead of two.

Verification

  • task lint
  • task build ✅ (all 8 binaries)
  • go vet ./...
  • task test:unit ✅ (33/33 packages passing, 0 failures)
  • 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 aflor024 and removed request for a team August 8, 2026 18:55
@privateip
privateip force-pushed the fix/cni-review-followups-doc-placement branch from 5f1d26b to 6cd671f Compare August 8, 2026 19:06
…nimaster

Follow-up to mattdjenkinson's review on #303 (the last of 5 points; the
other 4 are handled elsewhere — 2 already resolved upstream in this
stack, 1 is now moot, 2 landed in fix/cni-review-followups-doc-placement).

internal/cnitap reimplemented internal/cni's config.go (parseConf's
validation, log setup, status parsing, prevResult validation, base62/
error sanitizing), resource.go (newK8sClient, the tracker/cleanup
rollback sequence), and ops_check.go (checkNodeLevelState, cmdStatus,
probeAPIServer, validateHostInterface) almost verbatim — none of it is
interface-specific. A fix to one wouldn't have propagated to the other.

Extracted the interface-agnostic pieces into a new internal/cnimaster
package, the same kind of shared-library extraction this stack already
did for hostconf/crdnames/nadpatch/cniipam/cnibgp:

- PluginConf moves to cnimaster; internal/cni and internal/cnitap each
  keep `type PluginConf = cnimaster.PluginConf` (mirroring the existing
  HostConf = hostconf.HostConf alias pattern).
- ParseConf, LoadHostConf, ParseStatusConf, ValidatePrevResult(Add),
  IsValidBase62, SanitizeForError, UnwrapPathError, ParseLogLevel,
  SetupLogging move to cnimaster.config.go. Each package's own parseConf
  becomes a two-line wrapper binding ParseConf to its own cniConfig/
  ConfFile (these stay per-binary — each resolves its own GALACTIC_CNI_*
  environment independently).
- NewK8sClient (+ the backing scheme) and a new CleanupAttachment
  (parameterized by interface kind and a Delete func, replacing the
  copy-pasted rollback sequence) move to cnimaster.resource.go.
- CheckNodeLevelState, ValidateHostInterface, ProbeAPIServer(Fn), and a
  new RunStatus (the full cmdStatus body, since it wasn't interface-
  specific either) move to cnimaster.check.go. Each package's own
  cmdStatus becomes a one-line wrapper.
- cmdCheck's guest-side validation (checkGuestInterface,
  validateGuestInterface, validateIPOnInterface) stays in internal/cni
  only — tap has no guest netns to check, so this piece is genuinely
  interface-specific and wasn't touched.

Test coverage moved with the code: the generic-logic unit tests
(TestParseConf, TestIsValidBase62, TestSanitizeForError,
TestValidatePrevResult(Add), TestProbeAPIServer*, TestLoadHostConf,
TestParseLogLevel, TestLoggingSetup*, TestIPAMBlockPresenceIsTheOnly
Trigger) moved from internal/cni/cni_test.go into
internal/cnimaster/cnimaster_test.go, since cnitap_test.go never had
its own copies to begin with (it was implicitly relying on cni's tests
covering logic it silently duplicated — exactly the risk the review
comment flagged). internal/cnitap's own TestLoadHostConfMissingFile
(testing a function that no longer exists in that package) is removed
as now fully redundant with cnimaster's own coverage. Remaining tests
in both cni_test.go/cnitap_test.go are either interface-specific
(cmdAdd/cmdDel/cmdCheck/buildResult) or thin wiring smoke tests over
the shared functions.

Net effect: internal/cni and internal/cnitap together lose ~750 lines
of near-duplicate implementation+test code; internal/cnimaster adds
~460 (including its own full test suite) — about 250 fewer lines
overall, in one place instead of two.

task lint / task build / go vet / task test:unit all clean (33/33
packages passing, 0 failures).
@privateip
privateip force-pushed the fix/cni-review-followups-dedup branch from 29a713e to 1e314b0 Compare August 8, 2026 19:06
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.

1 participant