embed kustomize to crane#388
Merged
stillalearner merged 4 commits intoMay 21, 2026
Merged
Conversation
📝 WalkthroughWalkthroughEmbeds kustomize via a new in-process krusty-based Runner, migrates apply/transform code paths to call Runner.Build, removes external kubectl/oc probes/helpers, tightens allowed kustomize args, adds Runner tests, and updates go.mod dependencies. ChangesEmbed Kustomize via Internal Runner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Coverage ReportTotal: 42.6% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/apply/kustomize.go (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale
kubectlwording in errors/comments.This path now uses embedded kustomize, but errors/comments still say
kubectl kustomize, which is misleading during failures.Suggested patch
-// KustomizeApplier applies transformations using kubectl kustomize +// KustomizeApplier applies transformations using embedded kustomize @@ - // Run kubectl kustomize build + // Run embedded kustomize build @@ - return fmt.Errorf("kubectl kustomize build failed for stage %s: %w", stageName, err) + return fmt.Errorf("kustomize build failed for stage %s: %w", stageName, err) @@ - // Run kubectl kustomize build on the last stage + // Run embedded kustomize build on the last stage @@ - return fmt.Errorf("kubectl kustomize build failed for stage %s: %w", lastStage.DirName, err) + return fmt.Errorf("kustomize build failed for stage %s: %w", lastStage.DirName, err)As per coding guidelines, "Prefer explicit error messages with context in Go code".
Also applies to: 48-53, 88-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/apply/kustomize.go` at line 20, Update all comments and error strings that reference "kubectl kustomize" to instead refer to the embedded kustomize implementation and include actionable context; specifically, edit the KustomizeApplier type comment and any error messages in its constructor/methods (e.g., NewKustomizeApplier, Apply/Build functions referenced around the other noted ranges) to remove "kubectl" and use explicit messages such as "embedded kustomize: <action> failed: <error>" or similar so callers get clear context and the underlying error is included.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/kustomize/runner_test.go`:
- Line 17: Replace the ignored deferred os.RemoveAll calls with a deferred
closure that checks and reports any error (e.g. defer func() { if err :=
os.RemoveAll(tmpDir); err != nil { t.Errorf("cleanup failed: %v", err) } }()),
updating each occurrence of os.RemoveAll(tmpDir) in runner_test.go (the deferred
calls at the four locations) so the test fails or logs when cleanup fails
instead of silencing the error.
In `@internal/kustomize/runner.go`:
- Line 138: The os.Setenv and os.Unsetenv calls currently ignore returned errors
(e.g., the call using v.key and v.value and the subsequent os.Unsetenv calls);
modify the code to check the error return from os.Setenv/os.Unsetenv, and handle
it by returning the error (or wrapping it with context) from the surrounding
function or logging and returning a non-nil error so the caller can react;
update the code paths that set environment variables (the block that uses
v.key/v.value) and the corresponding unset calls to propagate or surface any
os.Setenv/os.Unsetenv error instead of discarding it.
- Around line 132-137: The loop over vars currently calls os.LookupEnv for each
entry and unconditionally assigns originals[v.key], which allows later repeated
--env mutations to overwrite the true original; update the loop so you record
the original environment value only once per key by first checking whether
originals already contains v.key (e.g., if _, seen := originals[v.key]; !seen {
... }), and only then call os.LookupEnv and set originals[v.key] or append to
unsetKeys accordingly; reference the vars iteration, originals map and unsetKeys
slice in your change.
- Around line 27-33: The Build function mutates process-global env vars via
setEnvVars around the krusty k.Run call which allows concurrent Build
invocations to leak env settings into each other; serialize these env-scoped
builds by adding a package-level sync.Mutex (e.g., envMutex) and acquire the
lock before calling setEnvVars and k.Run and release it only after restoring the
env (use defer to call restoreEnv() and then envMutex.Unlock() in a single
deferred closure) so the sequence setEnvVars -> k.Run -> restoreEnv is executed
under the mutex; ensure you import sync and reference the existing setEnvVars,
Build, and k.Run symbols when making this change.
In `@internal/transform/orchestrator.go`:
- Around line 304-306: The error returned after the kustomize build lacks the
stage directory context; update the error construction in the kustomize build
failure branch to include the stageDir (and optionally the stage name) so it
reads something like a descriptive message including stageDir and the wrapped
error; locate the kustomize build error handling in orchestrator.go (the block
that currently returns fmt.Errorf("kustomize build failed: %w", err)) and change
it to include stageDir (e.g., "kustomize build failed for stageDir %s: %w") so
callers can see which stage directory failed.
---
Outside diff comments:
In `@internal/apply/kustomize.go`:
- Line 20: Update all comments and error strings that reference "kubectl
kustomize" to instead refer to the embedded kustomize implementation and include
actionable context; specifically, edit the KustomizeApplier type comment and any
error messages in its constructor/methods (e.g., NewKustomizeApplier,
Apply/Build functions referenced around the other noted ranges) to remove
"kubectl" and use explicit messages such as "embedded kustomize: <action>
failed: <error>" or similar so callers get clear context and the underlying
error is included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4446b54c-affd-4365-8484-aa336b080ca8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/apply/apply.gogo.modinternal/apply/kustomize.gointernal/apply/kustomize_test.gointernal/file/file_helper.gointernal/kustomize/args.gointernal/kustomize/runner.gointernal/kustomize/runner_test.gointernal/transform/orchestrator.gointernal/transform/test_helpers.go
💤 Files with no reviewable changes (3)
- cmd/apply/apply.go
- internal/apply/kustomize_test.go
- internal/file/file_helper.go
| if err != nil { | ||
| t.Fatalf("Failed to create temp dir: %v", err) | ||
| } | ||
| defer os.RemoveAll(tmpDir) |
There was a problem hiding this comment.
Check deferred os.RemoveAll errors in tests.
At Line 17, Line 63, Line 131, and Line 187, os.RemoveAll errors are ignored; this fails errcheck and can hide cleanup problems.
💡 Suggested fix (apply to each occurrence)
- defer os.RemoveAll(tmpDir)
+ defer func() {
+ if err := os.RemoveAll(tmpDir); err != nil {
+ t.Errorf("failed to remove temp dir %q: %v", tmpDir, err)
+ }
+ }()As per coding guidelines, "Follow standard Go idioms and effective Go practices".
Also applies to: 63-63, 131-131, 187-187
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 17-17:
Error return value of os.RemoveAll is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/kustomize/runner_test.go` at line 17, Replace the ignored deferred
os.RemoveAll calls with a deferred closure that checks and reports any error
(e.g. defer func() { if err := os.RemoveAll(tmpDir); err != nil {
t.Errorf("cleanup failed: %v", err) } }()), updating each occurrence of
os.RemoveAll(tmpDir) in runner_test.go (the deferred calls at the four
locations) so the test fails or logs when cleanup fails instead of silencing the
error.
Contributor
Author
There was a problem hiding this comment.
Not needed. These are defer os.RemoveAll(tmpDir) for test cleanup .
There was a problem hiding this comment.
@stillalearner, understood — ignoring the return value of os.RemoveAll in deferred test cleanup is perfectly reasonable and a common Go pattern. I'll defer to your judgement here.
One thing worth noting: since golangci-lint's errcheck rule is still flagging these lines (causing a CI check failure), you may want to add //nolint:errcheck inline comments to suppress those warnings without changing the cleanup logic:
defer os.RemoveAll(tmpDir) //nolint:errcheckThat would keep the existing style while silencing the lint failures. Entirely up to you though!
(ᵔᴥᵔ) ~ rabbit shrug
Comment on lines
+27
to
+33
| // Set environment variables (used by helm) and restore after build | ||
| restoreEnv := setEnvVars(envVars) | ||
| defer restoreEnv() | ||
|
|
||
| k := krusty.MakeKustomizer(opts) | ||
| resMap, err := k.Run(filesys.MakeFsOnDisk(), dir) | ||
| if err != nil { |
There was a problem hiding this comment.
Serialize env-scoped builds to prevent cross-call contamination.
At Line 27 through Line 33, Build mutates process-global env vars around the krusty run. Concurrent Build calls can leak --env values into each other and produce nondeterministic output.
💡 Suggested fix
import (
"fmt"
"os"
"strings"
+ "sync"
@@
type Runner struct {
Log *logrus.Logger
Args []string
}
+
+var envMutationMu sync.Mutex
@@
func (r *Runner) Build(dir string) ([]byte, error) {
opts, envVars, err := r.buildOptions()
@@
- restoreEnv := setEnvVars(envVars)
- defer restoreEnv()
+ envMutationMu.Lock()
+ restoreEnv, err := setEnvVars(envVars)
+ if err != nil {
+ envMutationMu.Unlock()
+ return nil, fmt.Errorf("failed to set kustomize env vars: %w", err)
+ }
+ defer func() {
+ _ = restoreEnv()
+ envMutationMu.Unlock()
+ }()As per coding guidelines, "Follow standard Go idioms and effective Go practices".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/kustomize/runner.go` around lines 27 - 33, The Build function
mutates process-global env vars via setEnvVars around the krusty k.Run call
which allows concurrent Build invocations to leak env settings into each other;
serialize these env-scoped builds by adding a package-level sync.Mutex (e.g.,
envMutex) and acquire the lock before calling setEnvVars and k.Run and release
it only after restoring the env (use defer to call restoreEnv() and then
envMutex.Unlock() in a single deferred closure) so the sequence setEnvVars ->
k.Run -> restoreEnv is executed under the mutex; ensure you import sync and
reference the existing setEnvVars, Build, and k.Run symbols when making this
change.
Contributor
Author
There was a problem hiding this comment.
Crane is a CLI tool, Build is never called concurrently. The transform orchestrator runs stages sequentially, and apply calls it once.
There was a problem hiding this comment.
@stillalearner, that's a fair point — a single-process CLI with sequential stage execution and a single apply invocation has no concurrent callers of Build. The env mutation concern doesn't apply here. Thanks for the clarification!
✏️ Learnings added
Learnt from: stillalearner
Repo: migtools/crane PR: 388
File: internal/kustomize/runner.go:27-33
Timestamp: 2026-05-20T10:02:36.870Z
Learning: In the `migtools/crane` repository, `internal/kustomize.Runner.Build` is never called concurrently. Crane is a CLI tool: the transform orchestrator (`internal/transform/orchestrator.go`) runs stages sequentially, and `internal/apply/kustomize.go` calls `Build` exactly once per invocation. Process-global env var mutation in `Build` (via `setEnvVars`) is therefore safe and does not require a mutex guard.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
aufi
reviewed
May 20, 2026
aufi
left a comment
aufi
left a comment
Contributor
There was a problem hiding this comment.
Overall looks great, let comment on kustomize args parsing and please search for kubectl kustomize mentions in code/comments.
| } | ||
|
|
||
| // buildOptions maps CLI args to krusty.Options. | ||
| func (r *Runner) buildOptions() (*krusty.Options, []envVar, error) { |
Contributor
There was a problem hiding this comment.
Only suggestions I have are related to this kustomize args parsing. I'm aware of original PR adding kustomize-args options to crane CLI (that aprtially tried to solve potential problem with misusing crane binary to call dangerous commands as part of kustomize cmd execution with malformed parameters).
Looking on this method, I'd suggest:
- differentiate args to safe (in kustomize embeded into crane binary) and providing all crane use cases needs without custom adjustments (guessing plugins and not sure about load restrictors), those could be setup statically in code and removed from args parsing in method below (better UX since user don't have to care)
- others that are still needed to be adjusted by user (e.g. helm command name that is (at least for now) external)
Asking to re-check capabilities of args below and ensure we cover just those that are needed to be adjusted for kustomize in this embeded implementation and have the rest with reasonable working defaults.
Contributor
Author
There was a problem hiding this comment.
Yes, makes sense @aufi !
I am updating the --load-restrictor and --enable-alpha-plugins flag to default values, and removing those flags from the kustomize args
And only keeping helm and env flags.
…ther kubectl kustomize references
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/kustomize/runner.go (1)
69-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject empty
--helm-commandvalues and empty--envkeys inbuildOptions().
--helm-command=and--env =valuecurrently pass parsing and fail later with less actionable behavior. Validate these inputs at parse time.Suggested patch
case arg == "--helm-command": if i+1 >= len(r.Args) { return nil, nil, fmt.Errorf("--helm-command requires a value") } i++ + if strings.TrimSpace(r.Args[i]) == "" { + return nil, nil, fmt.Errorf("--helm-command requires a non-empty value") + } opts.PluginConfig.HelmConfig.Enabled = true opts.PluginConfig.HelmConfig.Command = r.Args[i] case strings.HasPrefix(arg, "--helm-command="): - val := strings.SplitN(arg, "=", 2)[1] + val := strings.TrimSpace(strings.SplitN(arg, "=", 2)[1]) + if val == "" { + return nil, nil, fmt.Errorf("--helm-command requires a non-empty value") + } opts.PluginConfig.HelmConfig.Enabled = true opts.PluginConfig.HelmConfig.Command = val @@ parts := strings.SplitN(r.Args[i], "=", 2) - if len(parts) != 2 { + if len(parts) != 2 || strings.TrimSpace(parts[0]) == "" { return nil, nil, fmt.Errorf("invalid env format %q, expected KEY=VALUE", r.Args[i]) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/kustomize/runner.go` around lines 69 - 90, Reject empty values when parsing --helm-command and validate env key when parsing --env/-e in buildOptions (or the function handling r.Args). Specifically, when handling the "--helm-command" case and the "--helm-command=" branch, check that the value (r.Args[i] or val) is not the empty string and return an error like "--helm-command requires a non-empty value" if it is; when parsing "--env" or "-e" split into parts with strings.SplitN and ensure parts[0] (the key) is not empty (return an error such as "env key must not be empty") before assigning to opts.PluginConfig.HelmConfig.Command or adding the env pair. Ensure opts.PluginConfig.HelmConfig.Enabled is only set after successful validation.
🧹 Nitpick comments (1)
internal/apply/kustomize.go (1)
146-153: 💤 Low valueHandle
encoder.Close()error.
yamlv3.Encoder.Close()can return an error when flushing the final document separator. While unlikely after a successfulEncode, ignoring it could mask issues.Suggested fix
encoder.SetIndent(2) // Set 2-space indentation if err := encoder.Encode(doc); err != nil { return fmt.Errorf("failed to encode YAML document: %w", err) } - encoder.Close() // Flush the encoder + if err := encoder.Close(); err != nil { + return fmt.Errorf("failed to finalize YAML document: %w", err) + } docBytes := buf.Bytes()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/apply/kustomize.go` around lines 146 - 153, The call to encoder.Close() is currently ignored; update the block that creates yamlv3.NewEncoder(&buf) so you capture and handle the error returned by encoder.Close() after calling encoder.Encode(doc) — e.g., check err from encoder.Close() and return fmt.Errorf("failed to close YAML encoder: %w", err) (or wrap appropriately) before using docBytes; reference the encoder variable, yamlv3.NewEncoder, encoder.Encode(doc), and encoder.Close() in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/kustomize/runner.go`:
- Around line 69-90: Reject empty values when parsing --helm-command and
validate env key when parsing --env/-e in buildOptions (or the function handling
r.Args). Specifically, when handling the "--helm-command" case and the
"--helm-command=" branch, check that the value (r.Args[i] or val) is not the
empty string and return an error like "--helm-command requires a non-empty
value" if it is; when parsing "--env" or "-e" split into parts with
strings.SplitN and ensure parts[0] (the key) is not empty (return an error such
as "env key must not be empty") before assigning to
opts.PluginConfig.HelmConfig.Command or adding the env pair. Ensure
opts.PluginConfig.HelmConfig.Enabled is only set after successful validation.
---
Nitpick comments:
In `@internal/apply/kustomize.go`:
- Around line 146-153: The call to encoder.Close() is currently ignored; update
the block that creates yamlv3.NewEncoder(&buf) so you capture and handle the
error returned by encoder.Close() after calling encoder.Encode(doc) — e.g.,
check err from encoder.Close() and return fmt.Errorf("failed to close YAML
encoder: %w", err) (or wrap appropriately) before using docBytes; reference the
encoder variable, yamlv3.NewEncoder, encoder.Encode(doc), and encoder.Close() in
your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2f930e7-6c79-4281-ab29-3d3037526de8
📒 Files selected for processing (9)
cmd/apply/apply.gocmd/transform/transform.gointernal/apply/kustomize.gointernal/kustomize/args.gointernal/kustomize/args_test.gointernal/kustomize/runner.gointernal/kustomize/runner_test.gointernal/transform/orchestrator.gointernal/transform/writer.go
✅ Files skipped from review due to trivial changes (2)
- internal/transform/writer.go
- cmd/transform/transform.go
aufi
approved these changes
May 21, 2026
| "strings" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| "sigs.k8s.io/kustomize/api/krusty" |
Contributor
There was a problem hiding this comment.
Good it looks it was already in deps (the kustomize evaluator).
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces all
exec.Commandcalls tokubectl kustomize/oc kustomizewith the embeddedsigs.k8s.io/kustomize/api/krustylibrary. Crane no longer requireskubectlorocto beinstalled for
transformandapplycommands.Addresses issue #369.
Approach
Created
internal/kustomize/Runnerthat wraps thekrusty.MakeKustomizer(opts).Run(fSys, path)API. The
Runner.Build(dir)method maps CLI--kustomize-argsflags tokrusty.Optionsfieldsand returns rendered YAML bytes — same interface as the old
exec.Commandcalls.Manual Testing on Minikube (kubectl removed from PATH)
All scenarios ran with
kubectlandocremoved fromPATHto prove crane operatesindependently. Export uses client-go (kubeconfig), not kubectl binary.
Scenario 1: Basic namespaced resources (no cluster-scoped)
crane-embed-s1with Deployment + ConfigMapScenario 2: Mixed namespaced + cluster-scoped (RBAC)
crane-embed-s2with Deployment + ServiceAccount + ClusterRole + ClusterRoleBindingresources/crane-embed-s2/, cluster-scoped underresources/_cluster/Scenario 3: Multi-stage transform
20_CustomEditssecond stage_cluster/separation preserved, apply picked last stage correctlyScenario 4: CRD + Custom Resource
widgets.cranetest.io+ Widget CR in namespacecrane-embed-s3_cluster/, Widget in namespace directory, both clean (no server metadata)Scenario 5: Content correctness
uid,resourceVersion,creationTimestamp,status— KubernetesPlugin patches applied correctly by embedded kustomizeUnit Tests
New:
internal/kustomize/runner_test.go(7 tests)TestBuild_BasicKustomizationTestBuild_WithPatchesTestBuild_WithLoadRestrictionsNone--load-restrictor=LoadRestrictionsNonemaps correctlyTestBuild_InvalidDirTestBuild_MixedNamespacedAndClusterScopedTestBuildOptions_ArgMappingExisting tests
All existing tests in
internal/transform/,internal/apply/,internal/kustomize/pass unchanged.Files Changed
internal/kustomize/runner.gointernal/kustomize/runner_test.gointernal/transform/orchestrator.gointernal/apply/kustomize.gointernal/file/file_helper.gocmd/apply/apply.gointernal/apply/kustomize_test.gointernal/transform/test_helpers.gointernal/kustomize/args.goSummary by CodeRabbit
New Features
Improvements
Behavioral Changes
Tests