diff --git a/go.mod b/go.mod index a17fe8c..e1ac8bc 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/hashicorp/terraform-plugin-go v0.26.0 github.com/hashicorp/terraform-plugin-testing v1.12.0 github.com/segmentio/public-api-sdk-go v0.0.0-20250113195817-34106b6e08dd + github.com/stretchr/testify v1.10.0 gotest.tools/gotestsum v1.13.0 ) @@ -19,8 +20,10 @@ require ( github.com/Kunde21/markdownfmt/v3 v3.1.0 // indirect github.com/bitfield/gotestdox v0.2.2 // indirect github.com/bmatcuk/doublestar/v4 v4.8.1 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/hashicorp/cli v1.1.7 // indirect github.com/mattn/go-runewidth v0.0.9 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/yuin/goldmark v1.7.7 // indirect github.com/yuin/goldmark-meta v1.1.0 // indirect go.abhg.dev/goldmark/frontmatter v0.2.0 // indirect diff --git a/internal/provider/destination_resource.go b/internal/provider/destination_resource.go index 4dd8210..38ab042 100644 --- a/internal/provider/destination_resource.go +++ b/internal/provider/destination_resource.go @@ -615,9 +615,18 @@ func (r *destinationResource) Read(ctx context.Context, req resource.ReadRequest return } - // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, false) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Destination settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } diags = resp.State.Set(ctx, &state) diff --git a/internal/provider/destination_subscription_resource.go b/internal/provider/destination_subscription_resource.go index be2bf95..ac385fc 100644 --- a/internal/provider/destination_subscription_resource.go +++ b/internal/provider/destination_subscription_resource.go @@ -255,8 +255,18 @@ func (r *destinationSubscriptionResource) Read(ctx context.Context, req resource return } + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, false) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Destination subscription settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } diags = resp.State.Set(ctx, &state) diff --git a/internal/provider/insert_function_instance_resource.go b/internal/provider/insert_function_instance_resource.go index 823da51..0cf365d 100644 --- a/internal/provider/insert_function_instance_resource.go +++ b/internal/provider/insert_function_instance_resource.go @@ -186,8 +186,18 @@ func (r *insertFunctionInstanceResource) Read(ctx context.Context, req resource. return } + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, false) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Insert Function instance settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } // This is to satisfy terraform requirements that the input fields must match the returned ones. The input FunctionID can be prefixed with "ifnd_" and the returned one is not. diff --git a/internal/provider/profiles_warehouse_resource.go b/internal/provider/profiles_warehouse_resource.go index bf9d47a..ec998b7 100644 --- a/internal/provider/profiles_warehouse_resource.go +++ b/internal/provider/profiles_warehouse_resource.go @@ -195,8 +195,18 @@ func (r *profilesWarehouseResource) Read(ctx context.Context, req resource.ReadR return } + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, true) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Profiles Warehouse settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } diags = resp.State.Set(ctx, &state) diff --git a/internal/provider/source_resource.go b/internal/provider/source_resource.go index 7a47e08..3a81273 100644 --- a/internal/provider/source_resource.go +++ b/internal/provider/source_resource.go @@ -415,9 +415,18 @@ func (r *sourceResource) Read(ctx context.Context, req resource.ReadRequest, res return } - // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, false) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Source settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } diags = resp.State.Set(ctx, &state) diff --git a/internal/provider/utils.go b/internal/provider/utils.go index 70dd36c..b0c3b33 100644 --- a/internal/provider/utils.go +++ b/internal/provider/utils.go @@ -3,8 +3,13 @@ package provider import ( "bytes" "encoding/json" + "fmt" "io" "net/http" + "strings" + + "github.com/hashicorp/terraform-plugin-framework-jsontypes/jsontypes" + "github.com/segmentio/terraform-provider-segment/internal/provider/models" ) func getError(err error, body *http.Response) string { @@ -23,3 +28,39 @@ func getError(err error, body *http.Response) string { return err.Error() + "\n" + formattedBody.String() } + +// mergeSettings merges config settings with remote settings, preserving only the keys defined in config. +func mergeSettings(configSettings, remoteSettings jsontypes.Normalized, isWarehouse bool) (jsontypes.Normalized, error) { + var configMap map[string]interface{} + if diags := configSettings.Unmarshal(&configMap); diags.HasError() { + return jsontypes.NewNormalizedNull(), fmt.Errorf("failed to unmarshal config settings: %s", diags.Errors()) + } + + var remoteMap map[string]interface{} + if diags := remoteSettings.Unmarshal(&remoteMap); diags.HasError() { + return jsontypes.NewNormalizedNull(), fmt.Errorf("failed to unmarshal remote settings: %s", diags.Errors()) + } + + // Create merged map with only config-defined keys that exist in remote (to detect drift) + // Keys in config but not in remote are excluded (they don't exist or aren't supported) + merged := make(map[string]interface{}) + for key := range configMap { + if isWarehouse && key == "password" { // Warehouses do not output password in the response + merged[key] = configMap[key] + } else if value, exists := remoteMap[key]; exists { + strValue, ok := value.(string) + if ok && strings.Contains(strValue, "•") { // If the secret is censored, do not update it + merged[key] = configMap[key] + } else { + merged[key] = value + } + } + } + + result, err := models.GetSettings(merged) + if err != nil { + return jsontypes.Normalized{}, fmt.Errorf("failed to merge settings: %w", err) + } + + return result, nil +} diff --git a/internal/provider/utils_test.go b/internal/provider/utils_test.go new file mode 100644 index 0000000..2b101e0 --- /dev/null +++ b/internal/provider/utils_test.go @@ -0,0 +1,266 @@ +package provider + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-framework-jsontypes/jsontypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMergeSettings(t *testing.T) { + t.Parallel() + + t.Run("merges config-defined settings with remote values", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"key1":"configValue1","key2":"configValue2"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key1":"remoteValue1","key2":"remoteValue2","key3":"remoteValue3"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should contain config-defined keys with remote values + assert.Equal(t, "remoteValue1", resultMap["key1"]) + assert.Equal(t, "remoteValue2", resultMap["key2"]) + + // Should NOT contain backend-generated keys + assert.NotContains(t, resultMap, "key3") + }) + + t.Run("excludes config keys missing in remote", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"key1":"configValue1","key2":"configValue2"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key1":"remoteValue1"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should use remote value when available + assert.Equal(t, "remoteValue1", resultMap["key1"]) + + // Should NOT include keys that are in config but not in remote + assert.NotContains(t, resultMap, "key2") + assert.Len(t, resultMap, 1) + }) + + t.Run("ignores backend-only settings", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"apiKey":"myKey"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"apiKey":"myKey","autoGeneratedField":"autoValue","internalFlag":true}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should only contain the config-defined key + assert.Len(t, resultMap, 1) + assert.Equal(t, "myKey", resultMap["apiKey"]) + assert.NotContains(t, resultMap, "autoGeneratedField") + assert.NotContains(t, resultMap, "internalFlag") + }) + + t.Run("detects drift in config-defined settings", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"key1":"originalValue","key2":"originalValue2"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key1":"driftedValue","key2":"originalValue2"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should detect drift by using remote value + assert.Equal(t, "driftedValue", resultMap["key1"]) + assert.Equal(t, "originalValue2", resultMap["key2"]) + }) + + t.Run("handles empty config settings", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key1":"remoteValue1"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should return empty when config has no keys + assert.Empty(t, resultMap) + }) + + t.Run("handles complex nested settings", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"nested":{"key1":"value1"},"simple":"value"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"nested":{"key1":"changedValue"},"simple":"value","extra":"ignored"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Should handle nested objects + assert.Contains(t, resultMap, "nested") + assert.Contains(t, resultMap, "simple") + assert.NotContains(t, resultMap, "extra") + }) + + t.Run("returns error for invalid config JSON", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`invalid json`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key":"value"}`) + + _, err := mergeSettings(configSettings, remoteSettings, false) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to unmarshal config settings") + }) + + t.Run("returns error for invalid remote JSON", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"key":"value"}`) + remoteSettings := jsontypes.NewNormalizedValue(`invalid json`) + + _, err := mergeSettings(configSettings, remoteSettings, false) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to unmarshal remote settings") + }) + + t.Run("preserves config password when isWarehouse is true", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"password":"secretPassword","username":"user123"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"username":"user123"}`) + + result, err := mergeSettings(configSettings, remoteSettings, true) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Password should be preserved from config even though it's not in remote + assert.Equal(t, "secretPassword", resultMap["password"]) + assert.Equal(t, "user123", resultMap["username"]) + }) + + t.Run("does not preserve password when isWarehouse is false", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"password":"secretPassword","username":"user123"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"username":"user123"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Password should NOT be included when not in remote and not a warehouse + assert.NotContains(t, resultMap, "password") + assert.Equal(t, "user123", resultMap["username"]) + }) + + t.Run("preserves config value when remote secret is censored", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"apiKey":"mySecretKey","publicKey":"publicValue"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"apiKey":"••••••••","publicKey":"publicValue"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Censored secret should use config value + assert.Equal(t, "mySecretKey", resultMap["apiKey"]) + // Non-censored value should use remote value + assert.Equal(t, "publicValue", resultMap["publicKey"]) + }) + + t.Run("handles multiple censored secrets", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"secret1":"value1","secret2":"value2","normal":"normalValue"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"secret1":"••••","secret2":"•••••••","normal":"normalValue"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Both censored secrets should use config values + assert.Equal(t, "value1", resultMap["secret1"]) + assert.Equal(t, "value2", resultMap["secret2"]) + // Normal value should use remote value + assert.Equal(t, "normalValue", resultMap["normal"]) + }) + + t.Run("handles censored secrets in warehouse mode", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"password":"dbPassword","apiKey":"myApiKey","username":"user"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"apiKey":"••••••••","username":"user"}`) + + result, err := mergeSettings(configSettings, remoteSettings, true) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Password should be preserved (warehouse mode) + assert.Equal(t, "dbPassword", resultMap["password"]) + // Censored API key should use config value + assert.Equal(t, "myApiKey", resultMap["apiKey"]) + // Username should use remote value + assert.Equal(t, "user", resultMap["username"]) + }) + + t.Run("only treats strings with bullet character as censored", func(t *testing.T) { + t.Parallel() + configSettings := jsontypes.NewNormalizedValue(`{"key1":"configValue","key2":"configValue2"}`) + remoteSettings := jsontypes.NewNormalizedValue(`{"key1":"dotdotdot...","key2":"••••"}`) + + result, err := mergeSettings(configSettings, remoteSettings, false) + + require.NoError(t, err) + + var resultMap map[string]interface{} + diags := result.Unmarshal(&resultMap) + require.False(t, diags.HasError()) + + // Regular dots should use remote value + assert.Equal(t, "dotdotdot...", resultMap["key1"]) + // Bullet character should use config value + assert.Equal(t, "configValue2", resultMap["key2"]) + }) +} diff --git a/internal/provider/warehouse_resource.go b/internal/provider/warehouse_resource.go index 77b9fae..0f963d7 100644 --- a/internal/provider/warehouse_resource.go +++ b/internal/provider/warehouse_resource.go @@ -316,9 +316,18 @@ func (r *warehouseResource) Read(ctx context.Context, req resource.ReadRequest, return } - // This is to satisfy terraform requirements that the returned fields must match the input ones because new settings can be generated in the response + // Merge settings: keep config-defined settings while ignoring backend-generated ones not in config if !previousState.Settings.IsNull() && !previousState.Settings.IsUnknown() { - state.Settings = previousState.Settings + mergedSettings, err := mergeSettings(previousState.Settings, state.Settings, true) + if err != nil { + resp.Diagnostics.AddError( + "Unable to merge Warehouse settings", + err.Error(), + ) + + return + } + state.Settings = mergedSettings } diags = resp.State.Set(ctx, &state)