From 709fb3ce28ce93bbc0ed891bf53ce0a12842452b Mon Sep 17 00:00:00 2001 From: Oscar Hermoso Date: Sun, 5 Oct 2025 13:24:58 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=8D=9E=20Fix=20load=20balancer=20caring?= =?UTF-8?q?=20about=20order=20of=20`server=5Fids`=20(#104)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/data-sources/load_balancer.md | 2 +- docs/resources/load_balancer.md | 3 +- internal/provider/load_balancer_resource.go | 121 +++++++++++------- .../provider/load_balancer_resource_test.go | 82 ++++++++++-- internal/provider/util.go | 40 ++++++ .../resources/load_balancer_resource_gen.go | 4 +- provider_code_spec.json | 5 +- scripts/extend-provider.sh | 18 +++ 8 files changed, 212 insertions(+), 63 deletions(-) diff --git a/docs/data-sources/load_balancer.md b/docs/data-sources/load_balancer.md index 8af4622..39e85a2 100644 --- a/docs/data-sources/load_balancer.md +++ b/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. ### Nested Schema for `forwarding_rules` diff --git a/docs/resources/load_balancer.md b/docs/resources/load_balancer.md index 592a475..f28a3fc 100644 --- a/docs/resources/load_balancer.md +++ b/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 diff --git a/internal/provider/load_balancer_resource.go b/internal/provider/load_balancer_resource.go index d88dea6..03091f3 100644 --- a/internal/provider/load_balancer_resource.go +++ b/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) + } +} diff --git a/internal/provider/load_balancer_resource_test.go b/internal/provider/load_balancer_resource_test.go index 1e8a334..423bd20 100644 --- a/internal/provider/load_balancer_resource_test.go +++ b/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) + } +} diff --git a/internal/provider/util.go b/internal/provider/util.go index af6dec3..31e73c6 100644 --- a/internal/provider/util.go +++ b/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() { diff --git a/internal/resources/load_balancer_resource_gen.go b/internal/resources/load_balancer_resource_gen.go index bbf806c..a937c25 100644 --- a/internal/resources/load_balancer_resource_gen.go +++ b/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{} diff --git a/provider_code_spec.json b/provider_code_spec.json index 7ad0b0b..7eeba9b 100644 --- a/provider_code_spec.json +++ b/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" } diff --git a/scripts/extend-provider.sh b/scripts/extend-provider.sh index deda142..3384840 100755 --- a/scripts/extend-provider.sh +++ b/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"