fix: handle custom instances field policy drift (#577)

Author: laimonasr

castai/resource_autoscaler.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	jsonpatch "github.com/evanphx/json-patch"
+	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
@@ -668,6 +669,8 @@ func getChangedPolicies(ctx context.Context, data types.ResourceProvider, meta i
 			return nil, fmt.Errorf("failed to deserialize policy definitions: %v", err)
 		}
 
+		adjustPolicyForDrift(data, policy)
+
 		data, err := json.Marshal(policy)
 		if err != nil {
 			return nil, fmt.Errorf("failed to serialize policy definition: %v", err)
@@ -740,6 +743,20 @@ func createValidationError(field, value string) error {
 		"Policy:\n%v", field, value)
 }
 
+// adjustPolicyForDrift removes values of certain fields which, if retained, might result in constant plan changes.
+// Changes to legacy autoscaler policies and default node template sync to one another, and if values of certain fields
+// are not equal, then they'll keep producing plan changes. Originally, some of these fields had default values
+// and their values are now stored in pre-existing states, so we cannot use the policies stored in state as-is. We null
+// those fields in case they are not specified in the configuration.
+func adjustPolicyForDrift(data types.ResourceProvider, policy *types.AutoscalerPolicy) {
+	if policy != nil && policy.UnschedulablePods != nil {
+		val, d := data.GetRawConfigAt(cty.GetAttrPath(FieldAutoscalerSettings).IndexInt(0).GetAttr(FieldUnschedulablePods).IndexInt(0).GetAttr(FieldCustomInstancesEnabled))
+		if !d.HasError() && val.IsNull() {
+			policy.UnschedulablePods.CustomInstances = nil
+		}
+	}
+}
+
 // toAutoscalerPolicy converts FieldAutoscalerSettings to types.AutoscalerPolicy for given data.
 func toAutoscalerPolicy(data types.ResourceProvider) (*types.AutoscalerPolicy, error) {
 	out, ok := extractNestedValues(data, FieldAutoscalerSettings, true, true)

castai/resource_autoscaler_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/samber/lo"
 	"github.com/stretchr/testify/require"
 
 	"github.com/castai/terraform-provider-castai/castai/sdk"
@@ -592,20 +593,17 @@ func TestAutoscalerResource_CustomizeDiff(t *testing.T) {
 }
 
 func TestAutoscalerResource_ToAutoscalerPolicy(t *testing.T) {
-	tt := []struct {
-		name        string
+	tt := map[string]struct {
 		data        cty.Value
 		expected    *types.AutoscalerPolicy
 		shouldFail  bool
 		expectedErr error
 	}{
-		{
-			name:     "should return nil when data is nil",
+		"should return nil when data is nil": {
 			data:     cty.NilVal,
 			expected: nil,
 		},
-		{
-			name: "should handle nested objects",
+		"should handle nested objects": {
 			data: cty.ObjectVal(
 				map[string]cty.Value{
 					FieldAutoscalerSettings: cty.ListVal(
@@ -640,7 +638,8 @@ func TestAutoscalerResource_ToAutoscalerPolicy(t *testing.T) {
 			expected: &types.AutoscalerPolicy{
 				Enabled: true,
 				UnschedulablePods: &types.UnschedulablePods{
-					Enabled: true,
+					Enabled:         true,
+					CustomInstances: lo.ToPtr(false),
 					PodPinner: &types.PodPinner{
 						Enabled: true,
 					},
@@ -649,10 +648,10 @@ func TestAutoscalerResource_ToAutoscalerPolicy(t *testing.T) {
 		},
 	}
 
-	for _, test := range tt {
+	for testName, test := range tt {
 		r := require.New(t)
 
-		t.Run(test.name, func(t *testing.T) {
+		t.Run(testName, func(t *testing.T) {
 			resource := resourceAutoscaler()
 			state := terraform.NewInstanceStateShimmedFromValue(test.data, 0)
 			actual, err := toAutoscalerPolicy(resource.Data(state))
@@ -857,6 +856,108 @@ func TestAutoscalerResource_GetChangePolicies_ComparePolicyJsonAndDef(t *testing
 	}
 }
 
+// Checks if the value of custom_instances_enabled is retained as received from the API
+// in case when it is not specified (null) in the resource configuration
+func TestAutoscalerResource_GetChangePolicies_AdjustPolicyForDrift(t *testing.T) {
+	r := require.New(t)
+	mockCtrl := gomock.NewController(t)
+	mockClient := mock_sdk.NewMockClientInterface(mockCtrl)
+	clusterID := "cluster_id"
+	provider := &ProviderConfig{
+		api: &sdk.ClientWithResponses{
+			ClientInterface: mockClient,
+		},
+	}
+	originalPolicyStr := `{
+		"enabled": true,
+		"isScopedMode": false,
+		"nodeTemplatesPartialMatchingEnabled": false,
+		"unschedulablePods": {
+			"enabled": true,
+			"custom_instances_enabled": true
+		}
+	}`
+	expectedPolicyStr := `{
+		"enabled": false,
+		"isScopedMode": false,
+		"nodeTemplatesPartialMatchingEnabled": false,
+		"unschedulablePods": {
+			"enabled": false,
+			"custom_instances_enabled": true
+		}
+	}`
+
+	policyJSON, err := normalizeJSON([]byte(originalPolicyStr))
+	r.NoError(err)
+
+	expectedPolicyJSON, err := normalizeJSON([]byte(expectedPolicyStr))
+	r.NoError(err)
+
+	mockClient.EXPECT().
+		PoliciesAPIGetClusterPolicies(gomock.Any(), gomock.Any(), gomock.Any()).
+		Times(1).
+		DoAndReturn(
+			func(_ context.Context, _ string) (*http.Response, error) {
+				return &http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader(policyJSON))}, nil
+			},
+		)
+
+	value := cty.ObjectVal(map[string]cty.Value{
+		FieldClusterId: cty.StringVal(clusterID),
+		FieldAutoscalerSettings: cty.ListVal(
+			[]cty.Value{
+				cty.ObjectVal(
+					map[string]cty.Value{
+						"enabled": cty.BoolVal(false),
+						"unschedulable_pods": cty.ListVal(
+							[]cty.Value{
+								cty.ObjectVal(
+									map[string]cty.Value{
+										"enabled": cty.BoolVal(false),
+									},
+								),
+							},
+						),
+					},
+				),
+			},
+		),
+	})
+	rawConfig := cty.ObjectVal(
+		map[string]cty.Value{
+			"autoscaler_settings": cty.ListVal(
+				[]cty.Value{
+					cty.ObjectVal(
+						map[string]cty.Value{
+							"enabled": cty.BoolVal(false),
+							"unschedulable_pods": cty.ListVal(
+								[]cty.Value{
+									cty.ObjectVal(
+										map[string]cty.Value{
+											"enabled":                  cty.BoolVal(false),
+											"custom_instances_enabled": cty.NullVal(cty.Bool),
+										},
+									),
+								},
+							),
+						},
+					),
+				},
+			),
+		},
+	)
+
+	stateWithPolicyDefinition := terraform.NewInstanceStateShimmedFromValue(value, 0)
+	stateWithPolicyDefinition.RawConfig = rawConfig
+
+	resource := resourceAutoscaler()
+	data := resource.Data(stateWithPolicyDefinition)
+	result, err := getChangedPolicies(context.Background(), data, provider, clusterID)
+
+	r.NoError(err)
+	r.Equal(string(expectedPolicyJSON), string(result))
+}
+
 func JSONBytesEqual(a, b []byte) (bool, error) {
 	var j, j2 interface{}
 	if err := json.Unmarshal(a, &j); err != nil {

castai/sdk/api.gen.go

@@ -15,7 +15,7 @@ type UnschedulablePods struct {
 	Headroom        *Headroom        `mapstructure:"headroom" json:"headroom,omitempty"`
 	HeadroomSpot    *Headroom        `mapstructure:"headroom_spot" json:"headroomSpot,omitempty"`
 	NodeConstraints *NodeConstraints `mapstructure:"node_constraints" json:"nodeConstraints,omitempty"`
-	CustomInstances bool             `mapstructure:"custom_instances_enabled" json:"customInstancesEnabled"`
+	CustomInstances *bool            `mapstructure:"custom_instances_enabled" json:"customInstancesEnabled,omitempty"`
 	PodPinner       *PodPinner       `mapstructure:"pod_pinner" json:"podPinner,omitempty"`
 }
 

castai/types/autoscaler_policies.go

@@ -1,5 +1,10 @@
 package types
 
+import (
+	"github.com/hashicorp/go-cty/cty"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
+)
+
 // ResourceProvider defines an common interface for *schema.ResourceData and *schema.ResourceDiff
 type ResourceProvider interface {
 	// GetOk functions the same way as ResourceData.GetOk, but it also checks the
@@ -15,4 +20,9 @@ type ResourceProvider interface {
 	//
 	// for more info: *schema.ResourceData or *schema.ResourceDiff
 	GetOkExists(key string) (interface{}, bool)
+
+	// GetRawConfigAt returns a value from the raw config at the given path. In some cases this is necessary
+	// to determine whether an optional field has been defined when accounting for potential state drift due to
+	// external changes.
+	GetRawConfigAt(valPath cty.Path) (cty.Value, diag.Diagnostics)
 }

castai/types/resource.go

@@ -1,6 +1,8 @@
 package main
 
 import (
+	"flag"
+
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/plugin"
 
@@ -13,9 +15,18 @@ var (
 )
 
 func main() {
-	plugin.Serve(&plugin.ServeOpts{
+	var debug bool
+
+	flag.BoolVar(&debug, "debug", false, "set to true to run the provider with support for debuggers like delve")
+	flag.Parse()
+
+	opts := &plugin.ServeOpts{
+		Debug:        debug,
+		ProviderAddr: "registry.terraform.io/castai/castai",
 		ProviderFunc: func() *schema.Provider {
 			return castai.Provider(version)
 		},
-	})
+	}
+
+	plugin.Serve(opts)
 }