diff --git a/docs/resources/objectstorage_bucket.md b/docs/resources/objectstorage_bucket.md index d33b52065..d567106d8 100644 --- a/docs/resources/objectstorage_bucket.md +++ b/docs/resources/objectstorage_bucket.md @@ -3,13 +3,13 @@ page_title: "stackit_objectstorage_bucket Resource - stackit" subcategory: "" description: |- - ObjectStorage bucket resource schema. Must have a region specified in the provider configuration. If you are creating credentialsgroup and bucket resources simultaneously, please include the depends_on field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background. + ObjectStorage bucket resource schema. Must have a region specified in the provider configuration. ~> This resource cannot be destroyed if the bucket contains objects. Please ensure the bucket is empty before attempting to destroy it. --- # stackit_objectstorage_bucket (Resource) -ObjectStorage bucket resource schema. Must have a `region` specified in the provider configuration. If you are creating `credentialsgroup` and `bucket` resources simultaneously, please include the `depends_on` field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background. +ObjectStorage bucket resource schema. Must have a `region` specified in the provider configuration. ~> This resource cannot be destroyed if the bucket contains objects. Please ensure the bucket is empty before attempting to destroy it. diff --git a/docs/resources/objectstorage_credentials_group.md b/docs/resources/objectstorage_credentials_group.md index a4c90d8b4..909566e4f 100644 --- a/docs/resources/objectstorage_credentials_group.md +++ b/docs/resources/objectstorage_credentials_group.md @@ -3,12 +3,12 @@ page_title: "stackit_objectstorage_credentials_group Resource - stackit" subcategory: "" description: |- - ObjectStorage credentials group resource schema. Must have a region specified in the provider configuration. If you are creating credentialsgroup and bucket resources simultaneously, please include the depends_on field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background. + ObjectStorage credentials group resource schema. Must have a region specified in the provider configuration. --- # stackit_objectstorage_credentials_group (Resource) -ObjectStorage credentials group resource schema. Must have a `region` specified in the provider configuration. If you are creating `credentialsgroup` and `bucket` resources simultaneously, please include the `depends_on` field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background. +ObjectStorage credentials group resource schema. Must have a `region` specified in the provider configuration. ## Example Usage diff --git a/stackit/internal/services/objectstorage/bucket/resource.go b/stackit/internal/services/objectstorage/bucket/resource.go index ccf9efa08..7bcb1cab5 100644 --- a/stackit/internal/services/objectstorage/bucket/resource.go +++ b/stackit/internal/services/objectstorage/bucket/resource.go @@ -119,7 +119,7 @@ func (r *bucketResource) Configure(ctx context.Context, req resource.ConfigureRe // Schema defines the schema for the resource. func (r *bucketResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { descriptions := map[string]string{ - "main": "ObjectStorage bucket resource schema. Must have a `region` specified in the provider configuration. If you are creating `credentialsgroup` and `bucket` resources simultaneously, please include the `depends_on` field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background.\n\n" + + "main": "ObjectStorage bucket resource schema. Must have a `region` specified in the provider configuration.\n\n" + "~> This resource cannot be destroyed if the bucket contains objects. Please ensure the bucket is empty before attempting to destroy it.", "id": "Terraform's internal resource identifier. It is structured as \"`project_id`,`region`,`name`\".", "name": "The bucket name. It must be DNS conform.", @@ -211,7 +211,7 @@ func (r *bucketResource) Create(ctx context.Context, req resource.CreateRequest, ctx = tflog.SetField(ctx, "region", region) // Handle project init - err := enableProject(ctx, &model, region, r.client.DefaultAPI) + err := objectstorageUtils.EnableProject(ctx, projectId, region, r.client.DefaultAPI) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating bucket", fmt.Sprintf("Enabling object storage project before creation: %v", err)) return @@ -393,15 +393,3 @@ func mapFields(bucketResp *objectstorage.GetBucketResponse, model *Model, region model.ObjectLock = types.BoolValue(bucket.ObjectLockEnabled) return nil } - -// 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 { - 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..2e0d739e1 100644 --- a/stackit/internal/services/objectstorage/bucket/resource_test.go +++ b/stackit/internal/services/objectstorage/bucket/resource_test.go @@ -1,7 +1,6 @@ package objectstorage import ( - "context" _ "embed" "fmt" "testing" @@ -11,22 +10,6 @@ import ( objectstorage "github.com/stackitcloud/stackit-sdk-go/services/objectstorage/v2api" ) -type mockSettings struct { - returnError bool -} - -func newAPIMock(settings *mockSettings) objectstorage.DefaultAPI { - return &objectstorage.DefaultAPIServiceMock{ - EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { - if settings.returnError { - return nil, fmt.Errorf("create project failed") - } - - return &objectstorage.ProjectStatus{}, nil - }), - } -} - func TestMapFields(t *testing.T) { const testRegion = "eu01" id := fmt.Sprintf("%s,%s,%s", "pid", testRegion, "bname") @@ -120,37 +103,3 @@ func TestMapFields(t *testing.T) { }) } } - -func TestEnableProject(t *testing.T) { - tests := []struct { - description string - enableFails bool - isValid bool - }{ - { - "default_values", - false, - true, - }, - { - "error_response", - true, - false, - }, - } - for _, tt := range tests { - t.Run(tt.description, func(t *testing.T) { - client := newAPIMock(&mockSettings{ - returnError: tt.enableFails, - }) - - err := enableProject(context.Background(), &Model{}, "eu01", client) - if !tt.isValid && err == nil { - t.Fatalf("Should have failed") - } - if tt.isValid && err != nil { - t.Fatalf("Should not have failed: %v", err) - } - }) - } -} diff --git a/stackit/internal/services/objectstorage/credential/resource.go b/stackit/internal/services/objectstorage/credential/resource.go index 996e77fdc..6321d5e15 100644 --- a/stackit/internal/services/objectstorage/credential/resource.go +++ b/stackit/internal/services/objectstorage/credential/resource.go @@ -281,7 +281,7 @@ func (r *credentialResource) Create(ctx context.Context, req resource.CreateRequ ctx = tflog.SetField(ctx, "region", region) // Handle project init - err := enableProject(ctx, &model, region, r.client.DefaultAPI) + err := objectstorageUtils.EnableProject(ctx, projectId, region, r.client.DefaultAPI) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating credential", fmt.Sprintf("Enabling object storage project before creation: %v", err)) return @@ -490,18 +490,6 @@ func (r *credentialResource) ImportState(ctx context.Context, req resource.Impor tflog.Info(ctx, "ObjectStorage credential state imported") } -// 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 { - return fmt.Errorf("failed to create object storage project: %w", err) - } - return nil -} - func toCreatePayload(model *Model) (*objectstorage.CreateAccessKeyPayload, error) { if model == nil { return nil, fmt.Errorf("nil model") diff --git a/stackit/internal/services/objectstorage/credential/resource_test.go b/stackit/internal/services/objectstorage/credential/resource_test.go index 6d55d8f1f..7a81e9b32 100644 --- a/stackit/internal/services/objectstorage/credential/resource_test.go +++ b/stackit/internal/services/objectstorage/credential/resource_test.go @@ -15,22 +15,6 @@ import ( objectstorage "github.com/stackitcloud/stackit-sdk-go/services/objectstorage/v2api" ) -type mockSettings struct { - returnError bool -} - -func newAPIMock(settings *mockSettings) objectstorage.DefaultAPI { - return &objectstorage.DefaultAPIServiceMock{ - EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { - if settings.returnError { - return nil, fmt.Errorf("create project failed") - } - - return &objectstorage.ProjectStatus{}, nil - }), - } -} - func TestMapFields(t *testing.T) { now := time.Now() const testRegion = "eu01" @@ -160,71 +144,6 @@ func TestMapFields(t *testing.T) { } } -func TestEnableProject(t *testing.T) { - const testRegion = "eu01" - id := fmt.Sprintf("%s,%s,%s", "pid", testRegion, "cgid,cid") - tests := []struct { - description string - expected Model - enableFails bool - isValid bool - }{ - { - "default_values", - Model{ - Id: types.StringValue(id), - ProjectId: types.StringValue("pid"), - CredentialsGroupId: types.StringValue("cgid"), - CredentialId: types.StringValue("cid"), - Name: types.StringNull(), - AccessKey: types.StringNull(), - SecretAccessKey: types.StringNull(), - ExpirationTimestamp: types.StringNull(), - RotateWhenChanged: types.MapNull(types.StringType), - }, - false, - true, - }, - { - "error_response", - Model{ - Id: types.StringValue(id), - ProjectId: types.StringValue("pid"), - CredentialsGroupId: types.StringValue("cgid"), - CredentialId: types.StringValue("cid"), - Name: types.StringNull(), - AccessKey: types.StringNull(), - SecretAccessKey: types.StringNull(), - ExpirationTimestamp: types.StringNull(), - RotateWhenChanged: types.MapNull(types.StringType), - }, - true, - false, - }, - } - for _, tt := range tests { - t.Run(tt.description, func(t *testing.T) { - client := newAPIMock(&mockSettings{ - returnError: tt.enableFails, - }) - - model := &Model{ - ProjectId: tt.expected.ProjectId, - CredentialsGroupId: tt.expected.CredentialsGroupId, - CredentialId: tt.expected.CredentialId, - RotateWhenChanged: types.MapNull(types.StringType), - } - err := enableProject(context.Background(), model, "eu01", client) - if !tt.isValid && err == nil { - t.Fatalf("Should have failed") - } - if tt.isValid && err != nil { - t.Fatalf("Should not have failed: %v", err) - } - }) - } -} - func TestReadCredentials(t *testing.T) { now := time.Now() const testRegion = "eu01" diff --git a/stackit/internal/services/objectstorage/credentialsgroup/resource.go b/stackit/internal/services/objectstorage/credentialsgroup/resource.go index 3a46711af..6d69d0413 100644 --- a/stackit/internal/services/objectstorage/credentialsgroup/resource.go +++ b/stackit/internal/services/objectstorage/credentialsgroup/resource.go @@ -108,7 +108,7 @@ func (r *credentialsGroupResource) Configure(ctx context.Context, req resource.C // Schema defines the schema for the resource. func (r *credentialsGroupResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) { descriptions := map[string]string{ - "main": "ObjectStorage credentials group resource schema. Must have a `region` specified in the provider configuration. If you are creating `credentialsgroup` and `bucket` resources simultaneously, please include the `depends_on` field so that they are created sequentially. This prevents errors from concurrent calls to the service enablement that is done in the background.", + "main": "ObjectStorage credentials group resource schema. Must have a `region` specified in the provider configuration.", "id": "Terraform's internal data source identifier. It is structured as \"`project_id`,`region`,`credentials_group_id`\".", "credentials_group_id": "The credentials group ID", "name": "The credentials group's display name.", @@ -192,7 +192,7 @@ func (r *credentialsGroupResource) Create(ctx context.Context, req resource.Crea } // Handle project init - err := enableProject(ctx, &model, region, r.client.DefaultAPI) + err := objectstorageUtils.EnableProject(ctx, projectId, region, r.client.DefaultAPI) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating credentials group", fmt.Sprintf("Enabling object storage project before creation: %v", err)) return @@ -380,18 +380,6 @@ func mapCredentialsGroup(credentialsGroup objectstorage.CredentialsGroup, model return nil } -// 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 { - return fmt.Errorf("failed to create object storage project: %w", err) - } - return nil -} - // readCredentialsGroups gets all the existing credentials groups for the specified project, // finds the credentials group that is being read and updates the state. // Returns True if the credential was found, False otherwise. diff --git a/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go b/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go index c044dc54e..11dff8e22 100644 --- a/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go +++ b/stackit/internal/services/objectstorage/credentialsgroup/resource_test.go @@ -17,13 +17,6 @@ type mockSettings struct { func newAPIMock(settings *mockSettings) objectstorage.DefaultAPI { return &objectstorage.DefaultAPIServiceMock{ - EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { - if settings.returnError { - return nil, fmt.Errorf("create project failed") - } - - return &objectstorage.ProjectStatus{}, nil - }), ListCredentialsGroupsExecuteMock: new(func(_ objectstorage.ApiListCredentialsGroupsRequest) (*objectstorage.ListCredentialsGroupsResponse, error) { if settings.returnError { return nil, fmt.Errorf("get credentials groups failed") @@ -130,40 +123,6 @@ func TestMapFields(t *testing.T) { } } -func TestEnableProject(t *testing.T) { - tests := []struct { - description string - enableFails bool - isValid bool - }{ - { - "default_values", - false, - true, - }, - { - "error_response", - true, - false, - }, - } - for _, tt := range tests { - t.Run(tt.description, func(t *testing.T) { - client := newAPIMock(&mockSettings{ - returnError: tt.enableFails, - }) - - err := enableProject(context.Background(), &Model{}, "eu01", client) - if !tt.isValid && err == nil { - t.Fatalf("Should have failed") - } - if tt.isValid && err != nil { - t.Fatalf("Should not have failed: %v", err) - } - }) - } -} - func TestReadCredentialsGroups(t *testing.T) { const testRegion = "eu01" id := fmt.Sprintf("%s,%s,%s", "pid", testRegion, "cid") diff --git a/stackit/internal/services/objectstorage/utils/util.go b/stackit/internal/services/objectstorage/utils/util.go index b107ced32..8e3ab1a8a 100644 --- a/stackit/internal/services/objectstorage/utils/util.go +++ b/stackit/internal/services/objectstorage/utils/util.go @@ -3,6 +3,8 @@ package utils import ( "context" "fmt" + "net/http" + "time" objectstorage "github.com/stackitcloud/stackit-sdk-go/services/objectstorage/v2api" @@ -13,6 +15,26 @@ import ( "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/utils" ) +const ( + enableProjectAttempts = 4 + enableProjectRetryDelay = 2 * time.Second +) + +// EnableProject enables object storage for the specified project. If the project is already enabled, nothing happens. +// Two resources created in the same apply call this concurrently and the API rejects the losing call with +// 409 project.create_conflict; retrying is safe, since enabling an already enabled project succeeds. +func EnableProject(ctx context.Context, projectId, region string, client objectstorage.DefaultAPI) error { + retryConfig := utils.RetryConfig{ + Attempts: enableProjectAttempts, + Delay: enableProjectRetryDelay, + RetryStatusCodes: []int{http.StatusConflict}, + } + if _, err := utils.RetryRequest(ctx, client.EnableService(ctx, projectId, region).Execute, retryConfig); err != nil { + return fmt.Errorf("enable object storage project: %w", err) + } + return nil +} + func ConfigureClient(ctx context.Context, providerData *core.ProviderData, diags *diag.Diagnostics) *objectstorage.APIClient { apiClientConfigOptions := []config.ConfigurationOption{ config.WithCustomAuth(providerData.RoundTripper), diff --git a/stackit/internal/services/objectstorage/utils/util_test.go b/stackit/internal/services/objectstorage/utils/util_test.go index 669241adb..1a174b0dd 100644 --- a/stackit/internal/services/objectstorage/utils/util_test.go +++ b/stackit/internal/services/objectstorage/utils/util_test.go @@ -2,13 +2,18 @@ package utils import ( "context" + "fmt" + "net/http" "os" "reflect" "testing" + "testing/synctest" + "time" "github.com/hashicorp/terraform-plugin-framework/diag" sdkClients "github.com/stackitcloud/stackit-sdk-go/core/clients" "github.com/stackitcloud/stackit-sdk-go/core/config" + "github.com/stackitcloud/stackit-sdk-go/core/oapierror" objectstorage "github.com/stackitcloud/stackit-sdk-go/services/objectstorage/v2api" "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/core" @@ -92,3 +97,108 @@ func TestConfigureClient(t *testing.T) { }) } } + +func TestEnableProject(t *testing.T) { + tests := []struct { + description string + enableFails bool + isValid bool + }{ + { + "default_values", + false, + true, + }, + { + "error_response", + true, + false, + }, + } + for _, tt := range tests { + t.Run(tt.description, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + client := &objectstorage.DefaultAPIServiceMock{ + EnableServiceExecuteMock: new(func(_ objectstorage.ApiEnableServiceRequest) (*objectstorage.ProjectStatus, error) { + if tt.enableFails { + return nil, fmt.Errorf("create project failed") + } + + return &objectstorage.ProjectStatus{}, nil + }), + } + + err := EnableProject(context.Background(), "pid", "eu01", client) + if !tt.isValid && err == nil { + t.Fatalf("Should have failed") + } + if tt.isValid && err != nil { + t.Fatalf("Should not have failed: %v", err) + } + }) + }) + } +} + +// A 409 from a concurrent enable call must be retried 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}, + } + + for _, tt := range tests { + t.Run(tt.description, func(t *testing.T) { + synctest.Test(t, 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, "pid", "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(), "pid", "eu01", client); err == nil { + t.Fatal("Should have failed") + } + if attempts != 1 { + t.Fatalf("Expected a single attempt, got %d", attempts) + } +}