Merge pull request #165 from jfrog/GH-141-fix-permission-state-drift

Fix permission resource not being deleted correctly

CHANGELOG.md

@@ -1,10 +1,16 @@
+# 1.18.2 (November 27, 2024)
+
+BUG FIXES:
+
+* resource/platform_permission: Fix permission resource not being deleted correctly. Permission resources that were in the Terraform state from previously apply but now are removed from the plan were not deleted. Now we check for the removal and delete the permission resource correctly. Issue: [#141](https://github.com/jfrog/terraform-provider-platform/issues/141) PR: [#165](https://github.com/jfrog/terraform-provider-platform/pull/165)
+
 # 1.18.1 (November 26, 2024). Tested on Artifactory 7.98.9 with Terraform 1.9.8 and OpenTofu 1.8.6
 
 BUG FIXES:
 
 * resource/platform_oidc_configuration: Update validation for `issuer` attribute to support GitHub actions customization for enterprise. See [Customizing the issuer value for an enterprise](https://docs.github.com/en/enterprise-cloud@latest/actions/security-for-github-actions/security-hardening-your-deployments/about-security-hardening-with-openid-connect#customizing-the-issuer-value-for-an-enterprise). PR: [#163](https://github.com/jfrog/terraform-provider-platform/pull/163) and [#164](https://github.com/jfrog/terraform-provider-platform/pull/164)
 
-## 1.18.0 (November 21, 2024). Tested on Artifactory 7.98.8 with Terraform 1.9.8 and OpenTofu 1.8.5
+## 1.18.0 (November 21, 2024). Tested on Artifactory 7.98.9 with Terraform 1.9.8 and OpenTofu 1.8.6
 
 IMPROVEMENTS:
 

go.mod

@@ -7,7 +7,7 @@ go 1.22.7
 
 require (
 	github.com/go-resty/resty/v2 v2.16.2
-	github.com/hashicorp/terraform-plugin-docs v0.20.0
+	github.com/hashicorp/terraform-plugin-docs v0.20.1
 	github.com/hashicorp/terraform-plugin-framework v1.13.0
 	github.com/hashicorp/terraform-plugin-framework-validators v0.15.0
 	github.com/hashicorp/terraform-plugin-go v0.25.0

go.sum

@@ -103,8 +103,8 @@ github.com/hashicorp/terraform-exec v0.21.0 h1:uNkLAe95ey5Uux6KJdua6+cv8asgILFVW
 github.com/hashicorp/terraform-exec v0.21.0/go.mod h1:1PPeMYou+KDUSSeRE9szMZ/oHf4fYUmB923Wzbq1ICg=
 github.com/hashicorp/terraform-json v0.23.0 h1:sniCkExU4iKtTADReHzACkk8fnpQXrdD2xoR+lppBkI=
 github.com/hashicorp/terraform-json v0.23.0/go.mod h1:MHdXbBAbSg0GvzuWazEGKAn/cyNfIB7mN6y7KJN6y2c=
-github.com/hashicorp/terraform-plugin-docs v0.20.0 h1:ox7rm1FN0dVZaJBUzkVVh10R1r3+FeMQWL0QopQ9d7o=
-github.com/hashicorp/terraform-plugin-docs v0.20.0/go.mod h1:A/+4SVMdAkQYtIBtaxV0H7AU862TxVZk/hhKaMDQB6Y=
+github.com/hashicorp/terraform-plugin-docs v0.20.1 h1:Fq7E/HrU8kuZu3hNliZGwloFWSYfWEOWnylFhYQIoys=
+github.com/hashicorp/terraform-plugin-docs v0.20.1/go.mod h1:Yz6HoK7/EgzSrHPB9J/lWFzwl9/xep2OPnc5jaJDV90=
 github.com/hashicorp/terraform-plugin-framework v1.13.0 h1:8OTG4+oZUfKgnfTdPTJwZ532Bh2BobF4H+yBiYJ/scw=
 github.com/hashicorp/terraform-plugin-framework v1.13.0/go.mod h1:j64rwMGpgM3NYXTKuxrCnyubQb/4VKldEKlcG8cvmjU=
 github.com/hashicorp/terraform-plugin-framework-validators v0.15.0 h1:RXMmu7JgpFjnI1a5QjMCBb11usrW2OtAG+iOTIj5c9Y=

pkg/platform/resource_permission.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 
+	"github.com/go-resty/resty/v2"
 	"github.com/hashicorp/terraform-plugin-framework-validators/resourcevalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/setvalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
@@ -820,16 +821,18 @@ func (r *permissionResource) Create(ctx context.Context, req resource.CreateRequ
 		return
 	}
 
+	var jfrogErrors util.JFrogErrors
 	response, err := r.ProviderData.Client.R().
 		SetBody(&permission).
+		SetError(&jfrogErrors).
 		Post(PermissionEndpoint)
 	if err != nil {
 		utilfw.UnableToCreateResourceError(resp, err.Error())
 		return
 	}
 
 	if response.IsError() {
-		utilfw.UnableToCreateResourceError(resp, response.String())
+		utilfw.UnableToCreateResourceError(resp, jfrogErrors.String())
 		return
 	}
 
@@ -847,10 +850,12 @@ func (r *permissionResource) Read(ctx context.Context, req resource.ReadRequest,
 	}
 
 	var permission PermissionAPIModel
+	var jfrogErrors util.JFrogErrors
 
 	response, err := r.ProviderData.Client.R().
 		SetPathParam("name", state.Name.ValueString()).
 		SetResult(&permission).
+		SetError(&jfrogErrors).
 		Get(PermissionEndpoint + "/{name}")
 
 	if err != nil {
@@ -866,7 +871,7 @@ func (r *permissionResource) Read(ctx context.Context, req resource.ReadRequest,
 	}
 
 	if response.IsError() {
-		utilfw.UnableToRefreshResourceError(resp, response.String())
+		utilfw.UnableToRefreshResourceError(resp, jfrogErrors.String())
 		return
 	}
 
@@ -884,39 +889,85 @@ func (r *permissionResource) Update(ctx context.Context, req resource.UpdateRequ
 	go util.SendUsageResourceUpdate(ctx, r.ProviderData.Client.R(), r.ProviderData.ProductId, r.TypeName)
 
 	var plan permissionResourceModel
+	var state permissionResourceModel
 
 	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
 
-	var permission PermissionAPIModel
-	resp.Diagnostics.Append(plan.toAPIModel(ctx, &permission)...)
+	resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
 
+	var planPermission PermissionAPIModel
+	resp.Diagnostics.Append(plan.toAPIModel(ctx, &planPermission)...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+
+	var statePermission PermissionAPIModel
+	resp.Diagnostics.Append(state.toAPIModel(ctx, &statePermission)...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+
+	var response *resty.Response
+	var err error
+	var jfrogErrors util.JFrogErrors
+
 	// permission can only be updated by resource type, not in its entirety!
 	// so loop through every field and update each value
-	for resourceType, resourceValue := range permission.Resources {
-		response, err := r.ProviderData.Client.R().
+	for resourceType, resourceValue := range planPermission.Resources {
+		request := r.ProviderData.Client.R().
 			SetPathParams(map[string]string{
 				"name":         plan.Name.ValueString(),
 				"resourceType": resourceType,
 			}).
+			SetError(&jfrogErrors)
+
+		// replace the permission resource
+		response, err = request.
 			SetBody(resourceValue).
 			Put(PermissionEndpoint + "/{name}/{resourceType}")
+
 		if err != nil {
 			utilfw.UnableToUpdateResourceError(resp, err.Error())
 			return
 		}
 
 		if response.IsError() {
-			utilfw.UnableToUpdateResourceError(resp, response.String())
+			utilfw.UnableToUpdateResourceError(resp, jfrogErrors.String())
 			return
 		}
 	}
 
+	// check if resource in the state no longer exists in the plan
+	for resourceType := range statePermission.Resources {
+		// resourceType doesn't exist in plan any more
+		if _, ok := planPermission.Resources[resourceType]; !ok {
+			// delete the permission resource
+			response, err = r.ProviderData.Client.R().
+				SetPathParams(map[string]string{
+					"name":         plan.Name.ValueString(),
+					"resourceType": resourceType,
+				}).
+				SetError(&jfrogErrors).
+				Delete(PermissionEndpoint + "/{name}/{resourceType}")
+
+			if err != nil {
+				utilfw.UnableToUpdateResourceError(resp, err.Error())
+				return
+			}
+
+			if response.IsError() {
+				utilfw.UnableToUpdateResourceError(resp, jfrogErrors.String())
+				return
+			}
+		}
+	}
+
 	resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)
 }
 
@@ -931,16 +982,20 @@ func (r *permissionResource) Delete(ctx context.Context, req resource.DeleteRequ
 		return
 	}
 
+	var jfrogErrors util.JFrogErrors
+
 	response, err := r.ProviderData.Client.R().
 		SetPathParam("name", data.Name.ValueString()).
+		SetError(&jfrogErrors).
 		Delete(PermissionEndpoint + "/{name}")
+
 	if err != nil {
 		utilfw.UnableToDeleteResourceError(resp, err.Error())
 		return
 	}
 
 	if response.IsError() {
-		utilfw.UnableToDeleteResourceError(resp, response.String())
+		utilfw.UnableToDeleteResourceError(resp, jfrogErrors.String())
 		return
 	}
 

pkg/platform/resource_permission_test.go

@@ -63,6 +63,21 @@ func TestAccPermission_full(t *testing.T) {
 				}
 			]
 		}
+
+		build = {
+			actions = {
+				users = []
+				groups = []
+			}
+
+			targets = [
+				{
+					name = "artifactory-build-info"
+					include_patterns = ["**"]
+					exclude_patterns = ["{{ .excludePattern }}"]
+				}
+			] 
+		}
 	}`
 
 	updatedTemp := `
@@ -116,21 +131,6 @@ func TestAccPermission_full(t *testing.T) {
 			]
 		}
 
-		build = {
-			actions = {
-				users = []
-				groups = []
-			}
-
-			targets = [
-				{
-					name = "artifactory-build-info"
-					include_patterns = ["**"]
-					exclude_patterns = ["{{ .excludePattern }}"]
-				}
-			] 
-		}
-
 		release_bundle = {
 			actions = {
 				users = [
@@ -241,6 +241,13 @@ func TestAccPermission_full(t *testing.T) {
 					resource.TestCheckResourceAttr(fqrn, "artifact.targets.0.include_patterns.0", "**"),
 					resource.TestCheckResourceAttr(fqrn, "artifact.targets.0.exclude_patterns.#", "1"),
 					resource.TestCheckResourceAttr(fqrn, "artifact.targets.0.exclude_patterns.0", testData["excludePattern"]),
+					resource.TestCheckResourceAttr(fqrn, "build.actions.users.#", "0"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.#", "1"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.0.name", "artifactory-build-info"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.0.include_patterns.#", "1"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.0.include_patterns.0", "**"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.0.exclude_patterns.#", "1"),
+					resource.TestCheckResourceAttr(fqrn, "build.targets.0.exclude_patterns.0", testData["excludePattern"]),
 				),
 			},
 			{
@@ -254,13 +261,7 @@ func TestAccPermission_full(t *testing.T) {
 					resource.TestCheckTypeSetElemAttr(fqrn, "artifact.actions.users.0.permissions.*", "WRITE"),
 					resource.TestCheckResourceAttr(fqrn, "artifact.actions.groups.#", "0"),
 					resource.TestCheckResourceAttr(fqrn, "artifact.targets.#", "4"),
-					resource.TestCheckResourceAttr(fqrn, "build.actions.users.#", "0"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.#", "1"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.0.name", "artifactory-build-info"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.0.include_patterns.#", "1"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.0.include_patterns.0", "**"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.0.exclude_patterns.#", "1"),
-					resource.TestCheckResourceAttr(fqrn, "build.targets.0.exclude_patterns.0", updatedTestData["excludePattern"]),
+					resource.TestCheckNoResourceAttr(fqrn, "build"),
 					resource.TestCheckResourceAttr(fqrn, "release_bundle.actions.users.#", "1"),
 					resource.TestCheckResourceAttr(fqrn, "release_bundle.actions.users.0.permissions.#", "2"),
 					resource.TestCheckTypeSetElemAttr(fqrn, "release_bundle.actions.users.0.permissions.*", "READ"),