Conversation
Vertex AI providers currently source project ID and region from global control plane env vars, preventing multi-tenant flexibility. This spec adds a `config` map to the Provider declaration schema so all provider configuration lives alongside the credential reference. The config map is generic — any provider type can use it for passthrough config to the OpenShell gateway. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bsquizz
left a comment
There was a problem hiding this comment.
🤖 Amber Analysis
Overall: Strong spec — the problem statement is clear, the design is sound, and the direction is correct. The OpenShell proto already has Config map[string]string on the Provider message, confirming the gateway is ready for this. However, the "Changes Required" section has gaps that would block implementation.
Confidence: 90% — verified all claims against the current codebase.
Key findings (5 inline comments):
-
Missing API-server + SDK changes. The spec jumps straight to control plane code, but providers are API-server resources (PostgreSQL-backed, fetched via SDK). Adding
Configrequires OpenAPI spec update → SDK regen → possible DB migration before the control plane can useprovDecl.Config. This is the biggest gap. -
Two additional
VertexProjectID/VertexRegioncall sites not addressed. The spec covers line ~1309 but misses the live code path at ~2823 and the dead-but-present path at ~1733. Also misses theKubeReconcilerConfigstruct fields at lines 88-89. -
Implicit
CLOUD_ML_REGIONdefault of"global"lost silently. Migration section should warn operators who relied on the default. -
VertexEnabledandVertexCredentialsPathfate unclear. These survive the change but gate a code path that injects the same env vars being removed — potential dual source of truth. -
"ConfigMap" storage claim conflicts with actual data flow. Providers are API-server resources, not ConfigMaps.
None of these are design objections — the approach is right. These are implementation surface gaps that would cause confusion or compilation failures during the build phase.
| **File:** `components/ambient-control-plane/internal/openshell/provider_mapping.go` | ||
|
|
||
| Replace `ProviderConfig` to read directly from the declaration config map. The function no longer takes global env-var-sourced parameters: | ||
|
|
There was a problem hiding this comment.
Incomplete change surface — SDK + API server not addressed.
The "provider declaration struct" referenced here is types.Provider in components/ambient-sdk/go-sdk/types/provider.go (line 13), which is auto-generated from the OpenAPI spec (components/ambient-api-server/openapi/openapi.yaml). It currently has no Config field.
Adding Config to providers requires:
- Update the OpenAPI spec to add the
configfield to the Provider schema - Update the API server's provider handler/persistence (possible DB migration if provider config is a new column)
- Regenerate the SDK types (
make generateor equivalent) - Then the control plane reconciler gets the field for free via
provDecl.Config
The spec should add a step 0 before the current step 1 that covers the API server + SDK side. Without it, provDecl.Config won't exist at compile time.
(The good news: the OpenShell proto already has Config map[string]string on the Provider message at datamodel.pb.go:312, so the gateway side is ready.)
| Config: openshell.ProviderConfig(provType, r.cfg.VertexProjectID, r.cfg.VertexRegion), | ||
|
|
||
| // After: | ||
| Config: openshell.ProviderConfig(provType, provDecl.Config), |
There was a problem hiding this comment.
Missing two additional call sites in kube_reconciler.go.
The spec correctly identifies the ProviderConfig call at line ~1309, but there are two more active code paths that inject VertexProjectID/VertexRegion directly into container env vars:
-
Line ~1733-1734 (
buildSandboxEnv):env["ANTHROPIC_VERTEX_PROJECT_ID"] = r.cfg.VertexProjectID env["CLOUD_ML_REGION"] = r.cfg.VertexRegion
This is currently behind
if false && r.cfg.VertexEnabled(dead code), but should be cleaned up as part of this change. -
Line ~2823-2824 (container spec builder):
envVar("ANTHROPIC_VERTEX_PROJECT_ID", r.cfg.VertexProjectID), envVar("CLOUD_ML_REGION", r.cfg.VertexRegion),
This is live code gated by
r.cfg.VertexEnabled. It injects these as container env vars directly — a separate path from the OpenShell provider config flow at line 1309.
Also missing: the KubeReconcilerConfig struct at lines 88-89 has its own VertexProjectID and VertexRegion fields (separate from config.go's struct) that also need removal.
The spec should enumerate all four sites to avoid leaving stale references behind.
| --- | ||
|
|
||
| ## Migration | ||
|
|
There was a problem hiding this comment.
Migration step missing: implicit CLOUD_ML_REGION default of "global" is being removed.
The current config.go (line 103) sets:
VertexRegion: envOrDefault("CLOUD_ML_REGION", "global"),Any deployment that does not explicitly set CLOUD_ML_REGION today gets "global" as the default silently. After this change, those deployments must add VERTEX_AI_REGION: global to their provider declaration's config map — or reconciliation will fail with a missing-key error.
The migration section should call out this implicit default so operators don't get surprised. Something like:
Note: If your deployment does not currently set
CLOUD_ML_REGION, the control plane was defaulting to"global". You must now explicitly setVERTEX_AI_REGION: globalin the provider config.
| Remove `ANTHROPIC_VERTEX_PROJECT_ID` and `CLOUD_ML_REGION` env var entries and their `operator-config` ConfigMap references. | ||
|
|
||
| ### 4. Reconciler call site | ||
|
|
There was a problem hiding this comment.
Relationship between VertexEnabled/VertexCredentialsPath and this change is unclear.
The spec removes VertexProjectID and VertexRegion from the config struct, but VertexEnabled (line 31) and VertexCredentialsPath (line 34) in config.go survive. These are used at kube_reconciler.go:2821 to gate an env-var injection code path that sets ANTHROPIC_VERTEX_PROJECT_ID, CLOUD_ML_REGION, and GOOGLE_APPLICATION_CREDENTIALS directly on containers.
Should this legacy Vertex env-var injection path also be removed (since OpenShell provider config now handles it), or does it serve a different purpose? If it stays, it'll conflict with the provider-scoped config — the container would get two sources of truth for the same values.
I'd suggest the spec explicitly addresses the fate of VertexEnabled and the associated env-var injection blocks, even if the decision is "leave them for now, clean up later."
|
|
||
| **File:** `components/ambient-control-plane/internal/reconciler/` (provider declaration struct) | ||
|
|
||
| Add `Config map[string]string` to the provider declaration struct. Parse it from the YAML alongside `name`, `type`, and `secret`. |
There was a problem hiding this comment.
Clarify where config is persisted — "ConfigMap" vs API-server resource.
This says config "lives in the ConfigMap alongside the provider declaration." But in the codebase, providers are API-server-managed resources fetched via sdk.Providers().List() (see kube_reconciler.go:1265), backed by PostgreSQL — not ConfigMaps.
The Provider SDK type (components/ambient-sdk/go-sdk/types/provider.go:13) is auto-generated from the OpenAPI spec, and the reconciler accesses provDecl.Secret, provDecl.Type, etc. from these API objects.
If config is a new field on the API server's Provider resource, the spec should say so — and note that it requires an OpenAPI schema + possible database migration. If it's a separate ConfigMap sidecar, the spec should describe how the reconciler joins the two.
Summary
configmap to the Provider declaration schema so Vertex AI providers declareVERTEX_AI_PROJECT_IDandVERTEX_AI_REGIONalongside their credential secretANTHROPIC_VERTEX_PROJECT_ID,CLOUD_ML_REGION) — provider config is the provider's responsibilityconfigfield is generic: any provider type (e.g.,google-cloud,aws-bedrock) can use it for passthrough config to the OpenShell gatewayTest plan
Provider.configproto fieldvertex)🤖 Generated with Claude Code