Skip to content

Commit

Permalink
azurerm_role_assignment - fix race condition in read after create (#…
Browse files Browse the repository at this point in the history 
…10134)
  • Loading branch information
@jackofallops
jackofallops committed Jan 12, 2021
1 parent 1a1fa48 commit 9f834a6
Show file tree
Hide file tree
Showing 4 changed files with 324 additions and 34 deletions.
90 changes: 90 additions & 0 deletions azurerm/internal/services/authorization/parse/role_assignment.go
View file
@@ -0,0 +1,90 @@
package parse

import (
"fmt"
"strings"

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure"
)

type RoleAssignmentId struct {
SubscriptionID string
ResourceGroup string
ManagementGroup string
Name string
}

func NewRoleAssignmentID(subscriptionId, resourceGroup, managementGroup, name string) (*RoleAssignmentId, error) {
if subscriptionId == "" && resourceGroup == "" && managementGroup == "" {
return nil, fmt.Errorf("one of subscriptionId, resourceGroup, or managementGroup must be provided")
}

if managementGroup != "" {
if subscriptionId != "" || resourceGroup != "" {
return nil, fmt.Errorf("cannot provide subscriptionId or resourceGroup when managementGroup is provided")
}
}

if resourceGroup != "" {
if subscriptionId == "" {
return nil, fmt.Errorf("subscriptionId must not be empty when resourceGroup is provided")
}
}

return &RoleAssignmentId{
SubscriptionID: subscriptionId,
ResourceGroup: resourceGroup,
ManagementGroup: managementGroup,
Name: name,
}, nil
}

func (id RoleAssignmentId) ID() string {
if id.ManagementGroup != "" {
fmtString := "/providers/Microsoft.Management/managementGroups/%s/providers/Microsoft.Authorization/roleAssignments/%s"
return fmt.Sprintf(fmtString, id.ManagementGroup, id.Name)
}

if id.ResourceGroup != "" {
fmtString := "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Authorization/roleAssignments/%s"
return fmt.Sprintf(fmtString, id.SubscriptionID, id.ResourceGroup, id.Name)
}

fmtString := "/subscriptions/%s/providers/Microsoft.Authorization/roleAssignments/%s"
return fmt.Sprintf(fmtString, id.SubscriptionID, id.Name)
}

func RoleAssignmentID(input string) (*RoleAssignmentId, error) {
if len(input) == 0 {
return nil, fmt.Errorf("Role Assignment ID is empty string")
}

roleAssignmentId := RoleAssignmentId{}

switch {
case strings.HasPrefix(input, "/subscriptions/"):
id, err := azure.ParseAzureResourceID(input)
if err != nil {
return nil, fmt.Errorf("could not parse %q as Azure resource ID", input)
}
roleAssignmentId.SubscriptionID = id.SubscriptionID
roleAssignmentId.ResourceGroup = id.ResourceGroup
if roleAssignmentId.Name, err = id.PopSegment("roleAssignments"); err != nil {
return nil, err
}
case strings.HasPrefix(input, "/providers/Microsoft.Management/"):
idParts := strings.Split(input, "/providers/Microsoft.Authorization/roleAssignments/")
if len(idParts) != 2 {
return nil, fmt.Errorf("could not parse Role Assignment ID %q for Management Group", input)
}
if idParts[1] == "" {
return nil, fmt.Errorf("ID was missing a value for the roleAssignments element")
}
roleAssignmentId.Name = idParts[1]
roleAssignmentId.ManagementGroup = strings.TrimPrefix(idParts[0], "/providers/Microsoft.Management/managementGroups/")
default:
return nil, fmt.Errorf("could not parse Role Assignment ID %q", input)
}

return &roleAssignmentId, nil
}
188 changes: 188 additions & 0 deletions azurerm/internal/services/authorization/parse/role_assignment_test.go
View file
@@ -0,0 +1,188 @@
package parse

import (
"testing"

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/resourceid"
)

var _ resourceid.Formatter = RoleAssignmentId{}

func TestRoleAssignmentIDFormatter(t *testing.T) {
testData := []struct {
SubscriptionId string
ResourceGroup string
ManagementGroup string
Name string
Expected string
}{
{
SubscriptionId: "",
ResourceGroup: "",
ManagementGroup: "",
Name: "23456781-2349-8764-5631-234567890121",
},
{
SubscriptionId: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "group1",
ManagementGroup: "managementGroup1",
Name: "23456781-2349-8764-5631-234567890121",
},
{
SubscriptionId: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "",
ManagementGroup: "managementGroup1",
Name: "23456781-2349-8764-5631-234567890121",
},
{
SubscriptionId: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "",
ManagementGroup: "",
Name: "23456781-2349-8764-5631-234567890121",
Expected: "/subscriptions/12345678-1234-9876-4563-123456789012/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
},
{
SubscriptionId: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "group1",
ManagementGroup: "",
Name: "23456781-2349-8764-5631-234567890121",
Expected: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/group1/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
},
{
SubscriptionId: "",
ResourceGroup: "",
ManagementGroup: "12345678-1234-9876-4563-123456789012",
Name: "23456781-2349-8764-5631-234567890121",
Expected: "/providers/Microsoft.Management/managementGroups/12345678-1234-9876-4563-123456789012/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
},
}
for _, v := range testData {
t.Logf("testing %+v", v)
actual, err := NewRoleAssignmentID(v.SubscriptionId, v.ResourceGroup, v.ManagementGroup, v.Name)
if err != nil {
if v.Expected == "" {
continue
}
}
actualId := actual.ID()
if actualId != v.Expected {
t.Fatalf("expected %q, got %q", v.Expected, actualId)
}
}
}

