From 88f485b5cc63964c1bbc6ebc068e4d954c6d6822 Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Mon, 17 Jun 2019 12:14:13 -0700 Subject: [PATCH] update to using one get per node --- pkg/cloudprovider/cloudprovider.go | 16 +++---- pkg/cloudprovider/identity.go | 2 +- pkg/cloudprovider/vm.go | 7 +-- pkg/cloudprovider/vmss.go | 7 +-- pkg/mic/mic.go | 76 +++++++++++++++--------------- 5 files changed, 52 insertions(+), 56 deletions(-) diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go index 102d442fb..beb92cfb2 100644 --- a/pkg/cloudprovider/cloudprovider.go +++ b/pkg/cloudprovider/cloudprovider.go @@ -32,7 +32,7 @@ type ClientInt interface { RemoveUserMSI(userAssignedMSIID string, node *corev1.Node) error AssignUserMSI(userAssignedMSIID string, node *corev1.Node) error AppendOrRemoveUserMSI(addUserAssignedMSIIDs []string, removeUserAssignedMSIIDs []string, node *corev1.Node) error - CheckUserMSI(userAssignedMSIID string, node *corev1.Node) bool + GetUserMSIs(node *corev1.Node) ([]string, error) } // NewCloudProvider returns a azure cloud provider client @@ -115,19 +115,19 @@ func withInspection() autorest.PrepareDecorator { } } -// CheckUserMSI checks if the identity has been assigned to the node -func (c *Client) CheckUserMSI(userAssignedMSIID string, node *corev1.Node) bool { +// GetUserMSIs will return a list of all identities on the node +func (c *Client) GetUserMSIs(node *corev1.Node) ([]string, error) { idH, _, err := c.getIdentityResource(node) if err != nil { - glog.Errorf("CheckUserMSI: get identity resource failed with error %v", err) - return false + glog.Errorf("GetUserMSIs: get identity resource failed with error %v", err) + return nil, err } - info := idH.IdentityInfo() if info == nil { - return false + return nil, fmt.Errorf("identity info is nil") } - return info.CheckUserIdentityExists(userAssignedMSIID) + idList := info.GetUserIdentityList() + return idList, nil } // AppendOrRemoveUserMSI will batch process the removal and addition of ids diff --git a/pkg/cloudprovider/identity.go b/pkg/cloudprovider/identity.go index 715f74c9c..47a497c32 100644 --- a/pkg/cloudprovider/identity.go +++ b/pkg/cloudprovider/identity.go @@ -21,7 +21,7 @@ type IdentityHolder interface { type IdentityInfo interface { AppendUserIdentity(id string) bool RemoveUserIdentity(id string) error - CheckUserIdentityExists(id string) bool + GetUserIdentityList() []string } var ( diff --git a/pkg/cloudprovider/vm.go b/pkg/cloudprovider/vm.go index e4d00bb20..eb265c037 100644 --- a/pkg/cloudprovider/vm.go +++ b/pkg/cloudprovider/vm.go @@ -114,9 +114,6 @@ func (i *vmIdentityInfo) AppendUserIdentity(id string) bool { return appendUserIdentity(&i.info.Type, i.info.IdentityIds, id) } -func (i *vmIdentityInfo) CheckUserIdentityExists(id string) bool { - if i.info.IdentityIds == nil || !checkIfIDInList(*i.info.IdentityIds, id) { - return false - } - return true +func (i *vmIdentityInfo) GetUserIdentityList() []string { + return *i.info.IdentityIds } diff --git a/pkg/cloudprovider/vmss.go b/pkg/cloudprovider/vmss.go index 6de35ac89..94edf5774 100644 --- a/pkg/cloudprovider/vmss.go +++ b/pkg/cloudprovider/vmss.go @@ -122,9 +122,6 @@ func (i *vmssIdentityInfo) AppendUserIdentity(id string) bool { return appendUserIdentity(&i.info.Type, i.info.IdentityIds, id) } -func (i *vmssIdentityInfo) CheckUserIdentityExists(id string) bool { - if i.info.IdentityIds == nil || !checkIfIDInList(*i.info.IdentityIds, id) { - return false - } - return true +func (i *vmssIdentityInfo) GetUserIdentityList() []string { + return *i.info.IdentityIds } diff --git a/pkg/mic/mic.go b/pkg/mic/mic.go index 5c09b83af..335680629 100644 --- a/pkg/mic/mic.go +++ b/pkg/mic/mic.go @@ -273,17 +273,6 @@ func (c *Client) Sync(exit <-chan struct{}) { continue } - glog.V(5).Infof("Initiating assigned id creation for pod - %s, binding - %s", createID.Spec.Pod, binding.Name) - - err = c.createAssignedIdentity(&createID) - if err != nil { - // Since k8s event has only Info and Warning, using Warning. - c.EventRecorder.Event(binding, corev1.EventTypeWarning, "binding apply error", - fmt.Sprintf("Creating assigned identity binding %s node %s for pod %s resulted in error %v", binding.Name, createID.Spec.NodeName, createID.Name, err)) - glog.Error(err) - continue - } - if isUserAssignedMSI { c.appendToAddListForNode(nodeMap, id.Spec.ResourceID, node) } @@ -501,6 +490,21 @@ func (c *Client) appendToAddListForNode(nodeMap map[string]trackUserAssignedMSII func (c *Client) handleNodeErrors(nodesWithError []string, addList *[]aadpodid.AzureAssignedIdentity, deleteList *[]aadpodid.AzureAssignedIdentity, newAssignedIDs []aadpodid.AzureAssignedIdentity) error { + nodeIdentityList := make(map[string][]string) + for _, n := range nodesWithError { + // get the list of msi's currently on node + node, err := c.NodeClient.Get(n) + if err != nil { + glog.Errorf("Lookup of node %s resulted in error %v", n, err) + continue + } + idList, err := c.getUserMSIListForNode(node) + if err != nil { + glog.Errorf("Getting list of msi's from node %s resulted in error %v", n, err) + } + nodeIdentityList[n] = idList + } + isNodeErrored := func(node string) bool { for _, n := range nodesWithError { if n == node { @@ -516,30 +520,24 @@ func (c *Client) handleNodeErrors(nodesWithError []string, addList *[]aadpodid.A binding := createID.Spec.AzureBindingRef isUserAssignedMSI := c.checkIfUserAssignedMSI(id) - node, err := c.NodeClient.Get(createID.Spec.NodeName) - if err != nil { - c.EventRecorder.Event(binding, corev1.EventTypeWarning, "get node error", - fmt.Sprintf("Lookup of node %s for pod %s resulted in error %v", createID.Spec.NodeName, createID.Name, err)) - continue - } - idExistsOnNode := c.checkIfMSIExistsOnNode(id, node) + idExistsOnNode := c.checkIfMSIExistsOnNode(id, createID.Spec.NodeName, nodeIdentityList) nodeHasErrors := isNodeErrored(createID.Spec.NodeName) if !nodeHasErrors || idExistsOnNode { // the identity was successfully assigned to the node c.EventRecorder.Event(binding, corev1.EventTypeNormal, "binding applied", fmt.Sprintf("Binding %s applied on node %s for pod %s", binding.Name, createID.Spec.NodeName, createID.Name)) - continue - } - if isUserAssignedMSI { - // Since k8s event has only Info and Warning, using Warning. - c.EventRecorder.Event(binding, corev1.EventTypeWarning, "binding apply error", - fmt.Sprintf("Applying binding %s node %s for pod %s resulted in error", binding.Name, createID.Spec.NodeName, createID.Name)) + glog.V(5).Infof("Initiating assigned id creation for pod - %s, binding - %s", createID.Spec.Pod, binding.Name) - newErr := c.removeAssignedIdentity(&createID) - if newErr != nil { - glog.Errorf("Error when removing assigned identity in create error path err: %v", newErr) + if isUserAssignedMSI { + err := c.createAssignedIdentity(&createID) + if err != nil { + // Since k8s event has only Info and Warning, using Warning. + c.EventRecorder.Event(binding, corev1.EventTypeWarning, "binding apply error", + fmt.Sprintf("Creating assigned identity binding %s node %s for pod %s resulted in error %v", binding.Name, createID.Spec.NodeName, createID.Name, err)) + glog.Error(err) + } } } } @@ -553,14 +551,7 @@ func (c *Client) handleNodeErrors(nodesWithError []string, addList *[]aadpodid.A isUserAssignedMSI := c.checkIfUserAssignedMSI(id) nodeHasErrors := isNodeErrored(delID.Spec.NodeName) - node, err := c.NodeClient.Get(delID.Spec.NodeName) - if err != nil { - c.EventRecorder.Event(removedBinding, corev1.EventTypeWarning, "get node error", - fmt.Sprintf("Lookup of node %s for pod %s resulted in error %v", delID.Spec.NodeName, delID.Name, err)) - continue - } - - idExistsOnNode := c.checkIfMSIExistsOnNode(id, node) + idExistsOnNode := c.checkIfMSIExistsOnNode(id, delID.Spec.NodeName, nodeIdentityList) if !nodeHasErrors || !idExistsOnNode { // the identity was successfully removed from node @@ -595,8 +586,19 @@ func (c *Client) getAssignedIDName(podName string, podNameSpace string, idName s return podName + "-" + podNameSpace + "-" + idName } -func (c *Client) checkIfMSIExistsOnNode(id *aadpodid.AzureIdentity, node *corev1.Node) bool { - return c.CloudClient.CheckUserMSI(id.Spec.ResourceID, node) +func (c *Client) checkIfMSIExistsOnNode(id *aadpodid.AzureIdentity, nodeName string, nodeMSIList map[string][]string) bool { + if nList, ok := nodeMSIList[nodeName]; ok { + for _, userAssignedMSI := range nList { + if userAssignedMSI == id.Spec.ResourceID { + return true + } + } + } + return false +} + +func (c *Client) getUserMSIListForNode(node *corev1.Node) ([]string, error) { + return c.CloudClient.GetUserMSIs(node) } func (c *Client) convertIDListToMap(arr []aadpodid.AzureIdentity) (m map[string]aadpodid.AzureIdentity, err error) {