Skip to content

spec(provider): add provider-scoped config - #471

Draft
bsquizz wants to merge 1 commit into
mainfrom
spec/provider-scoped-config
Draft

bsquizz wants to merge 1 commit into
mainfrom
spec/provider-scoped-config

Conversation

@bsquizz

@bsquizz bsquizz commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds a config map to the Provider declaration schema so Vertex AI providers declare VERTEX_AI_PROJECT_ID and VERTEX_AI_REGION alongside their credential secret
  • Removes the global fallback to control plane env vars (ANTHROPIC_VERTEX_PROJECT_ID, CLOUD_ML_REGION) — provider config is the provider's responsibility
  • The config field is generic: any provider type (e.g., google-cloud, aws-bedrock) can use it for passthrough config to the OpenShell gateway

Test plan

  • Review spec for completeness against OpenShell's Provider.config proto field
  • Validate that required keys table covers all enforced config (currently only vertex)
  • Confirm migration steps are sufficient for existing deployments

🤖 Generated with Claude Code

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 bsquizz left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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):

  1. 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 Config requires OpenAPI spec update → SDK regen → possible DB migration before the control plane can use provDecl.Config. This is the biggest gap.

  2. Two additional VertexProjectID/VertexRegion call 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 the KubeReconcilerConfig struct fields at lines 88-89.

  3. Implicit CLOUD_ML_REGION default of "global" lost silently. Migration section should warn operators who relied on the default.

  4. VertexEnabled and VertexCredentialsPath fate unclear. These survive the change but gate a code path that injects the same env vars being removed — potential dual source of truth.

  5. "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:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Update the OpenAPI spec to add the config field to the Provider schema
  2. Update the API server's provider handler/persistence (possible DB migration if provider config is a new column)
  3. Regenerate the SDK types (make generate or equivalent)
  4. 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),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 set VERTEX_AI_REGION: global in 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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bsquizz bsquizz added amber/reviewed Amber has completed an inline code review amber/change-requested labels Aug 3, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amber/change-requested amber/reviewed Amber has completed an inline code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant