🍞 Fix load balancer caring about order of server_ids (#104)

docs/data-sources/load_balancer.md

@@ -32,7 +32,7 @@ data "binarylane_load_balancer" "example" {
 - `ip` (String) The IPv4 address of the load balancer.
 - `name` (String) The hostname of the load balancer.
 - `region` (String) Leave null to create an anycast load balancer.
-- `server_ids` (List of Number) A list of server IDs to assign to this load balancer.
+- `server_ids` (Set of Number) A list of server IDs to assign to this load balancer.
 
 <a id="nestedatt--forwarding_rules"></a>
 ### Nested Schema for `forwarding_rules`

docs/resources/load_balancer.md

@@ -40,7 +40,7 @@ resource "binarylane_load_balancer" "example" {
 - `forwarding_rules` (Attributes List) The rules that control which traffic the load balancer will forward to servers in the pool. Leave null to accept a default "HTTP" only forwarding rule. (see [below for nested schema](#nestedatt--forwarding_rules))
 - `health_check` (Attributes) The rules that determine which servers are considered 'healthy' and in the server pool for the load balancer. Leave this null to accept appropriate defaults based on the forwarding_rules. (see [below for nested schema](#nestedatt--health_check))
 - `region` (String) Leave null to create an anycast load balancer.
-- `server_ids` (List of Number) A list of server IDs to assign to this load balancer.
+- `server_ids` (Set of Number) A list of server IDs to assign to this load balancer.
 - `timeouts` (Attributes) (see [below for nested schema](#nestedatt--timeouts))
 
 ### Read-Only
@@ -82,6 +82,7 @@ Optional:
 Optional:
 
 - `create` (String) A string that can be [parsed as a duration](https://pkg.go.dev/time#ParseDuration) consisting of numbers and unit suffixes, such as "30s" or "2h45m". Valid time units are "s" (seconds), "m" (minutes), "h" (hours).
+- `update` (String) A string that can be [parsed as a duration](https://pkg.go.dev/time#ParseDuration) consisting of numbers and unit suffixes, such as "30s" or "2h45m". Valid time units are "s" (seconds), "m" (minutes), "h" (hours).
 
 ## Import
 

internal/provider/load_balancer_resource.go

@@ -129,6 +129,9 @@ func loadBalancerSchema(ctx context.Context) schema.Schema {
 		Required:            false,
 		Optional:            false,
 		Computed:            true,
+		PlanModifiers: []planmodifier.String{
+			stringplanmodifier.UseStateForUnknown(),
+		},
 	}
 
 	return s
@@ -140,6 +143,7 @@ func (r *loadBalancerResource) Schema(ctx context.Context, req resource.SchemaRe
 	resp.Schema.Attributes["timeouts"] =
 		timeouts.Attributes(ctx, timeouts.Opts{
 			Create: true,
+			Update: true,
 		})
 }
 
@@ -153,9 +157,14 @@ func (r *loadBalancerResource) Create(ctx context.Context, req resource.CreateRe
 		return
 	}
 
+	timeout, diags := data.Timeouts.Create(ctx, 20*time.Minute)
+	resp.Diagnostics.Append(diags...)
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
 	// Create API call logic
 	forwardingRules := []binarylane.ForwardingRuleRequest{}
-	diags := data.LoadBalancerModel.ForwardingRules.ElementsAs(ctx, &forwardingRules, false)
+	diags = data.LoadBalancerModel.ForwardingRules.ElementsAs(ctx, &forwardingRules, false)
 	resp.Diagnostics.Append(diags...)
 	if resp.Diagnostics.HasError() {
 		return
@@ -209,60 +218,20 @@ func (r *loadBalancerResource) Create(ctx context.Context, req resource.CreateRe
 	// Save data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
 
-	// Wait for server to be ready
-	var createActionId int64
+	// Wait for load balancer to be ready
+	var actionId int64
 	for _, action := range *lbResp.JSON200.Links.Actions {
 		if *action.Rel == "create_load_balancer" {
-			createActionId = *action.Id
+			actionId = *action.Id
 			break
 		}
 	}
-	if createActionId == 0 {
-		resp.Diagnostics.AddError(
-			"Unable to wait for load balancer to be created, links.actions with rel=create_load_balancer missing from response",
-			fmt.Sprintf("Received %s creating new load balancer: name=%s. Details: %s", lbResp.Status(), data.Name.ValueString(), lbResp.Body))
+
+	err = r.waitForAction(ctx, actionId)
+	if err != nil {
+		resp.Diagnostics.AddError("Error waiting for load balancer to be created", err.Error())
 		return
 	}
-
-	createTimeout, diags := data.Timeouts.Create(ctx, 20*time.Minute)
-	resp.Diagnostics.Append(diags...)
-	ctx, cancel := context.WithTimeout(ctx, createTimeout)
-	defer cancel()
-	var lastReadyResp *binarylane.GetActionsActionIdResponse
-
-retryLoop:
-	for {
-		select {
-		case <-ctx.Done():
-			if lastReadyResp == nil {
-				return
-			}
-			resp.Diagnostics.AddError(
-				"Timed out waiting for load balancer to be created",
-				fmt.Sprintf(
-					"Timed out waiting for load balancer %s to be created, last response was status=%s, body: %s",
-					data.Name.ValueString(),
-					lastReadyResp.Status(),
-					lastReadyResp.Body,
-				),
-			)
-			return
-		default:
-			readyResp, err := r.bc.client.GetActionsActionIdWithResponse(ctx, createActionId)
-			if err != nil {
-				resp.Diagnostics.AddError("Error waiting for load balancer to be created", err.Error())
-				return
-			}
-
-			if readyResp.StatusCode() == http.StatusOK && readyResp.JSON200.Action.CompletedAt != nil {
-				break retryLoop
-			}
-
-			lastReadyResp = readyResp
-			tflog.Debug(ctx, fmt.Sprintf("Waiting for load balancer to be created: name=%s, status=%s, details: %s", data.Name.ValueString(), readyResp.Status(), readyResp.Body))
-		}
-		time.Sleep(time.Second * 5)
-	}
 }
 
 func (r *loadBalancerResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
@@ -346,6 +315,11 @@ func (r *loadBalancerResource) Update(ctx context.Context, req resource.UpdateRe
 		ServerIds: serverIds,
 	}
 
+	timeout, diags := data.Timeouts.Update(ctx, 20*time.Minute)
+	resp.Diagnostics.Append(diags...)
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+
 	lbResp, err := r.bc.client.PutLoadBalancersLoadBalancerIdWithResponse(ctx, data.Id.ValueInt64(), body)
 	if err != nil {
 		resp.Diagnostics.AddError(
@@ -369,6 +343,21 @@ func (r *loadBalancerResource) Update(ctx context.Context, req resource.UpdateRe
 		return
 	}
 
+	// Wait for load balancer to be ready
+	var actionId int64
+	for _, action := range *lbResp.JSON200.Links.Actions {
+		if *action.Rel == "update_load_balancer" {
+			actionId = *action.Id
+			break
+		}
+	}
+
+	err = r.waitForAction(ctx, actionId)
+	if err != nil {
+		resp.Diagnostics.AddError("Error waiting for load balancer to be updated", err.Error())
+		return
+	}
+
 	// Save updated data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
 }
@@ -473,7 +462,7 @@ func setLoadBalancerModelState(ctx context.Context, data *loadBalancerDataModel,
 		data.Region = types.StringValue(*lb.Region.Slug)
 	}
 
-	data.ServerIds, diags = types.ListValueFrom(ctx, types.Int64Type, lb.ServerIds)
+	data.ServerIds, diags = types.SetValueFrom(ctx, types.Int64Type, lb.ServerIds)
 
 	data.HealthCheck, diag = resources.NewHealthCheckValue(
 		resources.HealthCheckValue{}.AttributeTypes(ctx),
@@ -492,3 +481,37 @@ func setLoadBalancerModelState(ctx context.Context, data *loadBalancerDataModel,
 
 	return diags
 }
+
+func (r *loadBalancerResource) waitForAction(ctx context.Context, actionId int64) error {
+	var lastReadyResp *binarylane.GetActionsActionIdResponse
+
+	for {
+		select {
+		case <-ctx.Done():
+			if lastReadyResp == nil {
+				return fmt.Errorf("timed out waiting for action: action_id=%d", actionId)
+			} else {
+				return fmt.Errorf("timed out waiting for action: action_id=%d, last response was status=%s, body: %s",
+					actionId, lastReadyResp.Status(), lastReadyResp.Body)
+			}
+		default:
+			readyResp, err := r.bc.client.GetActionsActionIdWithResponse(ctx, actionId)
+			if err != nil {
+				return fmt.Errorf("unexpected error waiting for action: action_id=%d, error: %w", actionId, err)
+			}
+			if readyResp.StatusCode() == http.StatusOK && *readyResp.JSON200.Action.Status == binarylane.Errored {
+				return fmt.Errorf("action failed with error: action_id=%d, error: %s", actionId, readyResp.Body)
+			}
+			if readyResp.StatusCode() == http.StatusOK && readyResp.JSON200.Action.CompletedAt != nil {
+				return nil
+			}
+			lastReadyResp = readyResp
+			tflog.Debug(ctx,
+				fmt.Sprintf("waiting for action for action_id=%d: last response was status=%s, details: %s",
+					actionId, readyResp.Status(), readyResp.Body,
+				),
+			)
+		}
+		time.Sleep(time.Second * 5)
+	}
+}

internal/provider/load_balancer_resource_test.go

@@ -4,12 +4,15 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"maps"
 	"net/http"
+	"slices"
 	"strings"
 	"terraform-provider-binarylane/internal/binarylane"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/terraform"
 )
 
 func TestLoadBalancerResource(t *testing.T) {
@@ -50,8 +53,8 @@ data "binarylane_load_balancer" "test" {
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "name", "tf-test-lb"),
 					resource.TestCheckNoResourceAttr("binarylane_load_balancer.test", "region"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "server_ids.#", "2"),
-					resource.TestCheckResourceAttrPair("binarylane_load_balancer.test", "server_ids.0", "binarylane_server.test.0", "id"),
-					resource.TestCheckResourceAttrPair("binarylane_load_balancer.test", "server_ids.1", "binarylane_server.test.1", "id"),
+					testCheckIfResourceAttrContainsAttr("binarylane_load_balancer.test", "server_ids", "binarylane_server.test.0", "id"),
+					testCheckIfResourceAttrContainsAttr("binarylane_load_balancer.test", "server_ids", "binarylane_server.test.1", "id"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "forwarding_rules.#", "1"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "forwarding_rules.0.entry_protocol", "http"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "health_check.path", "/"),
@@ -63,8 +66,8 @@ data "binarylane_load_balancer" "test" {
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "name", "tf-test-lb"),
 					resource.TestCheckNoResourceAttr("data.binarylane_load_balancer.test", "region"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "server_ids.#", "2"),
-					resource.TestCheckResourceAttrPair("data.binarylane_load_balancer.test", "server_ids.0", "binarylane_server.test.0", "id"),
-					resource.TestCheckResourceAttrPair("data.binarylane_load_balancer.test", "server_ids.1", "binarylane_server.test.1", "id"),
+					testCheckIfResourceAttrContainsAttr("data.binarylane_load_balancer.test", "server_ids", "binarylane_server.test.0", "id"),
+					testCheckIfResourceAttrContainsAttr("data.binarylane_load_balancer.test", "server_ids", "binarylane_server.test.1", "id"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "forwarding_rules.#", "1"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "forwarding_rules.0.entry_protocol", "http"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "health_check.path", "/"),
@@ -86,6 +89,7 @@ data "binarylane_load_balancer" "test" {
 				ImportStateVerify: true,
 			},
 			{
+				Taint: []string{"binarylane_server.test[0]"},
 				Config: providerConfig + `
 resource "binarylane_server" "test" {
   count             = 2
@@ -120,8 +124,8 @@ data "binarylane_load_balancer" "test" {
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "name", "tf-test-lb"),
 					resource.TestCheckNoResourceAttr("binarylane_load_balancer.test", "region"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "server_ids.#", "2"),
-					resource.TestCheckResourceAttrPair("binarylane_load_balancer.test", "server_ids.0", "binarylane_server.test.0", "id"),
-					resource.TestCheckResourceAttrPair("binarylane_load_balancer.test", "server_ids.1", "binarylane_server.test.1", "id"),
+					testCheckIfResourceAttrContainsAttr("binarylane_load_balancer.test", "server_ids", "binarylane_server.test.0", "id"),
+					testCheckIfResourceAttrContainsAttr("binarylane_load_balancer.test", "server_ids", "binarylane_server.test.1", "id"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "forwarding_rules.#", "1"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "forwarding_rules.0.entry_protocol", "https"),
 					resource.TestCheckResourceAttr("binarylane_load_balancer.test", "health_check.path", "/test-health"),
@@ -133,8 +137,8 @@ data "binarylane_load_balancer" "test" {
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "name", "tf-test-lb"),
 					resource.TestCheckNoResourceAttr("data.binarylane_load_balancer.test", "region"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "server_ids.#", "2"),
-					resource.TestCheckResourceAttrPair("data.binarylane_load_balancer.test", "server_ids.0", "binarylane_server.test.0", "id"),
-					resource.TestCheckResourceAttrPair("data.binarylane_load_balancer.test", "server_ids.1", "binarylane_server.test.1", "id"),
+					testCheckIfResourceAttrContainsAttr("data.binarylane_load_balancer.test", "server_ids", "binarylane_server.test.0", "id"),
+					testCheckIfResourceAttrContainsAttr("data.binarylane_load_balancer.test", "server_ids", "binarylane_server.test.1", "id"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "forwarding_rules.#", "1"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "forwarding_rules.0.entry_protocol", "https"),
 					resource.TestCheckResourceAttr("data.binarylane_load_balancer.test", "health_check.path", "/test-health"),
@@ -202,3 +206,65 @@ func init() {
 		},
 	})
 }
+
+func primaryInstanceState(s *terraform.State, name string) (*terraform.InstanceState, error) {
+	ms := s.RootModule()
+
+	rs, ok := ms.Resources[name]
+	if !ok {
+		return nil, fmt.Errorf("Not found: %s in %s, resources: %v", name, ms.Path, slices.Collect(maps.Keys(ms.Resources)))
+	}
+	is := rs.Primary
+	if is == nil {
+		return nil, fmt.Errorf("No primary instance: %s in %s", name, ms.Path)
+	}
+	return is, nil
+}
+
+func testCheckIfResourceAttrContainsAttr(collectionKey, collectionPath, otherResourceKey, otherAttrPath string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		collectionState, err := primaryInstanceState(s, collectionKey)
+		if err != nil {
+			return err
+		}
+
+		elementState, err := primaryInstanceState(s, otherResourceKey)
+		if err != nil {
+			return err
+		}
+
+		if collectionKey == otherResourceKey && collectionPath == otherAttrPath {
+			return fmt.Errorf(
+				"comparing self: resource %s attribute %s",
+				collectionKey,
+				collectionPath,
+			)
+		}
+
+		vElement, okElement := elementState.Attributes[otherAttrPath]
+		if !okElement {
+			return fmt.Errorf("%s: Attribute %q not set, cannot be contained in %q from %s", otherResourceKey, otherAttrPath, collectionPath, collectionKey)
+		}
+
+		vCollection, okCollection := collectionState.Attributes[collectionPath+".#"]
+		if !okCollection {
+			return fmt.Errorf("%s: Attribute %q not a collection, cannot contain %q from %s", collectionKey, collectionPath, otherAttrPath, otherResourceKey)
+		}
+
+		for attrKey, attrValue := range collectionState.Attributes {
+			if strings.HasPrefix(attrKey, collectionPath+".") &&
+				!(strings.HasSuffix(attrKey, ".%")) &&
+				!strings.HasSuffix(attrKey, ".#") &&
+				attrValue == vElement {
+				return nil
+			}
+		}
+
+		return fmt.Errorf(
+			"%s: Attribute '%s' expected to contain %#v, got %#v",
+			collectionKey,
+			collectionPath,
+			vElement,
+			vCollection)
+	}
+}

internal/provider/util.go

@@ -127,6 +127,45 @@ func convertResourceAttrToDataSourceAttr(name string, attribute r_schema.Attribu
 		}, nil
 
 	default:
+		if t, isSet := attribute.(r_schema.SetNestedAttribute); isSet {
+			nestedObjectAttrs := make(map[string]d_schema.Attribute)
+			for name, attribute := range t.NestedObject.Attributes {
+				nestedAttribute, err := convertResourceAttrToDataSourceAttr(name, attribute, required, optional)
+				if err != nil {
+					return nil, err
+				}
+				nestedObjectAttrs[name] = nestedAttribute
+			}
+			return d_schema.SetNestedAttribute{
+				NestedObject: d_schema.NestedAttributeObject{
+					Attributes: nestedObjectAttrs,
+					CustomType: t.NestedObject.CustomType,
+					Validators: t.NestedObject.Validators,
+				},
+				CustomType:          t.CustomType,
+				Description:         attribute.GetDescription(),
+				Required:            required,
+				Optional:            optional,
+				Computed:            !required,
+				Sensitive:           attribute.IsSensitive(),
+				MarkdownDescription: attribute.GetMarkdownDescription(),
+				DeprecationMessage:  attribute.GetDeprecationMessage(),
+			}, nil
+		}
+
+		if t, isSet := attribute.GetType().(types.SetType); isSet {
+			return d_schema.SetAttribute{
+				ElementType:         t.ElemType,
+				Description:         attribute.GetDescription(),
+				Required:            required,
+				Optional:            optional,
+				Computed:            !required,
+				Sensitive:           attribute.IsSensitive(),
+				MarkdownDescription: attribute.GetMarkdownDescription(),
+				DeprecationMessage:  attribute.GetDeprecationMessage(),
+			}, nil
+		}
+
 		if t, isList := attribute.(r_schema.ListNestedAttribute); isList {
 			nestedObjectAttrs := make(map[string]d_schema.Attribute)
 			for name, attribute := range t.NestedObject.Attributes {
@@ -165,6 +204,7 @@ func convertResourceAttrToDataSourceAttr(name string, attribute r_schema.Attribu
 				DeprecationMessage:  attribute.GetDeprecationMessage(),
 			}, nil
 		}
+
 		if t, isObject := attribute.(r_schema.SingleNestedAttribute); isObject {
 			attributeTypes := make(map[string]attr.Type, len(t.GetAttributes()))
 			for name, attribute := range t.GetAttributes() {

internal/resources/load_balancer_resource_gen.go

@@ -95,7 +95,7 @@ func LoadBalancerResourceSchema(ctx context.Context) schema.Schema {
 				Description:         "Leave null to create an anycast load balancer.",
 				MarkdownDescription: "Leave null to create an anycast load balancer.",
 			},
-			"server_ids": schema.ListAttribute{
+			"server_ids": schema.SetAttribute{
 				ElementType:         types.Int64Type,
 				Optional:            true,
 				Computed:            true,
@@ -112,7 +112,7 @@ type LoadBalancerModel struct {
 	Id              types.Int64      `tfsdk:"id"`
 	Name            types.String     `tfsdk:"name"`
 	Region          types.String     `tfsdk:"region"`
-	ServerIds       types.List       `tfsdk:"server_ids"`
+	ServerIds       types.Set        `tfsdk:"server_ids"`
 }
 
 var _ basetypes.ObjectTypable = ForwardingRulesType{}

provider_code_spec.json

@@ -798,7 +798,7 @@
 					},
 					{
 						"name": "server_ids",
-						"list": {
+						"set": {
 							"computed_optional_required": "computed_optional",
 							"element_type": {
 								"int64": {}
@@ -1316,5 +1316,6 @@
 			}
 		}
 	],
-	"version": "0.1"
+	"version": "0.1",
+	"$schema": "https://raw.githubusercontent.com/hashicorp/terraform-plugin-codegen-spec/main/spec/v0.1/schema.json"
 }

scripts/extend-provider.sh

@@ -2,6 +2,24 @@
 
 GENERATOR_CONFIG=$(dirname "$0")/../provider_code_spec.json
 
+# Add JSON schema
+cat <<<$(jq --tab '. + {"$schema": "https://raw.githubusercontent.com/hashicorp/terraform-plugin-codegen-spec/main/spec/v0.1/schema.json"}' $GENERATOR_CONFIG) >$GENERATOR_CONFIG
+
 # Server configuration
 ADVANCED_FEATURES_CONFIG=$(dirname "$0")/data/server_advanced_features.json
 cat <<<$(jq --tab --slurpfile adv_feat_cfg $ADVANCED_FEATURES_CONFIG '.resources[1].schema.attributes |= . + $adv_feat_cfg' $GENERATOR_CONFIG) >$GENERATOR_CONFIG
+
+# Use set instead of list for server_ids in load_balancer resource
+jq --tab '
+  .resources |= map(
+    if .name == "load_balancer" then
+      .schema.attributes |= map(
+        if .name == "server_ids" then
+          with_entries(.key |= gsub("list"; "set"))
+        else .
+        end
+      )
+    else .
+    end
+  )
+' "$GENERATOR_CONFIG" >"$GENERATOR_CONFIG.tmp" && mv "$GENERATOR_CONFIG.tmp" "$GENERATOR_CONFIG"