Repository: Azure/aad-pod-identity
Pull request #199 (2019-04-24 20:14:37)

Reduce identity assignment time 
Changes to ensure that identities already assigned to underlying VMs/VMSS instances are not assigned again in a single sync cycle. This brings down the assignment times in sceanrios where a single node(in case of VMA) or instances from a single VMSS are assigned the same identity as result of different pods matching their bindings. Brings down the time take to assign identities in #145.

kkmsft: /azp run
azure-pipelines[bot]: Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
kkmsft: /azp run
azure-pipelines[bot]: Azure Pipelines successfully started running 1 pipeline(s).

Patch:
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index e5a3c767..be60d8a5 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -14,6 +14,7 @@ variables:
  GOROOT: '/usr/local/go1.11' # Go installation path
  GOPATH: '$(system.defaultWorkingDirectory)/gopath' # Go workspace path
  modulePath: '$(GOPATH)/src/github.com/$(build.repository.name)' # Path to the module's code
+ GO111MODULE: 'on'
 
 steps:
 - script: |
@@ -40,4 +41,4 @@ steps:
 - script: |
    go test -v $(go list ./... | grep -v test/e2e)
   workingDirectory: '$(modulePath)'
-  displayName: 'Run tests' 
+  displayName: 'Run tests'
diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go
index 35daecf5..aef78ff7 100644
--- a/pkg/cloudprovider/cloudprovider.go
+++ b/pkg/cloudprovider/cloudprovider.go
@@ -143,14 +143,15 @@ func (c *Client) AssignUserMSI(userAssignedMSIID string, node *corev1.Node) erro
 		info = idH.ResetIdentity()
 	}
 
-	info.AppendUserIdentity(userAssignedMSIID)
-
-	timeStarted = time.Now()
-	if err := updateFunc(); err != nil {
-		return err
+	if info.AppendUserIdentity(userAssignedMSIID) {
+		timeStarted = time.Now()
+		if err := updateFunc(); err != nil {
+			return err
+		}
+		glog.V(6).Infof("CreateOrUpdate of %s completed in %s", node.Name, time.Since(timeStarted))
+	} else {
+		glog.V(6).Infof("Identity %s already assigned to node %s. Skipping assignment.", userAssignedMSIID, node.Name)
 	}
-
-	glog.V(6).Infof("CreateOrUpdate of %s completed in %s", node.Name, time.Since(timeStarted))
 	return nil
 }
 