func TestRoleAssignmentID(t *testing.T) {
testData := []struct {
Input string
Error bool
Expected *RoleAssignmentId
}{
{
// empty
Input: "",
Error: true,
},

{
// missing SubscriptionId
Input: "/",
Error: true,
},

{
// just subscription
Input: "/subscriptions/12345678-1234-9876-4563-123456789012/",
Error: true,
},

{
// missing ResourceGroup value
Input: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/",
Error: true,
},

{
// missing Management Group value
Input: "/providers/Microsoft.Management/managementGroups/",
Error: true,
},

{
// missing Role Assignment value at Subscription Scope
Input: "/subscriptions/12345678-1234-9876-4563-123456789012/providers/Microsoft.Authorization/roleAssignments/",
Error: true,
},

{
// missing Role Assignment value at Management Group scope
Input: "/providers/Microsoft.Management/managementGroups/managementGroup1/providers/Microsoft.Authorization/roleAssignments/",
Error: true,
},

{
// valid at subscription scope
Input: "/subscriptions/12345678-1234-9876-4563-123456789012/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
Expected: &RoleAssignmentId{
SubscriptionID: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "",
ManagementGroup: "",
Name: "23456781-2349-8764-5631-234567890121",
},
},

{
// valid at resource group scope
Input: "/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/group1/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
Expected: &RoleAssignmentId{
SubscriptionID: "12345678-1234-9876-4563-123456789012",
ResourceGroup: "group1",
ManagementGroup: "",
Name: "23456781-2349-8764-5631-234567890121",
},
},

{
// valid at management group scope
Input: "/providers/Microsoft.Management/managementGroups/managementGroup1/providers/Microsoft.Authorization/roleAssignments/23456781-2349-8764-5631-234567890121",
Expected: &RoleAssignmentId{
SubscriptionID: "",
ResourceGroup: "",
ManagementGroup: "managementGroup1",
Name: "23456781-2349-8764-5631-234567890121",
},
},
}

for _, v := range testData {
t.Logf("[DEBUG] Testing %q", v.Input)

actual, err := RoleAssignmentID(v.Input)
if err != nil {
if v.Error {
continue
}

t.Fatalf("expected a value but got an error: %+v", err)
}

if v.Error {
t.Fatal("Expect an error but didn't get one")
}

if actual.Name != v.Expected.Name {
t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.Name, actual.Name)
}

if actual.SubscriptionID != v.Expected.SubscriptionID {
t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.SubscriptionID, actual.SubscriptionID)
}

if actual.ResourceGroup != v.Expected.ResourceGroup {
t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.ResourceGroup, actual.ResourceGroup)
}

if actual.ManagementGroup != v.Expected.ManagementGroup {
t.Fatalf("Expected %q but got %q for Role Assignment Name", v.Expected.ManagementGroup, actual.ManagementGroup)
}
}
}
42 changes: 23 additions & 19 deletions azurerm/internal/services/authorization/role_assignment_resource.go
View file
Expand Up @@ -150,10 +150,10 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
properties.RoleAssignmentProperties.PrincipalType = authorization.ServicePrincipal
}

// TODO: we should switch this to use the timeout
if err := resource.Retry(300*time.Second, retryRoleAssignmentsClient(d, scope, name, properties, meta)); err != nil {
if err := resource.Retry(d.Timeout(schema.TimeoutCreate), retryRoleAssignmentsClient(d, scope, name, properties, meta)); err != nil {
return err
}

read, err := roleAssignmentsClient.Get(ctx, scope, name)
if err != nil {
return err
Expand All @@ -162,23 +162,6 @@ func resourceArmRoleAssignmentCreate(d *schema.ResourceData, meta interface{}) e
return fmt.Errorf("Cannot read Role Assignment ID for %q (Scope %q)", name, scope)
}

stateConf := &resource.StateChangeConf{
Pending: []string{
"pending",
},
Target: []string{
"ready",
},
Refresh: roleAssignmentCreateStateRefreshFunc(ctx, roleAssignmentsClient, *read.ID),
MinTimeout: 5 * time.Second,
ContinuousTargetOccurence: 5,
Timeout: d.Timeout(schema.TimeoutCreate),
}

if _, err := stateConf.WaitForState(); err != nil {
return fmt.Errorf("failed waiting for Role Assignment %q to finish replicating: %+v", name, err)
}

d.SetId(*read.ID)
return resourceArmRoleAssignmentRead(d, meta)
}
Expand Down Expand Up @@ -262,6 +245,27 @@ func retryRoleAssignmentsClient(d *schema.ResourceData, scope string, name strin
return resource.NonRetryableError(err)
}

if resp.ID == nil {
return resource.NonRetryableError(fmt.Errorf("creation of Role Assignment %q did not return an id value", name))
}

stateConf := &resource.StateChangeConf{
Pending: []string{
"pending",
},
Target: []string{
"ready",
},
Refresh: roleAssignmentCreateStateRefreshFunc(ctx, roleAssignmentsClient, *resp.ID),
MinTimeout: 5 * time.Second,
ContinuousTargetOccurence: 5,
Timeout: d.Timeout(schema.TimeoutCreate),
}

if _, err := stateConf.WaitForState(); err != nil {
return resource.NonRetryableError(fmt.Errorf("failed waiting for Role Assignment %q to finish replicating: %+v", name, err))
}

return nil
}
}
Expand Down

0 comments on commit 9f834a6

Please sign in to comment.