From ef14077df7a4610c0663bdeb6d28ecf3504f1e4b Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Tue, 21 Oct 2025 10:38:21 +0200 Subject: [PATCH] Fix permanent drift in model_serving when using plaintext credentials Fixes #5074 The GET API for serving endpoints does not return sensitive *_plaintext credential fields for external models (e.g., openai_api_key_plaintext, google_service_account_key_plaintext). This causes Terraform to detect drift even when no actual changes have been made. Changes: - Added reflection-based copySensitiveFields() helper function that automatically copies all *Plaintext fields from Terraform state to API response during Read operations - Applied fix to both databricks_model_serving and databricks_model_serving_provisioned_throughput resources - Added comprehensive unit tests for the sensitive field copying logic - Refactored sensitive field tests to use MockWorkspaceClientFunc pattern The solution is generic and will automatically handle new external model providers and their plaintext credential fields without code changes. --- NEXT_CHANGELOG.md | 1 + serving/resource_model_serving.go | 102 ++++++ ...ce_model_serving_provisioned_throughput.go | 2 + serving/resource_model_serving_test.go | 308 +++++++++++++++++- 4 files changed, 412 insertions(+), 1 deletion(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 08572aea51..bcdf406527 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -9,6 +9,7 @@ ### Bug Fixes * Fix crash when error happens during reading `databricks_job` ([#5110](https://github.com/databricks/terraform-provider-databricks/pull/5110)) +* Fix permanent drift in `databricks_model_serving` when using `*_plaintext` credential fields for external models ([#5125](https://github.com/databricks/terraform-provider-databricks/pull/5125)) ### Documentation diff --git a/serving/resource_model_serving.go b/serving/resource_model_serving.go index c9bbfa3f0b..79c025fa42 100644 --- a/serving/resource_model_serving.go +++ b/serving/resource_model_serving.go @@ -3,6 +3,7 @@ package serving import ( "context" "log" + "reflect" "strings" "time" @@ -60,6 +61,105 @@ func suppressRouteModelEntityNameDiff(k, old, new string, d *schema.ResourceData return false } +// copySensitiveFields recursively copies sensitive plaintext fields from source to destination. +// This is needed because the GET API doesn't return sensitive values, causing drift in Terraform state. +// The function uses reflection to automatically handle all plaintext fields without manual enumeration. +func copySensitiveFields(src, dst reflect.Value) { + // Handle nil pointers + if !src.IsValid() || !dst.IsValid() { + return + } + + // Dereference pointers + if src.Kind() == reflect.Ptr { + if src.IsNil() { + return + } + src = src.Elem() + } + if dst.Kind() == reflect.Ptr { + if dst.IsNil() { + return + } + dst = dst.Elem() + } + + // Only process structs + if src.Kind() != reflect.Struct || dst.Kind() != reflect.Struct { + return + } + + // Ensure types match + if src.Type() != dst.Type() { + return + } + + // Iterate through all fields + for i := 0; i < src.NumField(); i++ { + srcField := src.Field(i) + dstField := dst.Field(i) + fieldType := src.Type().Field(i) + + // Skip unexported fields + if !dstField.CanSet() { + continue + } + + fieldName := fieldType.Name + + // Check if this is a sensitive plaintext field (ends with "Plaintext") + if strings.HasSuffix(fieldName, "Plaintext") && srcField.Kind() == reflect.String { + srcValue := srcField.String() + dstValue := dstField.String() + + // Copy from source to destination if source has a value and destination is empty + if srcValue != "" && dstValue == "" { + dstField.SetString(srcValue) + log.Printf("[DEBUG] Copied sensitive field %s from state", fieldName) + } + continue + } + + // Recursively process nested structs, pointers, slices, and maps + switch srcField.Kind() { + case reflect.Struct: + copySensitiveFields(srcField, dstField) + case reflect.Ptr: + if !srcField.IsNil() && !dstField.IsNil() { + copySensitiveFields(srcField, dstField) + } + case reflect.Slice: + // Process slice elements (e.g., served_entities) + if srcField.Len() > 0 && dstField.Len() > 0 { + minLen := srcField.Len() + if dstField.Len() < minLen { + minLen = dstField.Len() + } + for j := 0; j < minLen; j++ { + copySensitiveFields(srcField.Index(j), dstField.Index(j)) + } + } + case reflect.Map: + // Process map values if needed in the future + continue + } + } +} + +// copySensitiveExternalModelFields copies sensitive plaintext credential fields from the source +// endpoint (from state) to the destination endpoint (from API response). +func copySensitiveExternalModelFields(src, dst *serving.ServingEndpointDetailed) { + if src == nil || dst == nil { + return + } + + // Use reflection to copy all sensitive fields recursively + srcVal := reflect.ValueOf(src) + dstVal := reflect.ValueOf(dst) + + copySensitiveFields(srcVal, dstVal) +} + // updateConfig updates the configuration of the provided serving endpoint to the provided config. func updateConfig(ctx context.Context, w *databricks.WorkspaceClient, name string, e *serving.EndpointCoreConfigInput, d *schema.ResourceData) error { e.Name = name @@ -220,6 +320,8 @@ func ResourceModelServing() common.Resource { if err != nil { return err } + // Copy sensitive plaintext fields from state to API response to prevent drift + copySensitiveExternalModelFields(&sOrig, endpoint) if sOrig.Config == nil { // If it is a new resource, then we only return ServedEntities if endpoint.Config != nil { diff --git a/serving/resource_model_serving_provisioned_throughput.go b/serving/resource_model_serving_provisioned_throughput.go index bd0a933a04..5310bf5892 100644 --- a/serving/resource_model_serving_provisioned_throughput.go +++ b/serving/resource_model_serving_provisioned_throughput.go @@ -72,6 +72,8 @@ func ResourceModelServingProvisionedThroughput() common.Resource { if err != nil { return err } + // Copy sensitive plaintext fields from state to API response to prevent drift + copySensitiveExternalModelFields(&sOrig, endpoint) err = common.StructToData(*endpoint, s, d) if err != nil { return err diff --git a/serving/resource_model_serving_test.go b/serving/resource_model_serving_test.go index 59a42137f5..4b5c95a99a 100644 --- a/serving/resource_model_serving_test.go +++ b/serving/resource_model_serving_test.go @@ -2,14 +2,17 @@ package serving import ( "net/http" + "reflect" "testing" "github.com/databricks/databricks-sdk-go/apierr" - + "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/serving" "github.com/databricks/terraform-provider-databricks/qa" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func TestModelServingCornerCases(t *testing.T) { @@ -702,3 +705,306 @@ func TestModelServingDelete_Error(t *testing.T) { ID: "test-endpoint", }.ExpectError(t, "Internal error happened") } + +func TestModelServingReadWithSensitivePlaintextFields(t *testing.T) { + qa.ResourceFixture{ + MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) { + w.GetMockServingEndpointsAPI().EXPECT(). + GetByName(mock.Anything, "test-endpoint"). + Return(&serving.ServingEndpointDetailed{ + Id: "test-endpoint", + Name: "test-endpoint", + State: &serving.EndpointState{ + ConfigUpdate: serving.EndpointStateConfigUpdateNotUpdating, + }, + EndpointUrl: "https://example.com/endpoint", + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + Name: "gpt-4o-mini", + ExternalModel: &serving.ExternalModel{ + Name: "gpt-4o-mini", + Provider: serving.ExternalModelProviderOpenai, + Task: "llm/v1/chat", + OpenaiConfig: &serving.OpenAiConfig{ + // API doesn't return plaintext fields + OpenaiApiBase: "https://api.openai.com/v1", + }, + }, + }, + }, + TrafficConfig: &serving.TrafficConfig{ + Routes: []serving.Route{ + { + ServedEntityName: "gpt-4o-mini", + TrafficPercentage: 100, + }, + }, + }, + }, + }, nil) + }, + Resource: ResourceModelServing(), + Read: true, + ID: "test-endpoint", + InstanceState: map[string]string{ + "name": "test-endpoint", + "config.0.served_entities.0.external_model.0.openai_config.0.openai_api_key_plaintext": "sk-test-key-12345", + }, + }.ApplyAndExpectData(t, map[string]any{ + "serving_endpoint_id": "test-endpoint", + "endpoint_url": "https://example.com/endpoint", + // Verify the plaintext field is preserved from state + "config.0.served_entities.0.external_model.0.openai_config.0.openai_api_key_plaintext": "sk-test-key-12345", + }) +} + +func TestModelServingReadWithMultipleSensitiveFields(t *testing.T) { + qa.ResourceFixture{ + MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) { + w.GetMockServingEndpointsAPI().EXPECT(). + GetByName(mock.Anything, "test-endpoint"). + Return(&serving.ServingEndpointDetailed{ + Id: "test-endpoint", + Name: "test-endpoint", + State: &serving.EndpointState{ + ConfigUpdate: serving.EndpointStateConfigUpdateNotUpdating, + }, + EndpointUrl: "https://example.com/endpoint", + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + Name: "gpt-4o-mini", + ExternalModel: &serving.ExternalModel{ + Name: "gpt-4o-mini", + Provider: serving.ExternalModelProviderOpenai, + Task: "llm/v1/chat", + OpenaiConfig: &serving.OpenAiConfig{ + // API doesn't return plaintext fields + OpenaiApiBase: "https://api.openai.com/v1", + OpenaiApiType: "azuread", + MicrosoftEntraClientId: "client-id-123", + MicrosoftEntraTenantId: "tenant-id-456", + }, + }, + }, + { + Name: "vertex-ai-model", + ExternalModel: &serving.ExternalModel{ + Name: "vertex-ai-model", + Provider: serving.ExternalModelProviderGoogleCloudVertexAi, + Task: "llm/v1/chat", + GoogleCloudVertexAiConfig: &serving.GoogleCloudVertexAiConfig{ + // API doesn't return plaintext fields + ProjectId: "my-gcp-project", + Region: "us-central1", + }, + }, + }, + }, + TrafficConfig: &serving.TrafficConfig{ + Routes: []serving.Route{ + { + ServedEntityName: "gpt-4o-mini", + TrafficPercentage: 50, + }, + { + ServedEntityName: "vertex-ai-model", + TrafficPercentage: 50, + }, + }, + }, + }, + }, nil) + }, + Resource: ResourceModelServing(), + Read: true, + ID: "test-endpoint", + InstanceState: map[string]string{ + "name": "test-endpoint", + "config.0.served_entities.0.external_model.0.openai_config.0.openai_api_key_plaintext": "sk-test-key-12345", + "config.0.served_entities.0.external_model.0.openai_config.0.microsoft_entra_client_secret_plaintext": "client-secret-xyz", + "config.0.served_entities.1.external_model.0.google_cloud_vertex_ai_config.0.private_key_plaintext": "-----BEGIN PRIVATE KEY-----\ntest-key\n-----END PRIVATE KEY-----", // gitleaks:allow + }, + }.ApplyAndExpectData(t, map[string]any{ + "serving_endpoint_id": "test-endpoint", + "endpoint_url": "https://example.com/endpoint", + "config.0.served_entities.#": 2, + // Verify all plaintext fields are preserved from state + "config.0.served_entities.0.external_model.0.openai_config.0.openai_api_key_plaintext": "sk-test-key-12345", + "config.0.served_entities.0.external_model.0.openai_config.0.microsoft_entra_client_secret_plaintext": "client-secret-xyz", + "config.0.served_entities.1.external_model.0.google_cloud_vertex_ai_config.0.private_key_plaintext": "-----BEGIN PRIVATE KEY-----\ntest-key\n-----END PRIVATE KEY-----", + }) +} + +// TestCopySensitiveFields tests the reflection-based sensitive field copying logic +func TestCopySensitiveFields(t *testing.T) { + // Test case 1: Simple plaintext field copy + t.Run("SimpleOpenAIPlaintextCopy", func(t *testing.T) { + src := &serving.ServingEndpointDetailed{ + Name: "test-endpoint", + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + Name: "gpt-4o-mini", + ExternalModel: &serving.ExternalModel{ + Name: "gpt-4o-mini", + Provider: serving.ExternalModelProviderOpenai, + Task: "llm/v1/chat", + OpenaiConfig: &serving.OpenAiConfig{ + OpenaiApiKeyPlaintext: "sk-test-key-12345", + OpenaiApiBase: "https://api.openai.com/v1", + }, + }, + }, + }, + }, + } + + dst := &serving.ServingEndpointDetailed{ + Name: "test-endpoint", + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + Name: "gpt-4o-mini", + ExternalModel: &serving.ExternalModel{ + Name: "gpt-4o-mini", + Provider: serving.ExternalModelProviderOpenai, + Task: "llm/v1/chat", + OpenaiConfig: &serving.OpenAiConfig{ + // API doesn't return plaintext field + OpenaiApiBase: "https://api.openai.com/v1", + }, + }, + }, + }, + }, + } + + copySensitiveExternalModelFields(src, dst) + + assert.Equal(t, "sk-test-key-12345", dst.Config.ServedEntities[0].ExternalModel.OpenaiConfig.OpenaiApiKeyPlaintext, + "OpenAI plaintext API key should be copied from source") + }) + + // Test case 2: Multiple plaintext fields + t.Run("MultipleProviderPlaintextCopy", func(t *testing.T) { + src := &serving.ServingEndpointDetailed{ + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + ExternalModel: &serving.ExternalModel{ + OpenaiConfig: &serving.OpenAiConfig{ + OpenaiApiKeyPlaintext: "sk-test-key", + MicrosoftEntraClientSecretPlaintext: "client-secret", + }, + }, + }, + { + ExternalModel: &serving.ExternalModel{ + GoogleCloudVertexAiConfig: &serving.GoogleCloudVertexAiConfig{ + PrivateKeyPlaintext: "private-key-content", + }, + }, + }, + }, + }, + } + + dst := &serving.ServingEndpointDetailed{ + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + ExternalModel: &serving.ExternalModel{ + OpenaiConfig: &serving.OpenAiConfig{}, + }, + }, + { + ExternalModel: &serving.ExternalModel{ + GoogleCloudVertexAiConfig: &serving.GoogleCloudVertexAiConfig{}, + }, + }, + }, + }, + } + + copySensitiveExternalModelFields(src, dst) + + assert.Equal(t, "sk-test-key", dst.Config.ServedEntities[0].ExternalModel.OpenaiConfig.OpenaiApiKeyPlaintext) + assert.Equal(t, "client-secret", dst.Config.ServedEntities[0].ExternalModel.OpenaiConfig.MicrosoftEntraClientSecretPlaintext) + assert.Equal(t, "private-key-content", dst.Config.ServedEntities[1].ExternalModel.GoogleCloudVertexAiConfig.PrivateKeyPlaintext) + }) + + // Test case 3: Nil safety + t.Run("NilSafety", func(t *testing.T) { + // Should not panic + copySensitiveExternalModelFields(nil, nil) + + src := &serving.ServingEndpointDetailed{} + copySensitiveExternalModelFields(src, nil) + copySensitiveExternalModelFields(nil, src) + + dst := &serving.ServingEndpointDetailed{} + copySensitiveExternalModelFields(src, dst) // Both empty, should not panic + }) + + // Test case 4: Don't overwrite existing values + t.Run("DontOverwriteExisting", func(t *testing.T) { + src := &serving.ServingEndpointDetailed{ + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + ExternalModel: &serving.ExternalModel{ + OpenaiConfig: &serving.OpenAiConfig{ + OpenaiApiKeyPlaintext: "src-key", + }, + }, + }, + }, + }, + } + + dst := &serving.ServingEndpointDetailed{ + Config: &serving.EndpointCoreConfigOutput{ + ServedEntities: []serving.ServedEntityOutput{ + { + ExternalModel: &serving.ExternalModel{ + OpenaiConfig: &serving.OpenAiConfig{ + OpenaiApiKeyPlaintext: "dst-key-already-set", + }, + }, + }, + }, + }, + } + + copySensitiveExternalModelFields(src, dst) + + // Should not overwrite existing value + assert.Equal(t, "dst-key-already-set", dst.Config.ServedEntities[0].ExternalModel.OpenaiConfig.OpenaiApiKeyPlaintext) + }) + + // Test case 5: Test with reflection directly + t.Run("ReflectionBasedCopy", func(t *testing.T) { + type TestStruct struct { + RegularField string + SensitivePlaintext string + } + + src := TestStruct{ + RegularField: "regular", + SensitivePlaintext: "sensitive-value", + } + + dst := TestStruct{ + RegularField: "regular", + } + + srcVal := reflect.ValueOf(&src) + dstVal := reflect.ValueOf(&dst) + + copySensitiveFields(srcVal, dstVal) + + assert.Equal(t, "sensitive-value", dst.SensitivePlaintext) + }) +}