Skip to content

Commit

Permalink
Merge pull request #76 from jfrog/GH-75-fix-crash
Browse files Browse the repository at this point in the history 
Check for null resource or attributes in state upgrader func
  • Loading branch information
@alexhung
alexhung authored May 8, 2024
2 parents fe79529 + 266b4bd commit 0f54f69
Show file tree
Hide file tree
Showing 3 changed files with 235 additions and 13 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
94 changes: 81 additions & 13 deletions pkg/platform/resource_permission.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
},
},
}
Expand Down
148 changes: 148 additions & 0 deletions pkg/platform/resource_permission_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down

0 comments on commit 0f54f69

Please sign in to comment.