Fix: prevent perpetual drift in sub-environment configuration blocks (#1098)

* Fix: prevent perpetual drift in sub-environment configuration blocks

Sub-environment read overwrote state with API response in API order and
clobbered values for sensitive variables, causing plans to never converge.

Now merges in schema order, keeps the schema value for sensitive variables
(API returns redacted), and still surfaces remote-only variables as drift.

* Address review: schema-side sensitive flag, error/empty-id tests, doc trade-off

- OR remote.IsSensitive with the schema's is_sensitive flag so the schema value
  is kept even during the transition where the user toggles is_sensitive=true
  before the backend reflects it
- Drop unnecessary pointer-to-loop-var in the drift loop
- Add unit tests for setSubEnvironmentSchema: empty sub-env id skip and error
  propagation
- Add merge subtest for schema-only sensitive flag
- Note the sensitive-edit invisibility trade-off so the perpetual-drift bug is
  not silently reintroduced

Author: yarivg

env0/resource_environment.go

@@ -728,9 +728,8 @@ func resourceEnvironmentRead(ctx context.Context, d *schema.ResourceData, meta a
 	return nil
 }
 
-func setSubEnvironmentSchema(d *schema.ResourceData, apiClient client.ApiClientInterface) any {
+func setSubEnvironmentSchema(d *schema.ResourceData, apiClient client.ApiClientInterface) error {
 	iSubEnvironments, ok := d.GetOk("sub_environment_configuration")
-
 	if !ok {
 		return nil
 	}
@@ -741,19 +740,19 @@ func setSubEnvironmentSchema(d *schema.ResourceData, apiClient client.ApiClientI
 	for _, iSubEnvironment := range subEnvironmentsList {
 		subEnvironment := iSubEnvironment.(map[string]any)
 
-		environmentConfigurationVariables, err := apiClient.ConfigurationVariablesByScope(client.ScopeEnvironment, subEnvironment["id"].(string))
-		if err != nil {
-			return fmt.Errorf("could not fetch environment configuration variables for sub environment %s: %w", subEnvironment["id"].(string), err)
+		subEnvId, _ := subEnvironment["id"].(string)
+		if subEnvId == "" {
+			newSubEnvironments = append(newSubEnvironments, subEnvironment)
+			continue
 		}
 
-		newConfiguration := make([]any, 0, len(environmentConfigurationVariables))
-
-		for _, variable := range environmentConfigurationVariables {
-			newConfiguration = append(newConfiguration, createVariable(&variable))
+		remoteVariables, err := apiClient.ConfigurationVariablesByScope(client.ScopeEnvironment, subEnvId)
+		if err != nil {
+			return fmt.Errorf("could not fetch environment configuration variables for sub environment %s: %w", subEnvId, err)
 		}
 
-		subEnvironment["configuration"] = newConfiguration
-
+		stateVariables, _ := subEnvironment["configuration"].([]any)
+		subEnvironment["configuration"] = mergeSubEnvironmentConfiguration(stateVariables, remoteVariables)
 		newSubEnvironments = append(newSubEnvironments, subEnvironment)
 	}
 
@@ -762,6 +761,54 @@ func setSubEnvironmentSchema(d *schema.ResourceData, apiClient client.ApiClientI
 	return nil
 }
 
+// API returns a redacted value for sensitive variables, so the schema value is kept.
+// Consequence: remote value edits on sensitive variables are not detected — comparing values
+// would reintroduce the perpetual drift this function exists to fix.
+func mergeSubEnvironmentConfiguration(stateVariables []any, remoteVariables client.ConfigurationChanges) []any {
+	merged := make([]any, 0, len(stateVariables))
+
+	for _, ivariable := range stateVariables {
+		variable := ivariable.(map[string]any)
+		variableName := variable["name"].(string)
+		schemaIsSensitive, _ := variable["is_sensitive"].(bool)
+
+		for i := range remoteVariables {
+			remote := &remoteVariables[i]
+			if remote.Name != variableName {
+				continue
+			}
+
+			newVariable := createVariable(remote).(map[string]any)
+
+			remoteIsSensitive := remote.IsSensitive != nil && *remote.IsSensitive
+			if remoteIsSensitive || schemaIsSensitive {
+				newVariable["value"] = variable["value"]
+			}
+
+			merged = append(merged, newVariable)
+
+			break
+		}
+	}
+
+	for _, remote := range remoteVariables {
+		found := false
+
+		for _, ivariable := range stateVariables {
+			if ivariable.(map[string]any)["name"].(string) == remote.Name {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			merged = append(merged, createVariable(&remote))
+		}
+	}
+
+	return merged
+}
+
 func resourceEnvironmentUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
 	apiClient := meta.(client.ApiClientInterface)
 

env0/resource_environment_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/env0/terraform-provider-env0/client/http"
 	"github.com/google/uuid"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"go.uber.org/mock/gomock"
 )
 
@@ -3638,6 +3639,183 @@ func TestUnitEnvironmentWithSubEnvironment(t *testing.T) {
 	})
 }
 
+func TestMergeSubEnvironmentConfiguration(t *testing.T) {
+	varType := client.ConfigurationVariableType(0)
+	trueVal := true
+
+	stateVar := func(name, value string) map[string]any {
+		return map[string]any{"name": name, "value": value}
+	}
+
+	remoteVar := func(name, value string, sensitive *bool) client.ConfigurationVariable {
+		return client.ConfigurationVariable{
+			Name:        name,
+			Value:       value,
+			Type:        &varType,
+			IsSensitive: sensitive,
+			Schema:      &client.ConfigurationVariableSchema{Type: "string"},
+		}
+	}
+
+	t.Run("preserves schema order when API returns reversed", func(t *testing.T) {
+		state := []any{stateVar("a", "av"), stateVar("b", "bv"), stateVar("c", "cv")}
+		remote := client.ConfigurationChanges{
+			remoteVar("c", "cv", nil),
+			remoteVar("b", "bv", nil),
+			remoteVar("a", "av", nil),
+		}
+
+		merged := mergeSubEnvironmentConfiguration(state, remote)
+
+		if len(merged) != 3 {
+			t.Fatalf("expected 3 vars, got %d", len(merged))
+		}
+
+		names := []string{
+			merged[0].(map[string]any)["name"].(string),
+			merged[1].(map[string]any)["name"].(string),
+			merged[2].(map[string]any)["name"].(string),
+		}
+		expected := []string{"a", "b", "c"}
+
+		for i, name := range names {
+			if name != expected[i] {
+				t.Fatalf("order mismatch at %d: got %q want %q", i, name, expected[i])
+			}
+		}
+	})
+
+	t.Run("preserves schema value for sensitive vars without comparing API value", func(t *testing.T) {
+		state := []any{stateVar("secret", "real-secret")}
+		remote := client.ConfigurationChanges{remoteVar("secret", "", &trueVal)}
+
+		merged := mergeSubEnvironmentConfiguration(state, remote)
+
+		if len(merged) != 1 {
+			t.Fatalf("expected 1 var, got %d", len(merged))
+		}
+
+		got := merged[0].(map[string]any)["value"]
+		if got != "real-secret" {
+			t.Fatalf("sensitive value not preserved: got %q", got)
+		}
+	})
+
+	t.Run("preserves schema value when schema marks sensitive but API does not", func(t *testing.T) {
+		state := []any{map[string]any{"name": "secret", "value": "real-secret", "is_sensitive": true}}
+		remote := client.ConfigurationChanges{remoteVar("secret", "stale-from-api", nil)}
+
+		merged := mergeSubEnvironmentConfiguration(state, remote)
+
+		if len(merged) != 1 {
+			t.Fatalf("expected 1 var, got %d", len(merged))
+		}
+
+		got := merged[0].(map[string]any)["value"]
+		if got != "real-secret" {
+			t.Fatalf("schema-sensitive value not preserved: got %q", got)
+		}
+	})
+
+	t.Run("appends remote-only vars as drift", func(t *testing.T) {
+		state := []any{stateVar("a", "av")}
+		remote := client.ConfigurationChanges{
+			remoteVar("a", "av", nil),
+			remoteVar("b", "remote-only", nil),
+		}
+
+		merged := mergeSubEnvironmentConfiguration(state, remote)
+
+		if len(merged) != 2 {
+			t.Fatalf("expected 2 vars (incl drift), got %d", len(merged))
+		}
+
+		if merged[0].(map[string]any)["name"] != "a" {
+			t.Fatalf("expected schema var first, got %v", merged[0])
+		}
+
+		if merged[1].(map[string]any)["name"] != "b" {
+			t.Fatalf("expected drift var second, got %v", merged[1])
+		}
+	})
+
+	t.Run("schema var missing from API is dropped", func(t *testing.T) {
+		state := []any{stateVar("a", "av"), stateVar("gone", "v")}
+		remote := client.ConfigurationChanges{remoteVar("a", "av", nil)}
+
+		merged := mergeSubEnvironmentConfiguration(state, remote)
+
+		if len(merged) != 1 {
+			t.Fatalf("expected 1 var, got %d", len(merged))
+		}
+
+		if merged[0].(map[string]any)["name"] != "a" {
+			t.Fatalf("expected only 'a' var, got %v", merged[0])
+		}
+	})
+}
+
+func TestSetSubEnvironmentSchema(t *testing.T) {
+	t.Run("skips API call when sub-env id is empty (initial deploy not yet propagated)", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		defer ctrl.Finish()
+
+		mock := client.NewMockApiClientInterface(ctrl)
+
+		d := schema.TestResourceDataRaw(t, resourceEnvironment().Schema, map[string]any{
+			"name":        "env",
+			"project_id":  "p",
+			"template_id": "t",
+			"sub_environment_configuration": []any{
+				map[string]any{
+					"alias": "rootService1",
+					"configuration": []any{
+						map[string]any{"name": "ALPHA", "value": "av"},
+					},
+				},
+			},
+		})
+
+		if err := setSubEnvironmentSchema(d, mock); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("propagates API errors as wrapped error", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		defer ctrl.Finish()
+
+		mock := client.NewMockApiClientInterface(ctrl)
+		mock.EXPECT().
+			ConfigurationVariablesByScope(client.ScopeEnvironment, "sub-1").
+			Return(nil, errors.New("boom"))
+
+		d := schema.TestResourceDataRaw(t, resourceEnvironment().Schema, map[string]any{
+			"name":        "env",
+			"project_id":  "p",
+			"template_id": "t",
+			"sub_environment_configuration": []any{
+				map[string]any{
+					"id":    "sub-1",
+					"alias": "rootService1",
+					"configuration": []any{
+						map[string]any{"name": "ALPHA", "value": "av"},
+					},
+				},
+			},
+		})
+
+		err := setSubEnvironmentSchema(d, mock)
+		if err == nil {
+			t.Fatalf("expected error, got nil")
+		}
+
+		if !strings.Contains(err.Error(), "sub-1") || !strings.Contains(err.Error(), "boom") {
+			t.Fatalf("expected wrapped error mentioning sub-1 and boom, got %q", err.Error())
+		}
+	})
+}
+
 func TestUnitEnvironmentIsRequiredDeprecated(t *testing.T) {
 	resourceType := "env0_environment"
 	resourceName := "test"