From 331424e488ccc1ac305c53e9ffeb9e365cf9e921 Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 7 May 2024 17:22:00 -0700 Subject: [PATCH 1/2] Check for null resource or attributes in state upgrader func Also use path.MatchRoot().AtName() to find the correct path expression for setting attribute value. --- pkg/platform/resource_permission.go | 94 ++++++++++++-- pkg/platform/resource_permission_test.go | 148 +++++++++++++++++++++++ 2 files changed, 229 insertions(+), 13 deletions(-) diff --git a/pkg/platform/resource_permission.go b/pkg/platform/resource_permission.go index 83ede71..4d34625 100644 --- a/pkg/platform/resource_permission.go +++ b/pkg/platform/resource_permission.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/jfrog/terraform-provider-shared/util" utilfw "github.com/jfrog/terraform-provider-shared/util/fw" "github.com/samber/lo" @@ -273,23 +274,68 @@ func (r *permissionResource) Schema(ctx context.Context, req resource.SchemaRequ } } -func setUsersGroupsAttributeToNull(ctx context.Context, resource types.Object, resourceName string, resp *resource.UpgradeStateResponse) { +func setUsersGroupsAttributeToNull(ctx context.Context, resource types.Object, resourceName string, resp *resource.UpgradeStateResponse) diag.Diagnostics { + if resource.IsNull() { + return nil + } + attrs := resource.Attributes() - actionsAttrs := attrs["actions"].(types.Object).Attributes() - usersSet := actionsAttrs["users"].(types.Set) + a, ok := attrs["actions"] + if !ok { + return nil + } + actionsAttrs := a.(types.Object).Attributes() // actions.users and actions.groups no longer allows to be empty set. // they can either be null (not set) or set with items // When prior state has empty set then migrates the value to null + ds := diag.Diagnostics{} - if !usersSet.IsNull() && len(usersSet.Elements()) == 0 { - resp.State.SetAttribute(ctx, path.Root(fmt.Sprintf("%s.actions.users", resourceName)), types.SetNull(usersGroupsResourceModelAttributeTypes)) + u, ok := actionsAttrs["users"] + if !ok { + return nil + } + ds.Append(setAttributeToNull(ctx, u.(types.Set), resourceName, "users", resp)...) + if ds.HasError() { + return ds } - groupsSet := actionsAttrs["groups"].(types.Set) - if !groupsSet.IsNull() && len(groupsSet.Elements()) == 0 { - resp.State.SetAttribute(ctx, path.Root(fmt.Sprintf("%s.actions.groups", resourceName)), types.SetNull(usersGroupsResourceModelAttributeTypes)) + g, ok := actionsAttrs["groups"] + if !ok { + return nil } + ds.Append(setAttributeToNull(ctx, g.(types.Set), resourceName, "groups", resp)...) + if ds.HasError() { + return ds + } + + return ds +} + +func setAttributeToNull(ctx context.Context, stateSet types.Set, resourceName, attrName string, resp *resource.UpgradeStateResponse) diag.Diagnostics { + ds := diag.Diagnostics{} + if !stateSet.IsNull() && len(stateSet.Elements()) == 0 { + // find the attribute using Paths + paths, d := resp.State.PathMatches(ctx, path.MatchRoot(resourceName).AtName("actions").AtName(attrName)) + if d.HasError() { + ds.Append(d...) + return ds + } + if len(paths) == 0 { + tflog.Info(ctx, "no paths found", map[string]interface{}{ + "path": fmt.Sprintf("%s.actions.%s", resourceName, attrName), + }) + return ds + } + + // set attribute to null using the found Path + ds.Append(resp.State.SetAttribute(ctx, paths[0], types.SetNull(usersGroupsResourceModelAttributeTypes))...) + if ds.HasError() { + return ds + } + } + + return ds } func (r *permissionResource) UpgradeState(ctx context.Context) map[int64]resource.StateUpgrader { @@ -318,12 +364,34 @@ func (r *permissionResource) UpgradeState(ctx context.Context) map[int64]resourc } resp.Diagnostics.Append(resp.State.Set(ctx, upgradedStateData)...) + if resp.Diagnostics.HasError() { + return + } + + resp.Diagnostics.Append(setUsersGroupsAttributeToNull(ctx, upgradedStateData.Artifact, "artifact", resp)...) + if resp.Diagnostics.HasError() { + return + } - setUsersGroupsAttributeToNull(ctx, upgradedStateData.Artifact, "artifact", resp) - setUsersGroupsAttributeToNull(ctx, upgradedStateData.Build, "build", resp) - setUsersGroupsAttributeToNull(ctx, upgradedStateData.ReleaseBundle, "release_bundle", resp) - setUsersGroupsAttributeToNull(ctx, upgradedStateData.Destination, "destination", resp) - setUsersGroupsAttributeToNull(ctx, upgradedStateData.PipelineSource, "pipeline_source", resp) + resp.Diagnostics.Append(setUsersGroupsAttributeToNull(ctx, upgradedStateData.Build, "build", resp)...) + if resp.Diagnostics.HasError() { + return + } + + resp.Diagnostics.Append(setUsersGroupsAttributeToNull(ctx, upgradedStateData.ReleaseBundle, "release_bundle", resp)...) + if resp.Diagnostics.HasError() { + return + } + + resp.Diagnostics.Append(setUsersGroupsAttributeToNull(ctx, upgradedStateData.Destination, "destination", resp)...) + if resp.Diagnostics.HasError() { + return + } + + resp.Diagnostics.Append(setUsersGroupsAttributeToNull(ctx, upgradedStateData.PipelineSource, "pipeline_source", resp)...) + if resp.Diagnostics.HasError() { + return + } }, }, } diff --git a/pkg/platform/resource_permission_test.go b/pkg/platform/resource_permission_test.go index 57602ac..8627a0c 100644 --- a/pkg/platform/resource_permission_test.go +++ b/pkg/platform/resource_permission_test.go @@ -521,6 +521,79 @@ func TestAccPermission_empty_users_state_migration(t *testing.T) { }) } +func TestAccPermission_no_users_state_migration(t *testing.T) { + _, fqrn, permissionName := testutil.MkNames("test-permission", "platform_permission") + _, _, groupName := testutil.MkNames("test-group", "artifactory_group") + + temp := ` + resource "artifactory_group" "{{ .groupName }}" { + name = "{{ .groupName }}" + } + + resource "platform_permission" "{{ .name }}" { + name = "{{ .name }}" + + artifact = { + actions = { + groups = [ + { + name = artifactory_group.{{ .groupName }}.name + permissions = ["READ"] + } + ] + } + + targets = [] + } + }` + + testData := map[string]string{ + "name": permissionName, + "groupName": groupName, + } + + config := testutil.ExecuteTemplate(permissionName, temp, testData) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + CheckDestroy: testAccCheckPermissionDestroy(fqrn), + Steps: []resource.TestStep{ + { + Config: config, + ExternalProviders: map[string]resource.ExternalProvider{ + "artifactory": { + Source: "jfrog/artifactory", + }, + "platform": { + Source: "jfrog/platform", + VersionConstraint: "1.7.2", + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "name", testData["name"]), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.users.#", "0"), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.groups.#", "1"), + ), + ExpectNonEmptyPlan: true, + }, + { + Config: config, + ProtoV6ProviderFactories: testAccProviders(), + ExternalProviders: map[string]resource.ExternalProvider{ + "artifactory": { + Source: "jfrog/artifactory", + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "name", testData["name"]), + resource.TestCheckNoResourceAttr(fqrn, "artifact.actions.users"), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.groups.#", "1"), + ), + }, + }, + }) +} + func TestAccPermission_empty_groups_state_migration(t *testing.T) { _, fqrn, permissionName := testutil.MkNames("test-permission", "platform_permission") _, _, userName := testutil.MkNames("test-user", "artifactory_managed_user") @@ -746,6 +819,81 @@ func TestAccPermission_empty_groups_state_migration(t *testing.T) { }) } +func TestAccPermission_no_groups_state_migration(t *testing.T) { + _, fqrn, permissionName := testutil.MkNames("test-permission", "platform_permission") + _, _, userName := testutil.MkNames("test-user", "artifactory_managed_user") + + temp := ` + resource "artifactory_managed_user" "{{ .userName }}" { + name = "{{ .userName }}" + email = "{{ .userName }}@tempurl.org" + password = "Password!123" + } + + resource "platform_permission" "{{ .name }}" { + name = "{{ .name }}" + + artifact = { + actions = { + users = [ + { + name = artifactory_managed_user.{{ .userName }}.name + permissions = ["READ"] + } + ] + } + + targets = [] + } + }` + + testData := map[string]string{ + "name": permissionName, + "userName": userName, + } + + config := testutil.ExecuteTemplate(permissionName, temp, testData) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + CheckDestroy: testAccCheckPermissionDestroy(fqrn), + Steps: []resource.TestStep{ + { + Config: config, + ExternalProviders: map[string]resource.ExternalProvider{ + "artifactory": { + Source: "jfrog/artifactory", + }, + "platform": { + Source: "jfrog/platform", + VersionConstraint: "1.7.2", + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "name", testData["name"]), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.users.#", "1"), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.groups.#", "0"), + ), + ExpectNonEmptyPlan: true, + }, + { + Config: config, + ProtoV6ProviderFactories: testAccProviders(), + ExternalProviders: map[string]resource.ExternalProvider{ + "artifactory": { + Source: "jfrog/artifactory", + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(fqrn, "name", testData["name"]), + resource.TestCheckResourceAttr(fqrn, "artifact.actions.users.#", "1"), + resource.TestCheckNoResourceAttr(fqrn, "artifact.actions.groups"), + ), + }, + }, + }) +} + func TestAccPermission_name_change(t *testing.T) { _, fqrn, permissionName := testutil.MkNames("test-permission", "platform_permission") _, _, userName := testutil.MkNames("test-user", "artifactory_managed_user") From 266b4bdcb8c7a9c90b7bf8bd06a8079e266a0a4b Mon Sep 17 00:00:00 2001 From: Alex Hung Date: Tue, 7 May 2024 17:22:52 -0700 Subject: [PATCH 2/2] Update CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 531da12..cc727c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.7.4 (May 8, 2024) + +BUG FIXES: + +* resource/platform_permission: Fix state upgrader crash when upgrading from 1.7.2 to 1.7.3 with no `groups` or `users` attribute set. Issue: [#75](https://github.com/jfrog/terraform-provider-platform/issues/75) PR: [#76](https://github.com/jfrog/terraform-provider-platform/pull/76) + ## 1.7.3 (May 7, 2024) BUG FIXES: