diff --git a/cli/aibridged.go b/cli/aibridged.go index e838c1db5ab0a..2234fb6164650 100644 --- a/cli/aibridged.go +++ b/cli/aibridged.go @@ -327,16 +327,14 @@ func buildAIProviderKeyPool(providerName string, keys []database.AIProviderKey, } // bedrockConfigFromRow returns nil when the settings have no Bedrock -// discriminator or when the Bedrock fields are not actually configured. -// The provider row's BaseUrl is the generic upstream endpoint and is -// always non-empty, so it cannot serve as a Bedrock detection signal; -// gate on the settings blob alone via [codersdk.AIProviderBedrockSettings.IsConfigured]. +// discriminator or when neither the row BaseUrl nor the Bedrock fields +// identify usable Bedrock config. func bedrockConfigFromRow(row database.AIProvider, settings codersdk.AIProviderSettings) *aibridge.AWSBedrockConfig { if settings.Bedrock == nil { return nil } bedrockSettings := *settings.Bedrock - if !bedrockSettings.IsConfigured() { + if !codersdk.IsBedrockConfigured(row.BaseUrl, bedrockSettings) { return nil } accessKey := ptr.NilToEmpty(bedrockSettings.AccessKey) diff --git a/cli/aibridged_internal_test.go b/cli/aibridged_internal_test.go index dee00f841f2d8..63351a208aaad 100644 --- a/cli/aibridged_internal_test.go +++ b/cli/aibridged_internal_test.go @@ -159,20 +159,44 @@ func TestBuildProviders(t *testing.T) { t.Run("LegacyBedrockWithoutAnthropicKey", func(t *testing.T) { t.Parallel() - // Bedrock credentials alone should be enough to create an - // Anthropic provider. No CODER_AIBRIDGE_ANTHROPIC_KEY needed. - cfg := codersdk.AIBridgeConfig{} - cfg.LegacyBedrock.Region = serpent.String("us-west-2") - cfg.LegacyBedrock.AccessKey = serpent.String("AKID") - cfg.LegacyBedrock.AccessKeySecret = serpent.String("secret") - providers, err := buildFromEnv(t, cfg) - require.NoError(t, err) - require.Len(t, providers, 1) + // Bedrock config alone should be enough to create an Anthropic + // provider. No CODER_AIBRIDGE_ANTHROPIC_KEY needed. + tests := []struct { + name string + configure func(*codersdk.AIBridgeConfig) + }{ + { + name: "region-and-static-credentials", + configure: func(cfg *codersdk.AIBridgeConfig) { + cfg.LegacyBedrock.Region = serpent.String("us-west-2") + cfg.LegacyBedrock.AccessKey = serpent.String("AKID") + cfg.LegacyBedrock.AccessKeySecret = serpent.String("secret") + }, + }, + { + name: "base-url-only", + configure: func(cfg *codersdk.AIBridgeConfig) { + cfg.LegacyBedrock.BaseURL = serpent.String("https://bedrock.internal.example.com") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := codersdk.AIBridgeConfig{} + tt.configure(&cfg) - p := providers[0] - assert.Equal(t, aibridge.ProviderAnthropic, p.Type()) - assert.Equal(t, aibridge.ProviderAnthropic, p.Name()) + providers, err := buildFromEnv(t, cfg) + require.NoError(t, err) + require.Len(t, providers, 1) + + p := providers[0] + assert.Equal(t, aibridge.ProviderAnthropic, p.Type()) + assert.Equal(t, aibridge.ProviderAnthropic, p.Name()) + }) + } }) t.Run("UnknownType", func(t *testing.T) { @@ -256,45 +280,85 @@ func TestBuildProviders(t *testing.T) { assert.Nil(t, bedrockConfigFromRow(row, codersdk.AIProviderSettings{})) }) - t.Run("BedrockSettingsPresent", func(t *testing.T) { + t.Run("BedrockSettingsConfigured", func(t *testing.T) { t.Parallel() accessKey := "AKID" secret := "secret" model := "anthropic.claude-3-5-sonnet-20241022-v2:0" smallModel := "anthropic.claude-3-5-haiku-20241022-v1:0" - row := database.AIProvider{ - Type: database.AIProviderTypeAnthropic, - Name: "anthropic-bedrock", - BaseUrl: "https://bedrock-runtime.us-west-2.amazonaws.com/", - } - settings := codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-west-2", - AccessKey: &accessKey, - AccessKeySecret: &secret, - Model: model, - SmallFastModel: smallModel, + tests := []struct { + name string + row database.AIProvider + settings codersdk.AIProviderSettings + expectedBaseURL string + expectedRegion string + expectedAccessKey string + expectedAccessSecret string + expectedModel string + expectedSmallFastModel string + }{ + { + name: "region-and-static-credentials", + row: database.AIProvider{ + Type: database.AIProviderTypeAnthropic, + Name: "anthropic-bedrock", + BaseUrl: "https://bedrock-runtime.us-west-2.amazonaws.com/", + }, + settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Region: "us-west-2", + AccessKey: &accessKey, + AccessKeySecret: &secret, + Model: model, + SmallFastModel: smallModel, + }, + }, + expectedBaseURL: "https://bedrock-runtime.us-west-2.amazonaws.com/", + expectedRegion: "us-west-2", + expectedAccessKey: accessKey, + expectedAccessSecret: secret, + expectedModel: model, + expectedSmallFastModel: smallModel, }, + { + name: "base-url-only", + row: database.AIProvider{ + Type: database.AIProviderTypeAnthropic, + Name: "anthropic-bedrock-base-url", + BaseUrl: "https://bedrock.internal.example.com/", + }, + settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + Model: model, + SmallFastModel: smallModel, + }, + }, + expectedBaseURL: "https://bedrock.internal.example.com/", + expectedModel: model, + expectedSmallFastModel: smallModel, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := bedrockConfigFromRow(tt.row, tt.settings) + require.NotNil(t, got) + assert.Equal(t, tt.expectedBaseURL, got.BaseURL) + assert.Equal(t, tt.expectedRegion, got.Region) + assert.Equal(t, tt.expectedAccessKey, got.AccessKey) + assert.Equal(t, tt.expectedAccessSecret, got.AccessKeySecret) + assert.Equal(t, tt.expectedModel, got.Model) + assert.Equal(t, tt.expectedSmallFastModel, got.SmallFastModel) + }) } - got := bedrockConfigFromRow(row, settings) - require.NotNil(t, got) - assert.Equal(t, row.BaseUrl, got.BaseURL) - assert.Equal(t, "us-west-2", got.Region) - assert.Equal(t, accessKey, got.AccessKey) - assert.Equal(t, secret, got.AccessKeySecret) - assert.Equal(t, model, got.Model) - assert.Equal(t, smallModel, got.SmallFastModel) }) - t.Run("BedrockSettingsEmpty", func(t *testing.T) { + t.Run("BedrockSettingsUnconfigured", func(t *testing.T) { t.Parallel() - // A non-nil but zero-valued Bedrock settings blob should not - // produce a Bedrock config; the provider's generic BaseUrl is - // not a Bedrock detection signal. row := database.AIProvider{ - Type: database.AIProviderTypeAnthropic, - Name: "anthropic-empty-bedrock", - BaseUrl: "https://api.anthropic.com/", + Type: database.AIProviderTypeAnthropic, + Name: "anthropic-empty-bedrock", } settings := codersdk.AIProviderSettings{ Bedrock: &codersdk.AIProviderBedrockSettings{}, diff --git a/coderd/ai_providers.go b/coderd/ai_providers.go index 49e5bb8750403..da942347ee71d 100644 --- a/coderd/ai_providers.go +++ b/coderd/ai_providers.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "net/http" + "regexp" + "strings" "github.com/go-chi/chi/v5" "github.com/google/uuid" @@ -178,6 +180,28 @@ func (api *API) aiProvidersCreate(rw http.ResponseWriter, r *http.Request) { return } + bedrockRegionBaseURL := "" + if req.Type == codersdk.AIProviderTypeBedrock && req.Settings.Bedrock == nil { + bedrockRegionBaseURL = req.BaseURL + } else if req.Settings.Bedrock != nil && req.Settings.Bedrock.Region == "" { + bedrockRegionBaseURL = req.BaseURL + } + req.Settings = normalizeAIProviderSettings(req.Type, req.Settings, bedrockRegionBaseURL) + // Any provider carrying Bedrock settings must resolve to usable config. + // The UI submits Bedrock as type=anthropic with a bedrock discriminator, + // so gate on the settings blob rather than the provider type; otherwise + // aibridge silently drops the provider at runtime. + if req.Settings.Bedrock != nil && !codersdk.IsBedrockConfigured(req.BaseURL, *req.Settings.Bedrock) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid AI provider request.", + Validations: []codersdk.ValidationError{{ + Field: "settings", + Detail: errAIProviderBedrockSettingsRequired.Error(), + }}, + }) + return + } + settings, err := encodeAIProviderSettings(req.Settings) if err != nil { api.Logger.Error(ctx, "encode AI provider settings", slog.Error(err)) @@ -320,6 +344,15 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { if req.Settings != nil { existing = mergeAIProviderSettings(existing, *req.Settings) } + baseURL := ptr.NilToDefault(req.BaseURL, old.BaseUrl) + bedrockRegionBaseURL := "" + if codersdk.AIProviderType(old.Type) == codersdk.AIProviderTypeBedrock && existing.Bedrock == nil { + bedrockRegionBaseURL = baseURL + } else if req.BaseURL != nil && existing.Bedrock != nil && + (req.Settings == nil || req.Settings.Bedrock == nil || req.Settings.Bedrock.Region == "") { + bedrockRegionBaseURL = baseURL + } + existing = normalizeAIProviderSettings(codersdk.AIProviderType(old.Type), existing, bedrockRegionBaseURL) // Bedrock settings are only meaningful for anthropic- or // bedrock-typed providers; rejecting the mismatch keeps a // misconfiguration from sitting silently in the encrypted @@ -329,6 +362,12 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { old.Type != database.AIProviderTypeBedrock { return errAIProviderBedrockTypeMismatch } + // The mismatch check above guarantees a non-nil Bedrock blob only + // survives on anthropic- or bedrock-typed providers, so any such + // blob must resolve to usable config regardless of the type. + if existing.Bedrock != nil && !codersdk.IsBedrockConfigured(baseURL, *existing.Bedrock) { + return errAIProviderBedrockSettingsRequired + } settings, err := encodeAIProviderSettings(existing) if err != nil { return xerrors.Errorf("encode settings: %w", err) @@ -354,7 +393,7 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { Type: old.Type, DisplayName: displayName, Enabled: ptr.NilToDefault(req.Enabled, old.Enabled), - BaseUrl: ptr.NilToDefault(req.BaseURL, old.BaseUrl), + BaseUrl: baseURL, Settings: settings, // SettingsKeyID is set by the dbcrypt wrapper. SettingsKeyID: sql.NullString{}, @@ -400,6 +439,12 @@ func (api *API) aiProvidersUpdate(rw http.ResponseWriter, r *http.Request) { }) return } + if errors.Is(err, errAIProviderBedrockSettingsRequired) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: errAIProviderBedrockSettingsRequired.Error(), + }) + return + } if errors.Is(err, errAIProviderKeyUnknown) { // Use the sentinel directly so the response message does not // leak the "execute transaction:" wrapper xerrors added on the @@ -506,6 +551,10 @@ var errCopilotRejectsAPIKeys = xerrors.New("copilot providers do not accept api_ // the outer handler translates it into a 400. var errAIProviderBedrockTypeMismatch = xerrors.New("bedrock settings are only valid for type=anthropic or type=bedrock") +// errAIProviderBedrockSettingsRequired is returned when a provider's +// Bedrock settings cannot produce runtime Bedrock config. +var errAIProviderBedrockSettingsRequired = xerrors.New("Bedrock settings require a region or credentials") + // errAIProviderInvalidName is returned from lookupAIProvider when the // idOrName parameter is neither a UUID nor a syntactically-valid name. // The handler translates this into a 400 so an integrator gets a hint @@ -738,6 +787,23 @@ func applyAIProviderKeyOps(ctx context.Context, tx database.Store, providerID uu // into a 400. var errAIProviderKeyUnknown = xerrors.New("api_keys references an unknown id for this provider") +var canonicalBedrockBaseURLRegex = regexp.MustCompile(`(?i)^https://bedrock-runtime\.([a-z0-9-]+)\.amazonaws\.com/?$`) + +func normalizeAIProviderSettings(providerType codersdk.AIProviderType, settings codersdk.AIProviderSettings, bedrockRegionBaseURL string) codersdk.AIProviderSettings { + if providerType == codersdk.AIProviderTypeBedrock && settings.Bedrock == nil { + settings.Bedrock = &codersdk.AIProviderBedrockSettings{} + } + if settings.Bedrock == nil || bedrockRegionBaseURL == "" { + return settings + } + if match := canonicalBedrockBaseURLRegex.FindStringSubmatch(strings.TrimSpace(bedrockRegionBaseURL)); len(match) == 2 { + bedrock := *settings.Bedrock + bedrock.Region = strings.ToLower(match[1]) + settings.Bedrock = &bedrock + } + return settings +} + // encodeAIProviderSettings serializes a settings value into the // discriminated JSON form stored in ai_providers.settings. Empty // settings return an invalid sql.NullString so the row stores SQL NULL @@ -766,6 +832,9 @@ func mergeAIProviderSettings(existing, patch codersdk.AIProviderSettings) coders } merged := *patch.Bedrock if existing.Bedrock != nil { + if merged.Region == "" { + merged.Region = existing.Bedrock.Region + } if merged.AccessKey == nil { merged.AccessKey = existing.Bedrock.AccessKey } diff --git a/coderd/ai_providers_migrate.go b/coderd/ai_providers_migrate.go index 7ec6a997b0dfe..47ac4843eedd1 100644 --- a/coderd/ai_providers_migrate.go +++ b/coderd/ai_providers_migrate.go @@ -319,9 +319,8 @@ func providersFromEnv(ctx context.Context, cfg codersdk.AIBridgeConfig, logger s // Legacy Anthropic + Bedrock. Anthropic is enabled if either an // Anthropic key OR any Bedrock setting is explicitly configured. - // Detection goes through AIProviderBedrockSettings.IsConfigured() - // so the legacy and indexed paths agree on what counts as a - // Bedrock provider. + // Detection goes through IsBedrockConfigured so the legacy and + // indexed paths agree on what counts as a Bedrock provider. bedrock := codersdk.NewAIProviderBedrockSettings( cfg.LegacyBedrock.Region.String(), cfg.LegacyBedrock.AccessKey.String(), @@ -385,9 +384,9 @@ func providersFromEnv(ctx context.Context, cfg codersdk.AIBridgeConfig, logger s dp.BaseURL = p.BaseURL // Bedrock fields apply to Anthropic and the dedicated Bedrock - // type. Detection goes through - // AIProviderBedrockSettings.IsConfigured() so the legacy and - // indexed paths agree on what counts as a Bedrock provider. + // type. Detection goes through IsBedrockConfigured so the + // legacy and indexed paths agree on what counts as a Bedrock + // provider. isBedrock := false if dp.Type == database.AIProviderTypeAnthropic || dp.Type == database.AIProviderTypeBedrock { var accessKey, accessKeySecret string diff --git a/coderd/ai_providers_test.go b/coderd/ai_providers_test.go index b9bfd283f1c9e..81e88a16b7a5f 100644 --- a/coderd/ai_providers_test.go +++ b/coderd/ai_providers_test.go @@ -625,6 +625,164 @@ func TestAIProvidersCRUD(t *testing.T) { require.NotContains(t, body, `"access_key"`) require.NotContains(t, body, `"access_key_secret"`) }) + + t.Run("BedrockDerivesRegionFromBaseURL", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + // Two request shapes resolve to Bedrock: the native type=bedrock + // form sends no settings, while the UI sends type=anthropic with a + // bedrock settings discriminator. Region derivation must behave + // identically for both. + cases := []struct { + name string + providerType codersdk.AIProviderType + createSettings codersdk.AIProviderSettings + }{ + { + name: "bedrock-type", + providerType: codersdk.AIProviderTypeBedrock, + }, + { + name: "anthropic-type", + providerType: codersdk.AIProviderTypeAnthropic, + createSettings: codersdk.AIProviderSettings{Bedrock: &codersdk.AIProviderBedrockSettings{}}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // A canonical endpoint derives and lower-cases the region + // server-side without an explicit region. + //nolint:gocritic // Owner role is the audience for this endpoint. + created, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: tc.providerType, + Name: tc.name + "-derived", + Enabled: true, + BaseURL: "https://bedrock-runtime.US-EAST-1.amazonaws.com/", + Settings: tc.createSettings, + }) + require.NoError(t, err) + require.Equal(t, tc.providerType, created.Type) + require.NotNil(t, created.Settings.Bedrock) + require.Equal(t, "us-east-1", created.Settings.Bedrock.Region) + + // A new canonical base URL re-derives the region. + updated, err := client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ + BaseURL: ptr.Ref("https://bedrock-runtime.us-west-2.amazonaws.com"), + }) + require.NoError(t, err) + require.NotNil(t, updated.Settings.Bedrock) + require.Equal(t, "us-west-2", updated.Settings.Bedrock.Region) + + // An explicit region wins over derivation. + updated, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ + BaseURL: ptr.Ref("https://bedrock-runtime.us-east-2.amazonaws.com"), + Settings: &codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{Region: "us-gov-west-1"}, + }, + }) + require.NoError(t, err) + require.NotNil(t, updated.Settings.Bedrock) + require.Equal(t, "us-gov-west-1", updated.Settings.Bedrock.Region) + + // Switching to a custom endpoint keeps the stored region + // rather than clearing it. A custom URL has nothing to + // re-derive from, so clearing it would lock the operator out + // of an existing provider. + updated, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ + BaseURL: ptr.Ref("https://bedrock.internal.example.com"), + }) + require.NoError(t, err) + require.NotNil(t, updated.Settings.Bedrock) + require.Equal(t, "us-gov-west-1", updated.Settings.Bedrock.Region) + }) + } + }) + + t.Run("BedrockAllowsBaseURLOnlyConfig", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + // A custom endpoint alone is usable when AWS credentials come from + // the default credential chain. Both request shapes must accept it: + // the native type=bedrock form sends no settings, while the UI sends + // type=anthropic with a bedrock settings discriminator. + cases := []struct { + name string + providerType codersdk.AIProviderType + createSettings codersdk.AIProviderSettings + }{ + { + name: "bedrock-type", + providerType: codersdk.AIProviderTypeBedrock, + }, + { + name: "anthropic-type", + providerType: codersdk.AIProviderTypeAnthropic, + createSettings: codersdk.AIProviderSettings{Bedrock: &codersdk.AIProviderBedrockSettings{}}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + //nolint:gocritic // Owner role is the audience for this endpoint. + created, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: tc.providerType, + Name: tc.name + "-custom", + Enabled: true, + BaseURL: "https://bedrock.internal.example.com", + Settings: tc.createSettings, + }) + require.NoError(t, err) + require.NotNil(t, created.Settings.Bedrock) + require.Empty(t, created.Settings.Bedrock.Region) + + // An unrelated PATCH must validate against the stored base URL + // rather than rejecting the empty region and static credentials. + updated, err := client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ + Enabled: ptr.Ref(false), + }) + require.NoError(t, err) + require.False(t, updated.Enabled) + require.NotNil(t, updated.Settings.Bedrock) + require.Empty(t, updated.Settings.Bedrock.Region) + + // Clearing static credentials is also valid when a custom base + // URL remains, because AWS can resolve ambient credentials. + //nolint:gocritic // Owner role is the audience for this endpoint. + withCreds, err := client.CreateAIProvider(ctx, codersdk.CreateAIProviderRequest{ + Type: tc.providerType, + Name: tc.name + "-creds", + Enabled: true, + BaseURL: "https://bedrock.internal.example.com", + Settings: codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + AccessKey: ptr.Ref("AKIA-fixture"), //nolint:gosec // test fixture, not a real credential + AccessKeySecret: ptr.Ref("secret-fixture"), //nolint:gosec // test fixture, not a real credential + }, + }, + }) + require.NoError(t, err) + + cleared, err := client.UpdateAIProvider(ctx, withCreds.Name, codersdk.UpdateAIProviderRequest{ + Settings: &codersdk.AIProviderSettings{ + Bedrock: &codersdk.AIProviderBedrockSettings{ + AccessKey: ptr.Ref(""), + AccessKeySecret: ptr.Ref(""), + }, + }, + }) + require.NoError(t, err) + require.NotNil(t, cleared.Settings.Bedrock) + require.Empty(t, cleared.Settings.Bedrock.Region) + }) + } + }) } func TestAIProvidersKeyManagement(t *testing.T) { @@ -1357,11 +1515,11 @@ func TestAIProvidersKeyManagement(t *testing.T) { func TestAIProviderSettingsMerge(t *testing.T) { t.Parallel() - t.Run("OmittedSecretsPreserveExisting", func(t *testing.T) { + t.Run("OmittedRegionAndSecretsPreserveExisting", func(t *testing.T) { t.Parallel() - // A PATCH that only rotates non-secret fields must keep the - // existing AccessKey and AccessKeySecret intact so the provider - // keeps authenticating after the update. + // A PATCH that omits Region, AccessKey, and AccessKeySecret must + // keep the existing values so the provider keeps authenticating + // after the update. client, db := coderdtest.NewWithDatabase(t, nil) _ = coderdtest.CreateFirstUser(t, client) ctx := testutil.Context(t, testutil.WaitLong) @@ -1385,10 +1543,7 @@ func TestAIProviderSettingsMerge(t *testing.T) { _, err = client.UpdateAIProvider(ctx, created.Name, codersdk.UpdateAIProviderRequest{ Settings: &codersdk.AIProviderSettings{ - Bedrock: &codersdk.AIProviderBedrockSettings{ - Region: "us-west-2", - Model: "anthropic.claude-3-5-haiku", - }, + Bedrock: &codersdk.AIProviderBedrockSettings{Model: "anthropic.claude-3-5-haiku"}, }, }) require.NoError(t, err) @@ -1399,7 +1554,7 @@ func TestAIProviderSettingsMerge(t *testing.T) { persisted, err := db2sdk.AIProviderSettings(row.Settings) require.NoError(t, err) require.NotNil(t, persisted.Bedrock) - require.Equal(t, "us-west-2", persisted.Bedrock.Region) + require.Equal(t, "us-east-1", persisted.Bedrock.Region) require.Equal(t, "anthropic.claude-3-5-haiku", persisted.Bedrock.Model) require.NotNil(t, persisted.Bedrock.AccessKey) require.Equal(t, "AKIA-old", *persisted.Bedrock.AccessKey) diff --git a/codersdk/aiproviders.go b/codersdk/aiproviders.go index 7b513340bca62..c0169db2ff5da 100644 --- a/codersdk/aiproviders.go +++ b/codersdk/aiproviders.go @@ -234,12 +234,6 @@ func (req CreateAIProviderRequest) Validate() []ValidationError { Detail: "bedrock settings are only valid for type=anthropic or type=bedrock", }) } - if req.Type == AIProviderTypeBedrock && (req.Settings.Bedrock == nil || !req.Settings.Bedrock.IsConfigured()) { - validations = append(validations, ValidationError{ - Field: "settings", - Detail: "type=bedrock requires bedrock settings", - }) - } if req.Type == AIProviderTypeBedrock && len(req.APIKeys) > 0 { validations = append(validations, ValidationError{ Field: "api_keys", diff --git a/site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx b/site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx index 99a1d24a92945..58f3d83e68fda 100644 --- a/site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx +++ b/site/src/pages/AISettingsPage/ProvidersPage/components/ProviderForm.tsx @@ -30,19 +30,10 @@ export type ProviderFormValues = { }; const HTTP_SCHEME_REGEX = /^https?:\/\//i; -const BEDROCK_CANONICAL_URL_REGEX = - /^https:\/\/bedrock-runtime\.([a-z0-9-]+)\.amazonaws\.com\/?$/i; const PROVIDER_NAME_REGEX = /^[a-z0-9]+(-[a-z0-9]+)*$/; export const SAVED_CREDENTIAL_MASK = "********"; -export const parseBedrockRegionFromBaseUrl = ( - baseUrl: string, -): string | undefined => { - const match = BEDROCK_CANONICAL_URL_REGEX.exec(baseUrl.trim()); - return match?.[1]?.toLowerCase(); -}; - const makeNameSchema = (editing: boolean) => editing ? Yup.string() @@ -160,10 +151,7 @@ const makeBedrockSchema = (editing: boolean) => displayName: makeDisplayNameSchema(editing), baseUrl: Yup.string() .url("Endpoint must be a valid URL") - .matches( - BEDROCK_CANONICAL_URL_REGEX, - "Endpoint must be a standard AWS Bedrock URL.", - ) + .matches(HTTP_SCHEME_REGEX, "Endpoint must use http or https.") .required("Endpoint is required"), apiKey: Yup.string(), model: Yup.string().required("Model is required"), @@ -461,7 +449,7 @@ export const ProviderForm: FC = ({ label="Endpoint" description={ <> - In the format of{" "} + For standard AWS endpoints, use{" "} {"https://bedrock-runtime.{region}.amazonaws.com"} diff --git a/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.test.ts b/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.test.ts index 1ae786008cc36..e4ed977f5cdbd 100644 --- a/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.test.ts +++ b/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.test.ts @@ -6,11 +6,7 @@ import { MockAIProviderCopilot, MockAIProviderOpenAI, } from "#/testHelpers/entities"; -import { - type ProviderFormValues, - parseBedrockRegionFromBaseUrl, - SAVED_CREDENTIAL_MASK, -} from "./ProviderForm"; +import { type ProviderFormValues, SAVED_CREDENTIAL_MASK } from "./ProviderForm"; import { aiProviderToFormValues, getProviderDisplayType, @@ -65,76 +61,6 @@ const baseCopilotFormValues: ProviderFormValues = { const settings = (raw: Record): AIProvider["settings"] => raw as unknown as AIProvider["settings"]; -describe("parseBedrockRegionFromBaseUrl", () => { - it("extracts the region from a canonical AWS Bedrock URL", () => { - expect( - parseBedrockRegionFromBaseUrl( - "https://bedrock-runtime.us-east-1.amazonaws.com", - ), - ).toBe("us-east-1"); - }); - - it("accepts a trailing slash", () => { - expect( - parseBedrockRegionFromBaseUrl( - "https://bedrock-runtime.us-west-2.amazonaws.com/", - ), - ).toBe("us-west-2"); - }); - - it("lowercases the region", () => { - expect( - parseBedrockRegionFromBaseUrl( - "https://bedrock-runtime.US-EAST-1.amazonaws.com", - ), - ).toBe("us-east-1"); - }); - - it("trims surrounding whitespace before matching", () => { - expect( - parseBedrockRegionFromBaseUrl( - " https://bedrock-runtime.us-east-1.amazonaws.com ", - ), - ).toBe("us-east-1"); - }); - - it("returns undefined for a non-AWS URL", () => { - expect( - parseBedrockRegionFromBaseUrl("https://bedrock.internal.example.com"), - ).toBeUndefined(); - }); - - it("returns undefined for an empty string", () => { - expect(parseBedrockRegionFromBaseUrl("")).toBeUndefined(); - }); - - it("returns undefined for an http (non-https) URL", () => { - expect( - parseBedrockRegionFromBaseUrl( - "http://bedrock-runtime.us-east-1.amazonaws.com", - ), - ).toBeUndefined(); - }); - - it("returns undefined for a URL with a path", () => { - expect( - parseBedrockRegionFromBaseUrl( - "https://bedrock-runtime.us-east-1.amazonaws.com/v1/something", - ), - ).toBeUndefined(); - }); - - it("returns undefined for the China partition (different TLD)", () => { - // AWS China uses *.amazonaws.com.cn, which the canonical regex does - // not match by design; cn-* users get the explicit Region input. - expect( - parseBedrockRegionFromBaseUrl( - "https://bedrock-runtime.cn-north-1.amazonaws.com.cn", - ), - ).toBeUndefined(); - }); -}); - describe("isBedrockProvider", () => { it("recognises a discriminated bedrock provider", () => { expect(isBedrockProvider(MockAIProviderBedrock)).toBe(true); @@ -334,22 +260,10 @@ describe("providerFormValuesToCreate", () => { expect(req.type).toBe("anthropic"); }); - it("derives the region from a canonical AWS URL", () => { + it("leaves region derivation to the backend", () => { const req = providerFormValuesToCreate(baseBedrockFormValues); const s = req.settings as unknown as Record; expect(s._type).toBe("bedrock"); - expect(s.region).toBe("us-east-1"); - }); - - it("omits the region when the URL is non-canonical", () => { - // The form schema blocks non-canonical endpoints before submit; the - // helper itself stays strict, returning an undefined region rather - // than inventing a value. - const req = providerFormValuesToCreate({ - ...baseBedrockFormValues, - baseUrl: "https://bedrock.internal.example.com", - }); - const s = req.settings as unknown as Record; expect(s.region).toBeUndefined(); }); @@ -371,18 +285,14 @@ describe("providerFormValuesToCreate", () => { expect(s.access_key_secret).toBeUndefined(); }); - it("keeps the region so the backend recognises the Bedrock provider when access keys are omitted", () => { - // The backend treats Region as a configuration signal - // (codersdk.AIProviderBedrockSettings.IsConfigured), so omitting - // the keys must not also strip the region; otherwise the request - // would fail with "type=bedrock requires bedrock settings". + it("omits the region when access keys are omitted", () => { const req = providerFormValuesToCreate({ ...baseBedrockFormValues, accessKey: "", accessKeySecret: "", }); const s = req.settings as unknown as Record; - expect(s.region).toBe("us-east-1"); + expect(s.region).toBeUndefined(); expect(s._type).toBe("bedrock"); }); @@ -484,7 +394,7 @@ describe("providerFormValuesToUpdate", () => { }); describe("Bedrock", () => { - it("derives the region from the canonical URL", () => { + it("leaves region derivation to the backend", () => { const req = providerFormValuesToUpdate( { ...baseBedrockFormValues, @@ -495,23 +405,6 @@ describe("providerFormValuesToUpdate", () => { MockAIProviderBedrock, ); const s = req.settings as unknown as Record; - expect(s.region).toBe("us-west-2"); - }); - - it("omits the region when the URL is non-canonical", () => { - // The form schema blocks non-canonical endpoints before submit; the - // helper itself stays strict, returning an undefined region rather - // than inventing a value. - const req = providerFormValuesToUpdate( - { - ...baseBedrockFormValues, - baseUrl: "https://bedrock.internal.example.com", - accessKey: SAVED_CREDENTIAL_MASK, - accessKeySecret: SAVED_CREDENTIAL_MASK, - }, - MockAIProviderBedrock, - ); - const s = req.settings as unknown as Record; expect(s.region).toBeUndefined(); }); diff --git a/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.ts b/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.ts index 67eec7e4d913a..1df857d0fad23 100644 --- a/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.ts +++ b/site/src/pages/AISettingsPage/ProvidersPage/components/providerFormApiMap.ts @@ -7,11 +7,7 @@ import type { CreateAIProviderRequest, UpdateAIProviderRequest, } from "#/api/typesGenerated"; -import { - type ProviderFormValues, - parseBedrockRegionFromBaseUrl, - SAVED_CREDENTIAL_MASK, -} from "./ProviderForm"; +import { type ProviderFormValues, SAVED_CREDENTIAL_MASK } from "./ProviderForm"; /** Drop placeholder masks so they don't round-trip back to the API. */ const sanitizeCredential = ( @@ -104,7 +100,6 @@ export const getProviderDisplayType = ( }; const buildBedrockSettings = ( - region: string | undefined, model: string, smallFastModel: string, accessKey: string, @@ -112,7 +107,6 @@ const buildBedrockSettings = ( ): BedrockSettingsWire => ({ _type: BEDROCK_SETTINGS_TYPE, _version: BEDROCK_SETTINGS_VERSION, - ...(region ? { region } : {}), model, small_fast_model: smallFastModel, ...(accessKey ? { access_key: accessKey } : {}), @@ -134,9 +128,7 @@ export const providerFormValuesToCreate = ( }; if (values.type === "bedrock") { - const region = parseBedrockRegionFromBaseUrl(base.base_url); const settings = buildBedrockSettings( - region, values.model.trim(), values.smallFastModel.trim(), sanitizeCredential(values.accessKey), @@ -205,12 +197,7 @@ export const providerFormValuesToUpdate = ( // the user is rotating credentials. const credentialsChanged = newAccessKey !== "" && newAccessKeySecret !== ""; - // Yup blocks non-canonical Bedrock URLs upstream, so any `undefined` - // region here is a real bug that should surface, not be papered over. - const region = parseBedrockRegionFromBaseUrl(base.base_url ?? ""); - const settings = buildBedrockSettings( - region, values.model.trim(), values.smallFastModel.trim(), credentialsChanged ? newAccessKey : "",