diff --git a/pkg/cloudprovider/identity.go b/pkg/cloudprovider/identity.go
index 3e26f254..3163a6ff 100644
--- a/pkg/cloudprovider/identity.go
+++ b/pkg/cloudprovider/identity.go
@@ -19,7 +19,7 @@ type IdentityHolder interface {
 // have different identity types.
 // This abstracts those differences.
 type IdentityInfo interface {
-	AppendUserIdentity(id string)
+	AppendUserIdentity(id string) bool
 	RemoveUserIdentity(id string) error
 }
 
@@ -74,13 +74,13 @@ func filter(ls *[]string, filter string) {
 
 // appendUserIdentity provides a common implementation for adding a new identity
 // to an identity object.
-func appendUserIdentity(idType *compute.ResourceIdentityType, idList *[]string, newID string) {
+func appendUserIdentity(idType *compute.ResourceIdentityType, idList *[]string, newID string) bool {
 	switch *idType {
 	case compute.ResourceIdentityTypeUserAssigned, compute.ResourceIdentityTypeSystemAssignedUserAssigned:
 		// check if this ID is already in the list
 		for _, id := range *idList {
 			if id == newID {
-				return
+				return false
 			}
 		}
 	case compute.ResourceIdentityTypeSystemAssigned:
@@ -90,4 +90,5 @@ func appendUserIdentity(idType *compute.ResourceIdentityType, idList *[]string,
 	}
 
 	*idList = append(*idList, newID)
+	return true
 }
diff --git a/pkg/cloudprovider/vm.go b/pkg/cloudprovider/vm.go
index 8dd3b670..fbc49924 100644
--- a/pkg/cloudprovider/vm.go
+++ b/pkg/cloudprovider/vm.go
@@ -100,10 +100,10 @@ func (i *vmIdentityInfo) RemoveUserIdentity(id string) error {
 	return nil
 }
 
-func (i *vmIdentityInfo) AppendUserIdentity(id string) {
+func (i *vmIdentityInfo) AppendUserIdentity(id string) bool {
 	if i.info.IdentityIds == nil {
 		var ids []string
 		i.info.IdentityIds = &ids
 	}
-	appendUserIdentity(&i.info.Type, i.info.IdentityIds, id)
+	return appendUserIdentity(&i.info.Type, i.info.IdentityIds, id)
 }
diff --git a/pkg/cloudprovider/vmss.go b/pkg/cloudprovider/vmss.go
index e8d53719..04d17654 100644
--- a/pkg/cloudprovider/vmss.go
+++ b/pkg/cloudprovider/vmss.go
@@ -108,10 +108,10 @@ func (i *vmssIdentityInfo) RemoveUserIdentity(id string) error {
 	return nil
 }
 
-func (i *vmssIdentityInfo) AppendUserIdentity(id string) {
+func (i *vmssIdentityInfo) AppendUserIdentity(id string) bool {
 	if i.info.IdentityIds == nil {
 		var ids []string
 		i.info.IdentityIds = &ids
 	}
-	appendUserIdentity(&i.info.Type, i.info.IdentityIds, id)
+	return appendUserIdentity(&i.info.Type, i.info.IdentityIds, id)
 }


Test patch:
diff --git a/pkg/cloudprovider/identity_test.go b/pkg/cloudprovider/identity_test.go
index 0b9cea82..ab3d05bb 100644
--- a/pkg/cloudprovider/identity_test.go
+++ b/pkg/cloudprovider/identity_test.go
@@ -67,7 +67,10 @@ func TestAppendUserIdentity(t *testing.T) {
 		idType compute.ResourceIdentityType
 	)
 
-	appendUserIdentity(&idType, &idList, "A")
+	append := appendUserIdentity(&idType, &idList, "A")
+	if !append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
 	expect := []string{"A"}
 	checkIDList(t, expect, idList)
 	if idType != compute.ResourceIdentityTypeUserAssigned {
@@ -75,13 +78,19 @@ func TestAppendUserIdentity(t *testing.T) {
 	}
 
 	// Append the same value again, should not change anything
-	appendUserIdentity(&idType, &idList, "A")
+	append = appendUserIdentity(&idType, &idList, "A")
+	if append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
 	checkIDList(t, expect, idList)
 	if idType != compute.ResourceIdentityTypeUserAssigned {
 		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeUserAssigned, idType)
 	}
 
-	appendUserIdentity(&idType, &idList, "B")
+	append = appendUserIdentity(&idType, &idList, "B")
+	if !append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
 	expect = []string{"A", "B"}
 	checkIDList(t, expect, idList)
 	if idType != compute.ResourceIdentityTypeUserAssigned {
@@ -91,7 +100,10 @@ func TestAppendUserIdentity(t *testing.T) {
 	idType = compute.ResourceIdentityTypeSystemAssigned
 	idList = []string{"A"}
 	expect = []string{"A", "B"}
-	appendUserIdentity(&idType, &idList, "B")
+	append = appendUserIdentity(&idType, &idList, "B")
+	if !append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
 	checkIDList(t, expect, idList)
 	if idType != compute.ResourceIdentityTypeSystemAssignedUserAssigned {
 		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeSystemAssignedUserAssigned, idType)
@@ -100,7 +112,10 @@ func TestAppendUserIdentity(t *testing.T) {
 	idType = compute.ResourceIdentityTypeNone
 	idList = []string{}
 	expect = []string{"A"}
-	appendUserIdentity(&idType, &idList, "A")
+	append = appendUserIdentity(&idType, &idList, "A")
+	if !append {
+		t.Fatalf("Expecting the id to be not present. But present returned by Append.")
+	}
 	checkIDList(t, expect, idList)
 	if idType != compute.ResourceIdentityTypeUserAssigned {
 		t.Fatalf("expected type %s, got: %s", compute.ResourceIdentityTypeUserAssigned, idType)