From cbcbdc9955356e4a36ed8c74d031f2e9520e2855 Mon Sep 17 00:00:00 2001 From: Louis Ruch Date: Mon, 31 Jan 2022 16:03:41 -0800 Subject: [PATCH] feat(role): Support partial success in role create/update (#171) * feat(role): Support partial success in role create/update --- internal/provider/resource_role.go | 50 +++++---- internal/provider/resource_role_test.go | 130 +++++++++++++++++++----- 2 files changed, 135 insertions(+), 45 deletions(-) diff --git a/internal/provider/resource_role.go b/internal/provider/resource_role.go index 116d6d85..29d7c25b 100644 --- a/internal/provider/resource_role.go +++ b/internal/provider/resource_role.go @@ -164,33 +164,36 @@ func resourceRoleCreate(ctx context.Context, d *schema.ResourceData, meta interf } raw := tcr.GetResponse().Map + var diags diag.Diagnostics if principalIds != nil { tspr, err := rc.SetPrincipals(ctx, tcr.Item.Id, 0, principalIds, roles.WithAutomaticVersioning(true)) - if err != nil { - return diag.Errorf("error setting principal IDs on role: %v", err) + switch { + case err != nil: + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "error setting principals", Detail: err.Error()}) + case tspr == nil: + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "nil role after setting principal IDs"}) + default: + raw = tspr.GetResponse().Map } - if tspr == nil { - return diag.Errorf("nil role after setting principal IDs") - } - raw = tspr.GetResponse().Map } if grantStrings != nil { tsgr, err := rc.SetGrants(ctx, tcr.Item.Id, 0, grantStrings, roles.WithAutomaticVersioning(true)) - if err != nil { - return diag.Errorf("error setting grant strings on role: %v", err) + switch { + case err != nil: + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "error setting grants", Detail: err.Error()}) + case tsgr == nil: + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "nil role after setting grant strings"}) + default: + raw = tsgr.GetResponse().Map } - if tsgr == nil { - return diag.Errorf("nil role after setting grant strings") - } - raw = tsgr.GetResponse().Map } if err := setFromRoleResponseMap(d, raw); err != nil { return diag.FromErr(err) } - return nil + return diags } func resourceRoleRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { @@ -279,6 +282,7 @@ func resourceRoleUpdate(ctx context.Context, d *schema.ResourceData, meta interf } } + var diags diag.Diagnostics if d.HasChange(roleGrantStringsKey) { var grantStrings []string if grantStringsVal, ok := d.GetOk(roleGrantStringsKey); ok { @@ -289,10 +293,11 @@ func resourceRoleUpdate(ctx context.Context, d *schema.ResourceData, meta interf } _, err := rc.SetGrants(ctx, d.Id(), 0, grantStrings, roles.WithAutomaticVersioning(true)) if err != nil { - return diag.Errorf("error updating grant strings on role: %v", err) - } - if err := d.Set(roleGrantStringsKey, grantStrings); err != nil { - return diag.FromErr(err) + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "error setting grants", Detail: err.Error()}) + } else { + if err := d.Set(roleGrantStringsKey, grantStrings); err != nil { + return diag.FromErr(err) + } } } @@ -306,14 +311,15 @@ func resourceRoleUpdate(ctx context.Context, d *schema.ResourceData, meta interf } _, err := rc.SetPrincipals(ctx, d.Id(), 0, principalIds, roles.WithAutomaticVersioning(true)) if err != nil { - return diag.Errorf("error updating grant strings on role: %v", err) - } - if err := d.Set(rolePrincipalIdsKey, principalIds); err != nil { - return diag.FromErr(err) + diags = append(diags, diag.Diagnostic{Severity: diag.Error, Summary: "error setting principals", Detail: err.Error()}) + } else { + if err := d.Set(rolePrincipalIdsKey, principalIds); err != nil { + return diag.FromErr(err) + } } } - return nil + return diags } func resourceRoleDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { diff --git a/internal/provider/resource_role_test.go b/internal/provider/resource_role_test.go index bf0c23c8..abc04c07 100644 --- a/internal/provider/resource_role_test.go +++ b/internal/provider/resource_role_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "regexp" "strings" "testing" @@ -54,13 +55,23 @@ resource "boundary_role" "foo" { depends_on = [boundary_role.proj1_admin] }`, fooRoleDescriptionUpdate) - projRoleWithPrincipal = ` + fooUser = ` resource "boundary_user" "foo" { name = "foo" scope_id = boundary_scope.org1.id depends_on = [boundary_role.org1_admin] } +` + barUser = ` +resource "boundary_user" "bar" { + name = "bar" + scope_id = boundary_scope.org1.id + depends_on = [boundary_role.org1_admin] +} +` + + projRoleWithPrincipal = ` resource "boundary_role" "with_principal" { name = "with_principal" description = "with principal" @@ -69,27 +80,33 @@ resource "boundary_role" "with_principal" { depends_on = [boundary_role.proj1_admin] }` - projRoleWithPrincipalUpdate = ` -resource "boundary_user" "foo" { - name = "foo" - scope_id = boundary_scope.org1.id - depends_on = [boundary_role.org1_admin] -} - -resource "boundary_user" "bar" { - name = "bar" - scope_id = boundary_scope.org1.id - depends_on = [boundary_role.org1_admin] -} - + projRoleWithInvalidPrincipal = ` resource "boundary_role" "with_principal" { name = "with_principal" description = "with principal" + principal_ids = ["u_fakeuser"] + scope_id = boundary_scope.proj1.id + depends_on = [boundary_role.proj1_admin] +}` + + projRoleWithPrincipalUpdate = ` +resource "boundary_role" "with_principal" { + name = "with_principal_update" + description = "with principal update" principal_ids = [boundary_user.foo.id, boundary_user.bar.id] scope_id = boundary_scope.proj1.id depends_on = [boundary_role.proj1_admin] }` + projRoleWithInvalidPrincipalUpdate = ` +resource "boundary_role" "with_principal" { + name = "with_principal_update" + description = "with principal update" + principal_ids = [boundary_user.foo.id, "u_fakeuser"] + scope_id = boundary_scope.proj1.id + depends_on = [boundary_role.proj1_admin] +}` + projRoleWithGroups = ` resource "boundary_group" "foo" { name = "foo" @@ -128,6 +145,7 @@ resource "boundary_role" "with_groups" { readonlyGrant = "id=*;type=*;actions=read" readonlyGrantUpdate = "id=*;type=*;actions=read,create" + invalidGrant = "id=*;type=*;actions=badaction" projRoleWithGrants = fmt.Sprintf(` resource "boundary_role" "with_grants" { @@ -138,10 +156,28 @@ resource "boundary_role" "with_grants" { depends_on = [boundary_role.proj1_admin] }`, readonlyGrant) - projRoleWithGrantsUpdate = fmt.Sprintf(` + projRoleWithInvalidGrants = fmt.Sprintf(` resource "boundary_role" "with_grants" { name = "with_grants" description = "with grants" + grant_strings = ["%s"] + scope_id = boundary_scope.proj1.id + depends_on = [boundary_role.proj1_admin] +}`, invalidGrant) + + projRoleWithInvalidGrantsUpdate = fmt.Sprintf(` +resource "boundary_role" "with_grants" { + name = "with_grants_update" + description = "with grants update" + grant_strings = ["%s", "%s"] + scope_id = boundary_scope.proj1.id + depends_on = [boundary_role.proj1_admin] +}`, readonlyGrant, invalidGrant) + + projRoleWithGrantsUpdate = fmt.Sprintf(` +resource "boundary_role" "with_grants" { + name = "with_grants_update" + description = "with grants update" grant_strings = ["%s", "%s"] scope_id = boundary_scope.proj1.id depends_on = [boundary_role.proj1_admin] @@ -209,11 +245,23 @@ func TestAccRoleWithGrants(t *testing.T) { var provider *schema.Provider resource.Test(t, resource.TestCase{ + IsUnitTest: true, ProviderFactories: providerFactories(&provider), CheckDestroy: testAccCheckRoleResourceDestroy(t, provider), Steps: []resource.TestStep{ { - // test project role create with grants + // Create should return error due to invalid grant, however the role will still + // be created and should be set in state. + Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithInvalidGrants), + Check: resource.ComposeTestCheckFunc( + testAccCheckRoleResourceExists(provider, "boundary_role.with_grants"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "name", "with_grants"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "description", "with grants"), + ), + ExpectError: regexp.MustCompile(`Improperly formatted grant`), + }, + { + // Create again with valid grants should succeed Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithGrants), Check: resource.ComposeTestCheckFunc( testAccCheckRoleResourceExists(provider, "boundary_role.with_grants"), @@ -224,13 +272,24 @@ func TestAccRoleWithGrants(t *testing.T) { }, importStep("boundary_role.with_grants"), { - // test project role update with grants + // Update should return error due to invalid grant, however the role will still + // be updated and should be set in state. + Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithInvalidGrantsUpdate), + Check: resource.ComposeTestCheckFunc( + testAccCheckRoleResourceExists(provider, "boundary_role.with_grants"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "name", "with_grants_update"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "description", "with grants update"), + ), + ExpectError: regexp.MustCompile(`Improperly formatted grant`), + }, + { + // Update should now succeed Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithGrantsUpdate), Check: resource.ComposeTestCheckFunc( testAccCheckRoleResourceExists(provider, "boundary_role.with_grants"), testAccCheckRoleResourceGrantsSet(provider, "boundary_role.with_grants", []string{readonlyGrant, readonlyGrantUpdate}), - resource.TestCheckResourceAttr("boundary_role.with_grants", "name", "with_grants"), - resource.TestCheckResourceAttr("boundary_role.with_grants", "description", "with grants"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "name", "with_grants_update"), + resource.TestCheckResourceAttr("boundary_role.with_grants", "description", "with grants update"), ), }, importStep("boundary_role.with_grants"), @@ -245,12 +304,24 @@ func TestAccRoleWithPrincipals(t *testing.T) { var provider *schema.Provider resource.Test(t, resource.TestCase{ + IsUnitTest: true, ProviderFactories: providerFactories(&provider), CheckDestroy: testAccCheckRoleResourceDestroy(t, provider), Steps: []resource.TestStep{ { - // test create - Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithPrincipal), + // Create with invalid principal should create role but return empty plan + // since principal was not set correctly. + Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithInvalidPrincipal), + Check: resource.ComposeTestCheckFunc( + testAccCheckRoleResourceExists(provider, "boundary_role.with_principal"), + resource.TestCheckResourceAttr("boundary_role.with_principal", DescriptionKey, "with principal"), + resource.TestCheckResourceAttr("boundary_role.with_principal", NameKey, "with_principal"), + ), + ExpectError: regexp.MustCompile(`Unable to set principals on role`), + }, + { + // Create again without invalid principal should produce empty plan + Config: testConfig(url, fooOrg, firstProjectFoo, fooUser, projRoleWithPrincipal), Check: resource.ComposeTestCheckFunc( testAccCheckRoleResourceExists(provider, "boundary_role.with_principal"), testAccCheckRoleResourcePrincipalsSet(provider, "boundary_role.with_principal", []string{"boundary_user.foo"}), @@ -262,13 +333,26 @@ func TestAccRoleWithPrincipals(t *testing.T) { }, importStep("boundary_role.with_principal"), { - // test update - Config: testConfig(url, fooOrg, firstProjectFoo, projRoleWithPrincipalUpdate), + // Update with invalid principal should update role but return empty plan + // since principal was not set correctly. + Config: testConfig(url, fooOrg, firstProjectFoo, fooUser, projRoleWithInvalidPrincipalUpdate), + Check: resource.ComposeTestCheckFunc( + testAccCheckRoleResourceExists(provider, "boundary_role.with_principal"), + resource.TestCheckResourceAttr("boundary_role.with_principal", DescriptionKey, "with principal update"), + resource.TestCheckResourceAttr("boundary_role.with_principal", NameKey, "with_principal_update"), + ), + ExpectError: regexp.MustCompile(`Unable to set principals on role`), + }, + { + // Update again without invalid principal should produce empty plan + Config: testConfig(url, fooOrg, firstProjectFoo, fooUser, barUser, projRoleWithPrincipalUpdate), Check: resource.ComposeTestCheckFunc( testAccCheckRoleResourceExists(provider, "boundary_role.with_principal"), testAccCheckUserResourceExists(provider, "boundary_user.foo"), testAccCheckUserResourceExists(provider, "boundary_user.bar"), testAccCheckRoleResourcePrincipalsSet(provider, "boundary_role.with_principal", []string{"boundary_user.foo", "boundary_user.bar"}), + resource.TestCheckResourceAttr("boundary_role.with_principal", DescriptionKey, "with principal update"), + resource.TestCheckResourceAttr("boundary_role.with_principal", NameKey, "with_principal_update"), ), }, importStep("boundary_role.with_principal"),