Merge remote-tracking branch 'origin/main' into benvin/reverse-name

internal/provider/server_firewall_rules_resource.go

@@ -121,11 +121,6 @@ func (r *serverFirewallRulesResource) Create(ctx context.Context, req resource.C
 		return
 	}
 
-	resp.Diagnostics.Append(resolveUnknownDestinationPorts(ctx, &data)...)
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
 	// Save data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
 }
@@ -212,11 +207,6 @@ func (r *serverFirewallRulesResource) Update(ctx context.Context, req resource.U
 		return
 	}
 
-	resp.Diagnostics.Append(resolveUnknownDestinationPorts(ctx, &data)...)
-	if resp.Diagnostics.HasError() {
-		return
-	}
-
 	// Save updated data into Terraform state
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
 }
@@ -255,22 +245,6 @@ func (r *serverFirewallRulesResource) Delete(ctx context.Context, req resource.D
 	}
 }
 
-// Omitted destination_ports (all ports) are planned as unknown because the attribute is computed.
-func resolveUnknownDestinationPorts(ctx context.Context, data *serverFirewallRulesResourceModel) diag.Diagnostics {
-	var rules []resources.FirewallRulesValue
-	diags := data.FirewallRules.ElementsAs(ctx, &rules, false)
-	if diags.HasError() {
-		return diags
-	}
-	for i := range rules {
-		if rules[i].DestinationPorts.IsUnknown() {
-			rules[i].DestinationPorts = types.ListNull(types.StringType)
-		}
-	}
-	data.FirewallRules, diags = types.ListValueFrom(ctx, resources.FirewallRulesValue{}.Type(ctx), rules)
-	return diags
-}
-
 func GetFirewallRulesState(ctx context.Context, firewallRules *[]binarylane.AdvancedFirewallRule) (basetypes.ListValue, diag.Diagnostics) {
 	var diags diag.Diagnostics
 
@@ -284,7 +258,7 @@ func GetFirewallRulesState(ctx context.Context, firewallRules *[]binarylane.Adva
 			destinationAddresses, diag := types.ListValueFrom(ctx, types.StringType, rule.DestinationAddresses)
 			diags.Append(diag...)
 
-			destinationPorts := types.ListNull(types.StringType)
+			var destinationPorts basetypes.ListValue
 			if rule.DestinationPorts != nil {
 				destinationPorts, diag = types.ListValueFrom(ctx, types.StringType, rule.DestinationPorts)
 				diags.Append(diag...)

internal/provider/server_firewall_rules_resource_unit_test.go

@@ -1,94 +0,0 @@
-package provider
-
-import (
-	"context"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"terraform-provider-binarylane/internal/binarylane"
-	"terraform-provider-binarylane/internal/resources"
-
-	"github.com/hashicorp/terraform-plugin-framework/attr"
-	"github.com/hashicorp/terraform-plugin-framework/path"
-	"github.com/hashicorp/terraform-plugin-framework/resource"
-	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
-	"github.com/hashicorp/terraform-plugin-framework/types"
-	"github.com/hashicorp/terraform-plugin-go/tftypes"
-)
-
-func TestServerFirewallRulesResourceOmittedPorts(t *testing.T) {
-	ctx := context.Background()
-
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.Header().Set("Content-Type", "application/json")
-		if r.Method == http.MethodGet {
-			_, _ = w.Write([]byte(`{"firewall_rules":[{"action":"accept","protocol":"icmp","source_addresses":["10.0.0.0/8"],"destination_addresses":["10.0.0.1"],"destination_ports":null}]}`))
-			return
-		}
-		_, _ = w.Write([]byte(`{}`))
-	}))
-	defer srv.Close()
-
-	client, err := binarylane.NewClientWithConfig(srv.URL, "test")
-	if err != nil {
-		t.Fatal(err)
-	}
-	r := &serverFirewallRulesResource{bc: &BinarylaneClient{client: client}}
-
-	schemaResp := resource.SchemaResponse{}
-	r.Schema(ctx, resource.SchemaRequest{}, &schemaResp)
-	s := schemaResp.Schema
-	emptyState := func() tfsdk.State {
-		return tfsdk.State{Schema: s, Raw: tftypes.NewValue(s.Type().TerraformType(ctx), nil)}
-	}
-
-	ruleType := resources.FirewallRulesValue{}
-	rule := resources.NewFirewallRulesValueMust(ruleType.AttributeTypes(ctx), map[string]attr.Value{
-		"description":           types.StringNull(),
-		"action":                types.StringValue("accept"),
-		"protocol":              types.StringValue("icmp"),
-		"destination_addresses": types.ListValueMust(types.StringType, []attr.Value{types.StringValue("10.0.0.1")}),
-		"destination_ports":     types.ListUnknown(types.StringType),
-		"source_addresses":      types.ListValueMust(types.StringType, []attr.Value{types.StringValue("10.0.0.0/8")}),
-	})
-	model := serverFirewallRulesResourceModel{}
-	model.ServerId = types.Int64Value(1)
-	model.FirewallRules = types.ListValueMust(ruleType.Type(ctx), []attr.Value{rule})
-
-	plan := tfsdk.Plan{Schema: s, Raw: tftypes.NewValue(s.Type().TerraformType(ctx), nil)}
-	if diags := plan.Set(ctx, &model); diags.HasError() {
-		t.Fatal(diags)
-	}
-
-	createResp := resource.CreateResponse{State: emptyState()}
-	r.Create(ctx, resource.CreateRequest{Plan: plan}, &createResp)
-	if createResp.Diagnostics.HasError() {
-		t.Fatal(createResp.Diagnostics)
-	}
-	var ports types.List
-	createResp.State.GetAttribute(ctx, path.Root("firewall_rules").AtListIndex(0).AtName("destination_ports"), &ports)
-	if !ports.IsNull() {
-		t.Fatalf("create: expected null destination_ports, got %s", ports)
-	}
-
-	updateResp := resource.UpdateResponse{State: emptyState()}
-	r.Update(ctx, resource.UpdateRequest{Plan: plan}, &updateResp)
-	if updateResp.Diagnostics.HasError() {
-		t.Fatal(updateResp.Diagnostics)
-	}
-	updateResp.State.GetAttribute(ctx, path.Root("firewall_rules").AtListIndex(0).AtName("destination_ports"), &ports)
-	if !ports.IsNull() {
-		t.Fatalf("update: expected null destination_ports, got %s", ports)
-	}
-
-	readResp := resource.ReadResponse{State: createResp.State}
-	r.Read(ctx, resource.ReadRequest{State: createResp.State}, &readResp)
-	if readResp.Diagnostics.HasError() {
-		t.Fatal(readResp.Diagnostics)
-	}
-	readResp.State.GetAttribute(ctx, path.Root("firewall_rules").AtListIndex(0).AtName("destination_ports"), &ports)
-	if !ports.IsNull() {
-		t.Fatalf("read: expected null destination_ports, got %s", ports)
-	}
-}

