cli: closurejvm instrument (Go CLI, first command)

[ianp94]

The first slice of the CLI to launch tests — a thin Go binary over the operator's CRDs, so instrumenting an app isn't hand-written YAML + kubectl. Scope: instrument only (by design; run/status/dashboard are the next CLI PRs).

What's here (operator/cmd/closurejvm/)

• Reuses the operator's api/v1alpha1 types + a controller-runtime client to apply real typed ClosureJVMTarget CRs (not templated YAML).
• instrument: builds a target from flags (--deployment, --container, --jvm-opts-var, --coverage-includes/--coverage-port/--coverage-service, --invariant-mode/--latency-max-ms/--heap-delta-max-kb, --thread-tracker), applies create-or-update (idempotent re-apply), and with --wait blocks until Injected, printing phase transitions + the coverageEndpoint.
• Kubeconfig via the standard loading rules + --context/-n; namespace defaults to the context's.
• stdlib flag + subcommand dispatch — no new module deps (client-go/clientcmd + controller-runtime client were already vendored).
• make -C operator cli → operator/bin/closurejvm.

Tests + validation

• Unit tests on the pure buildTarget + validateInstrument (coverage on iff includes set; port default; the reject cases).
• Validated against the kind cluster: instrumented a fresh Deployment to Injected ✓ with the coverage Service, idempotent re-apply, and both validation errors (--deployment required; --coverage-service needs --coverage-includes).
• docs/OPERATOR-USAGE.md §3 gains a CLI alternative to the target YAML.

@claude please review — focus on: (1) buildTarget correctness vs the CRD (coverage-enabled-iff-includes, the JVMOptsVar-empty-lets-CRD-default choice, ThreadTracker with omitempty); (2) the apply create-or-update + waitForInjected loop (error/timeout/Error-phase handling); (3) kubeconfig/context/namespace resolution; (4) anything that should be validated client-side but isn't.

🤖 Generated with Claude Code

[ianp94]

@claude please review this CLI PR. Focus: (1) buildTarget correctness vs the CRD (coverage-enabled-iff-includes; JVMOptsVar-empty-lets-CRD-default; ThreadTracker omitempty behavior); (2) applyTarget create-or-update + waitForInjected loop (error/timeout/Error-phase); (3) kubeconfig/context/namespace resolution via clientcmd; (4) client-side validation gaps. Unit-tested + validated against kind (instrument→Injected, idempotent re-apply, validation errors).

[claude]

Claude finished @ianp94's task in 4m 47s —— View job

---

Review: closurejvm instrument (Go CLI)

• Gather context (diff, CRD types, controller reconcile logic)
• Review buildTarget vs CRD semantics
• Review applyTarget create-or-update + waitForInjected loop
• Review kubeconfig/context/namespace resolution
• Review client-side validation gaps
• Post final review

Solid first slice — reusing the typed api/v1alpha1 client instead of templated YAML is the right call, and the buildTarget/validateInstrument split makes the core logic genuinely unit-testable. One correctness bug and one race worth fixing before this ships broadly; the rest are minor.

1. --thread-tracker=false can never actually take effect (high)

operator/cmd/closurejvm/instrument.go:54 sets spec.Agents.ThreadTracker straight from the flag, but AgentsSpec.ThreadTracker (operator/api/v1alpha1/closurejvmtarget_types.go:63) is json:"threadTracker,omitempty" and carries +kubebuilder:default=true. Go's encoding/json omitempty drops a bool field when it's false, so --thread-tracker=false produces a request where threadTracker is entirely absent — and CRD structural defaulting re-fills it with true on both create and update (kubebuilder defaulting applies to any request where the field is unset, not just create). Net effect: there is no way to disable the thread tracker via this CLI; every apply — including a re-apply meant to turn it off on an already-Injected target — ends up with threadTracker: true server-side, silently contradicting the flag.

This is exactly the CRD-defaulting/omitempty interaction the PR description flagged as worth checking, and it's real. Fixing it cleanly likely means changing ThreadTracker to *bool (or otherwise making "explicit false" distinguishable from "unset") in the CRD type, which is shared with the controller — worth a quick follow-up rather than blocking this PR outright, but it should at least be called out/tracked since the flag is documented as controlling this.

Fix this →

(As a side effect, --valve isn't exposed as a flag at all, so every target also ends up with agents.valve: true via the same CRD default regardless of app type — probably fine to leave out of scope for instrument v1, but worth a doc note since it's silently always-on.)

2. waitForInjected can report success against a stale status (medium)

operator/cmd/closurejvm/instrument.go:176-201 treats Status.Phase == Injected as terminal without checking Status.ObservedGeneration against metadata.generation. The status type has ObservedGeneration precisely for this (closurejvmtarget_types.go:145-147), and the controller sets it on every reconcile (closurejvmtarget_controller.go:100). Since applyTarget (instrument.go:161-173) does a spec-only update on an existing target, the idempotent-reapply path this PR calls out as validated — re-running instrument with --wait against an already-Injected target whose spec actually changed (e.g. flipping --coverage-includes) — can read the pre-update Injected status in the first Get and return success immediately, before the controller has reconciled the new spec at all. Suggest gating the PhaseInjected/PhaseError terminal checks on t.Status.ObservedGeneration >= t.Generation.

Fix this →

3. buildTarget vs. the CRD — otherwise correct

• Coverage-enabled-iff-includes (instrument.go:56-62): matches the CRD's CEL rule (CoverageSpec XValidation in closurejvmtarget_types.go:41) exactly — coverage is only ever turned on alongside a non-empty Includes, so the DD-022 wildcard footgun can't reappear through this path. Good.
• JVMOptsVar left "" when the flag isn't passed (instrument.go:52) correctly defers to the CRD's +kubebuilder:default=JAVA_TOOL_OPTIONS.
• Port defaulting (instrument.go:57-60) is redundant with the flag's own 6300 default (instrument.go:100) but harmless — it only matters if someone calls buildTarget directly with coveragePort: 0, which the unit test does exercise.
• InvariantsSpec/AgentsSpec/CoverageSpec being non-pointer structs with omitempty json tags means those fields are never actually omitted from the request body (Go's omitempty is a no-op on non-pointer struct fields) — inconsequential here since nested field defaulting still cascades correctly, but worth knowing it's not doing what the tag visually suggests.

4. applyTarget create-or-update

Correct shape: Get → IsNotFound → Create; otherwise copy desired.Spec onto the fetched existing (preserving resourceVersion) and Update. That's the right way to avoid conflict errors on re-apply. No retry on a Get/Update conflict, but for a one-shot CLI invocation that's a reasonable tradeoff rather than a bug.

5. Kubeconfig / context / namespace resolution

clientcmd.NewDefaultClientConfigLoadingRules() + ConfigOverrides.CurrentContext (instrument.go:116-121) is the standard, correct pattern, and validation happens before cc.ClientConfig() is invoked, so --deployment-missing errors don't require a working kubeconfig at all — matches the PR's claim that both validation errors were exercised independently of cluster state. Namespace resolution (context's namespace, else default) is right and matches the flag help text. No issues found here.

6. Other client-side validation gaps

• --invariant-mode isn't checked against the CRD's Enum=soft;hard (closurejvmtarget_types.go:77) — an invalid value passes validateInstrument and only fails later with a raw Kubernetes CEL/enum error instead of a clean CLI message.
• --jvm-opts-var similarly isn't checked against Enum=CATALINA_OPTS;JAVA_TOOL_OPTIONS (closurejvmtarget_types.go:107) client-side.
• waitForInjected treats any Get error as immediately fatal (instrument.go:181-183), aborting the whole --wait on a single transient API-server hiccup rather than retrying until the timeout — minor, but worth a note given the loop already has its own deadline/timeout machinery.
• Minor UX nit: instrument -h goes through fs.Parse → flag.ErrHelp → runInstrument returns it as an error → main.go prints error: flag: help requested and exits 1, on top of the flag package's own usage output. Not wrong, just a rough edge for -h/--help.

Nothing here blocks the PR from an architecture standpoint — #1 and #2 are the two worth fixing (or explicitly tracking) before run/status build on top of this, since both are silent-failure modes that unit tests and a single manual kind validation pass wouldn't surface.

[ianp94]

Both real — fixed in `db2a7f4`.

#1 --thread-tracker=false (high): exactly the omitempty+default=true trap. Made AgentsSpec.ThreadTracker a *bool (nil = default on); the CLI sends an explicit pointer, injection.go reads nil || *ptr. Regenerated CRD/deepcopy and synced the chart's CRD copy. Verified live: --thread-tracker=false now stores threadTracker: false, an omitted one stores true.

#2 --wait stale-status race (medium): waitForInjected now gates the terminal Injected/Error checks on status.observedGeneration >= metadata.generation, so a --wait re-apply that changed the spec can't return on the pre-update status. Also made transient Get errors retry until the deadline instead of aborting the wait.

Minor items: added client-side enum validation for --jvm-opts-var and --invariant-mode (clean CLI error instead of a raw server CEL error), and clean -h/--help handling (flag.ErrHelp no longer surfaces as error: ...). Tests added for the *bool false-preservation and the enum rejects.

On the non-blocking notes: --valve staying out of instrument v1 is intentional (valve mounting is deferred anyway) — I'll expose it when it's wired. The nested-struct omitempty-is-a-no-op observation is noted; harmless as you said since defaulting cascades. Merging.