diff --git a/docs/resources/ai_provider.md b/docs/resources/ai_provider.md index 7523697..99f62cd 100644 --- a/docs/resources/ai_provider.md +++ b/docs/resources/ai_provider.md @@ -102,7 +102,7 @@ resource "coderd_ai_provider" "openai" { > **NOTE**: [Write-only arguments](https://developer.hashicorp.com/terraform/language/resources/ephemeral#write-only-arguments) are supported in Terraform 1.11 and later. -- `api_key_wo` (String, Sensitive, [Write-only](https://developer.hashicorp.com/terraform/language/resources/ephemeral#write-only-arguments)) Plaintext API key for the provider. Not valid for `bedrock` or `copilot`, or when `settings.bedrock` is set. Optional with `settings.claude_platform_aws`; see that attribute. Removing it from configuration does not delete a stored key; replace the resource to delete it. Bump `api_key_wo_version` to rotate it. +- `api_key_wo` (String, Sensitive, [Write-only](https://developer.hashicorp.com/terraform/language/resources/ephemeral#write-only-arguments)) Plaintext API key for the provider. Not valid for `bedrock` or `copilot`, or when `settings.bedrock` is set. Optional with `settings.claude_platform_aws`; see that attribute. Removing it from configuration keeps the stored key. Bump `api_key_wo_version` to rotate it, or set it to `""` and bump the version to delete the stored key. - `api_key_wo_version` (Number) Version for the write-only API key. Required when `api_key_wo` is set; bump it whenever `api_key_wo` changes to rotate the stored key. - `display_name` (String) Display name shown in Coder. If omitted, defaults to the provider name. - `enabled` (Boolean) Whether this AI provider is enabled. Defaults to true. diff --git a/internal/provider/ai_provider_resource.go b/internal/provider/ai_provider_resource.go index 919522d..bf83996 100644 --- a/internal/provider/ai_provider_resource.go +++ b/internal/provider/ai_provider_resource.go @@ -91,6 +91,33 @@ func (r *AIProviderResource) ModifyPlan(ctx context.Context, req resource.Modify "Experimental Resource", "coderd_ai_provider is experimental. Changes are expected, and it is not recommended for production use.", ) + if req.State.Raw.IsNull() { + return + } + + var version, priorVersion types.Int64 + var key types.String + resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("api_key_wo_version"), &version)...) + resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root("api_key_wo_version"), &priorVersion)...) + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, path.Root("api_key_wo"), &key)...) + if resp.Diagnostics.HasError() || version.IsNull() || version.IsUnknown() || version.Equal(priorVersion) || key.IsUnknown() { + return + } + if key.IsNull() { + addMissingAPIKeyError(&resp.Diagnostics) + return + } + if key.ValueString() == "" { + resp.Diagnostics.AddAttributeWarning( + path.Root("api_key_wo"), + "Deleting Stored API Key", + "`api_key_wo` is `\"\"` and `api_key_wo_version` changed, so this apply deletes the stored API key. The plan shows this only as a version change.", + ) + } +} + +func addMissingAPIKeyError(diags *diag.Diagnostics) { + diags.AddAttributeError(path.Root("api_key_wo"), "Missing API Key", "`api_key_wo` must be configured when `api_key_wo_version` changes. Set it to `\"\"` to delete the stored key.") } func (r *AIProviderResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { @@ -159,12 +186,11 @@ func (r *AIProviderResource) Schema(ctx context.Context, req resource.SchemaRequ Required: true, }, "api_key_wo": schema.StringAttribute{ - MarkdownDescription: "Plaintext API key for the provider. Not valid for `bedrock` or `copilot`, or when `settings.bedrock` is set. Optional with `settings.claude_platform_aws`; see that attribute. Removing it from configuration does not delete a stored key; replace the resource to delete it. Bump `api_key_wo_version` to rotate it.", + MarkdownDescription: "Plaintext API key for the provider. Not valid for `bedrock` or `copilot`, or when `settings.bedrock` is set. Optional with `settings.claude_platform_aws`; see that attribute. Removing it from configuration keeps the stored key. Bump `api_key_wo_version` to rotate it, or set it to `\"\"` and bump the version to delete the stored key.", Optional: true, Sensitive: true, WriteOnly: true, Validators: []validator.String{ - stringvalidator.LengthAtLeast(1), stringvalidator.AlsoRequires(path.MatchRoot("api_key_wo_version")), }, }, @@ -581,8 +607,8 @@ func (r *AIProviderResource) ImportState(ctx context.Context, req resource.Impor func (m AIProviderResourceModel) createRequest(config AIProviderResourceModel, diags *diag.Diagnostics) codersdk.CreateAIProviderRequest { var apiKeys []string - if !config.APIKeyWO.IsNull() && !config.APIKeyWO.IsUnknown() { - apiKeys = []string{config.APIKeyWO.ValueString()} + if key := config.APIKeyWO.ValueString(); key != "" { + apiKeys = []string{key} } return codersdk.CreateAIProviderRequest{ Type: codersdk.AIProviderType(m.Type.ValueString()), @@ -618,11 +644,16 @@ func (m AIProviderResourceModel) updateRequest(state, config AIProviderResourceM } // Rotate the API key only when its version changes to a concrete value. A - // null version preserves the stored key rather than clearing it. + // null version preserves the stored key rather than clearing it; an empty + // key deletes it. if !m.APIKeyWOVersion.IsNull() && !m.APIKeyWOVersion.Equal(state.APIKeyWOVersion) { - if config.APIKeyWO.IsNull() || config.APIKeyWO.IsUnknown() { - diags.AddAttributeError(path.Root("api_key_wo"), "Missing API Key", "`api_key_wo` must be configured when `api_key_wo_version` changes.") - } else { + switch { + case config.APIKeyWO.IsNull() || config.APIKeyWO.IsUnknown(): + // ModifyPlan reports this at plan time unless the version was unknown then. + addMissingAPIKeyError(diags) + case config.APIKeyWO.ValueString() == "": + patch.APIKeys = &[]codersdk.AIProviderKeyMutation{} + default: patch.APIKeys = &[]codersdk.AIProviderKeyMutation{{APIKey: stringPtrOrNil(config.APIKeyWO)}} } } diff --git a/internal/provider/ai_provider_resource_test.go b/internal/provider/ai_provider_resource_test.go index 5709704..84e6234 100644 --- a/internal/provider/ai_provider_resource_test.go +++ b/internal/provider/ai_provider_resource_test.go @@ -57,6 +57,9 @@ func TestAccAIProviderResource(t *testing.T) { // Changing base_url to a different region must re-derive settings.bedrock.region. cfg3 := cfg2 cfg3.BedrockBaseURL = "https://bedrock-runtime.us-west-2.amazonaws.com" + cfg4 := cfg3 + cfg4.OpenAIKey = "" + cfg4.OpenAIKeyVersion = 3 resource.Test(t, resource.TestCase{ IsUnitTest: true, @@ -93,6 +96,10 @@ func TestAccAIProviderResource(t *testing.T) { resource.TestCheckResourceAttr("coderd_ai_provider.bedrock", "settings.bedrock.region", "us-west-2"), ), }, + { + Config: cfg4.String(t), + Check: resource.TestCheckNoResourceAttr("coderd_ai_provider.openai", "api_key_masked"), + }, }, }) } @@ -254,17 +261,6 @@ func TestAIProviderResourceSchemaValidation(t *testing.T) { `, wantError: `api_key_wo_version`, }, - "api key cannot be empty": { - body: `resource "coderd_ai_provider" "test" { - type = "openai" - name = "openai-test" - base_url = "https://api.openai.com/v1" - api_key_wo = "" - api_key_wo_version = 1 -} -`, - wantError: `(?s)string length must be at\s+least\s+1`, - }, "bedrock known config requires region or credentials": { body: `resource "coderd_ai_provider" "test" { type = "bedrock" @@ -760,6 +756,26 @@ func TestAIProviderCreateRequestBedrockWithoutCredentials(t *testing.T) { require.Nil(t, req.Settings.Bedrock.AccessKeySecret) } +func TestAIProviderCreateRequestEmptyAPIKey(t *testing.T) { + t.Parallel() + + plan := AIProviderResourceModel{ + Type: types.StringValue(string(codersdk.AIProviderTypeOpenAI)), + Name: types.StringValue("openai"), + Enabled: types.BoolValue(true), + BaseURL: types.StringValue("https://api.openai.com/v1"), + APIKeyWOVersion: types.Int64Value(1), + } + config := plan + config.APIKeyWO = types.StringValue("") + + var diags diag.Diagnostics + req := plan.createRequest(config, &diags) + require.False(t, diags.HasError(), diags.Errors()) + require.Empty(t, req.APIKeys) + require.Empty(t, req.Validate()) +} + func TestAIProviderCreateRequestBedrockMantleProtocol(t *testing.T) { t.Parallel() @@ -907,6 +923,67 @@ func TestAIProviderUpdateRequestAPIKeyRotation(t *testing.T) { }) } +func TestAIProviderModifyPlanAPIKey(t *testing.T) { + t.Parallel() + + ctx := t.Context() + r := &AIProviderResource{} + schemaResp := &fwresource.SchemaResponse{} + r.Schema(ctx, fwresource.SchemaRequest{}, schemaResp) + require.Empty(t, schemaResp.Diagnostics) + s := schemaResp.Schema + + raw := func(t *testing.T, m *AIProviderResourceModel) tftypes.Value { + state := tfsdk.State{Schema: s, Raw: tftypes.NewValue(s.Type().TerraformType(ctx), nil)} + if m != nil { + require.Empty(t, state.Set(ctx, m)) + } + return state.Raw + } + model := func(version types.Int64, key types.String) *AIProviderResourceModel { + return &AIProviderResourceModel{APIKeyWOVersion: version, APIKeyWO: key} + } + prior := model(types.Int64Value(1), types.StringNull()) + + for _, tc := range []struct { + name string + state *AIProviderResourceModel + config *AIProviderResourceModel + wantErrors []string + wantWarnings []string + }{ + {name: "version bumped without key", state: prior, config: model(types.Int64Value(2), types.StringNull()), wantErrors: []string{"Missing API Key"}}, + {name: "version bumped with empty key", state: prior, config: model(types.Int64Value(2), types.StringValue("")), wantWarnings: []string{"Deleting Stored API Key"}}, + {name: "version bumped with key", state: prior, config: model(types.Int64Value(2), types.StringValue("sk-rotated"))}, + {name: "version unchanged without key", state: prior, config: model(types.Int64Value(1), types.StringNull())}, + {name: "version unknown without key", state: prior, config: model(types.Int64Unknown(), types.StringNull())}, + {name: "create with empty key", config: model(types.Int64Value(1), types.StringValue(""))}, + } { + t.Run(tc.name, func(t *testing.T) { + // Write-only values are null in the plan; ModifyPlan must read them from config. + planned := *tc.config + planned.APIKeyWO = types.StringNull() + plan := tfsdk.Plan{Schema: s, Raw: raw(t, &planned)} + resp := &fwresource.ModifyPlanResponse{Plan: plan} + r.ModifyPlan(ctx, fwresource.ModifyPlanRequest{ + Config: tfsdk.Config{Schema: s, Raw: raw(t, tc.config)}, + Plan: plan, + State: tfsdk.State{Schema: s, Raw: raw(t, tc.state)}, + }, resp) + + var errs, warnings []string + for _, d := range resp.Diagnostics.Errors() { + errs = append(errs, d.Summary()) + } + for _, d := range resp.Diagnostics.Warnings() { + warnings = append(warnings, d.Summary()) + } + require.Equal(t, tc.wantErrors, errs) + require.Equal(t, append([]string{"Experimental Resource"}, tc.wantWarnings...), warnings) + }) + } +} + func TestAIProviderUpdateRejectsBedrockCredentialBumpWithoutCredentials(t *testing.T) { t.Parallel()