Repository navigation
Conversation
Neelabh94
added
enhancement
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the job orchestration layer to support generic Kubernetes clusters alongside GKE. By introducing a flexible provider-based detection mechanism and decoupling environment prerequisites, the CLI now adapts its behavior based on the detected cluster type. Additionally, it improves OCI registry interaction and packaging logic to support private or air-gapped environments while maintaining full parity for existing GKE workflows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for generic and air-gapped Kubernetes (k8s) clusters alongside GKE, allowing users to manage jobs and build container images on non-GCP environments. It implements automatic orchestrator detection, adds configuration options for air-gapped environments (GCLUSTER_AIR_GAPPED) and insecure registries (GCLUSTER_INSECURE_REGISTRY), and updates prerequisite checks to bypass Google Cloud dependencies when in Kubernetes mode. Additionally, the image builder is enhanced to support non-GAR OCI registries with proper working directory alignment and credential verification. Feedback on the changes suggests adding a defensive check in pkg/imagebuilder/crane_builder.go to ensure the retrieved image configuration is not nil before deep-copying it, preventing a potential nil pointer dereference.
| cf, err := newImg.ConfigFile() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to pull base image %q: %w", baseImage, err) | ||
| return nil, fmt.Errorf("failed to inspect image config for WORKDIR update: %w", err) | ||
| } | ||
|
|
||
| newImg, err := appendLayers(baseImg, tarLayer) | ||
| cfg := cf.Config.DeepCopy() |
There was a problem hiding this comment.
In appendLayerAndWorkdir, newImg.ConfigFile() is called to retrieve the image configuration. While the error is checked, cf itself can theoretically be nil in some edge cases or custom image implementations. To prevent a potential nil pointer dereference when calling cf.Config.DeepCopy(), add a defensive check to ensure cf is not nil before accessing its fields.
cf, err := newImg.ConfigFile()
if err != nil {
return nil, fmt.Errorf("failed to inspect image config for WORKDIR update: %w", err)
}
if cf == nil {
return nil, fmt.Errorf("failed to inspect image config for WORKDIR update: config file is nil")
}
cfg := cf.Config.DeepCopy()
ffc169f to
f1a5bb5
Compare
Neelabh94
changed the title
1d8ea0d to
108ec87
Compare
…lIn, and air-gap telemetry guard - Add provider-agnostic kubeconfig loading, deterministic context resolution, cross-OS exec-plugin basename normalization, and --context pinning helpers in pkg/orchestrator/k8s. - Refactor pkg/orchestrator/gke kube_auth, inspector, and namespace resolution onto pkg/orchestrator/k8s, move kube_auth type declarations into types.go, add empty-context-safe kubectlIn, and extend TestNoUnpinnedKubectlCalls across pkg/orchestrator/... - Add non-probing GCLUSTER_AIR_GAPPED Environment state classification and skip Clearcut CLI telemetry uploads when air-gapped so disconnected runs exit immediately.
…onor current-context, and reuse ExtractRegion
…tighten doc comments, and defer ResolveContext
…ersionFunc in TestExecute
6dda440 to
cfd69aa
Compare
cfd69aa to
0b7e890
Compare
Summary
Decouples
gcluster jobfrom hardcoded Google Cloud / GKE workstation prerequisites by introducing provider-registered Layer-1 orchestrator detection and orchestrator-scoped CLI prerequisites while preserving 100% GKE behavior parity.pkg/orchestrator/orchestrator.go,pkg/orchestrator/detector.go,pkg/orchestrator/k8s/kubeconfig.go,pkg/orchestrator/gke/kube_auth.go,pkg/orchestrator/gke/k8s_bridge.go):ProviderSpecregistration (Prerequisites,ResolveContext,Probe,Factory) plusk8s.ResolveContext,k8s.ClusterServerHostname(including schemelesshost:portentries), andk8s.HasClientCertificateAuthsopkg/orchestratorandpkg/orchestrator/k8sremain provider-agnostic.GCLUSTER_ORCHESTRATOR(gke|k8s) override ->gke-gcloud-auth-pluginexec credential check (including Windows backslash and.exepaths) ->*.gke.googAPI server endpoint check -> bounded2skubectl --context <ctx> version -o json --request-timeout=2scheck onserverVersion.gitVersioncontaining-gke.(skipped when air-gapped or using X.509 client-certificate kubeconfigs such askubeadm/kind) ->TypeK8s.TypeK8sregistration and context/hint helpers inpkg/orchestrator/gke/k8s_bridge.go, routescmd/jobthrough(&orchestrator.Detector{}).Detectandorchestrator.NewJobOrchestrator(), switchesGKEOrchestrator.isGKEinpkg/orchestrator/gke/telemetry.goto use the detectedmode, guardsTypeK8sagainst 0-capacity KueueClusterQueuegeneration when aLocalQueueis absent, and replaces Google Cloud Console URLs with context-pinnedgcluster job inspect --name/gcluster job logs/gcluster job cancel(--cluster/--gke-namespace) andkubectl --contexthints inTypeK8smode while keeping in-cluster SPS endpoint logs intact. (Note: migrating the pure-Kubernetes domain filescancel.go,kueue.go,infra_manager.go,manifest_generator.go, andscheduling.gointopkg/orchestrator/k8sbehind a provider interface is intentionally staged into a dedicated follow-up PR so this PR stays review-sized.)TypeK8sCLI Prerequisites & Saved Context Isolation (pkg/orchestrator/prereqs.go,cmd/job/prereq.go,cmd/job/job.go):BasicPrerequisitesqueries each provider's registeredProviderSpec.Prerequisites(TypeK8sdeclares[PrereqKubectl], skippinggcloud,gke-gcloud-auth-plugin, and Google Cloud ADC checks) without cross-modePrereqStatecache poisoning or unexpired GKE tool-cache loss, while preserving--cluster/--location/--projectvalidation onTypeGKE.~/.gcluster/context.jsonGKE defaults whenGCLUSTER_ORCHESTRATOR=k8sis set or whenDetectresolvesTypeK8s, clearing non-explicitlocation/projectIDdefaults while preserving savedjob configresolution forTypeGKE.Cloud-Docs: none
Testing
KUBECONFIG=/dev/null GOOGLE_API_USE_CLIENT_CERTIFICATE=false go test -race -count=1 ./pkg/shell/... ./pkg/orchestrator/... ./pkg/telemetry/... ./cmd/job/...TestResolveContext,TestClusterServerHostname,TestHasClientCertificateAuth,TestProviderRegistryAndFactory,TestRegisterProvider_PanicsOnInvalidSpec,TestDetector_ProviderDispatchAndFallback,TestDetector_EnvOverride,TestRegisteredTypes_IncludesGKEAndK8s,TestDetect_FourStepPrecedenceMatrix,TestDetect_HonorsDefaultEnvironmentAirGappedState,TestBasicPrerequisites,TestTypeK8s_ZeroGCloudAcrossLifecycle,TestTypeK8s_ConfigureKubectlUnseeded,TestTypeK8s_LogsAndConflictHintsOmitCloudConsole,TestTypeK8s_QueueGuardAndCompletionStatus,TestIsGKE_UsesDetectedPlatformMode,TestJobCmd_TypeK8sRequiresOnlyKubeconfigAndZeroGCloud,TestJobCmd_MissingContextAndRequiredGKEFlags,TestJobCmd_MissingClusterNotMaskedByActiveGKEContext,TestJobCmd_TypeK8sSubmitPrebuiltImageZeroFlags,TestJobCmd_SavedGKEConfigDoesNotBreakTypeK8sOrEnvOverride,TestEnsureBasicPrerequisitesFor_NoCrossModeCachePoisoning, andTestEnsureK8sOnlyBasicPrerequisites_MissingKubectl.