From 8dc894caccab0e67fd5d6f25d2ea17c46436289a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Palet?= Date: Tue, 30 Jul 2024 13:32:49 +0100 Subject: [PATCH] Preserve order of project members even if API re-orders them (#484) * Preserve order of project members even if API re-orders them * Adjust role field description * Fix backwards compatibility of deprecated owner_email field * Fix typo --- docs/data-sources/resourcemanager_project.md | 2 +- docs/resources/resourcemanager_project.md | 2 +- .../resourcemanager/project/datasource.go | 4 +- .../resourcemanager/project/resource.go | 90 ++++++++++---- .../resourcemanager/project/resource_test.go | 114 ++++++++++++++++-- 5 files changed, 174 insertions(+), 38 deletions(-) diff --git a/docs/data-sources/resourcemanager_project.md b/docs/data-sources/resourcemanager_project.md index a1898fd1..4946912f 100644 --- a/docs/data-sources/resourcemanager_project.md +++ b/docs/data-sources/resourcemanager_project.md @@ -43,5 +43,5 @@ data "stackit_resourcemanager_project" "example" { Read-Only: -- `role` (String) The role of the member in the project. At least one user must have the `owner` role. Legacy roles (`project.admin`, `project.auditor`, `project.member`, `project.owner`) are not supported. +- `role` (String) The role of the member in the project. Legacy roles (`project.admin`, `project.auditor`, `project.member`, `project.owner`) are not supported. - `subject` (String) Unique identifier of the user, service account or client. This is usually the email address for users or service accounts, and the name in case of clients. diff --git a/docs/resources/resourcemanager_project.md b/docs/resources/resourcemanager_project.md index c06eccef..c5b7fc76 100644 --- a/docs/resources/resourcemanager_project.md +++ b/docs/resources/resourcemanager_project.md @@ -50,5 +50,5 @@ resource "stackit_resourcemanager_project" "example" { Required: -- `role` (String) The role of the member in the project. Possible values include, but are not limited to: `owner`, `editor`, `reader`. At least one user must have the `owner` role. Legacy roles (`project.admin`, `project.auditor`, `project.member`, `project.owner`) are not supported. +- `role` (String) The role of the member in the project. Possible values include, but are not limited to: `owner`, `editor`, `reader`. Legacy roles (`project.admin`, `project.auditor`, `project.member`, `project.owner`) are not supported. - `subject` (String) Unique identifier of the user, service account or client. This is usually the email address for users or service accounts, and the name in case of clients. diff --git a/stackit/internal/services/resourcemanager/project/datasource.go b/stackit/internal/services/resourcemanager/project/datasource.go index bb07b9b7..5fec6612 100644 --- a/stackit/internal/services/resourcemanager/project/datasource.go +++ b/stackit/internal/services/resourcemanager/project/datasource.go @@ -112,7 +112,7 @@ func (d *projectDataSource) Schema(_ context.Context, _ datasource.SchemaRequest "owner_email": "Email address of the owner of the project. This value is only considered during creation. Changing it afterwards will have no effect.", "owner_email_deprecation_message": "The \"owner_email\" field has been deprecated in favor of the \"members\" field. Please use the \"members\" field to assign the owner role to a user, by setting the \"role\" field to `owner`.", "members": "The members assigned to the project. At least one subject needs to be a user, and not a client or service account.", - "members.role": fmt.Sprintf("The role of the member in the project. At least one user must have the `owner` role. Legacy roles (%s) are not supported.", strings.Join(utils.QuoteValues(utils.LegacyProjectRoles), ", ")), + "members.role": fmt.Sprintf("The role of the member in the project. Legacy roles (%s) are not supported.", strings.Join(utils.QuoteValues(utils.LegacyProjectRoles), ", ")), "members.subject": "Unique identifier of the user, service account or client. This is usually the email address for users or service accounts, and the name in case of clients.", } @@ -246,7 +246,7 @@ func (d *projectDataSource) Read(ctx context.Context, req datasource.ReadRequest return } - err = mapMembersFields(membersResp.Members, &model) + err = mapMembersFields(ctx, membersResp.Members, &model) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error reading project", fmt.Sprintf("Processing API response: %v", err)) return diff --git a/stackit/internal/services/resourcemanager/project/resource.go b/stackit/internal/services/resourcemanager/project/resource.go index 0ad56281..c7507775 100644 --- a/stackit/internal/services/resourcemanager/project/resource.go +++ b/stackit/internal/services/resourcemanager/project/resource.go @@ -157,7 +157,7 @@ func (r *projectResource) Schema(_ context.Context, _ resource.SchemaRequest, re "owner_email": "Email address of the owner of the project. This value is only considered during creation. Changing it afterwards will have no effect.", "owner_email_deprecation_message": "The \"owner_email\" field has been deprecated in favor of the \"members\" field. Please use the \"members\" field to assign the owner role to a user, by setting the \"role\" field to `owner`.", "members": "The members assigned to the project. At least one subject needs to be a user, and not a client or service account.", - "members.role": fmt.Sprintf("The role of the member in the project. Possible values include, but are not limited to: `owner`, `editor`, `reader`. At least one user must have the `owner` role. Legacy roles (%s) are not supported.", strings.Join(utils.QuoteValues(utils.LegacyProjectRoles), ", ")), + "members.role": fmt.Sprintf("The role of the member in the project. Possible values include, but are not limited to: `owner`, `editor`, `reader`. Legacy roles (%s) are not supported.", strings.Join(utils.QuoteValues(utils.LegacyProjectRoles), ", ")), "members.subject": "Unique identifier of the user, service account or client. This is usually the email address for users or service accounts, and the name in case of clients.", } @@ -316,7 +316,7 @@ func (r *projectResource) Create(ctx context.Context, req resource.CreateRequest return } - err = mapMembersFields(membersResp.Members, &model) + err = mapMembersFields(ctx, membersResp.Members, &model) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating project", fmt.Sprintf("Processing API payload: %v", err)) return @@ -364,7 +364,7 @@ func (r *projectResource) Read(ctx context.Context, req resource.ReadRequest, re return } - err = mapMembersFields(membersResp.Members, &model) + err = mapMembersFields(ctx, membersResp.Members, &model) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error reading project", fmt.Sprintf("Processing API response: %v", err)) return @@ -428,7 +428,7 @@ func (r *projectResource) Update(ctx context.Context, req resource.UpdateRequest return } - err = mapMembersFields(members, &model) + err = mapMembersFields(ctx, members, &model) if err != nil { core.LogAndAddError(ctx, &resp.Diagnostics, "Error updating project", fmt.Sprintf("Processing API response: %v", err)) return @@ -561,7 +561,7 @@ func mapProjectFields(ctx context.Context, projectResp *resourcemanager.GetProje return nil } -func mapMembersFields(members *[]authorization.Member, model *Model) error { +func mapMembersFields(ctx context.Context, members *[]authorization.Member, model *Model) error { if members == nil { model.Members = types.ListNull(types.ObjectType{AttrTypes: memberTypes}) return nil @@ -574,14 +574,42 @@ func mapMembersFields(members *[]authorization.Member, model *Model) error { return nil } - membersList := []attr.Value{} - for i, m := range *members { + modelMembers := []member{} + if !(model.Members.IsNull() || model.Members.IsUnknown()) { + diags := model.Members.ElementsAs(ctx, &modelMembers, false) + if diags.HasError() { + return fmt.Errorf("processing members: %w", core.DiagsToError(diags)) + } + } + modelMemberIds := make([]string, len(modelMembers)) + for i, m := range modelMembers { + modelMemberIds[i] = memberId(authorization.Member{ + Role: m.Role.ValueStringPointer(), + Subject: m.Subject.ValueStringPointer(), + }) + } + + apiMemberIds := []string{} + for _, m := range *members { if utils.IsLegacyProjectRole(*m.Role) { continue } + apiMemberIds = append(apiMemberIds, memberId(m)) + } + + reconciledMembersIds := utils.ReconcileStringSlices(modelMemberIds, apiMemberIds) + + membersList := []attr.Value{} + for i, m := range reconciledMembersIds { + role := roleFromId(m) + subject := subjectFromId(m) + if role == "" || subject == "" { + return fmt.Errorf("reconcile list of members") + } + membersMap := map[string]attr.Value{ - "subject": types.StringPointerValue(m.Subject), - "role": types.StringPointerValue(m.Role), + "subject": types.StringValue(subject), + "role": types.StringValue(role), } memberTF, diags := types.ObjectValue(memberTypes, membersMap) @@ -609,7 +637,16 @@ func toMembersPayload(ctx context.Context, model *Model) (*[]authorization.Membe return nil, fmt.Errorf("nil model") } if model.Members.IsNull() || model.Members.IsUnknown() { - return &[]authorization.Member{}, nil + if model.OwnerEmail.IsNull() { + return nil, fmt.Errorf("members and owner_email are both null or unknown") + } + + return &[]authorization.Member{ + { + Subject: model.OwnerEmail.ValueStringPointer(), + Role: sdkUtils.Ptr(projectOwnerRole), + }, + }, nil } membersModel := []member{} @@ -618,19 +655,12 @@ func toMembersPayload(ctx context.Context, model *Model) (*[]authorization.Membe return nil, core.DiagsToError(diags) } - members := []authorization.Member{} // If the new "members" fields is set, it has precedence over the deprecated "owner_email" field - if !model.Members.IsNull() && !model.Members.IsUnknown() { - for _, m := range membersModel { - members = append(members, authorization.Member{ - Role: m.Role.ValueStringPointer(), - Subject: m.Subject.ValueStringPointer(), - }) - } - } else { + members := []authorization.Member{} + for _, m := range membersModel { members = append(members, authorization.Member{ - Subject: model.OwnerEmail.ValueStringPointer(), - Role: sdkUtils.Ptr(projectOwnerRole), + Role: m.Role.ValueStringPointer(), + Subject: m.Subject.ValueStringPointer(), }) } @@ -791,3 +821,21 @@ func updateMembers(ctx context.Context, projectId string, modelMembers *[]author func memberId(member authorization.Member) string { return fmt.Sprintf("%s,%s", *member.Subject, *member.Role) } + +// Extract the role from the member ID representation +func roleFromId(id string) string { + parts := strings.Split(id, ",") + if len(parts) != 2 { + return "" + } + return parts[1] +} + +// Extract the subject from the member ID representation +func subjectFromId(id string) string { + parts := strings.Split(id, ",") + if len(parts) != 2 { + return "" + } + return parts[0] +} diff --git a/stackit/internal/services/resourcemanager/project/resource_test.go b/stackit/internal/services/resourcemanager/project/resource_test.go index b0690452..4e60fc5a 100644 --- a/stackit/internal/services/resourcemanager/project/resource_test.go +++ b/stackit/internal/services/resourcemanager/project/resource_test.go @@ -12,6 +12,7 @@ import ( "github.com/gorilla/mux" "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/stackitcloud/stackit-sdk-go/core/config" "github.com/stackitcloud/stackit-sdk-go/core/utils" "github.com/stackitcloud/stackit-sdk-go/services/authorization" @@ -165,6 +166,7 @@ func TestMapProjectFields(t *testing.T) { func TestMapMembersFields(t *testing.T) { tests := []struct { description string + configMembers basetypes.ListValue membersResp *[]authorization.Member expected Model expectedLabels *map[string]string @@ -172,6 +174,7 @@ func TestMapMembersFields(t *testing.T) { }{ { "default_ok", + types.ListNull(types.ObjectType{AttrTypes: memberTypes}), &[]authorization.Member{ { Subject: utils.Ptr("owner_email"), @@ -209,8 +212,64 @@ func TestMapMembersFields(t *testing.T) { nil, true, }, + { + "default_ok (preserve model order)", + types.ListValueMust(types.ObjectType{AttrTypes: memberTypes}, []attr.Value{ + types.ObjectValueMust( + memberTypes, + map[string]attr.Value{ + "subject": types.StringValue("reader_email"), + "role": types.StringValue("reader"), + }, + ), + types.ObjectValueMust( + memberTypes, + map[string]attr.Value{ + "subject": types.StringValue("owner_email"), + "role": types.StringValue("owner"), + }, + ), + }), + &[]authorization.Member{ + { + Subject: utils.Ptr("owner_email"), + Role: utils.Ptr("owner"), + }, + { + Subject: utils.Ptr("reader_email"), + Role: utils.Ptr("reader"), + }, + }, + Model{ + Id: types.StringNull(), + ProjectId: types.StringNull(), + ContainerId: types.StringNull(), + ContainerParentId: types.StringNull(), + Name: types.StringNull(), + Labels: types.MapNull(types.StringType), + Members: types.ListValueMust(types.ObjectType{AttrTypes: memberTypes}, []attr.Value{ + types.ObjectValueMust( + memberTypes, + map[string]attr.Value{ + "subject": types.StringValue("reader_email"), + "role": types.StringValue("reader"), + }, + ), + types.ObjectValueMust( + memberTypes, + map[string]attr.Value{ + "subject": types.StringValue("owner_email"), + "role": types.StringValue("owner"), + }, + ), + }), + }, + nil, + true, + }, { "empty members", + types.ListNull(types.ObjectType{AttrTypes: memberTypes}), &[]authorization.Member{}, Model{ Id: types.StringNull(), @@ -226,6 +285,7 @@ func TestMapMembersFields(t *testing.T) { }, { "nil members", + types.ListNull(types.ObjectType{AttrTypes: memberTypes}), nil, Model{ Id: types.StringNull(), @@ -250,7 +310,10 @@ func TestMapMembersFields(t *testing.T) { Name: types.StringNull(), Labels: types.MapNull(types.StringType), } - err := mapMembersFields(tt.membersResp, state) + if !tt.configMembers.IsNull() { + state.Members = tt.configMembers + } + err := mapMembersFields(context.Background(), tt.membersResp, state) if !tt.isValid && err == nil { t.Fatalf("Should have failed") } @@ -275,18 +338,6 @@ func TestToCreatePayload(t *testing.T) { expected *resourcemanager.CreateProjectPayload isValid bool }{ - { - "default_ok", - &Model{}, - nil, - &resourcemanager.CreateProjectPayload{ - ContainerParentId: nil, - Labels: nil, - Members: nil, - Name: nil, - }, - true, - }, { "mapping_with_conversions_single_member", &Model{ @@ -404,6 +455,43 @@ func TestToCreatePayload(t *testing.T) { }, true, }, + { + "deprecated owner_email field still works", + &Model{ + ContainerParentId: types.StringValue("pid"), + Name: types.StringValue("name"), + OwnerEmail: types.StringValue("some_email_deprecated"), + }, + &map[string]string{ + "label1": "1", + "label2": "2", + }, + &resourcemanager.CreateProjectPayload{ + ContainerParentId: utils.Ptr("pid"), + Labels: &map[string]string{ + "label1": "1", + "label2": "2", + }, + Members: &[]resourcemanager.Member{ + { + Subject: utils.Ptr("some_email_deprecated"), + Role: utils.Ptr("owner"), + }, + }, + Name: utils.Ptr("name"), + }, + true, + }, + { + "no members or owner_email fails", + &Model{ + ContainerParentId: types.StringValue("pid"), + Name: types.StringValue("name"), + }, + &map[string]string{}, + nil, + false, + }, { "nil_model", nil,