diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fc36124d1..fe9c02eae 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,6 +9,7 @@ Your contribution is welcome! Thank you for your interest in contributing to the - [Repository structure](#repository-structure) - [Implementing a new resource](#implementing-a-new-resource) - [Resource file structure](#resource-file-structure) + - [Implementing write-only attributes](#implementing-write-only-attributes) - [Implementing a new datasource](#implementing-a-new-datasource) - [Onboarding a new STACKIT service](#onboarding-a-new-stackit-service) - [Implementing IAM Role Bindings](#implementing-iam-role-bindings) @@ -66,6 +67,14 @@ https://github.com/stackitcloud/terraform-provider-stackit/blob/main/.github/doc If the new resource `bar` is the first resource in the TFP using a STACKIT service `foo`, please refer to [Onboarding a new STACKIT service](./CONTRIBUTING.md/#onboarding-a-new-stackit-service). +#### Implementing write-only attributes + +When implementing [write-only attributes](https://developer.hashicorp.com/terraform/language/resources/ephemeral#write-only-arguments) (supported in Terraform 1.11.0 and later), keep in mind that Terraform never populates write-only values in the plan or state models. They are only available in the config model, i.e. they must be read via `req.Config.Get(...)` in the `Create`/`Update` handlers, while all other values are read from the plan model as usual. + +You can find a reference implementation of write-only attributes (including the accompanying `_wo_version` rotation counter pattern) in the VPN connection resource: + +https://github.com/stackitcloud/terraform-provider-stackit/blob/main/stackit/internal/services/vpn/connection/resource.go + ### Implementing a new datasource The process to implement a new datasource is similar to [implementing a new resource](#implementing-a-new-resource). Some differences worth noting are: diff --git a/stackit/internal/services/iaas/volume/resource.go b/stackit/internal/services/iaas/volume/resource.go index 4fe39e8b1..109cf36d0 100644 --- a/stackit/internal/services/iaas/volume/resource.go +++ b/stackit/internal/services/iaas/volume/resource.go @@ -433,6 +433,16 @@ func (r *volumeResource) Create(ctx context.Context, req resource.CreateRequest, return } + // The config model - this has to be used because Terraform doesn't include write-only field values in the + // plan and state models - for security measures. Write-only values should be only kept in the config model + // so that they never end up in the state (or plan). + var configModel Model + diags = req.Config.Get(ctx, &configModel) + resp.Diagnostics.Append(diags...) + if resp.Diagnostics.HasError() { + return + } + ctx = core.InitProviderContext(ctx) projectId := model.ProjectId.ValueString() @@ -450,7 +460,7 @@ func (r *volumeResource) Create(ctx context.Context, req resource.CreateRequest, } // Generate API request body from model - payload, err := toCreatePayload(ctx, &model, source) + payload, err := toCreatePayload(ctx, &model, &configModel, source) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating volume", fmt.Sprintf("Creating API payload: %v", err)) return @@ -753,10 +763,13 @@ func mapFields(ctx context.Context, volumeResp *iaas.Volume, model *Model, regio return nil } -func toCreatePayload(ctx context.Context, model *Model, source *sourceModel) (*iaas.CreateVolumePayload, error) { +func toCreatePayload(ctx context.Context, model, configModel *Model, source *sourceModel) (*iaas.CreateVolumePayload, error) { if model == nil { return nil, fmt.Errorf("nil model") } + if configModel == nil { + return nil, fmt.Errorf("nil config model") + } labels, err := conversion.ToStringInterfaceMap(ctx, model.Labels) if err != nil { @@ -782,12 +795,19 @@ func toCreatePayload(ctx context.Context, model *Model, source *sourceModel) (*i Source: sourcePayload, } - if model.EncryptionParameters != nil { + if model.EncryptionParameters != nil && configModel.EncryptionParameters != nil { + // Terraform keeps the write-only field values in the config model - and they shouldn't leave this config model + // to make sure they don't end up being stored in the state. In the plan model the write-only field values are just + // empty. That's why write-only field values must be read from the config model. Everything else comes from the + // plan model. var keyPayload *string - if !utils.IsUndefined(model.EncryptionParameters.KeyPayloadBase64WriteOnly) { - keyPayload = conversion.StringValueToPointer(model.EncryptionParameters.KeyPayloadBase64WriteOnly) - } else if !utils.IsUndefined(model.EncryptionParameters.KeyPayloadBase64) { + if !utils.IsUndefined(model.EncryptionParameters.KeyPayloadBase64) { + // handle the legacy fallback logic keyPayload = conversion.StringValueToPointer(model.EncryptionParameters.KeyPayloadBase64) + } else if !utils.IsUndefined(configModel.EncryptionParameters.KeyPayloadBase64WriteOnly) && + !utils.IsUndefined(model.EncryptionParameters.KeyPayloadBase64WriteOnlyVersion) { + // the user is using the write-only field + keyPayload = conversion.StringValueToPointer(configModel.EncryptionParameters.KeyPayloadBase64WriteOnly) } payload.EncryptionParameters = &iaas.VolumeEncryptionParameter{ diff --git a/stackit/internal/services/iaas/volume/resource_create_test.go b/stackit/internal/services/iaas/volume/resource_create_test.go new file mode 100644 index 000000000..4d0e929e5 --- /dev/null +++ b/stackit/internal/services/iaas/volume/resource_create_test.go @@ -0,0 +1,135 @@ +package volume_test + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "testing" + + "github.com/google/uuid" + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + + iaas "github.com/stackitcloud/stackit-sdk-go/services/iaas/v2api" + + "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/testutil" +) + +// TestCreateWriteOnlyKeyPayload is a regression test for the bug where the write-only key payload +// was read from the plan model instead of the config model. +// The test asserts that the value configured via key_payload_base64_wo is actually +// sent to the API in the create request. +func TestCreateWriteOnlyKeyPayload(t *testing.T) { + projectId := uuid.NewString() + volumeId := uuid.NewString() + kekKeyId := uuid.NewString() + kekKeyringId := uuid.NewString() + const ( + region = "eu01" + availabilityZone = "eu01-1" + name = "test-volume" + size = 16 + serviceAccount = "test-sa@sa.stackit.cloud" + testKeyPayload = "VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4=" + volumeStatusCreated = "AVAILABLE" + ) + s := testutil.NewMockServer(t) + t.Cleanup(s.Server.Close) + tfConfig := fmt.Sprintf(` +provider "stackit" { + default_region = "%s" + iaas_custom_endpoint = "%s" + service_account_token = "mock-server-needs-no-auth" +} + +resource "stackit_volume" "volume" { + project_id = "%s" + availability_zone = "%s" + name = "%s" + size = %d + encryption_parameters = { + kek_key_id = "%s" + kek_key_version = 1 + kek_keyring_id = "%s" + key_payload_base64_wo = "%s" + key_payload_base64_wo_version = 1 + service_account = "%s" + } +} +`, region, s.Server.URL, projectId, availabilityZone, name, size, kekKeyId, kekKeyringId, testKeyPayload, serviceAccount) + + volumeName := name + volumeSize := int64(size) + volume := iaas.Volume{ + Id: &volumeId, + Status: new(volumeStatusCreated), + AvailabilityZone: availabilityZone, + Name: &volumeName, + Size: &volumeSize, + } + + var capturedKeyPayload *string + createCalled := false + createVolume := testutil.MockResponse{ + Description: "create", + Handler: func(w http.ResponseWriter, req *http.Request) { + expected := fmt.Sprintf("/v2/projects/%s/regions/%s/volumes", projectId, region) + if req.URL.Path != expected { + t.Errorf("expected request to %s, got %s", expected, req.URL.Path) + } + createCalled = true + body, err := io.ReadAll(req.Body) + if err != nil { + t.Errorf("failed to read create request body: %v", err) + w.WriteHeader(http.StatusBadRequest) + return + } + var payload iaas.CreateVolumePayload + if err := json.Unmarshal(body, &payload); err != nil { + t.Errorf("failed to unmarshal create request body: %v", err) + w.WriteHeader(http.StatusBadRequest) + return + } + if payload.EncryptionParameters != nil { + capturedKeyPayload = payload.EncryptionParameters.KeyPayload + } + + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(iaas.Volume{Id: &volumeId}) + }, + } + + resource.UnitTest(t, resource.TestCase{ + ProtoV6ProviderFactories: testutil.TestAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + PreConfig: func() { + s.Reset( + createVolume, + testutil.MockResponse{Description: "create waiter", ToJsonBody: volume}, + testutil.MockResponse{Description: "get", ToJsonBody: volume}, + testutil.MockResponse{Description: "delete", StatusCode: http.StatusAccepted}, + testutil.MockResponse{Description: "delete waiter", StatusCode: http.StatusNotFound}, + ) + }, + Config: tfConfig, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("stackit_volume.volume", "volume_id", volumeId), + resource.TestCheckResourceAttr("stackit_volume.volume", "region", region), + resource.TestCheckNoResourceAttr("stackit_volume.volume", "encryption_parameters.key_payload_base64_wo"), + resource.TestCheckResourceAttr("stackit_volume.volume", "encryption_parameters.key_payload_base64_wo_version", "1"), + ), + }, + }, + }) + + if !createCalled { + t.Fatalf("Expected the create endpoint to be called") + } + if capturedKeyPayload == nil { + t.Fatalf("Expected key payload %q to be sent to the API, but none was sent", testKeyPayload) + } + if *capturedKeyPayload != testKeyPayload { + t.Fatalf("Wrong key payload sent to the API: expected %q, got %q", testKeyPayload, *capturedKeyPayload) + } +} diff --git a/stackit/internal/services/iaas/volume/resource_test.go b/stackit/internal/services/iaas/volume/resource_test.go index 511a2a2fd..74d715fcf 100644 --- a/stackit/internal/services/iaas/volume/resource_test.go +++ b/stackit/internal/services/iaas/volume/resource_test.go @@ -186,32 +186,39 @@ func TestMapFields(t *testing.T) { } func TestToCreatePayload(t *testing.T) { + type args struct { + planModel *Model + configModel *Model + source *sourceModel + } tests := []struct { description string - input *Model - source *sourceModel + args args expected *iaas.CreateVolumePayload isValid bool }{ { description: "no volume encryption", - input: &Model{ - Name: types.StringValue("name"), - AvailabilityZone: types.StringValue("zone"), - Labels: types.MapValueMust(types.StringType, map[string]attr.Value{ - "key": types.StringValue("value"), - }), - Description: types.StringValue("desc"), - PerformanceClass: types.StringValue("class"), - Size: types.Int64Value(1), - Source: types.ObjectValueMust(sourceTypes, map[string]attr.Value{ - "type": types.StringNull(), - "id": types.StringNull(), - }), - }, - source: &sourceModel{ - Type: types.StringValue("volume"), - Id: types.StringValue("id"), + args: args{ + planModel: &Model{ + Name: types.StringValue("name"), + AvailabilityZone: types.StringValue("zone"), + Labels: types.MapValueMust(types.StringType, map[string]attr.Value{ + "key": types.StringValue("value"), + }), + Description: types.StringValue("desc"), + PerformanceClass: types.StringValue("class"), + Size: types.Int64Value(1), + Source: types.ObjectValueMust(sourceTypes, map[string]attr.Value{ + "type": types.StringNull(), + "id": types.StringNull(), + }), + }, + configModel: &Model{}, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), + }, }, expected: &iaas.CreateVolumePayload{ Name: new("name"), @@ -231,19 +238,24 @@ func TestToCreatePayload(t *testing.T) { }, { description: "with volume encryption without key payload", - input: &Model{ - Labels: types.MapNull(types.StringType), - EncryptionParameters: &encryptionParametersModel{ - KekKeyId: types.StringValue("kek-key-id"), - KekKeyVersion: types.Int64Value(int64(1)), - KekKeyringId: types.StringValue("kek-keyring-id"), - KeyPayloadBase64: types.StringNull(), - ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + args: args{ + planModel: &Model{ + Labels: types.MapNull(types.StringType), + EncryptionParameters: &encryptionParametersModel{ + KekKeyId: types.StringValue("kek-key-id"), + KekKeyVersion: types.Int64Value(int64(1)), + KekKeyringId: types.StringValue("kek-keyring-id"), + KeyPayloadBase64: types.StringNull(), + ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + }, + }, + configModel: &Model{ + EncryptionParameters: &encryptionParametersModel{}, + }, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), }, - }, - source: &sourceModel{ - Type: types.StringValue("volume"), - Id: types.StringValue("id"), }, expected: &iaas.CreateVolumePayload{ Source: &iaas.VolumeSource{ @@ -263,20 +275,109 @@ func TestToCreatePayload(t *testing.T) { isValid: true, }, { - description: "with volume encryption including key payload", - input: &Model{ - Labels: types.MapNull(types.StringType), - EncryptionParameters: &encryptionParametersModel{ - KekKeyId: types.StringValue("kek-key-id"), - KekKeyVersion: types.Int64Value(int64(1)), - KekKeyringId: types.StringValue("kek-keyring-id"), - KeyPayloadBase64: types.StringValue("VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4="), // The quick brown fox jumps over 13 lazy dogs. - ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + description: "with volume encryption including key payload via legacy field", + args: args{ + planModel: &Model{ + Labels: types.MapNull(types.StringType), + EncryptionParameters: &encryptionParametersModel{ + KekKeyId: types.StringValue("kek-key-id"), + KekKeyVersion: types.Int64Value(int64(1)), + KekKeyringId: types.StringValue("kek-keyring-id"), + KeyPayloadBase64: types.StringValue("VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4="), // The quick brown fox jumps over 13 lazy dogs. + ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + }, + }, + configModel: &Model{ + EncryptionParameters: &encryptionParametersModel{}, + }, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), + }, + }, + expected: &iaas.CreateVolumePayload{ + Source: &iaas.VolumeSource{ + Type: "volume", + Id: "id", + }, + Labels: map[string]any{}, + EncryptionParameters: &iaas.VolumeEncryptionParameter{ + KekKeyId: "kek-key-id", + KekKeyVersion: int64(1), + KekKeyringId: "kek-keyring-id", + KekProjectId: nil, + KeyPayload: new("VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4="), + ServiceAccount: "test-sa@sa.stackit.cloud", + }, + }, + isValid: true, + }, + { + description: "with volume encryption including key payload via legacy field which takes precedence over write-only field", + args: args{ + planModel: &Model{ + Labels: types.MapNull(types.StringType), + EncryptionParameters: &encryptionParametersModel{ + KekKeyId: types.StringValue("kek-key-id"), + KekKeyVersion: types.Int64Value(int64(1)), + KekKeyringId: types.StringValue("kek-keyring-id"), + KeyPayloadBase64: types.StringValue("bGVnYWN5LWtleS1wYXlsb2Fk"), + KeyPayloadBase64WriteOnly: types.StringNull(), + KeyPayloadBase64WriteOnlyVersion: types.Int64Value(1), + ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + }, + }, + configModel: &Model{ + EncryptionParameters: &encryptionParametersModel{ + KeyPayloadBase64WriteOnly: types.StringValue("d3JpdGUtb25seS1rZXktcGF5bG9hZA=="), + }, + }, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), + }, + }, + expected: &iaas.CreateVolumePayload{ + Source: &iaas.VolumeSource{ + Type: "volume", + Id: "id", + }, + Labels: map[string]any{}, + EncryptionParameters: &iaas.VolumeEncryptionParameter{ + KekKeyId: "kek-key-id", + KekKeyVersion: int64(1), + KekKeyringId: "kek-keyring-id", + KekProjectId: nil, + KeyPayload: new("bGVnYWN5LWtleS1wYXlsb2Fk"), + ServiceAccount: "test-sa@sa.stackit.cloud", }, }, - source: &sourceModel{ - Type: types.StringValue("volume"), - Id: types.StringValue("id"), + isValid: true, + }, + { + description: "with volume encryption including key payload via write-only field together with write-only version", + args: args{ + planModel: &Model{ + Labels: types.MapNull(types.StringType), + EncryptionParameters: &encryptionParametersModel{ + KekKeyId: types.StringValue("kek-key-id"), + KekKeyVersion: types.Int64Value(int64(1)), + KekKeyringId: types.StringValue("kek-keyring-id"), + KeyPayloadBase64: types.StringNull(), + KeyPayloadBase64WriteOnly: types.StringNull(), + KeyPayloadBase64WriteOnlyVersion: types.Int64Value(1), + ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + }, + }, + configModel: &Model{ + EncryptionParameters: &encryptionParametersModel{ + KeyPayloadBase64WriteOnly: types.StringValue("VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4="), // The quick brown fox jumps over 13 lazy dogs. + }, + }, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), + }, }, expected: &iaas.CreateVolumePayload{ Source: &iaas.VolumeSource{ @@ -295,10 +396,73 @@ func TestToCreatePayload(t *testing.T) { }, isValid: true, }, + { + description: "with volume encryption including key payload via write-only field but write-only version not set", + args: args{ + planModel: &Model{ + Labels: types.MapNull(types.StringType), + EncryptionParameters: &encryptionParametersModel{ + KekKeyId: types.StringValue("kek-key-id"), + KekKeyVersion: types.Int64Value(int64(1)), + KekKeyringId: types.StringValue("kek-keyring-id"), + KeyPayloadBase64: types.StringNull(), + KeyPayloadBase64WriteOnly: types.StringNull(), + KeyPayloadBase64WriteOnlyVersion: types.Int64Null(), + ServiceAccount: types.StringValue("test-sa@sa.stackit.cloud"), + }, + }, + configModel: &Model{ + EncryptionParameters: &encryptionParametersModel{ + KeyPayloadBase64WriteOnly: types.StringValue("VGhlIHF1aWNrIGJyb3duIGZveCBqdW1wcyBvdmVyIDEzIGxhenkgZG9ncy4="), // The quick brown fox jumps over 13 lazy dogs. + }, + }, + source: &sourceModel{ + Type: types.StringValue("volume"), + Id: types.StringValue("id"), + }, + }, + expected: &iaas.CreateVolumePayload{ + Source: &iaas.VolumeSource{ + Type: "volume", + Id: "id", + }, + Labels: map[string]any{}, + EncryptionParameters: &iaas.VolumeEncryptionParameter{ + KekKeyId: "kek-key-id", + KekKeyVersion: int64(1), + KekKeyringId: "kek-keyring-id", + KekProjectId: nil, + // must be nil because the write-only version is not set + KeyPayload: nil, + ServiceAccount: "test-sa@sa.stackit.cloud", + }, + }, + isValid: true, + }, + { + description: "plan model is nil", + args: args{ + planModel: nil, + configModel: &Model{}, + source: &sourceModel{}, + }, + expected: nil, + isValid: false, + }, + { + description: "config model is nil", + args: args{ + planModel: &Model{}, + configModel: nil, + source: &sourceModel{}, + }, + expected: nil, + isValid: false, + }, } for _, tt := range tests { t.Run(tt.description, func(t *testing.T) { - output, err := toCreatePayload(context.Background(), tt.input, tt.source) + output, err := toCreatePayload(context.Background(), tt.args.planModel, tt.args.configModel, tt.args.source) if !tt.isValid && err == nil { t.Fatalf("Should have failed") }