Cloud-SDK extraction B/C/D — PR 9: gke cross-process wiring#681
Merged
Conversation
Addresses the code-reviewer findings on the grpcKubernetesBackend adapter: - Critical: injectCredentials wrote camelCase config_json keys (projectId/serviceAccountJSON) but the deleted in-core gkeBackend and buildResourceSpec's own doc-comment use snake_case. Resolved credentials now land under project_id/service_account_json — the keys workflow-plugin-gcp's GKEDriver (Task 22) actually reads. - Important: pinned the config_json WRITE-key contract as k8sConfigKey* consts, symmetric with the already-pinned outputs_json READ-key consts (k8sOutputKey*). Per ADR 0037 the host adapter owns both halves of the key contract. - Important: dropped the "1.29" GKE-version default from the host adapter — version defaulting is GKE-domain knowledge that belongs in the plugin's GKEDriver, not this generic adapter. The user-supplied version flows through verbatim. Added a test asserting resolved credentials use the pinned snake_case keys and guarding against a camelCase regression. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds cross-process wiring so platform.kubernetes can dispatch plugin-provided cluster backends (notably type: gke) via the existing ResourceDriver gRPC contract, enabling the ongoing cloud-SDK extraction work.
Changes:
- Introduces a
plugin.KubernetesBackendProvideroptional interface and implements it inExternalPluginAdapterto exposeResourceDriverClientinstances for kubernetes backends. - Adds an engine-populated, module-side
kubernetesBackendClientRegistryand updatesPlatformKubernetes.Init()to route non-core cluster types (e.g.gke) to a new gRPC-backed backend. - Adds
grpcKubernetesBackend(host-side adapter overpb.ResourceDriverClient) plus focused unit tests for registry behavior, dispatch, and CRUD/status round-trips.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/kubernetes_backend_provider.go | Adds the optional plugin interface for exposing kubernetes backend gRPC clients. |
| plugin/external/adapter.go | Implements KubernetesBackendClients() by checking the contract registry + Capabilities for infra.k8s_cluster. |
| module/platform_kubernetes.go | Routes core types to in-process factories; routes non-core types to the plugin client registry and gRPC adapter. |
| module/platform_kubernetes_plugin_registry.go | Adds the singleton registry to map cluster type → pb.ResourceDriverClient, with reserved-name protection. |
| module/platform_kubernetes_plugin_registry_test.go | Tests registry behavior and PlatformKubernetes.Init() dispatch/errors for type: gke. |
| module/platform_kubernetes_grpc.go | Adds the grpcKubernetesBackend adapter implementing the in-core kubernetesBackend interface over ResourceDriver. |
| module/platform_kubernetes_grpc_test.go | Adds lifecycle/round-trip tests for plan/apply/status/destroy and JSON-bytes projection. |
| engine.go | Registers plugin-provided kubernetes backend clients into the module registry during plugin load. |
Comment on lines
+238
to
+241
| cfg[k8sConfigKeyProjectID] = creds.ProjectID | ||
| } | ||
| if len(creds.ServiceAccountJSON) > 0 { | ||
| cfg[k8sConfigKeyServiceAccountJSON] = string(creds.ServiceAccountJSON) |
Contributor
Author
Comment on lines
+185
to
+192
| // cluster by — keyed on the cluster name and the infra.k8s_cluster type. | ||
| func (b *grpcKubernetesBackend) buildResourceRef(k *PlatformKubernetes) *pb.ResourceRef { | ||
| return &pb.ResourceRef{ | ||
| Name: k.clusterName(), | ||
| Type: gkeResourceType, | ||
| } | ||
| } | ||
|
|
Contributor
Author
Comment on lines
+249
to
+252
| // map→struct projection ADR 0037 assigns to Tasks 25/26. The adapter sets | ||
| // Provider="gke" itself and tolerates a missing/empty outputs_json. | ||
| func kubernetesClusterStateFromOutput(name string, out *pb.ResourceOutput) (*KubernetesClusterState, error) { | ||
| st := &KubernetesClusterState{Name: name, Provider: "gke", Status: "not-found"} |
Contributor
Author
Comment on lines
+101
to
+103
| if _, ok := kubernetesBackendClientRegistryInstance.resolve("gke"); ok { | ||
| t.Skip("a gke client is registered by a concurrent test; skipping the negative case") | ||
| } |
Contributor
Author
Comment on lines
+9
to
+13
| // kubernetesBackend → ResourceDriver RPC | ||
| // plan → Read (probe existence, synthesize create|noop plan) | ||
| // apply → Create (AlreadyExists resolves to success) | ||
| // status → Read (project outputs_json onto KubernetesClusterState) | ||
| // destroy → Delete (NotFound resolves to success) |
Contributor
Author
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
5 substantive Copilot comments on module/platform_kubernetes_grpc.go, all addressed: 1. injectCredentials precedence — module config now wins; cloud-account creds only fill missing project_id / service_account_json keys. Mirrors the in-core gkeBackend's config-first precedence. 2. buildResourceRef.ProviderId — now populated with the fully-qualified GKE path projects/<project>/locations/<location>/clusters/<name> (GKE cluster names are not globally unique). gkeProject / gkeLocation helpers port the in-core resolution: config-first, cloud-account fallback. 3. kubernetesClusterStateFromOutput — call sites now pass k.name (the module name) instead of k.clusterName(); matches the in-core PlatformKubernetes.Init semantics where state.Name = m.name and is stable when clusterName differs. 4. TestPlatformKubernetes_GKEWithoutPluginErrors — replaced the nondeterministic t.Skip with deterministic mutex-guarded clear-and-restore of the registry entry. Added tests locking each new behavior (module-config-first precedence, fully-qualified ProviderId in all 3 resolution paths, state.Name=module name when clusterName differs). PR description (item 5) updated separately to match the implementation: plan→Read / apply→Create-only per ADR 0037. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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.
Summary
module/platform_kubernetes_grpc.go—grpcKubernetesBackendadapter implementing the in-corekubernetesBackendinterface, delegating topb.ResourceDriverClientper ADR 0037 Option 1 (ResourceDriver fold). plan→Read, apply→Create, status→Read, destroy→Delete (AlreadyExistsresolves apply to success;NotFoundresolves destroy to success and yields a clean not-found status — preserving the deleted in-coregkeBackend's swallow-behavior). JSON-bytes converters; no proto change.module/platform_kubernetes_plugin_registry.go—kubernetesBackendClientRegistry(gke→pb.ResourceDriverClient);RegisterKubernetesBackendClient. Mirrors the Phase Aiac_state_plugin_registry.goprecedent.engine.goloadPluginInternalseam populating the registry fromplugin.KubernetesBackendProvider-implementing plugins.ExternalPluginAdapter.KubernetesBackendClients()checks the plugin advertisespb.ResourceDrivervia the contract registry, callsCapabilities, and returns clients forgkeonly when the plugin listsinfra.k8s_cluster.module/platform_kubernetes.goInit()—type: kind|k3s|eks|aksuse the in-core factory map unchanged; any other type (gke) consults the new client registry. Clean "install workflow-plugin-gcp" error when no client.config_jsonwrite-key contract pinned ask8sConfigKey*consts (project_id,service_account_json); read-sidek8sOutputKey*consts; symmetric, snake_case, ADR-0037-faithful. Module-config-first precedence: explicitproject_id/service_account_jsonin module config win; cloud-account creds only fill missing keys (mirrors the in-coregkeBackend).buildResourceRef.ProviderIdpopulated with the fully-qualified GKE pathprojects/<project>/locations/<location>/clusters/<name>(GKE cluster names are not globally unique; matches the in-core addressing).Plan PR 9 of the cloud-SDK-extraction B/C/D plan. Gates PR 10 (Phase C core deletion).
Cross-process Read/Delete prerequisite
The cross-process gke flow's Read/Delete path requires the
workflow-plugin-gcpplugin to handle the fully-qualifiedResourceRef.ProviderIdthis PR writes. That fix lives on thefeat/gcs-gke-storagebranch ofworkflow-plugin-gcp(gkeResourceNamechokepoint inrealGKEClient— commitsce0059f0and earlier) and ships via plan #2's PR 3, not a separate locked-plan PR. A workflow-core engine on this PR will require aworkflow-plugin-gcpbuild that includes the FQN-aware client for cross-process gke Read/Delete to function.Test plan
platform_kubernetes_test:type: gkewith registered client →grpcKubernetesBackend; with no client → "install plugin" error (deterministic — mutex-guarded clear-and-restore, not.Skip)grpcKubernetesBackendlifecycle tests (plan/apply/status/destroy incl.KubernetesClusterStatethrough JSON-bytes; idempotency onAlreadyExists/NotFound)project_id/service_account_jsonin module config win over cloud account)ProviderIdtest (3 resolution paths: module config, cloud-account fallback, unresolvable→empty)state.Name= module name (notclusterName) teststructpb/Any(iac.proto invariant)🤖 Generated with Claude Code