review findings

Author: rubenhoenle

docs/data-sources/kms_keyring.md

@@ -3,12 +3,12 @@
 page_title: "stackit_kms_keyring Data Source - stackit"
 subcategory: ""
 description: |-
-  KMS Keyring resource schema.
+  KMS Keyring datasource schema. Uses the default_region specified in the provider configuration as a fallback in case no region is defined on datasource level.
 ---
 
 # stackit_kms_keyring (Data Source)
 
-KMS Keyring resource schema.
+KMS Keyring datasource schema. Uses the `default_region` specified in the provider configuration as a fallback in case no `region` is defined on datasource level.
 
 ## Example Usage
 
@@ -27,9 +27,12 @@ data "stackit_kms_keyring" "example" {
 - `keyring_id` (String) An auto generated unique id which identifies the keyring.
 - `project_id` (String) STACKIT project ID to which the keyring is associated.
 
+### Optional
+
+- `region` (String) The resource region. If not defined, the provider region is used.
+
 ### Read-Only
 
 - `description` (String) A user chosen description to distinguish multiple keyrings.
 - `display_name` (String) The display name to distinguish multiple keyrings.
 - `id` (String) Terraform's internal resource ID. It is structured as "`project_id`,`region`,`keyring_id`".
-- `region` (String) The resource region. If not defined, the provider region is used.

docs/resources/kms_keyring.md

@@ -3,13 +3,13 @@
 page_title: "stackit_kms_keyring Resource - stackit"
 subcategory: ""
 description: |-
-  KMS Keyring resource schema.
+  KMS Keyring resource schema. Uses the default_region specified in the provider configuration as a fallback in case no region is defined on resource level.
   ~> Keyrings will not be destroyed by terraform during a terraform destroy. They will just be thrown out of the Terraform state and not deleted on API side. This way we can ensure no keyring setups are deleted by accident and it gives you the option to recover your keys within the grace period.
 ---
 
 # stackit_kms_keyring (Resource)
 
-KMS Keyring resource schema.
+KMS Keyring resource schema. Uses the `default_region` specified in the provider configuration as a fallback in case no `region` is defined on resource level.
 
  ~> Keyrings will **not** be destroyed by terraform during a `terraform destroy`. They will just be thrown out of the Terraform state and not deleted on API side. **This way we can ensure no keyring setups are deleted by accident and it gives you the option to recover your keys within the grace period.**
 

stackit/internal/core/core.go

@@ -12,14 +12,17 @@ import (
 	"github.com/hashicorp/terraform-plugin-log/tflog"
 )
 
-// Separator used for concatenation of TF-internal resource ID
-const Separator = ","
-
 type ResourceType string
 
 const (
 	Resource   ResourceType = "resource"
 	Datasource ResourceType = "datasource"
+
+	// Separator used for concatenation of TF-internal resource ID
+	Separator = ","
+
+	ResourceRegionFallbackDocstring   = "Uses the `default_region` specified in the provider configuration as a fallback in case no `region` is defined on resource level."
+	DatasourceRegionFallbackDocstring = "Uses the `default_region` specified in the provider configuration as a fallback in case no `region` is defined on datasource level."
 )
 
 type ProviderData struct {

stackit/internal/services/kms/keyring/datasource.go

@@ -47,12 +47,12 @@ func (k *keyRingDataSource) Configure(ctx context.Context, request datasource.Co
 	}
 
 	k.client = apiClient
-	tflog.Info(ctx, "Keyring configured")
+	tflog.Info(ctx, "KMS client configured")
 }
 
 func (k *keyRingDataSource) Schema(_ context.Context, _ datasource.SchemaRequest, response *datasource.SchemaResponse) {
 	response.Schema = schema.Schema{
-		Description: "KMS Keyring resource schema.",
+		Description: fmt.Sprintf("KMS Keyring datasource schema. %s", core.DatasourceRegionFallbackDocstring),
 		Attributes: map[string]schema.Attribute{
 			"description": schema.StringAttribute{
 				Description: "A user chosen description to distinguish multiple keyrings.",
@@ -64,7 +64,6 @@ func (k *keyRingDataSource) Schema(_ context.Context, _ datasource.SchemaRequest
 			},
 			"keyring_id": schema.StringAttribute{
 				Description: "An auto generated unique id which identifies the keyring.",
-				Computed:    false,
 				Required:    true,
 				Validators: []validator.String{
 					validate.UUID(),
@@ -84,6 +83,8 @@ func (k *keyRingDataSource) Schema(_ context.Context, _ datasource.SchemaRequest
 				},
 			},
 			"region": schema.StringAttribute{
+				Optional: true,
+				// must be computed to allow for storing the override value from the provider
 				Computed:    true,
 				Description: "The resource region. If not defined, the provider region is used.",
 			},

stackit/internal/services/kms/keyring/resource.go

@@ -2,6 +2,7 @@ package kms
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -65,11 +66,12 @@ func (r *keyRingResource) Configure(ctx context.Context, request resource.Config
 		return
 	}
 
-	apiClient := kmsUtils.ConfigureClient(ctx, &r.providerData, &response.Diagnostics)
+	r.client = kmsUtils.ConfigureClient(ctx, &r.providerData, &response.Diagnostics)
 	if response.Diagnostics.HasError() {
 		return
 	}
-	r.client = apiClient
+
+	tflog.Info(ctx, "KMS client configured")
 }
 
 // ModifyPlan implements resource.ResourceWithModifyPlan.
@@ -103,7 +105,7 @@ func (r *keyRingResource) ModifyPlan(ctx context.Context, req resource.ModifyPla
 }
 
 func (r *keyRingResource) Schema(_ context.Context, _ resource.SchemaRequest, response *resource.SchemaResponse) {
-	description := "KMS Keyring resource schema."
+	description := fmt.Sprintf("KMS Keyring resource schema. %s", core.ResourceRegionFallbackDocstring)
 
 	response.Schema = schema.Schema{
 		Description:         description,
@@ -193,6 +195,11 @@ func (r *keyRingResource) Create(ctx context.Context, req resource.CreateRequest
 		return
 	}
 
+	if createResponse == nil || createResponse.Id == nil {
+		core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating keyring", fmt.Sprintf("API returned empty response"))
+		return
+	}
+
 	keyRingId := *createResponse.Id
 	// Write id attributes to state before polling via the wait handler - just in case anything goes wrong during the wait handler
 	utils.SetAndLogStateFields(ctx, &resp.Diagnostics, &resp.State, map[string]any{
@@ -239,7 +246,8 @@ func (r *keyRingResource) Read(ctx context.Context, req resource.ReadRequest, re
 
 	keyRingResponse, err := r.client.GetKeyRing(ctx, projectId, region, keyRingId).Execute()
 	if err != nil {
-		oapiErr, ok := err.(*oapierror.GenericOpenAPIError) //nolint:errorlint //complaining that error.As should be used to catch wrapped errors, but this error should not be wrapped
+		var oapiErr *oapierror.GenericOpenAPIError
+		ok := errors.As(err, &oapiErr)
 		if ok && oapiErr.StatusCode == http.StatusNotFound {
 			resp.State.RemoveResource(ctx)
 			return

stackit/internal/services/kms/kms_acc_test.go

@@ -1,11 +1,22 @@
 package kms_test
 
 import (
+	"context"
 	_ "embed"
+	"errors"
 	"fmt"
 	"maps"
+	"net/http"
+	"strings"
+	"sync"
 	"testing"
 
+	"github.com/hashicorp/terraform-plugin-testing/plancheck"
+	coreConfig "github.com/stackitcloud/stackit-sdk-go/core/config"
+	"github.com/stackitcloud/stackit-sdk-go/core/oapierror"
+	"github.com/stackitcloud/stackit-sdk-go/services/kms"
+	"github.com/stackitcloud/terraform-provider-stackit/stackit/internal/core"
+
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
@@ -50,11 +61,17 @@ var testConfigKeyRingVarsMaxUpdated = func() config.Variables {
 func TestAccKeyRingMin(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: testutil.TestAccProtoV6ProviderFactories,
+		CheckDestroy:             testAccCheckDestroy,
 		Steps: []resource.TestStep{
 			// Creation
 			{
 				ConfigVariables: testConfigKeyRingVarsMin,
 				Config:          fmt.Sprintf("%s\n%s", testutil.KMSProviderConfig(), resourceKeyRingMinConfig),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionCreate),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "region", testutil.Region),
@@ -77,6 +94,11 @@ func TestAccKeyRingMin(t *testing.T) {
 					`,
 					testutil.KMSProviderConfig(), resourceKeyRingMinConfig,
 				),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionNoop),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr("data.stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
 					resource.TestCheckResourceAttr("data.stackit_kms_keyring.keyring", "region", testutil.Region),
@@ -111,6 +133,11 @@ func TestAccKeyRingMin(t *testing.T) {
 			{
 				ConfigVariables: testConfigKeyRingVarsMinUpdated(),
 				Config:          fmt.Sprintf("%s\n%s", testutil.KMSProviderConfig(), resourceKeyRingMinConfig),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionReplace),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "region", testutil.Region),
@@ -127,11 +154,17 @@ func TestAccKeyRingMin(t *testing.T) {
 func TestAccKeyRingMax(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: testutil.TestAccProtoV6ProviderFactories,
+		CheckDestroy:             testAccCheckDestroy,
 		Steps: []resource.TestStep{
 			// Creation
 			{
 				ConfigVariables: testConfigKeyRingVarsMax,
 				Config:          fmt.Sprintf("%s\n%s", testutil.KMSProviderConfig(), resourceKeyRingMaxConfig),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionCreate),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "region", testutil.Region),
@@ -154,6 +187,11 @@ func TestAccKeyRingMax(t *testing.T) {
 					`,
 					testutil.KMSProviderConfig(), resourceKeyRingMaxConfig,
 				),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionNoop),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.ComposeAggregateTestCheckFunc(
 						resource.TestCheckResourceAttr("data.stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
@@ -190,6 +228,11 @@ func TestAccKeyRingMax(t *testing.T) {
 			{
 				ConfigVariables: testConfigKeyRingVarsMaxUpdated(),
 				Config:          fmt.Sprintf("%s\n%s", testutil.KMSProviderConfig(), resourceKeyRingMaxConfig),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						plancheck.ExpectResourceAction("stackit_kms_keyring.keyring", plancheck.ResourceActionReplace),
+					},
+				},
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "project_id", testutil.ProjectId),
 					resource.TestCheckResourceAttr("stackit_kms_keyring.keyring", "region", testutil.Region),
@@ -202,3 +245,71 @@ func TestAccKeyRingMax(t *testing.T) {
 		},
 	})
 }
+
+func testAccCheckDestroy(s *terraform.State) error {
+	checkFunctions := []func(s *terraform.State) error{
+		testAccCheckKeyRingDestroy,
+	}
+
+	var errs []error
+
+	wg := sync.WaitGroup{}
+	wg.Add(len(checkFunctions))
+
+	for _, f := range checkFunctions {
+		go func() {
+			err := f(s)
+			if err != nil {
+				errs = append(errs, err)
+			}
+			wg.Done()
+		}()
+	}
+	wg.Wait()
+	return errors.Join(errs...)
+}
+
+func testAccCheckKeyRingDestroy(s *terraform.State) error {
+	ctx := context.Background()
+	var client *kms.APIClient
+	var err error
+	if testutil.KMSCustomEndpoint == "" {
+		client, err = kms.NewAPIClient()
+	} else {
+		client, err = kms.NewAPIClient(
+			coreConfig.WithEndpoint(testutil.KMSCustomEndpoint),
+		)
+	}
+	if err != nil {
+		return fmt.Errorf("creating client: %w", err)
+	}
+
+	var errs []error
+
+	for _, rs := range s.RootModule().Resources {
+		if rs.Type != "stackit_kms_keyring" {
+			continue
+		}
+		keyRingId := strings.Split(rs.Primary.ID, core.Separator)[2]
+		err := client.DeleteKeyRingExecute(ctx, testutil.ProjectId, testutil.Region, keyRingId)
+		if err != nil {
+			var oapiErr *oapierror.GenericOpenAPIError
+			if errors.As(err, &oapiErr) {
+				if oapiErr.StatusCode == http.StatusNotFound {
+					continue
+				}
+
+				// Workaround: when the delete endpoint is called for a keyring which has keys inside it (no matter if
+				// they are scheduled for deletion or not, it will throw an HTTP 400 error and the keyring can't be
+				// deleted then).
+				// But at least we can delete all empty keyrings created by the keyring acc tests this way.
+				if oapiErr.StatusCode == http.StatusBadRequest {
+					continue
+				}
+			}
+			errs = append(errs, fmt.Errorf("cannot trigger keyring deletion %q: %w", keyRingId, err))
+		}
+	}
+
+	return errors.Join(errs...)
+}