fix(k8s-client): fix error classification for failed kubeconfig ingestion#1743
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/k8s/client/client_test.go`:
- Around line 41-56: Extend the invalid kubeconfig cases in the BuildKubeClient
tests to assert the structured “kubeconfig” error context contains the resolved
path. Cover both the explicit kubeconfigArg and kubeconfigEnv scenarios,
including the additional affected cases, while preserving the existing error
code and message assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 88525997-b835-48ed-ae9c-9c55df83d25c
📒 Files selected for processing (2)
pkg/k8s/client/client.gopkg/k8s/client/client_test.go
6f1c6fd to
afc51eb
Compare
mchmarny
left a comment
There was a problem hiding this comment.
Direction is right and the test coverage is solid — file-derived kubeconfig failures are deterministic caller config and should not report as retryable INTERNAL. One medium finding: four call sites (serializer, snapshotter, cli/validate) still re-wrap these errors with ErrCodeInternal, so the new code gets masked on exactly the SDK paths the description targets; pkg/validator's PropagateOrWrap precedent is the fix. Plus one godoc nit. Nothing blocking merge.
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/cli/validate.go`:
- Around line 159-161: Update deployAgentForValidation to create the Kubernetes
client through the config-aware helper using cfg.kubeconfig instead of the
default GetKubeClient call. Also update the snapshotter error handling in the
same function to propagate the original error while preserving the appropriate
error context, rather than flattening it to ErrCodeInternal.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7390712a-30b3-42d4-8331-5d000757d3bb
📒 Files selected for processing (7)
pkg/cli/validate.gopkg/serializer/configmap.gopkg/serializer/configmap_test.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/snapshotter/agent.gopkg/snapshotter/agent_test.go
…tion Signed-off-by: Tjark Gunnar Rasche <trasche@nvidia.com>
4560d70 to
ff4a5d7
Compare
mchmarny
left a comment
There was a problem hiding this comment.
Latest head rechecked. File-derived kubeconfig failures now preserve ErrCodeInvalidRequest and path context through the affected consumers; in-cluster failures remain ErrCodeInternal. Focused race tests and lint pass, CI is green, and no merge blockers remain.
Summary
Classify kubeconfig loading, parsing, and file-derived client construction failures as
ErrCodeInvalidRequestinstead ofErrCodeInternal.Surfaced while working on a unrelated PR as a finding by @mchmarny: #1735 (comment)
Motivation / Context
Invalid caller configuration is deterministic and should not be reported as a retryable internal failure to SDK and API consumers.
Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
File-derived configuration errors are now
INVALID_REQUEST; in-cluster configuration failures remainINTERNAL. Existing causes and kubeconfig-path context are preserved.Testing
All tests, lint checks, e2e tests, and scans passed.
Risk Assessment
Rollout notes: No migration required. This changes the structured code returned for invalid kubeconfig input.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info