Repository: Azure/aad-pod-identity
Pull request #539 (2020-03-25 18:16:58)

set resource identity type explicitly
<!-- Thank you for helping AAD Pod Identity with a pull request! -->

**Reason for Change**:
<!-- What does this PR improve or fix in AAD Pod Identity? Why is it needed? -->
In case of VMSS, if no identities exist the RP assumes the identity type to be `UserAssigned` if not explicitly set. This is not the case for VMAS. So this PR explicitly sets the type based on the current resource identity type.

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->

**Notes for Reviewers**:



Patch:
diff --git a/pkg/cloudprovider/identity.go b/pkg/cloudprovider/identity.go
index f308409d..0b7c66ee 100644
--- a/pkg/cloudprovider/identity.go
+++ b/pkg/cloudprovider/identity.go
@@ -2,6 +2,8 @@ package cloudprovider
 
 import (
 	"strings"
+
+	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute"
 )
 
 // IdentityHolder represents a resource that contains an Identity object
@@ -29,3 +31,16 @@ func checkIfIDInList(idList []string, desiredID string) bool {
 	}
 	return false
 }
+
+// getUpdatedResourceIdentityType returns the new resource identity type
+// to be set on the VM/VMSS based on current type
+func getUpdatedResourceIdentityType(identityType compute.ResourceIdentityType) compute.ResourceIdentityType {
+	switch identityType {
+	case "", compute.ResourceIdentityTypeNone, compute.ResourceIdentityTypeUserAssigned:
+		return compute.ResourceIdentityTypeUserAssigned
+	case compute.ResourceIdentityTypeSystemAssigned, compute.ResourceIdentityTypeSystemAssignedUserAssigned:
+		return compute.ResourceIdentityTypeSystemAssignedUserAssigned
+	default:
+		return compute.ResourceIdentityTypeNone
+	}
+}
diff --git a/pkg/cloudprovider/vm.go b/pkg/cloudprovider/vm.go
index b4e2a837..94a608c2 100644
--- a/pkg/cloudprovider/vm.go
+++ b/pkg/cloudprovider/vm.go
@@ -177,13 +177,7 @@ func (i *vmIdentityInfo) SetUserIdentities(ids map[string]bool) bool {
 		return true
 	}
 
-	if i.info.Type == compute.ResourceIdentityTypeSystemAssigned {
-		i.info.Type = compute.ResourceIdentityTypeSystemAssignedUserAssigned
-	}
-	if i.info.Type == compute.ResourceIdentityTypeNone {
-		i.info.Type = compute.ResourceIdentityTypeUserAssigned
-	}
-
+	i.info.Type = getUpdatedResourceIdentityType(i.info.Type)
 	i.info.UserAssignedIdentities = userAssignedIdentities
 	return len(i.info.UserAssignedIdentities) > 0
 }
diff --git a/pkg/cloudprovider/vmss.go b/pkg/cloudprovider/vmss.go
index a1db164e..86ebf3ea 100644
--- a/pkg/cloudprovider/vmss.go
+++ b/pkg/cloudprovider/vmss.go
@@ -179,13 +179,7 @@ func (i *vmssIdentityInfo) SetUserIdentities(ids map[string]bool) bool {
 		return true
 	}
 
-	if i.info.Type == compute.ResourceIdentityTypeSystemAssigned {
-		i.info.Type = compute.ResourceIdentityTypeSystemAssignedUserAssigned
-	}
-	if i.info.Type == compute.ResourceIdentityTypeNone {
-		i.info.Type = compute.ResourceIdentityTypeUserAssigned
-	}
-
+	i.info.Type = getUpdatedResourceIdentityType(i.info.Type)
 	i.info.UserAssignedIdentities = userAssignedIdentities
 	return len(i.info.UserAssignedIdentities) > 0
 }


Test patch:
diff --git a/pkg/cloudprovider/identity_test.go b/pkg/cloudprovider/identity_test.go
index dadd869c..53a845c4 100644
--- a/pkg/cloudprovider/identity_test.go
+++ b/pkg/cloudprovider/identity_test.go
@@ -2,6 +2,8 @@ package cloudprovider
 
 import (
 	"testing"
+
+	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-12-01/compute"
 )
 
 func checkIDList(t *testing.T, expect, actual []string) {
@@ -15,3 +17,38 @@ func checkIDList(t *testing.T, expect, actual []string) {
 		}
 	}
 }
+
+func TestGetUpdatedResourceIdentityType(t *testing.T) {
+	cases := []struct {
+		current  compute.ResourceIdentityType
+		expected compute.ResourceIdentityType
+	}{
+		{
+			current:  "",
+			expected: compute.ResourceIdentityTypeUserAssigned,
+		},
+		{
+			current:  compute.ResourceIdentityTypeNone,
+			expected: compute.ResourceIdentityTypeUserAssigned,
+		},
+		{
+			current:  compute.ResourceIdentityTypeUserAssigned,
+			expected: compute.ResourceIdentityTypeUserAssigned,
+		},
+		{
+			current:  compute.ResourceIdentityTypeSystemAssigned,
+			expected: compute.ResourceIdentityTypeSystemAssignedUserAssigned,
+		},
+		{
+			current:  compute.ResourceIdentityTypeSystemAssignedUserAssigned,
+			expected: compute.ResourceIdentityTypeSystemAssignedUserAssigned,
+		},
+	}
+
+	for _, tc := range cases {
+		actual := getUpdatedResourceIdentityType(tc.current)
+		if tc.expected != actual {
+			t.Fatalf("expected: %v, got: %v", tc.expected, actual)
+		}
+	}
+}