internal/provider/server_resource.go

@@ -1361,36 +1361,28 @@ func (r *serverResource) ImportState(
 	resp.Diagnostics.Append(diags...)
 }
 
-var (
-	serverActionPollInterval = 5 * time.Second
-	serverActionTimeout      = 20 * time.Minute
-)
-
 func (r *serverResource) waitForServerAction(ctx context.Context, serverId int64, actionId int64) error {
-	if _, ok := ctx.Deadline(); !ok {
-		var cancel context.CancelFunc
-		ctx, cancel = context.WithTimeout(ctx, serverActionTimeout)
-		defer cancel()
-	}
-
 	var lastReadyResp *binarylane.GetServersServerIdActionsActionIdResponse
 
 	for {
-		readyResp, err := r.bc.client.GetServersServerIdActionsActionIdWithResponse(ctx, serverId, actionId)
-		if err != nil && ctx.Err() == nil {
-			return fmt.Errorf("unexpected error waiting for server action: server_id=%d, action_id=%d, error: %w", serverId, actionId, err)
-		}
-		if err == nil {
-			if readyResp.StatusCode() == http.StatusNotFound {
-				return fmt.Errorf("server action not found: server_id=%d, action_id=%d, body: %s", serverId, actionId, readyResp.Body)
+		select {
+		case <-ctx.Done():
+			if lastReadyResp == nil {
+				return fmt.Errorf("timed out waiting for server action: server_id=%d, action_id=%d", serverId, actionId)
+			} else {
+				return fmt.Errorf("timed out waiting for server action: server_id=%d, action_id=%d, last response was status=%s, body: %s",
+					serverId, actionId, lastReadyResp.Status(), lastReadyResp.Body)
 			}
-			if readyResp.StatusCode() == http.StatusOK {
-				switch {
-				case readyResp.JSON200.Action.Status == binarylane.Errored:
-					return fmt.Errorf("server action failed to with error: server_id=%d, action_id=%d, error: %s", serverId, actionId, readyResp.Body)
-				case readyResp.JSON200.Action.Status == binarylane.Completed, readyResp.JSON200.Action.CompletedAt != nil:
-					return nil
-				}
+		default:
+			readyResp, err := r.bc.client.GetServersServerIdActionsActionIdWithResponse(ctx, serverId, actionId)
+			if err != nil {
+				return fmt.Errorf("unexpected error waiting for server action: server_id=%d, action_id=%d, error: %w", serverId, actionId, err)
+			}
+			if readyResp.StatusCode() == http.StatusOK && readyResp.JSON200.Action.Status == binarylane.Errored {
+				return fmt.Errorf("server action failed to with error: server_id=%d, action_id=%d, error: %s", serverId, actionId, readyResp.Body)
+			}
+			if readyResp.StatusCode() == http.StatusOK && readyResp.JSON200.Action.CompletedAt != nil {
+				return nil
 			}
 			lastReadyResp = readyResp
 			tflog.Debug(ctx,
@@ -1399,16 +1391,7 @@ func (r *serverResource) waitForServerAction(ctx context.Context, serverId int64
 				),
 			)
 		}
-
-		select {
-		case <-ctx.Done():
-			if lastReadyResp == nil {
-				return fmt.Errorf("timed out waiting for server action: server_id=%d, action_id=%d", serverId, actionId)
-			}
-			return fmt.Errorf("timed out waiting for server action: server_id=%d, action_id=%d, last response was status=%s, body: %s",
-				serverId, actionId, lastReadyResp.Status(), lastReadyResp.Body)
-		case <-time.After(serverActionPollInterval):
-		}
+		time.Sleep(time.Second * 5)
 	}
 }
 

internal/provider/server_reverse_name_resource.go

@@ -8,7 +8,6 @@ import (
 	"strconv"
 	"strings"
 	"terraform-provider-binarylane/internal/binarylane"
-	"time"
 
 	"github.com/hashicorp/terraform-plugin-framework-validators/int64validator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
@@ -263,40 +262,17 @@ func (r *serverReverseNameResource) changeReverseName(ctx context.Context, serve
 	if actionResp.StatusCode() != http.StatusOK && actionResp.StatusCode() != http.StatusAccepted {
 		return fmt.Errorf("unexpected HTTP status code changing reverse name for server: server_id=%d, details: %s", serverId, actionResp.Body)
 	}
-	// The change_reverse_name action can stay unresolvable, so wait for the result on the server instead
-	if reverseName == nil {
+	// 202 Accepted carries no action ID to poll
+	if actionResp.JSON200 == nil {
 		return nil
 	}
-	return r.waitForReverseName(ctx, serverId, ipv4Address, *reverseName)
-}
 
-func (r *serverReverseNameResource) waitForReverseName(ctx context.Context, serverId int64, ipv4Address string, want string) error {
-	ctx, cancel := context.WithTimeout(ctx, serverActionTimeout)
-	defer cancel()
-
-	var last string
-	for {
-		serverResp, err := r.bc.client.GetServersServerIdWithResponse(ctx, serverId)
-		if err == nil && serverResp.StatusCode() == http.StatusOK {
-			for _, n := range serverResp.JSON200.Server.Networks.V4 {
-				if n.IpAddress != ipv4Address {
-					continue
-				}
-				if n.ReverseName != nil {
-					if *n.ReverseName == want {
-						return nil
-					}
-					last = *n.ReverseName
-				}
-			}
-		}
-
-		select {
-		case <-ctx.Done():
-			return fmt.Errorf("timed out waiting for reverse name: server_id=%d, ipv4_address=%s, want=%q, last=%q", serverId, ipv4Address, want, last)
-		case <-time.After(serverActionPollInterval):
-		}
+	sr := serverResource{bc: r.bc}
+	if err := sr.waitForServerAction(ctx, serverId, actionResp.JSON200.Action.Id); err != nil {
+		return fmt.Errorf("error changing reverse name: %w", err)
 	}
+
+	return nil
 }
 
 func formatServerReverseNameId(serverId int64, ipv4Address string) string {

internal/provider/server_reverse_name_resource_unit_test.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"terraform-provider-binarylane/internal/binarylane"
 
@@ -41,6 +40,8 @@ func TestServerReverseNameResourceUnit(t *testing.T) {
 			if postStatus == http.StatusOK {
 				_, _ = w.Write([]byte(`{"action":{"id":9,"status":"in-progress"}}`))
 			}
+		case r.Method == http.MethodGet && r.URL.Path == "/servers/1/actions/9":
+			_, _ = w.Write([]byte(`{"action":{"id":9,"status":"completed","completed_at":"2026-01-01T00:00:00Z"}}`))
 		case r.Method == http.MethodGet && r.URL.Path == "/servers/1":
 			if !serverFound {
 				w.WriteHeader(http.StatusNotFound)
@@ -119,7 +120,6 @@ func TestServerReverseNameResourceUnit(t *testing.T) {
 		t.Fatal("read: expected state removed when server is 404")
 	}
 
-	serverFound, serverIp, serverName = true, "203.0.113.7", "ptr.example.com"
 	postStatus = http.StatusAccepted
 	updateResp := resource.UpdateResponse{State: emptyState()}
 	r.Update(ctx, resource.UpdateRequest{Plan: plan, State: state}, &updateResp)
@@ -139,84 +139,3 @@ func TestServerReverseNameResourceUnit(t *testing.T) {
 		t.Fatalf("delete: unexpected request body %v", lastBody)
 	}
 }
-
-// The change_reverse_name action never resolves while the PTR is already applied on the server.
-func TestServerReverseNameCreateStuckActionUnit(t *testing.T) {
-	serverActionPollInterval = time.Millisecond
-	t.Cleanup(func() { serverActionPollInterval = 5 * time.Second })
-
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
-
-	serverGets := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.Header().Set("Content-Type", "application/json")
-		switch {
-		case r.Method == http.MethodPost && r.URL.Path == "/servers/1/actions":
-			_, _ = w.Write([]byte(`{"action":{"id":9,"status":"in-progress"}}`))
-		case r.Method == http.MethodGet && r.URL.Path == "/servers/1/actions/9":
-			w.WriteHeader(http.StatusNotFound)
-		case r.Method == http.MethodGet && r.URL.Path == "/servers/1":
-			serverGets++
-			name := "default.example.net"
-			if serverGets > 2 {
-				name = "ptr.example.com"
-			}
-			_, _ = fmt.Fprintf(w, `{"server":{"id":1,"networks":{"v4":[{"ip_address":"203.0.113.7","type":"public","reverse_name":%q}]}}}`, name)
-		default:
-			t.Errorf("unexpected request %s %s", r.Method, r.URL.Path)
-			w.WriteHeader(http.StatusTeapot)
-		}
-	}))
-	defer srv.Close()
-
-	client, err := binarylane.NewClientWithConfig(srv.URL, "test")
-	if err != nil {
-		t.Fatal(err)
-	}
-	r := &serverReverseNameResource{bc: &BinarylaneClient{client: client}}
-	want := "ptr.example.com"
-	if err := r.changeReverseName(ctx, 1, "203.0.113.7", &want); err != nil {
-		t.Fatalf("create must finish once the server reports the reverse name: %v", err)
-	}
-	if serverGets != 3 {
-		t.Fatalf("expected to poll the server until the reverse name applied, got %d GETs", serverGets)
-	}
-}
-
-func TestWaitForServerActionUnit(t *testing.T) {
-	serverActionPollInterval = time.Millisecond
-	t.Cleanup(func() { serverActionPollInterval = 5 * time.Second })
-
-	for name, tc := range map[string]struct {
-		status  int
-		body    string
-		wantErr bool
-	}{
-		"completed_at set":          {http.StatusOK, `{"action":{"id":9,"status":"in-progress","completed_at":"2026-01-01T00:00:00Z"}}`, false},
-		"completed status only":     {http.StatusOK, `{"action":{"id":9,"status":"completed"}}`, false},
-		"errored":                   {http.StatusOK, `{"action":{"id":9,"status":"errored"}}`, true},
-		"not found":                 {http.StatusNotFound, `{"message":"Action not found."}`, true},
-		"in progress until timeout": {http.StatusOK, `{"action":{"id":9,"status":"in-progress"}}`, true},
-	} {
-		t.Run(name, func(t *testing.T) {
-			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				w.Header().Set("Content-Type", "application/json")
-				w.WriteHeader(tc.status)
-				_, _ = w.Write([]byte(tc.body))
-			}))
-			defer srv.Close()
-
-			client, err := binarylane.NewClientWithConfig(srv.URL, "test")
-			if err != nil {
-				t.Fatal(err)
-			}
-			sr := serverResource{bc: &BinarylaneClient{client: client}}
-			ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
-			defer cancel()
-			if err := sr.waitForServerAction(ctx, 1, 9); (err != nil) != tc.wantErr {
-				t.Fatalf("wantErr=%v, got %v", tc.wantErr, err)
-			}
-		})
-	}
-}