feat: add experimental coderd_ai_provider resource#368
Conversation
7c12298 to
844cbf5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dde8c9a39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 891e3b5e17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b8ee453 to
3ca1539
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ca1539a17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
28abf98 to
4e28632
Compare
|
@codex review re:
Verified this against coder/coder at current main — the premise is correct, but loosening only the provider here wouldn't actually unblock proxy deployments, so I'm holding off. The provider is faithfully mirroring the server's create gate. CreateAIProviderRequest.Validate() in codersdk/aiproviders.go rejects type=bedrock when req.Settings.Bedrock == nil || !req.Settings.Bedrock.IsConfigured() — and IsConfigured() only checks region + credentials, ignoring base_url, exactly like this check. The create handler (coderd/ai_providers.go) calls that same Validate(), so a base-URL-only Bedrock provider (AWS SDK default credential chain, no region) currently returns an HTTP 400 from Coder regardless of what Terraform does. The base-URL-aware IsBedrockConfigured(baseURL, settings) does exist, but it's only used by legacy env-var seeding (cli/server.go) and migration detection (coderd/ai_providers_migrate.go) — never in the create/update API validation path. So if I switch this check to be base-URL-aware in isolation, the apply still fails — just with a server-side 400 instead of a clean client-side diagnostic, which is worse UX. This is an inconsistency in Coder itself between the Bedrock docs and CreateAIProviderRequest.Validate(). The right fix is upstream: have CreateAIProviderRequest.Validate() use IsBedrockConfigured(baseURL, ...) (or correct the docs). Once the API accepts base-URL-only Bedrock, I'll mirror it here |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e28632a3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
78ae1a4 to
1cf58fc
Compare
1cf58fc to
ee995a9
Compare
82b36d9 to
2219c80
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee995a996b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ee995a9 to
d2f68e3
Compare
2219c80 to
293eac1
Compare
|
@codex review re:
Verified this against coder/coder at current main — the premise is correct, but loosening only the provider here wouldn't actually unblock proxy deployments, so I'm holding off. The provider is faithfully mirroring the server's create gate. CreateAIProviderRequest.Validate() in codersdk/aiproviders.go rejects type=bedrock when req.Settings.Bedrock == nil || !req.Settings.Bedrock.IsConfigured() — and IsConfigured() only checks region + credentials, ignoring base_url, exactly like this check. The create handler (coderd/ai_providers.go) calls that same Validate(), so a base-URL-only Bedrock provider (AWS SDK default credential chain, no region) currently returns an HTTP 400 from Coder regardless of what Terraform does. The base-URL-aware IsBedrockConfigured(baseURL, settings) does exist, but it's only used by legacy env-var seeding (cli/server.go) and migration detection (coderd/ai_providers_migrate.go) — never in the create/update API validation path. So if I switch this check to be base-URL-aware in isolation, the apply still fails — just with a server-side 400 instead of a clean client-side diagnostic, which is worse UX. This is an inconsistency in Coder itself between the Bedrock docs and CreateAIProviderRequest.Validate(). The right fix is upstream: have CreateAIProviderRequest.Validate() use IsBedrockConfigured(baseURL, ...) (or correct the docs). Once the API accepts base-URL-only Bedrock, I'll mirror it here |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 21 findings (5 P2, 8 P3, 4 Nit, 4 Note), APPROVE. Review Finding inventoryFinding InventoryFindings
Law analysisEffective LOC: 1222. Head SHA: d2f68e3. Verdict: Don't split. Enforcement: Advisory. Round logRound 1Panel. 4 P2, 6 P3, 2 Nit, 4 Note. 16 posted. Reviewed against 293eac1..d2f68e3. Panel: Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Ging-Go, Kurapika, Chopper, Komugi, Meruem, Kite (wildcard), Knov (wildcard). Netero clean, Law advisory don't-split. Round 2Churn guard: PROCEED (12 addressed, 4 acknowledged). Netero clean, all R1 fixes verified. Panel: Bisky, Mafuuu, Hisoka, Chopper, Meruem, Knov (wildcard). 1 P2, 2 P3, 2 Nit new. Reviewed against 293eac1..1ee8a77. Round 3Churn guard: PROCEED (5 addressed). Netero clean, all R2 fixes verified. Panel: Mafuuu, Hisoka, Meruem. 0 new findings. All 21 findings resolved (17 fixed, 4 acknowledged). Reviewed against 293eac1..d51dad0. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
b1d81a3 to
6221b88
Compare
a3b1745 to
936dab2
Compare
c4c0db1 to
a97aba5
Compare
> **Stack** — base PR; #368 (experimental AI provider resource) builds on this. ## Problem The Terraform acceptance matrix was failing intermittently across all lanes with `coder failed to become ready in time`. `integration.StartCoder` boots `ghcr.io/coder/coder` without `CODER_PG_CONNECTION_URL`, so Coder falls back to its embedded PostgreSQL. The image doesn't bundle the Postgres binary, so each startup downloads the embedded-postgres jar from Maven Central. GitHub runners' shared egress IPs get rate-limited by Cloudflare, and a single non-200 crashes Coder before it binds — reddening the lane. ## Fix Give Coder a real PostgreSQL instead of the embedded one. Setting `CODER_PG_CONNECTION_URL` bypasses the Maven download entirely. Per test, `integration.StartCoder` now starts a Postgres sidecar on a user-defined Docker network (aliased `postgres`), wires Coder onto it, and points `CODER_PG_CONNECTION_URL` at it. No readiness wait needed — Coder retries its DB connection for ~30s, covering sidecar boot. The image is `us-docker.pkg.dev/coder-v2-images-public/public/postgres:17`, the public mirror coder/coder uses in its own tests, which avoids Docker Hub's anonymous pull rate limit. This is environment-level, so it also covers the version-pinned back-compat tests. CI wall time is essentially unchanged (~206s/lane): the Postgres pull and boot costs about what the Maven download it replaces did. ## Also: raise the Terraform support floor to 1.5 Terraform 1.0–1.4 are EOL. This drops them from the CI matrix (now `1.5.*`–`1.14.*`, so we also test the latest releases), sets the README floor to `>= 1.5`, and removes the `template_resource_test.go` skip guard for a Terraform 1.0 panic.
Adds a Terraform resource for declarative Coder AI provider configuration, supporting all SDK provider types with AWS Bedrock and API-key providers. Secrets use Terraform 1.11+ write-only arguments and are never stored in state.
936dab2 to
9fba23a
Compare
a97aba5 to
839989b
Compare
839989b to
0d0e3b8
Compare
0d0e3b8 to
de42560
Compare
de42560 to
2e2abe6
Compare
2e2abe6 to
18615e5
Compare
|
Going to handle the new |
dannykopping
left a comment
There was a problem hiding this comment.
Looking good, thanks a mil for doing this @ethanndickson!
Have you tested this against a local instance?
| DisplayName types.String `tfsdk:"display_name"` | ||
| Enabled types.Bool `tfsdk:"enabled"` | ||
| BaseURL types.String `tfsdk:"base_url"` | ||
| APIKeyWO types.String `tfsdk:"api_key_wo"` |
There was a problem hiding this comment.
Cool solution! I'd prefer we didn't couple the implementation (write-only secret) with the field name.
Let's be specific and call it api_key_plaintext or something?
| "enabled": schema.BoolAttribute{ | ||
| MarkdownDescription: "Whether this AI provider is enabled. Defaults to true.", | ||
| Optional: true, | ||
| Computed: true, |
There was a problem hiding this comment.
Howcome this is computed? It can be defined when creating a provider.
| "model": schema.StringAttribute{ | ||
| MarkdownDescription: "Primary Bedrock model identifier.", | ||
| Optional: true, | ||
| Computed: true, |
| "small_fast_model": schema.StringAttribute{ | ||
| MarkdownDescription: "Small/fast Bedrock model identifier used for background tasks.", | ||
| Optional: true, | ||
| Computed: true, |
| if !cfgBedrock.AccessKeyWO.IsNull() && !cfgBedrock.AccessKeyWO.IsUnknown() { | ||
| accessKey := cfgBedrock.AccessKeyWO.ValueString() | ||
| settings.AccessKey = &accessKey | ||
| } | ||
| if !cfgBedrock.AccessKeySecretWO.IsNull() && !cfgBedrock.AccessKeySecretWO.IsUnknown() { | ||
| accessKeySecret := cfgBedrock.AccessKeySecretWO.ValueString() | ||
| settings.AccessKeySecret = &accessKeySecret |
There was a problem hiding this comment.
Nit: might be good to add an abstraction which handles this. Seeing a lot of repeated patterns here around this write-only value.
| // preserve the configured/state values. | ||
| APIKeyWO: types.StringNull(), | ||
| APIKeyWOVersion: m.APIKeyWOVersion, | ||
| APIKeyMasked: types.StringNull(), |

Summary
Adds
coderd_ai_providerfor managing Coder AI Gateway providers from Terraform — OpenAI-style API-key providers and AWS Bedrock providers — with full CRUD + import, generated docs/examples, and acceptance/unit tests. The resource is marked experimental via a warning callout in its docs.Approach
*_wo), so secrets reach Coder but never land in state. Each secret is paired with a*_versionfield; bumping the version rotates the secret. A null version means "unmanaged/preserve" rather than "clear", so removing it never silently wipes server-side secrets.ValidateConfighandles type-dependent combinations (e.g.api_key_wois rejected forbedrock/copilotor anysettings.bedrockconfig, mirroring the server) and Bedrock's region-or-credentials requirement. Validation defers while values are unknown during the validate/plan walk.regionis omitted it derives from a canonical Bedrockbase_url(bedrock-runtime.<region>.amazonaws.com), so it re-derives correctly whenbase_urlchanges regions.Schema
Computed:
id,display_name,enabled,api_key_masked,created_at,updated_at, andsettings.bedrock.region(derived frombase_url).Notes
Requires Terraform 1.11+ when configured (write-only arguments); acceptance cases skip earlier versions.
An integration test combining this with the model resource is planned.
Relates to CODAGT-607