Kubectl removal - Phase 1#22
Conversation
…overy Implements the foundation & client setup for kubectl query removal. - Add pkg/k8s/client.go with NewClient(), GetCurrentNamespace(), WrapError() - kubectl-compatible kubeconfig discovery (KUBECONFIG env -> ~/.kube/config -> in-cluster) - Namespace resolution matching kubectl behavior - Error wrapping with apierrors.IsNotFound() and IsForbidden() - Add client-go v0.35.0 and apimachinery v0.35.0 dependencies - 15 unit tests Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Replace kubectl exec with clientset.CoreV1().Nodes().List() - Add context.Context and kubernetes.Interface parameters - Use corev1.NodeExternalIP/NodeInternalIP constants - Add extractAddresses helper for typed field access - Remove getNodeIPsByType kubectl-based helper Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- TestGetNodeIPs_ExternalIP: prefer external IPs when available - TestGetNodeIPs_FallbackToInternal: fallback to internal IPs - TestGetNodeIPs_NoNodes: error on empty cluster - TestGetNodeIPs_NoAddresses: error when node has no addresses - TestGetNodeIPs_Deduplication: deduplicate same IPs across nodes - TestGetNodeIPs_MixedAddressTypes: only extract IP types, not Hostname/DNS - TestGetNodeIPs_OnlyHostname: error when only Hostname available Uses fake.NewSimpleClientset() for unit testing without cluster Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Replace kubectl exec with clientset.NodeV1().RuntimeClasses().List() - Update function signature: add context.Context and kubernetes.Interface params - Remove custom runtimeClassList struct, use official nodev1.RuntimeClass - Update cmd/init.go and cmd/apply.go callers to use new signature - Maintain graceful degradation: return default on error Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Add 8 test cases using fake.NewSimpleClientset() - Test SNP handler detection - Test TDX handler detection (when no SNP present) - Test no match returns default - Test empty cluster returns default - Test case-insensitive handler matching - Test handler contains "snp" substring - Test handler contains "tdx" substring Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Change runApply to use cmd parameter for context access - Thread context through transformManifest and handleSidecarServerCert - Replace context.Background() with passed context for proper signal handling - Enables graceful cancellation of Kubernetes API calls on SIGINT/SIGTERM Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
The test was calling runInit without setting a context on the cobra command. When auto-detecting RuntimeClass, cmd.Context() returned nil, causing a panic in the Kubernetes client. Add cmd.SetContext(context.Background()) before runInit to provide a valid context for Kubernetes API operations. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates several Kubernetes “read” operations away from kubectl subprocess calls to client-go, introducing a shared Kubernetes client factory and expanding test coverage using fake clientsets.
Changes:
- Added
pkg/k8sclient factory (NewClient,GetCurrentNamespace,WrapError) and migrated namespace resolution to use it. - Reworked cluster/runtime, node IP, trustee deployment, and secret inspection read paths to use
client-gowithcontext.Context. - Added extensive unit/integration tests (fake clientsets) and improved offline/
--skip-applybehavior (including namespace precedence and sidecar cert YAML output).
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/trustee/trustee.go |
Replace kubectl-based “is deployed” and namespace existence checks with client-go APIs; update Deploy signature. |
pkg/trustee/trustee_test.go |
Add tests for IsDeployed and ensureNamespace. |
pkg/trustee/kbs.go |
Use client-go for KBS pod discovery; thread context through upload paths. |
pkg/trustee/kbs_test.go |
Add fake-client tests for KBS pod selection behavior. |
pkg/secrets/kubernetes.go |
Replace kubectl JSON parsing with typed client-go reads; generate Secret YAML via native YAML marshal. |
pkg/secrets/kubernetes_test.go |
Large new test suite for secret inspection/offline mode/YAML generation. |
pkg/secrets/secrets.go |
Use pkg/k8s namespace resolution; add ctx + client-go fallback for imagePullSecrets via default ServiceAccount. |
pkg/secrets/output_test.go |
Switch config parsing expectations from JSON to YAML. |
pkg/k8s/client.go |
New shared kubeconfig discovery + namespace resolution + API error wrapper. |
pkg/k8s/client_test.go |
Tests for client factory, namespace discovery, and error wrapping. |
pkg/cluster/runtimeclass.go |
Replace kubectl runtimeclass detection with client-go list. |
pkg/cluster/runtimeclass_test.go |
Tests for runtime class selection logic. |
pkg/cluster/nodes.go |
Replace kubectl node IP extraction with client-go list + address filtering. |
pkg/cluster/nodes_test.go |
Tests for node IP selection/dedup logic. |
cmd/root.go |
Add kubectl availability detection cached in context. |
cmd/init.go / cmd/init_test.go |
Use client-go for runtime class + trustee checks; ensure tests set cobra context. |
cmd/apply.go |
Add --namespace flag + kubectl-optional --skip-apply flows; offline secret resolution; sidecar cert file output. |
cmd/apply_skip_test.go |
Tests for namespace precedence, skip-apply sidecar cert YAML output, and actionable error formatting. |
integration_test/workflow_test.go |
Add integration-ish tests validating key queries via fake clientsets without kubectl. |
go.mod / go.sum |
Add Kubernetes client-go dependencies and update module metadata. |
.planning/phases/02-simple-read-operations/02-UAT.md |
UAT notes and resolution for init test context issue. |
Comments suppressed due to low confidence (1)
pkg/trustee/trustee.go:145
ensureNamespacereturns nil for any error other than NotFound, which can mask real failures (e.g., cluster unreachable, auth errors) and lead to harder-to-debug follow-on errors later. Consider explicitly handling Forbidden (if you want to assume the namespace exists) but returning a wrapped error for other unexpected failures.
_, err := clientset.CoreV1().ServiceAccounts(namespace).List(ctx, metav1.ListOptions{Limit: 1})
if err == nil {
// Successfully accessed resources in namespace, so it exists
return nil
}
// Check if the error indicates namespace doesn't exist
if apierrors.IsNotFound(err) {
// Namespace doesn't exist, try to create it with kubectl
// (namespace creation via client-go is out of scope for this phase)
cmd := exec.Command("kubectl", "create", "namespace", namespace)
output, err := cmd.CombinedOutput()
if err != nil && !strings.Contains(string(output), "AlreadyExists") {
return fmt.Errorf("failed to create namespace: %w\n%s", err, output)
}
return nil
}
// For any other error (e.g., Forbidden when user lacks namespace creation permissions
// but namespace exists), assume namespace exists and proceed.
// Subsequent operations will fail appropriately if the namespace truly doesn't exist.
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // isKubectlAvailable retrieves the cached kubectl availability from context | ||
| func isKubectlAvailable(ctx context.Context) bool { | ||
| if v := ctx.Value(kubectlAvailableKey); v != nil { | ||
| return v.(bool) |
There was a problem hiding this comment.
isKubectlAvailable uses a direct type assertion v.(bool), which will panic if the context value is ever set to a non-bool (or if a different key collides). Prefer a safe assertion (b, ok := v.(bool)) and default to false when it’s missing/unexpected.
| return v.(bool) | |
| if b, ok := v.(bool); ok { | |
| return b | |
| } |
pkg/trustee/trustee_test.go
Outdated
| err := ensureNamespace(ctx, fakeClient, "nonexistent-ns") | ||
|
|
||
| // The function will try to run kubectl create namespace, which will fail in test environment. | ||
| // We verify it doesn't panic and gracefully handles the missing namespace. | ||
| t.Logf("ensureNamespace returned: %v (expected: nil or kubectl error)", err) |
There was a problem hiding this comment.
This test doesn’t currently assert any behavior (it only logs). Also, fake.NewSimpleClientset() typically won’t return a NotFound error for listing ServiceAccounts in a non-existent namespace, so it won’t exercise the apierrors.IsNotFound(err) branch. Consider using a client-go reactor to force a NotFound on the List call and assert whether ensureNamespace returns the expected error/success for the create-namespace path.
- Replace kubectl subprocess calls with clientset.CoreV1().Secrets().Get() - InspectSecret now returns *corev1.Secret (typed API) instead of *SecretKeys - InspectSecrets returns map[string]*corev1.Secret for batch operations - Empty namespace resolves via k8s.GetCurrentNamespace() before API call - Add SecretToSecretKeys/SecretsToSecretKeys adapters for backward compatibility - Remove GetCurrentNamespace function (replaced with k8s.GetCurrentNamespace) - Update cmd/apply.go to create k8s.Client and use new API signatures - Update pkg/secrets/secrets.go to use k8s.GetCurrentNamespace() Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- TestInspectSecret_Found - Secret exists in fake clientset - TestInspectSecret_NotFound - Secret doesn't exist - TestInspectSecret_WithExplicitNamespace - Explicit namespace provided - TestInspectSecret_EmptyNamespaceResolution - Empty namespace resolves to context namespace - TestInspectSecret_DataFieldDecoding - Data values are auto-decoded - TestInspectSecrets_MultipleSecrets - Batch query multiple secrets - TestInspectSecrets_NoLookupNeeded - Ref with NeedsLookup=false - TestInspectSecrets_FailFast - First error stops batch processing All tests use fake.NewSimpleClientset for testability Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
102fcb5 to
993dd6c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace kubectl subprocess with clientset.AppsV1().Deployments().List() - Add ctx and clientset parameters to IsDeployed and Deploy functions - Handle not found errors as false instead of error - Add 3 unit tests with fake clientset (found/not found/wrong label) - Update Deploy function signature to thread ctx and clientset to ensureNamespace Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Replace kubectl subprocess call with clientset.CoreV1().Pods().List() - Use LabelSelector "app=kbs" to filter pods - Return first matching pod name or error if none found - Update WaitForKBSReady to accept ctx + clientset parameters - Update populateSecrets to create k8s client for pod queries - Add comprehensive unit tests with fake clientset - TestGetKBSPodName_Found: pod with label exists - TestGetKBSPodName_NotFound: no pods found - TestGetKBSPodName_MultiplePods: returns first pod Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Replace the kubectl-based namespace creation with a direct client-go Namespaces().Create() call and add comprehensive unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace kubectl subprocess call with clientset.CoreV1().ServiceAccounts().Get() - Access ImagePullSecrets via typed field sa.ImagePullSecrets[0].Name - Resolve empty namespace to current context namespace before API call - Remove custom ServiceAccount struct (lines 277-286) - Remove unused encoding/json import - Update DetectImagePullSecretsWithServiceAccount to create k8s client - Add comprehensive unit tests with fake clientset - TestGetServiceAccountImagePullSecrets_Found: returns first secret - TestGetServiceAccountImagePullSecrets_NotFound: serviceaccount missing - TestGetServiceAccountImagePullSecrets_NoSecrets: empty imagePullSecrets - TestGetServiceAccountImagePullSecrets_EmptyNamespace: namespace resolution Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Pass cmd parameter to handleTrusteeSetup for context access - Create k8s client and pass ctx+clientset to IsDeployed and Deploy - Fixes build failure from Phase 4 signature changes Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Add contextKey type and kubectlAvailableKey constant - Add detectKubectl function using exec.LookPath - Add isKubectlAvailable helper to retrieve cached value - Add requireKubectl guard with installation instructions Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- cmd/init.go: detectKubectl in handleTrusteeSetup, requireKubectl before trustee.Deploy - cmd/apply.go: detectKubectl at start of runApply, requireKubectl check before any operations - Thread enhanced context through transformManifest and downstream calls Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Add TestWorkflow_InitDetection: validates RuntimeClass detection, node IP extraction, and Trustee deployment checks - Add TestWorkflow_SecretInspection: validates single and batch secret queries - Add TestWorkflow_TrusteeQueries: validates Trustee deployment and KBS pod queries - All tests use fake.NewSimpleClientset pattern - Tests validate query operations work via client-go when kubectl not available Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Add --namespace/-n flag to apply command for explicit namespace override - Implement resolveNamespace() with kubectl precedence: flag > manifest > kubeconfig > default - Return error when --namespace flag conflicts with manifest namespace (kubectl behavior) - Replace all getCurrentNamespace() calls in apply.go with resolveNamespace() or k8s.GetCurrentNamespace() - Use k8s.GetCurrentNamespace() for offline kubeconfig reading (no kubectl binary needed) Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Check skipApply flag before kubectl detection in runApply() - Print informational message when skipApply=true and kubectl not found - Preserve existing requireKubectl() error for non-skip-apply mode - Skip-apply mode uses cmd.Context() directly (no kubectl detection needed) Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Add skipApply and manifestPath parameters to handleSidecarServerCert()
- Guard trustee.UploadResources() call with if !skipApply condition
- Implement saveSidecarCertsToYAML() to save kubernetes.io/tls Secret
- Cert file uses {basename}-sidecar-certs.yaml naming convention
- File permissions set to 0600 matching existing secret file pattern
- Update call site in transformManifest() to pass new parameters
Assisted-by: AI
Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Test namespace resolution with --namespace flag (priority, matching, conflict) - Test namespace resolution from manifest metadata.namespace - Test namespace resolution fallback to kubeconfig context namespace - Test namespace resolution fallback to "default" when no source available - Test sidecar cert file saving (YAML structure, TLS Secret format, permissions) Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
…ntset guard
- Replace exec.Command("kubectl") with yaml.Marshal() in GenerateSealedSecretYAML
- Use stringData field (functionally equivalent to data for kubectl apply)
- Add nil clientset guard in InspectSecrets for NeedsLookup=true refs
- Add describeUsageTypes helper for descriptive error messages
- Eliminates kubectl dependency for offline skip-apply sealed secret generation
Assisted-by: AI
Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- TestGenerateSealedSecretYAML_NativeYAML: verify correct Secret structure - TestGenerateSealedSecretYAML_EmptyNamespace: verify namespace omission - TestGenerateSealedSecretYAML_SpecialCharacters: verify JWS-format handling - TestInspectSecrets_NilClientset_OfflineRefsSucceed: offline path works - TestInspectSecrets_NilClientset_ClusterRefsError: cluster refs fail with nil - TestInspectSecrets_NilClientset_MixedRefs: mixed refs fail-fast behavior Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- Split secret refs into offlineRefs (NeedsLookup=false) and clusterRefs (NeedsLookup=true) - Process offline refs with nil clientset (no cluster connection required) - Process cluster refs with real client, providing actionable errors on failure - Add secretsClusterUnreachableError helper for descriptive offline mode errors - Add secretsClusterQueryError helper for cluster query failures - Offline refs skip cluster connection entirely - Cluster refs provide clear guidance when unreachable Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- handleImagePullSecrets returns nil early when cluster unreachable in skip-apply mode - Print informative message about skipped imagePullSecret inspection - Preserve cluster requirement for non-skip-apply mode (no behavior change) - Allow offline manifest generation even when imagePullSecrets can't be inspected - Users see clear guidance to ensure cluster is reachable if imagePullSecrets needed Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- TestOfflineSecretResolution_EndToEnd: Full pipeline (InspectSecrets -> ConvertSecrets -> URIs) - TestOfflineSecretResolution_ConsistentWithCluster: Proves offline and cluster URIs match - Ensures URI format consistency across resolution paths Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
- TestSkipApply_SecretsClusterUnreachableError_Format: Validates actionable error content - TestSkipApply_SecretsClusterQueryError_Format: Validates query error format - TestSkipApply_SecretRefSplitting: Validates NeedsLookup-based partitioning - Error messages include secret names, usage types, and fix suggestions Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
…utput GenerateTrusteeConfig marshals to YAML via yaml.Marshal, but tests were using json.Unmarshal to parse the output, causing 3 test failures with "invalid character 's' looking for beginning of value". Changed tests to use yaml.Unmarshal to match the actual output format. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
GetCurrentNamespace intentionally falls back to in-cluster namespace or "default" when kubeconfig is missing or invalid. The previous code checked err != nil but returned nil error, which triggered the nilerr linter. Simplified by ignoring the error with _ since the empty-namespace check that follows handles the fallback identically. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Replace manual os.Setenv/os.Unsetenv calls with t.Setenv which automatically restores the original value on test cleanup. This fixes errcheck lint violations Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Rename SecretsToSecretKeys to ToSecretKeys to avoid stuttering when called as secrets.ToSecretKeys (revive lint). Update all callers in cmd/apply.go and kubernetes_test.go. Also fix unused parameter 't' in TestEnsureNamespace_NotFound by replacing bare _ = err with t.Logf to log the result. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
…allers populateSecrets created context.Background() and its own k8s.NewClient internally. Thread both context.Context and kubernetes.Interface as parameters so callers pass their existing context and client. Updated UploadResource, UploadResources, and all callers in cmd/apply.go and cmd/init.go. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
resolveNamespace(namespaceFlag, m.GetNamespace()) was called at line 204 (stored in resolvedNamespace) and again at line 224 inside the sidecar block with identical arguments. The function is pure, so the second call always returns the same value. Use the existing variable. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
applyWithKubectl created context.Background() internally, discarding the command context from cmd.Context(). This meant SIGINT during kubectl apply would not terminate the subprocess. Accept ctx as a parameter so the caller's context propagates to exec.CommandContext. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Client-go uses kubeconfig for cluster access, not kubectl. Update error messages to say "Ensure kubeconfig is properly configured" instead of "Ensure kubectl is configured". Also fix CA cert error to reference 'cococtl init' instead of 'kubectl coco init'. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
applyManifest and its callers (createAuthSecretFromKeys, deployConfigMaps, deployPCCSConfigMap, deployKBS) did not accept context, so kubectl subprocesses could not be cancelled via SIGINT. Thread ctx from Deploy() through to exec.CommandContext in applyManifest. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
The function created its own k8s.NewClient internally for the service account fallback lookup. Its caller handleImagePullSecrets then created a second client for InspectSecrets. Accept kubernetes.Interface as a parameter so the caller passes one client for both operations. When clientset is nil, the SA fallback is skipped gracefully. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
handleSecrets, handleImagePullSecrets, and handleSidecarServerCert each created their own k8s.NewClient, resulting in three redundant client initializations per apply invocation. Create the client once in transformManifest and pass kubernetes.Interface + clientErr to all three handlers. Assisted-by: AI Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
993dd6c to
82dde93
Compare
Allows the kubectl call to be cancelled Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
No description provided.