From 812e2835e71818e340a161792b85513993a6e4a0 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 26 Nov 2024 14:30:09 -0800 Subject: [PATCH 1/3] Fix permission resource not being deleted correctly Permission resources that were in the TF state but now are removed from the plan were not deleted. Now we check then delete the permission resource correctly. Use the JFrogErrors struct to get nicer error message. --- go.mod | 2 +- go.sum | 4 +- pkg/platform/resource_permission.go | 71 +++++++++++++++++++++--- pkg/platform/resource_permission_test.go | 45 +++++++-------- 4 files changed, 89 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index 51040fb..6d81317 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index b5c43c0..fba2fe8 100644 --- a/go.sum +++ b/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= diff --git a/pkg/platform/resource_permission.go b/pkg/platform/resource_permission.go index 6a13eaa..c87c254 100644 --- a/pkg/platform/resource_permission.go +++ b/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,8 +821,10 @@ 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()) @@ -829,7 +832,7 @@ func (r *permissionResource) Create(ctx context.Context, req resource.CreateRequ } 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 } diff --git a/pkg/platform/resource_permission_test.go b/pkg/platform/resource_permission_test.go index 629ee09..2059fec 100644 --- a/pkg/platform/resource_permission_test.go +++ b/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"), From 733b6cd355ece1ea2767e93ee0c815da23bfce70 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 26 Nov 2024 14:33:43 -0800 Subject: [PATCH 2/3] Update CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 481f899..227634e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# 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: From 88e5775386a7478fc82d31517827e4ea85264c4a Mon Sep 17 00:00:00 2001 From: JFrog CI Date: Tue, 26 Nov 2024 22:39:33 +0000 Subject: [PATCH 3/3] JFrog Pipelines - Add Artifactory version to CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 227634e..fd1fade 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ 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: