From 6d2a21691dc5d4a25d095f416c37289ef1011126 Mon Sep 17 00:00:00 2001 From: Jan Schreier Date: Fri, 4 Sep 2026 12:52:32 +0200 Subject: [PATCH] feat(sfs): make resource pool wait timeouts configurable CreateResourcePoolWaitHandler and its update/delete counterparts default to 10 minutes. The resource passed a context without a deadline, so that default was the only limit and no configuration could reach it. A pool that STACKIT needs longer than 10 minutes to provision could not be created at all. The SDK wait handler applies its own timeout only when the incoming context carries no deadline (core/wait.WaitWithContext). Setting a context deadline in each CRUD method therefore replaces the hardcoded value, which is what the new `timeouts` attribute does. Defaults stay at the wait handler value plus core.DefaultTimeoutMargin, so unconfigured resources keep their behavior. The configured timeouts are written to state together with the IDs before the create wait starts. Without that, a failed wait leaves an entry whose refresh and destroy fall back to the default timeouts - on exactly the recovery path those values are needed for. The error raised when the create wait handler gives up now says that Terraform marks the resource tainted and replaces it on the next run, names `untaint` and the import ID, and mentions `timeouts.create` only when this context's deadline is what ended the wait. The handler reports terminal error states and failing polls through the same error, which are not timeouts. TestWaitHandlerTimeoutIsBoundedByContext pins the SDK behavior the attribute depends on, so an SDK bump that enforces the handler timeout unconditionally fails the build instead of silently capping the configured value again. --- docs/resources/sfs_resource_pool.md | 12 + .../services/sfs/resourcepool/resource.go | 88 +++++-- stackit/internal/services/sfs/sfs_test.go | 234 ++++++++++++++++++ .../sfs/testdata/resource-pool-max.tf | 7 + 4 files changed, 327 insertions(+), 14 deletions(-) diff --git a/docs/resources/sfs_resource_pool.md b/docs/resources/sfs_resource_pool.md index 60368822f..ca0cdffcb 100644 --- a/docs/resources/sfs_resource_pool.md +++ b/docs/resources/sfs_resource_pool.md @@ -51,6 +51,7 @@ resource "stackit_sfs_resource_pool" "resourcepool" { - `region` (String) The resource region. If not defined, the provider region is used. - `snapshot_policy` (Attributes) Name of the snapshot policy. (see [below for nested schema](#nestedatt--snapshot_policy)) - `snapshots_are_visible` (Boolean) If set to true, snapshots are visible and accessible to users. (default: false) +- `timeouts` (Attributes) (see [below for nested schema](#nestedatt--timeouts)) ### Read-Only @@ -68,6 +69,17 @@ Read-Only: - `name` (String) Name of the snapshot policy. + + +### Nested Schema for `timeouts` + +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). +- `delete` (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). Setting a timeout for a Delete operation is only applicable if changes are saved into state before the destroy operation occurs. +- `read` (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). Read operations occur during any refresh or planning operation when refresh is enabled. +- `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 Import is supported using the following syntax: diff --git a/stackit/internal/services/sfs/resourcepool/resource.go b/stackit/internal/services/sfs/resourcepool/resource.go index 096510ceb..15995dd73 100644 --- a/stackit/internal/services/sfs/resourcepool/resource.go +++ b/stackit/internal/services/sfs/resourcepool/resource.go @@ -8,6 +8,7 @@ import ( "net/http" "strings" + "github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" @@ -43,18 +44,19 @@ var ( ) type Model struct { - Id types.String `tfsdk:"id"` // needed by TF - ProjectId types.String `tfsdk:"project_id"` - ResourcePoolId types.String `tfsdk:"resource_pool_id"` - AvailabilityZone types.String `tfsdk:"availability_zone"` - IpAcl types.List `tfsdk:"ip_acl"` - Name types.String `tfsdk:"name"` - Labels types.Map `tfsdk:"labels"` - PerformanceClass types.String `tfsdk:"performance_class"` - SizeGigabytes types.Int32 `tfsdk:"size_gigabytes"` - SnapshotPolicy types.Object `tfsdk:"snapshot_policy"` - Region types.String `tfsdk:"region"` - SnapshotsAreVisible types.Bool `tfsdk:"snapshots_are_visible"` + Id types.String `tfsdk:"id"` // needed by TF + ProjectId types.String `tfsdk:"project_id"` + ResourcePoolId types.String `tfsdk:"resource_pool_id"` + AvailabilityZone types.String `tfsdk:"availability_zone"` + IpAcl types.List `tfsdk:"ip_acl"` + Name types.String `tfsdk:"name"` + Labels types.Map `tfsdk:"labels"` + PerformanceClass types.String `tfsdk:"performance_class"` + SizeGigabytes types.Int32 `tfsdk:"size_gigabytes"` + SnapshotPolicy types.Object `tfsdk:"snapshot_policy"` + Region types.String `tfsdk:"region"` + SnapshotsAreVisible types.Bool `tfsdk:"snapshots_are_visible"` + Timeouts timeouts.Value `tfsdk:"timeouts"` } type SnapshotPolicyModel struct { @@ -134,7 +136,7 @@ func (r *resourcePoolResource) Configure(ctx context.Context, req resource.Confi } // Schema defines the schema for the resource. -func (r *resourcePoolResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { +func (r *resourcePoolResource) Schema(ctx context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { description := "Resource-pool resource schema. Must have a `region` specified in the provider configuration." resp.Schema = schema.Schema{ MarkdownDescription: features.AddBetaDescription(description, core.Resource), @@ -245,6 +247,7 @@ func (r *resourcePoolResource) Schema(_ context.Context, _ resource.SchemaReques }, }, }, + "timeouts": timeouts.AttributesAll(ctx), }, } } @@ -259,6 +262,17 @@ func (r *resourcePoolResource) Create(ctx context.Context, req resource.CreateRe return } + // The wait handler only enforces its own timeout when the context carries no deadline, + // so the context deadline set here is what actually bounds the polling. + waiterTimeout := wait.CreateResourcePoolWaitHandler(ctx, r.client.DefaultAPI, "", "", "").GetTimeout() //nolint:tfctxinit,tfwriteid // false positive - only called to read the default wait handler timeout + createTimeout, diags := model.Timeouts.Create(ctx, waiterTimeout+core.DefaultTimeoutMargin) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + ctx, cancel := context.WithTimeout(ctx, createTimeout) + defer cancel() + projectId := model.ProjectId.ValueString() region := model.Region.ValueString() ctx = tflog.SetField(ctx, "project_id", projectId) @@ -297,11 +311,31 @@ func (r *resourcePoolResource) Create(ctx context.Context, req resource.CreateRe if resp.Diagnostics.HasError() { return } + // The configured timeouts belong in that partial state as well. Without them a failed wait leaves an entry whose + // read and delete fall back to the defaults instead of the values the operator configured. + resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("timeouts"), model.Timeouts)...) + if resp.Diagnostics.HasError() { + return + } response, err := wait.CreateResourcePoolWaitHandler(ctx, r.client.DefaultAPI, projectId, region, *resourcePool.ResourcePool.Id). WaitWithContext(ctx) if err != nil { - core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating resource pool", fmt.Sprintf("resource pool creation waiting: %v", err)) + // The wait handler reports a timeout, a terminal error state and a failing poll through the same error, so + // only mention the create timeout when this context's deadline is what ended the wait. + timeoutHint := "" + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + timeoutHint = fmt.Sprintf(" The wait gave up after the configured `timeouts.create` of %s; raise it if the creation regularly needs longer.", createTimeout) + } + core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating resource pool", fmt.Sprintf( + "resource pool creation waiting: %v\n"+ + "The resource pool was created, and Terraform marks this resource as tainted, so the next apply replaces it. "+ + "Run `terraform untaint` on it first if the next run should refresh the existing pool instead. "+ + "If the state entry is lost, import the resource pool with the ID %q.%s", + err, + utils.BuildInternalTerraformId(projectId, region, *resourcePool.ResourcePool.Id).ValueString(), + timeoutHint, + )) return } ctx = tflog.SetField(ctx, "resource_pool_id", response.ResourcePool.Id) @@ -343,6 +377,14 @@ func (r *resourcePoolResource) Read(ctx context.Context, req resource.ReadReques if resp.Diagnostics.HasError() { return } + readTimeout, diags := model.Timeouts.Read(ctx, core.DefaultOperationTimeout) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + ctx, cancel := context.WithTimeout(ctx, readTimeout) + defer cancel() + projectId := model.ProjectId.ValueString() resourcePoolId := model.ResourcePoolId.ValueString() if resourcePoolId == "" { @@ -395,6 +437,15 @@ func (r *resourcePoolResource) Update(ctx context.Context, req resource.UpdateRe if resp.Diagnostics.HasError() { return } + waiterTimeout := wait.UpdateResourcePoolWaitHandler(ctx, r.client.DefaultAPI, "", "", "").GetTimeout() //nolint:tfctxinit // false positive - only called to read the default wait handler timeout + updateTimeout, diags := model.Timeouts.Update(ctx, waiterTimeout+core.DefaultTimeoutMargin) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + ctx, cancel := context.WithTimeout(ctx, updateTimeout) + defer cancel() + projectId := model.ProjectId.ValueString() resourcePoolId := model.ResourcePoolId.ValueString() region := model.Region.ValueString() @@ -471,6 +522,15 @@ func (r *resourcePoolResource) Delete(ctx context.Context, req resource.DeleteRe return } + waiterTimeout := wait.DeleteResourcePoolWaitHandler(ctx, r.client.DefaultAPI, "", "", "").GetTimeout() //nolint:tfctxinit // false positive - only called to read the default wait handler timeout + deleteTimeout, diags := model.Timeouts.Delete(ctx, waiterTimeout+core.DefaultTimeoutMargin) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + ctx, cancel := context.WithTimeout(ctx, deleteTimeout) + defer cancel() + projectId := model.ProjectId.ValueString() resourcePoolId := model.ResourcePoolId.ValueString() region := model.Region.ValueString() diff --git a/stackit/internal/services/sfs/sfs_test.go b/stackit/internal/services/sfs/sfs_test.go index 5567f9c54..8b5a0a635 100644 --- a/stackit/internal/services/sfs/sfs_test.go +++ b/stackit/internal/services/sfs/sfs_test.go @@ -1,13 +1,19 @@ package sfs import ( + "context" + "encoding/json" "fmt" "net/http" + "net/http/httptest" "regexp" + "sync" "testing" + "time" "github.com/google/uuid" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + corewait "github.com/stackitcloud/stackit-sdk-go/core/wait" sfs "github.com/stackitcloud/stackit-sdk-go/services/sfs/v1api" "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/testutil" @@ -162,3 +168,231 @@ resource "stackit_sfs_share" "example" { }, }) } + +// TestSfsResourcePoolCreateTimeoutIsConfigurable asserts that the configured `timeouts.create` value is what ends +// the create wait. The wait handler applies its own hardcoded 10 minutes otherwise, which no configuration reaches. +// +// Three signals are needed, because the provider reports every wait failure through the same message: the error must +// name the configured value (only the deadline branch does that), the pool must have been polled more than once, and +// the polling window must be close to the configured value rather than to zero or to the mock's poll budget. +func TestSfsResourcePoolCreateTimeoutIsConfigurable(t *testing.T) { + projectId := uuid.NewString() + resourcePoolId := uuid.NewString() + const ( + region = "eu01" + // Longer than the wait handler's 5s throttle, so several polls happen before the deadline ends the wait. + createTimeout = 12 * time.Second + // Safety valve. Without a context deadline in Create the wait would run for the handler's own 10 minutes and + // blow the package test timeout before any assertion could report the regression. + pollBudget = 30 * time.Second + poolState = "creating" + ) + + var ( + mu sync.Mutex + createdAt time.Time + lastPollAt time.Time + polls int + deleted bool + ) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + mu.Lock() + defer mu.Unlock() + switch req.Method { + case http.MethodPost: + createdAt = time.Now() + writeJSON(t, w, sfs.CreateResourcePoolResponse{ + ResourcePool: &sfs.ResourcePool{Id: new(resourcePoolId)}, + }) + case http.MethodDelete: + deleted = true + w.WriteHeader(http.StatusAccepted) + default: + // Report the pool as gone once the test cleanup has deleted it, so the delete wait can finish. + if deleted { + w.WriteHeader(http.StatusNotFound) + return + } + polls++ + lastPollAt = time.Now() + if time.Since(createdAt) > pollBudget { + w.WriteHeader(http.StatusInternalServerError) + return + } + // The pool never becomes ready, so only a timeout can end the wait. + writeJSON(t, w, sfs.GetResourcePoolResponse{ + ResourcePool: &sfs.ResourcePool{Id: new(resourcePoolId), State: new(poolState)}, + }) + } + })) + defer server.Close() + + tfConfig := fmt.Sprintf(` +provider "stackit" { + default_region = "%s" + sfs_custom_endpoint = "%s" + service_account_token = "mock-server-needs-no-auth" + enable_beta_resources = true +} +resource "stackit_sfs_resource_pool" "resourcepool" { + project_id = "%s" + name = "sfs-instance" + availability_zone = "eu01-m" + performance_class = "Standard" + size_gigabytes = 512 + ip_acl = ["192.168.2.0/24"] + + timeouts = { + create = "%s" + } +} +`, region, server.URL, projectId, createTimeout) + + resource.UnitTest(t, resource.TestCase{ + ProtoV6ProviderFactories: testutil.TestAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + Config: tfConfig, + // Only the deadline branch of the error names the configured value. + ExpectError: regexp.MustCompile(regexp.QuoteMeta(createTimeout.String())), + }, + }, + }) + + mu.Lock() + defer mu.Unlock() + if polls < 2 { + t.Errorf("the create wait polled %d times, expected it to keep polling until the configured timeout of %s", + polls, createTimeout) + } + waited := lastPollAt.Sub(createdAt) + if waited < createTimeout/2 { + t.Errorf("the create wait ran for %s, expected roughly the configured %s: it failed for some other reason "+ + "before the timeout was reached", waited, createTimeout) + } + if waited > pollBudget-5*time.Second { + t.Errorf("the create wait ran for %s, expected it to give up after the configured %s: the configured value "+ + "is ignored and the wait ran into the mock's poll budget", waited, createTimeout) + } +} + +func writeJSON(t *testing.T, w http.ResponseWriter, body any) { + t.Helper() + w.Header().Set("content-type", "application/json") + if err := json.NewEncoder(w).Encode(body); err != nil { + t.Errorf("Error writing response body: %v", err) + } +} + +// TestSfsResourcePoolKeepsConfiguredTimeoutsOnError asserts that the configured timeouts are part of the partial +// state the resource writes before it starts waiting. They are needed there: after a failed create Terraform marks +// the resource tainted and the next run refreshes it and destroys it, and both operations read their timeout from +// that state entry. Were the attribute missing, those steps would silently fall back to the default timeouts. +func TestSfsResourcePoolKeepsConfiguredTimeoutsOnError(t *testing.T) { + projectId := uuid.NewString() + resourcePoolId := uuid.NewString() + const ( + region = "eu01" + deleteTimeout = "42m" + ) + + s := testutil.NewMockServer(t) + defer s.Server.Close() + tfConfig := fmt.Sprintf(` +provider "stackit" { + default_region = "%s" + sfs_custom_endpoint = "%s" + service_account_token = "mock-server-needs-no-auth" + enable_beta_resources = true +} +resource "stackit_sfs_resource_pool" "resourcepool" { + project_id = "%s" + name = "sfs-instance" + availability_zone = "eu01-m" + performance_class = "Standard" + size_gigabytes = 512 + ip_acl = ["192.168.2.0/24"] + + timeouts = { + delete = "%s" + } +} +`, region, s.Server.URL, projectId, deleteTimeout) + + resource.UnitTest(t, resource.TestCase{ + ProtoV6ProviderFactories: testutil.TestAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + PreConfig: func() { + s.Reset( + testutil.MockResponse{ + Description: "create resource pool", + ToJsonBody: sfs.CreateResourcePoolResponse{ + ResourcePool: &sfs.ResourcePool{Id: new(resourcePoolId)}, + }, + }, + testutil.MockResponse{ + Description: "failing waiter", + StatusCode: http.StatusInternalServerError, + }, + ) + }, + Config: tfConfig, + ExpectError: regexp.MustCompile("Error creating resource pool"), + }, + { + PreConfig: func() { + pool := testutil.MockResponse{ + Description: "read resource pool", + ToJsonBody: sfs.GetResourcePoolResponse{ + ResourcePool: &sfs.ResourcePool{Id: new(resourcePoolId)}, + }, + } + // The step refreshes and then plans, and both read the resource. + s.Reset( + pool, + pool, + testutil.MockResponse{Description: "delete", StatusCode: http.StatusAccepted}, + testutil.MockResponse{Description: "delete waiter", StatusCode: http.StatusNotFound}, + ) + }, + RefreshState: true, + // The failed create left the resource tainted, so the follow-up plan is a replacement. + ExpectNonEmptyPlan: true, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("stackit_sfs_resource_pool.resourcepool", "resource_pool_id", resourcePoolId), + resource.TestCheckResourceAttr("stackit_sfs_resource_pool.resourcepool", "timeouts.delete", deleteTimeout), + ), + }, + }, + }) +} + +// TestWaitHandlerTimeoutIsBoundedByContext pins the SDK behavior that makes the `timeouts` +// attribute effective at all: the wait handler applies its own timeout only when the passed +// context carries no deadline. Should a future SDK version enforce the handler timeout +// unconditionally, a `timeouts.create` above the handler default would silently be capped +// again, which is the bug the attribute was added for. +func TestWaitHandlerTimeoutIsBoundedByContext(t *testing.T) { + const ( + handlerTimeout = 50 * time.Millisecond + contextTimeout = time.Second + ) + + // The check never finishes, so only one of the two timeouts can end the wait. + handler := corewait.New(func() (bool, *struct{}, error) { return false, nil, nil }) + handler.SetTimeout(handlerTimeout).SetThrottle(10 * time.Millisecond) + + ctx, cancel := context.WithTimeout(context.Background(), contextTimeout) + defer cancel() + + start := time.Now() + if _, err := handler.WaitWithContext(ctx); err == nil { + t.Fatal("expected the wait to time out") + } + if elapsed := time.Since(start); elapsed < contextTimeout { + t.Errorf("wait gave up after %s, expected it to run until the context deadline at %s: "+ + "the SDK wait handler enforces its own timeout despite the context deadline, so the "+ + "`timeouts` attribute can no longer raise it", elapsed, contextTimeout) + } +} diff --git a/stackit/internal/services/sfs/testdata/resource-pool-max.tf b/stackit/internal/services/sfs/testdata/resource-pool-max.tf index 149ebc251..531768f2b 100644 --- a/stackit/internal/services/sfs/testdata/resource-pool-max.tf +++ b/stackit/internal/services/sfs/testdata/resource-pool-max.tf @@ -25,4 +25,11 @@ resource "stackit_sfs_resource_pool" "resourcepool" { snapshot_policy = { id = var.snapshot_policy_id } + + timeouts = { + create = "20m" + read = "20m" + update = "20m" + delete = "20m" + } }