fix: idempotent lifecycle ops

[DominikPildner]

Description

Issue: #1338

This PR brings resource lifecycle operations in line with Terraform plugin framework best practices (see Read and Delete guidelines), to fix handling of missing resources:

1. Read with missing ID: When Read is called before Create (e.g. by tools that observe existing infrastructure), resource IDs that are server-assigned will be empty. Previously this caused API errors from requests with empty IDs. Now the resource is gracefully removed from state instead.

2. Delete of already-deleted resource: When a resource has already been deleted outside of Terraform, Delete now handles the not-found response gracefully instead of returning an error.

Checklist

• Issue was linked above
• Code format was applied: make fmt
• Examples were added / adjusted (see examples/ directory)
• Docs are up-to-date: make generate-docs (will be checked by CI)
• Unit tests got implemented or updated
• Acceptance tests got implemented or updated (see e.g. here)
• Unit tests are passing: make test (will be checked by CI)
• No linter issues: make lint (will be checked by CI)

[github-actions]

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

[DominikPildner]

Hello, is there anything I can do to help move this PR forward? Such as creating a Issue to link to it?

[marceljk]

Hello,
I created a ticket in our internal backlog to review your PR soon.

[karawedi]

Hi @DominikPildner! We see the same behavior with SKE. Does this PR fix this aswell implicitly? I cannot see any SKE-Code touched.

[DominikPildner]

@karawedi good point, for the 404 Code check I only covered resources that I used in my tests. I now tried to find all similar places and applied the same fix. This depends on the API actually returning 404 though, which I did not test for the new changes.

[github-actions]

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

[DominikPildner]

waiting for review

[cgoetz-inovex]

Hi @DominikPildner,

don't know how to make a suggestion here to an unchanged file.
Could you please add this patch, so that our contribution example matches your changes?

Index: .github/docs/contribution-guide/resource.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/docs/contribution-guide/resource.go b/.github/docs/contribution-guide/resource.go
--- a/.github/docs/contribution-guide/resource.go	(revision 937cb6eaae39c0a32dc51099681e5997ab333cbd)
+++ b/.github/docs/contribution-guide/resource.go	(date 1776259148171)
@@ -2,7 +2,9 @@
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 
 	"github.com/hashicorp/terraform-plugin-framework/resource"
@@ -12,10 +14,12 @@
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-log/tflog"
+	"github.com/stackitcloud/stackit-sdk-go/core/oapierror"
 	"github.com/stackitcloud/terraform-provider-stackit/stackit/internal/conversion"
 	"github.com/stackitcloud/terraform-provider-stackit/stackit/internal/core"
 	fooUtils "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/services/foo/utils"
 	"github.com/stackitcloud/terraform-provider-stackit/stackit/internal/utils"
 
 	"github.com/stackitcloud/stackit-sdk-go/services/foo"      // Import service "foo" from the STACKIT SDK for Go
 	"github.com/stackitcloud/stackit-sdk-go/services/foo/wait" // Import service "foo" waiters from the STACKIT SDK for Go (in case the service API has asynchronous endpoints)
@@ -255,12 +259,21 @@
 	projectId := model.ProjectId.ValueString()
 	region := r.providerData.GetRegionWithOverride(model.Region)
 	barId := model.BarId.ValueString()
+	if barId == "" {
+		// Resource not yet created; ID is unknown.
+		resp.State.RemoveResource(ctx)
+		return
+	}
 	ctx = tflog.SetField(ctx, "project_id", projectId)
 	ctx = tflog.SetField(ctx, "region", region)
 	ctx = tflog.SetField(ctx, "bar_id", barId)
 
 	barResp, err := r.client.GetBar(ctx, projectId, region, barId).Execute()
 	if err != nil {
+		if oapiErr, ok := errors.AsType[*oapierror.GenericOpenAPIError](err); ok && oapiErr.StatusCode == http.StatusNotFound {
+			resp.State.RemoveResource(ctx)
+			return
+		}
 		core.LogAndAddError(ctx, &resp.Diagnostics, "Error reading bar", fmt.Sprintf("Calling API: %v", err))
 		return
 	}
@@ -308,6 +321,10 @@
 	// Delete existing bar
 	_, err := r.client.DeleteBar(ctx, projectId, region, barId).Execute()
 	if err != nil {
+		if oapiErr, ok := errors.AsType[*oapierror.GenericOpenAPIError](err); ok && oapiErr.StatusCode == http.StatusNotFound {
+			resp.State.RemoveResource(ctx)
+			return
+		}
 		core.LogAndAddError(ctx, &resp.Diagnostics, "Error deleting bar", fmt.Sprintf("Calling API: %v", err))
 	}
 

[cgoetz-inovex]

Thanks for your contribution!
Checked request funcs used without 404 check, if they can return a 404.

Hint for 2nd review:
In the Delete funcs there are inconsistencies regarding calling of RemoveResource(). TF will remove a resource if Delete does not return an error. So this mixture is ok.

[DominikPildner]

Thanks for the feedback, all requested changes were implemented.

[cgoetz-inovex]

Hi @DominikPildner,
the branch currently has some conflicts, could you please either resolve them or give us permission to do so?
See: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork for the 2nd option.

[DominikPildner]

@cgoetz-inovex I have resolved the conflicts. Seems the option to grant maintainers access is not available for organization repos.

[cgoetz-inovex]

@DominikPildner , would it the be ok for you if I copy your commits into a branch on this repo?
I think finding a time window, where the branch is up to date and we able to click the merge button, will be difficult.

[marceljk]

Thanks for your contribution!

[DominikPildner]

All requested changes implemented.

404 checks added in Delete methods:

• authorization/customrole/resource.go
• edgecloud/instance/resource.go
• logs/accesstoken/resource.go
• logs/instance/resource.go
• scf/organization/resource.go
• serverbackup/schedule/resource.go and serverbackup/enable/resource.go
• serverupdate/schedule/resource.go and serverupdate/enable/resource.go
• sfs/export-policy/resource.go, sfs/resourcepool/resource.go, sfs/share/resource.go

Other fixes:

• iaas/networkarea/resource.go treat 400 as already-deleted
• loadbalancer/observability-credential/resource.go added empty credentialsRef guard in Read

[marceljk]

Thanks for the update. Looks good to me and can be merged

[cgoetz-inovex]

thanks for you contribution, merged!