diff --git a/stackit/internal/services/objectstorage/bucket/resource.go b/stackit/internal/services/objectstorage/bucket/resource.go index ccf9efa08..f6e4d68fa 100644 --- a/stackit/internal/services/objectstorage/bucket/resource.go +++ b/stackit/internal/services/objectstorage/bucket/resource.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "strings" + "time" "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" @@ -394,13 +395,29 @@ func mapFields(bucketResp *objectstorage.GetBucketResponse, model *Model, region return nil } +const ( + // Two object storage resources created in the same apply enable the project concurrently; + // the API answers the losing call with 409. See enableProject. + enableProjectAttempts = 4 +) + +// Overridden in tests to keep them fast. +var enableProjectRetryDelay = 2 * time.Second + // enableProject enables object storage for the specified project. If the project is already enabled, nothing happens func enableProject(ctx context.Context, model *Model, region string, client objectstorage.DefaultAPI) error { projectId := model.ProjectId.ValueString() - // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate - _, err := client.EnableService(ctx, projectId, region).Execute() - if err != nil { + // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate. + // That holds for sequential calls. Two object storage resources created in the same apply call this concurrently, + // and the API rejects the second one with 409 project.create_conflict ("Two concurrent calls try to create the + // same project"). Retrying is safe: once the competing call has finished, enabling an already enabled project succeeds. + config := utils.RetryConfig{ + Attempts: enableProjectAttempts, + Delay: enableProjectRetryDelay, + RetryStatusCodes: []int{http.StatusConflict}, + } + if _, err := utils.RetryRequest(ctx, client.EnableService(ctx, projectId, region).Execute, config); err != nil { return fmt.Errorf("failed to create object storage project: %w", err) } return nil diff --git a/stackit/internal/services/objectstorage/bucket/resource_test.go b/stackit/internal/services/objectstorage/bucket/resource_test.go index 97625d2ff..530f7f486 100644 --- a/stackit/internal/services/objectstorage/bucket/resource_test.go +++ b/stackit/internal/services/objectstorage/bucket/resource_test.go @@ -5,6 +5,7 @@ import ( _ "embed" "fmt" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/hashicorp/terraform-plugin-framework/types" @@ -122,6 +123,14 @@ func TestMapFields(t *testing.T) { } func TestEnableProject(t *testing.T) { + // enableProject retries, and the mock returns a plain error rather than an + // *oapierror.GenericOpenAPIError - RetryRequest only filters by status code + // when it can type-assert the error, so the failing case uses up every + // attempt. Without shrinking the delay this test would sleep for seconds. + oldDelay := enableProjectRetryDelay + enableProjectRetryDelay = time.Millisecond + defer func() { enableProjectRetryDelay = oldDelay }() + tests := []struct { description string enableFails bool diff --git a/stackit/internal/services/objectstorage/credential/resource.go b/stackit/internal/services/objectstorage/credential/resource.go index cd57d4c9c..7f261fe76 100644 --- a/stackit/internal/services/objectstorage/credential/resource.go +++ b/stackit/internal/services/objectstorage/credential/resource.go @@ -490,13 +490,29 @@ func (r *credentialResource) ImportState(ctx context.Context, req resource.Impor tflog.Info(ctx, "ObjectStorage credential state imported") } +const ( + // Two object storage resources created in the same apply enable the project concurrently; + // the API answers the losing call with 409. See enableProject. + enableProjectAttempts = 4 +) + +// Overridden in tests to keep them fast. +var enableProjectRetryDelay = 2 * time.Second + // enableProject enables object storage for the specified project. If the project is already enabled, nothing happens func enableProject(ctx context.Context, model *Model, region string, client objectstorage.DefaultAPI) error { projectId := model.ProjectId.ValueString() - // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate - _, err := client.EnableService(ctx, projectId, region).Execute() - if err != nil { + // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate. + // That holds for sequential calls. Two object storage resources created in the same apply call this concurrently, + // and the API rejects the second one with 409 project.create_conflict ("Two concurrent calls try to create the + // same project"). Retrying is safe: once the competing call has finished, enabling an already enabled project succeeds. + config := utils.RetryConfig{ + Attempts: enableProjectAttempts, + Delay: enableProjectRetryDelay, + RetryStatusCodes: []int{http.StatusConflict}, + } + if _, err := utils.RetryRequest(ctx, client.EnableService(ctx, projectId, region).Execute, config); err != nil { return fmt.Errorf("failed to create object storage project: %w", err) } return nil diff --git a/stackit/internal/services/objectstorage/credential/resource_test.go b/stackit/internal/services/objectstorage/credential/resource_test.go index 6d55d8f1f..35207feb3 100644 --- a/stackit/internal/services/objectstorage/credential/resource_test.go +++ b/stackit/internal/services/objectstorage/credential/resource_test.go @@ -161,6 +161,14 @@ func TestMapFields(t *testing.T) { } func TestEnableProject(t *testing.T) { + // enableProject retries, and the mock returns a plain error rather than an + // *oapierror.GenericOpenAPIError - RetryRequest only filters by status code + // when it can type-assert the error, so the failing case uses up every + // attempt. Without shrinking the delay this test would sleep for seconds. + oldDelay := enableProjectRetryDelay + enableProjectRetryDelay = time.Millisecond + defer func() { enableProjectRetryDelay = oldDelay }() + const testRegion = "eu01" id := fmt.Sprintf("%s,%s,%s", "pid", testRegion, "cgid,cid") tests := []struct { diff --git a/stackit/internal/services/objectstorage/credentialsgroup/resource.go b/stackit/internal/services/objectstorage/credentialsgroup/resource.go index e0c34f284..6035d6f8c 100644 --- a/stackit/internal/services/objectstorage/credentialsgroup/resource.go +++ b/stackit/internal/services/objectstorage/credentialsgroup/resource.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "strings" + "time" "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/conversion" objectstorageUtils "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/services/objectstorage/utils" @@ -380,13 +381,29 @@ func mapCredentialsGroup(credentialsGroup objectstorage.CredentialsGroup, model return nil } +const ( + // Two object storage resources created in the same apply enable the project concurrently; + // the API answers the losing call with 409. See enableProject. + enableProjectAttempts = 4 +) + +// Overridden in tests to keep them fast. +var enableProjectRetryDelay = 2 * time.Second + // enableProject enables object storage for the specified project. If the project is already enabled, nothing happens func enableProject(ctx context.Context, model *Model, region string, client objectstorage.DefaultAPI) error { projectId := model.ProjectId.ValueString() - // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate - _, err := client.EnableService(ctx, projectId, region).Execute() - if err != nil { + // From the object storage OAS: Creation will also be successful if the project is already enabled, but will not create a duplicate. + // That holds for sequential calls. Two object storage resources created in the same apply call this concurrently, + // and the API rejects the second one with 409 project.create_conflict ("Two concurrent calls try to create the + // same project"). Retrying is safe: once the competing call has finished, enabling an already enabled project succeeds. + config := utils.RetryConfig{ + Attempts: enableProjectAttempts, + Delay: enableProjectRetryDelay, + RetryStatusCodes: []int{http.StatusConflict}, + } + if _, err := utils.RetryRequest(ctx, client.EnableService(ctx, projectId, region).Execute, config); err != nil { return fmt.Errorf("failed to create object storage project: %w", err) } return nil diff --git a/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go b/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go index c044dc54e..2154b8ffb 100644 --- a/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go +++ b/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go @@ -3,7 +3,11 @@ package objectstorage import ( "context" "fmt" + "net/http" "testing" + "time" + + "github.com/stackitcloud/stackit-sdk-go/core/oapierror" "github.com/google/go-cmp/cmp" "github.com/hashicorp/terraform-plugin-framework/types" @@ -131,6 +135,14 @@ func TestMapFields(t *testing.T) { } func TestEnableProject(t *testing.T) { + // enableProject retries, and the mock returns a plain error rather than an + // *oapierror.GenericOpenAPIError - RetryRequest only filters by status code + // when it can type-assert the error, so the failing case uses up every + // attempt. Without shrinking the delay this test would sleep for seconds. + oldDelay := enableProjectRetryDelay + enableProjectRetryDelay = time.Millisecond + defer func() { enableProjectRetryDelay = oldDelay }() + tests := []struct { description string enableFails bool @@ -317,3 +329,70 @@ func TestReadCredentialsGroups(t *testing.T) { }) } } + +// Two object storage resources created in the same apply enable the project concurrently. +// The API answers the losing call with 409 project.create_conflict; enableProject must retry +// instead of failing the apply. +func TestEnableProjectRetriesOnConflict(t *testing.T) { + tests := []struct { + description string + conflicts int + isValid bool + wantAttempts int + }{ + {"succeeds immediately", 0, true, 1}, + {"one conflict, then success", 1, true, 2}, + {"conflicts until the attempts are used up", enableProjectAttempts, false, enableProjectAttempts}, + } + + old := enableProjectRetryDelay + enableProjectRetryDelay = time.Millisecond + defer func() { enableProjectRetryDelay = old }() + + for _, tt := range tests { + t.Run(tt.description, func(t *testing.T) { + attempts := 0 + client := &objectstorage.DefaultAPIServiceMock{ + EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { + attempts++ + if attempts <= tt.conflicts { + return nil, &oapierror.GenericOpenAPIError{StatusCode: http.StatusConflict} + } + return &objectstorage.ProjectStatus{}, nil + }), + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + err := enableProject(ctx, &Model{}, "eu01", client) + if tt.isValid && err != nil { + t.Fatalf("Should not have failed: %v", err) + } + if !tt.isValid && err == nil { + t.Fatal("Should have failed") + } + if attempts != tt.wantAttempts { + t.Fatalf("Expected %d attempts, got %d", tt.wantAttempts, attempts) + } + }) + } +} + +// A non-conflict error must not be retried. +func TestEnableProjectDoesNotRetryOtherErrors(t *testing.T) { + attempts := 0 + client := &objectstorage.DefaultAPIServiceMock{ + EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { + attempts++ + return nil, &oapierror.GenericOpenAPIError{StatusCode: http.StatusForbidden} + }), + } + + if err := enableProject(context.Background(), &Model{}, "eu01", client); err == nil { + t.Fatal("Should have failed") + } + if attempts != 1 { + t.Fatalf("Expected a single attempt, got %d", attempts) + } +}