Repository: Azure/aad-pod-identity
Pull request #252 (2019-06-18 18:10:02)

fix: handle new OAuthConfig error consistently
**Reason for Change**:
The error from `adal.NewOAuthConfig` was ignored, so the subsequent error handling block could never be entered since it was comparing the previous `err` to `nil` again. I also made the handling consistent with the rest of the function.

<!-- What does this PR improve or fix in AAD Pod Identity? Why is it needed? -->


**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->

**Notes for Reviewers**:


mboersma: /azp run
azure-pipelines[bot]: <samp>
Commenter does not have sufficient privileges for PR 252 in repo Azure/aad-pod-identity<br>

</samp>
aramase: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/cmd/demo/main.go b/cmd/demo/main.go
index 2fbcf4d9..15bc7c1d 100644
--- a/cmd/demo/main.go
+++ b/cmd/demo/main.go
@@ -83,7 +83,7 @@ func doARMOperations(logger *log.Entry, subscriptionID, resourceGroup string) {
 		return
 	}
 
-	logger.Infof("succesfull doARMOperations vm count %d", len(vmlist.Values()))
+	logger.Infof("successful doARMOperations vm count %d", len(vmlist.Values()))
 }
 
 func testMSIEndpoint(logger *log.Entry, msiEndpoint, resource string) *adal.Token {
@@ -101,7 +101,7 @@ func testMSIEndpoint(logger *log.Entry, msiEndpoint, resource string) *adal.Toke
 		logger.Errorf("zero token found, MSI VM extension, msiEndpoint(%s)", msiEndpoint)
 		return nil
 	}
-	logger.Infof("succesfully acquired a token using the MSI, msiEndpoint(%s)", msiEndpoint)
+	logger.Infof("successfully acquired a token using the MSI, msiEndpoint(%s)", msiEndpoint)
 	return &token
 }
 
@@ -120,7 +120,7 @@ func testMSIEndpointFromUserAssignedID(logger *log.Entry, msiEndpoint, userAssig
 		logger.Errorf("zero token found, userAssignedID MSI, msiEndpoint(%s) clientID(%s)", msiEndpoint, userAssignedID)
 		return nil
 	}
-	logger.Infof("succesfully acquired a token, userAssignedID MSI, msiEndpoint(%s) clientID(%s)", msiEndpoint, userAssignedID)
+	logger.Infof("successfully acquired a token, userAssignedID MSI, msiEndpoint(%s) clientID(%s)", msiEndpoint, userAssignedID)
 	return &token
 }
 
@@ -147,5 +147,5 @@ func testInstanceMetadataRequests(logger *log.Entry) {
 		logger.Error(err)
 		return
 	}
-	logger.Infof("succesfully made GET on instance metadata, %s", body)
+	logger.Infof("successfully made GET on instance metadata, %s", body)
 }
diff --git a/pkg/apis/aadpodidentity/v1/utils.go b/pkg/apis/aadpodidentity/v1/utils.go
index a1ff6d75..a697916b 100644
--- a/pkg/apis/aadpodidentity/v1/utils.go
+++ b/pkg/apis/aadpodidentity/v1/utils.go
@@ -1,7 +1,7 @@
 package v1
 
-func IsNamespacedIdentity(azureId *AzureIdentity) bool {
-	if val, ok := azureId.Annotations[BehaviorKey]; ok {
+func IsNamespacedIdentity(azureID *AzureIdentity) bool {
+	if val, ok := azureID.Annotations[BehaviorKey]; ok {
 		if val == BehaviorNamespaced {
 			return true
 		}
diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go
index d3ff7958..0006e91a 100644
--- a/pkg/auth/auth.go
+++ b/pkg/auth/auth.go
@@ -8,7 +8,6 @@ import (
 
 const (
 	activeDirectoryEndpoint = "https://login.microsoftonline.com/"
-	resourceManagerEndpoint = "https://management.azure.com/"
 )
 
 // GetServicePrincipalTokenFromMSIWithUserAssignedID return the token for the assigned user
@@ -36,7 +35,7 @@ func GetServicePrincipalTokenFromMSIWithUserAssignedID(clientID, resource string
 }
 
 // GetServicePrincipalToken return the token for the assigned user
-func GetServicePrincipalToken(tenantID, clientID, secret string) (*adal.Token, error) {
+func GetServicePrincipalToken(tenantID, clientID, secret, resource string) (*adal.Token, error) {
 	oauthConfig, err := adal.NewOAuthConfig(activeDirectoryEndpoint, tenantID)
 	if err != nil {
 		return nil, fmt.Errorf("creating the OAuth config: %v", err)
@@ -45,7 +44,7 @@ func GetServicePrincipalToken(tenantID, clientID, secret string) (*adal.Token, e
 		*oauthConfig,
 		clientID,
 		secret,
-		resourceManagerEndpoint,
+		resource,
 	)
 	if err != nil {
 		return nil, err
diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go
index 57790878..e00206d8 100644
--- a/pkg/cloudprovider/cloudprovider.go
+++ b/pkg/cloudprovider/cloudprovider.go
@@ -62,11 +62,11 @@ func NewCloudProvider(configFile string) (c *Client, e error) {
 		glog.Errorf("Get cloud env error: %+v", err)
 		return nil, err
 	}
-	oauthConfig, _ := adal.NewOAuthConfig(azureEnv.ActiveDirectoryEndpoint, azureConfig.TenantID)
+	oauthConfig, err := adal.NewOAuthConfig(azureEnv.ActiveDirectoryEndpoint, azureConfig.TenantID)
 	if err != nil {
-		return nil, fmt.Errorf("creating the OAuth config: %v", err)
+		glog.Errorf("Create OAuth config error: %+v", err)
+		return nil, err
 	}
-	//glog.Info("%+v\n", oauthConfig)
 	spt, err := adal.NewServicePrincipalToken(
 		*oauthConfig,
 		azureConfig.ClientID,
diff --git a/pkg/nmi/server/server.go b/pkg/nmi/server/server.go
index 33948631..001a016d 100644
--- a/pkg/nmi/server/server.go
+++ b/pkg/nmi/server/server.go
@@ -214,9 +214,9 @@ func (s *Server) hostHandler(logger *log.Entry, w http.ResponseWriter, r *http.R
 }
 
 // msiHandler uses the remote address to identify the pod ip and uses it
-// to find maching client id, and then returns the token sourced through
+// to find a matching client id, and then returns the token sourced through
 // AAD using adal
-// if the requests contains client id it validates it againsts the admin
+// if the requests contains client id it validates it against the admin
 // configured id.
 func (s *Server) msiHandler(logger *log.Entry, w http.ResponseWriter, r *http.Request) {
 	podIP := parseRemoteAddr(r.RemoteAddr)
@@ -271,7 +271,7 @@ func getTokenForMatchingID(kubeClient k8s.Client, logger *log.Entry, rqClientID
 			return token, clientID, err
 		case aadpodid.ServicePrincipal:
 			tenantid := v.Spec.TenantID
-			logger.Infof("matched identityType:%v tenantid:%s clientid:%s", idType, tenantid, clientID)
+			logger.Infof("matched identityType:%v tenantid:%s clientid:%s resource:%s", idType, tenantid, clientID, rqResource)
 			secret, err := kubeClient.GetSecret(&v.Spec.ClientPassword)
 			if err != nil {
 				return nil, clientID, err
@@ -281,7 +281,7 @@ func getTokenForMatchingID(kubeClient k8s.Client, logger *log.Entry, rqClientID
 				clientSecret = string(v)
 				break
 			}
-			token, err := auth.GetServicePrincipalToken(tenantid, clientID, clientSecret)
+			token, err := auth.GetServicePrincipalToken(tenantid, clientID, clientSecret, rqResource)
 			return token, clientID, err
 		default:
 			return nil, clientID, fmt.Errorf("unsupported identity type %+v", idType)
diff --git a/pkg/stats/stats.go b/pkg/stats/stats.go
index f53b598e..3a0449b6 100644
--- a/pkg/stats/stats.go
+++ b/pkg/stats/stats.go
@@ -33,7 +33,7 @@ const (
 )
 
 func Init() {
-	GlobalStats = make(map[StatsType]time.Duration, 0)
+	GlobalStats = make(map[StatsType]time.Duration)
 }
 
 func Put(key StatsType, val time.Duration) {


Test patch:
diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go
index 195980bb..40b72891 100644
--- a/pkg/auth/auth_test.go
+++ b/pkg/auth/auth_test.go
@@ -5,7 +5,7 @@ import (
 )
 
 func TestGetServicePrincipalToken(t *testing.T) {
-	_, err := GetServicePrincipalToken("tid", "cid", "")
+	_, err := GetServicePrincipalToken("tid", "cid", "", "")
 	if err == nil {
 		t.Fatal("should be error with empty secret")
 	}
diff --git a/pkg/cloudprovider/cloudprovider_test.go b/pkg/cloudprovider/cloudprovider_test.go
index 73ac1ca0..a138e497 100644
--- a/pkg/cloudprovider/cloudprovider_test.go
+++ b/pkg/cloudprovider/cloudprovider_test.go
@@ -307,7 +307,7 @@ func (c *TestCloudClient) UnSetError() {
 }
 
 func NewTestVMClient() *TestVMClient {
-	nodeMap := make(map[string]*compute.VirtualMachine, 0)
+	nodeMap := make(map[string]*compute.VirtualMachine)
 	vmClient := &VMClient{}
 
 	return &TestVMClient{
@@ -318,7 +318,7 @@ func NewTestVMClient() *TestVMClient {
 }
 
 func NewTestVMSSClient() *TestVMSSClient {
-	nodeMap := make(map[string]*compute.VirtualMachineScaleSet, 0)
+	nodeMap := make(map[string]*compute.VirtualMachineScaleSet)
 	vmssClient := &VMSSClient{}
 
 	return &TestVMSSClient{
diff --git a/pkg/crd/crd_test.go b/pkg/crd/crd_test.go
index e31399b1..74dc569c 100644
--- a/pkg/crd/crd_test.go
+++ b/pkg/crd/crd_test.go
@@ -17,9 +17,9 @@ type TestCrdClient struct {
 
 func NewTestCrdClient(config *rest.Config) *TestCrdClient {
 	return &TestCrdClient{
-		assignedIDMap: make(map[string]*aadpodid.AzureAssignedIdentity, 0),
-		bindingMap:    make(map[string]*aadpodid.AzureIdentityBinding, 0),
-		idMap:         make(map[string]*aadpodid.AzureIdentity, 0),
+		assignedIDMap: make(map[string]*aadpodid.AzureAssignedIdentity),
+		bindingMap:    make(map[string]*aadpodid.AzureIdentityBinding),
+		idMap:         make(map[string]*aadpodid.AzureIdentity),
 	}
 }
 
@@ -59,18 +59,18 @@ func (c *TestCrdClient) CreateBinding(bindingName string, idName string, selecto
 	c.bindingMap[bindingName] = binding
 }
 
-func (c *TestCrdClient) CreateId(idName string, t aadpodid.IdentityType, rId string, cId string, cp *api.SecretReference, tId string, adRId string, adEpt string) {
+func (c *TestCrdClient) CreateID(idName string, t aadpodid.IdentityType, rID string, cID string, cp *api.SecretReference, tID string, adRID string, adEpt string) {
 	id := &aadpodid.AzureIdentity{
 		ObjectMeta: v1.ObjectMeta{
 			Name: idName,
 		},
 		Spec: aadpodid.AzureIdentitySpec{
 			Type:       t,
-			ResourceID: rId,
-			ClientID:   cId,
+			ResourceID: rID,
+			ClientID:   cID,
 			//ClientPassword: *cp,
-			TenantID:     tId,
-			ADResourceID: adRId,
+			TenantID:     tID,
+			ADResourceID: adRID,
 			ADEndpoint:   adEpt,
 		},
 	}
@@ -94,9 +94,9 @@ func (c *TestCrdClient) ListBindings() (res *[]aadpodid.AzureIdentityBinding, er
 }
 
 func (c *TestCrdClient) ListAssignedIDs() (res *[]aadpodid.AzureAssignedIdentity, err error) {
-	assignedIdList := make([]aadpodid.AzureAssignedIdentity, 0)
+	assignedIDList := make([]aadpodid.AzureAssignedIdentity, 0)
 	for _, v := range c.assignedIDMap {
-		assignedIdList = append(assignedIdList, *v)
+		assignedIDList = append(assignedIDList, *v)
 	}
-	return &assignedIdList, nil
+	return &assignedIDList, nil
 }
diff --git a/pkg/mic/mic_test.go b/pkg/mic/mic_test.go
index 64c310d6..d39b7e34 100644
--- a/pkg/mic/mic_test.go
+++ b/pkg/mic/mic_test.go
@@ -177,7 +177,7 @@ func (c *TestCloudClient) UnSetError() {
 }
 
 func NewTestVMClient() *TestVMClient {
-	nodeMap := make(map[string]*compute.VirtualMachine, 0)
+	nodeMap := make(map[string]*compute.VirtualMachine)
 	vmClient := &cp.VMClient{}
 
 	return &TestVMClient{
@@ -188,7 +188,7 @@ func NewTestVMClient() *TestVMClient {
 }
 
 func NewTestVMSSClient() *TestVMSSClient {
-	nodeMap := make(map[string]*compute.VirtualMachineScaleSet, 0)
+	nodeMap := make(map[string]*compute.VirtualMachineScaleSet)
 	vmssClient := &cp.VMSSClient{}
 
 	return &TestVMSSClient{
@@ -236,7 +236,7 @@ func (c TestPodClient) GetPods() (pods []*corev1.Pod, err error) {
 }
 
 func (c *TestPodClient) AddPod(podName string, podNs string, nodeName string, binding string) {
-	labels := make(map[string]string, 0)
+	labels := make(map[string]string)
 	labels[aadpodid.CRDLabelKey] = binding
 	pod := &corev1.Pod{
 		ObjectMeta: v1.ObjectMeta{
@@ -278,9 +278,9 @@ type TestCrdClient struct {
 
 func NewTestCrdClient(config *rest.Config) *TestCrdClient {
 	return &TestCrdClient{
-		assignedIDMap: make(map[string]*aadpodid.AzureAssignedIdentity, 0),
-		bindingMap:    make(map[string]*aadpodid.AzureIdentityBinding, 0),
-		idMap:         make(map[string]*aadpodid.AzureIdentity, 0),
+		assignedIDMap: make(map[string]*aadpodid.AzureAssignedIdentity),
+		bindingMap:    make(map[string]*aadpodid.AzureIdentityBinding),
+		idMap:         make(map[string]*aadpodid.AzureIdentity),
 	}
 }
 
@@ -321,18 +321,18 @@ func (c *TestCrdClient) CreateBinding(bindingName string, idName string, selecto
 	c.bindingMap[bindingName] = binding
 }
 
-func (c *TestCrdClient) CreateId(idName string, t aadpodid.IdentityType, rId string, cId string, cp *api.SecretReference, tId string, adRId string, adEpt string) {
+func (c *TestCrdClient) CreateID(idName string, t aadpodid.IdentityType, rID string, cID string, cp *api.SecretReference, tID string, adRID string, adEpt string) {
 	id := &aadpodid.AzureIdentity{
 		ObjectMeta: v1.ObjectMeta{
 			Name: idName,
 		},
 		Spec: aadpodid.AzureIdentitySpec{
 			Type:       t,
-			ResourceID: rId,
-			ClientID:   cId,
+			ResourceID: rID,
+			ClientID:   cID,
 			//ClientPassword: *cp,
-			TenantID:     tId,
-			ADResourceID: adRId,
+			TenantID:     tID,
+			ADResourceID: adRID,
 			ADEndpoint:   adEpt,
 		},
 	}
@@ -356,11 +356,11 @@ func (c *TestCrdClient) ListBindings() (res *[]aadpodid.AzureIdentityBinding, er
 }
 
 func (c *TestCrdClient) ListAssignedIDs() (res *[]aadpodid.AzureAssignedIdentity, err error) {
-	assignedIdList := make([]aadpodid.AzureAssignedIdentity, 0)
+	assignedIDList := make([]aadpodid.AzureAssignedIdentity, 0)
 	for _, v := range c.assignedIDMap {
-		assignedIdList = append(assignedIdList, *v)
+		assignedIDList = append(assignedIDList, *v)
 	}
-	return &assignedIdList, nil
+	return &assignedIDList, nil
 }
 func (c *Client) ListPodIds(podns, podname string) (*[]aadpodid.AzureIdentity, error) {
 	return &[]aadpodid.AzureIdentity{}, nil
@@ -517,7 +517,7 @@ func TestMapMICClient(t *testing.T) {
 
 func TestSimpleMICClient(t *testing.T) {
 
-	exit := make(<-chan struct{}, 0)
+	exit := make(<-chan struct{})
 	eventCh := make(chan aadpodid.EventType, 100)
 	cloudClient := NewTestCloudClient(config.AzureConfig{})
 	crdClient := NewTestCrdClient(nil)
@@ -529,7 +529,7 @@ func TestSimpleMICClient(t *testing.T) {
 
 	micClient := NewMICTestClient(eventCh, cloudClient, crdClient, podClient, nodeClient, &evtRecorder)
 
-	crdClient.CreateId("test-id", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
+	crdClient.CreateID("test-id", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
 	crdClient.CreateBinding("testbinding", "test-id", "test-select")
 
 	nodeClient.AddNode("test-node")
@@ -642,7 +642,7 @@ func TestSimpleMICClient(t *testing.T) {
 }
 
 func TestAddDelMICClient(t *testing.T) {
-	exit := make(<-chan struct{}, 0)
+	exit := make(<-chan struct{})
 	eventCh := make(chan aadpodid.EventType, 100)
 	cloudClient := NewTestCloudClient(config.AzureConfig{})
 	crdClient := NewTestCrdClient(nil)
@@ -656,14 +656,14 @@ func TestAddDelMICClient(t *testing.T) {
 
 	// Test to add and delete at the same time.
 	// Add a pod, identity and binding.
-	crdClient.CreateId("test-id2", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
+	crdClient.CreateID("test-id2", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
 	crdClient.CreateBinding("testbinding2", "test-id2", "test-select2")
 
 	nodeClient.AddNode("test-node2")
 	podClient.AddPod("test-pod2", "default", "test-node2", "test-select2")
 	podClient.GetPods()
 
-	crdClient.CreateId("test-id4", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
+	crdClient.CreateID("test-id4", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
 	crdClient.CreateBinding("testbinding4", "test-id4", "test-select4")
 	podClient.AddPod("test-pod4", "default", "test-node2", "test-select4")
 	podClient.GetPods()
@@ -693,7 +693,7 @@ func TestAddDelMICClient(t *testing.T) {
 	podClient.DeletePod("test-pod4", "default")
 
 	//Add a new pod, with different id and binding on the same node.
-	crdClient.CreateId("test-id3", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
+	crdClient.CreateID("test-id3", aadpodid.UserAssignedMSI, "test-user-msi-resourceid", "test-user-msi-clientid", nil, "", "", "")
 	crdClient.CreateBinding("testbinding3", "test-id3", "test-select3")
 	podClient.AddPod("test-pod3", "default", "test-node2", "test-select3")
 	podClient.GetPods()
diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go
index f99ca483..b3fb3dfd 100644
--- a/pkg/pod/pod_test.go
+++ b/pkg/pod/pod_test.go
@@ -29,7 +29,7 @@ func (c TestPodClient) GetPods() (pods []*corev1.Pod, err error) {
 }
 
 func (c *TestPodClient) AddPod(podName string, podNs string, nodeName string, binding string) {
-	labels := make(map[string]string, 0)
+	labels := make(map[string]string)
 	labels[aadpodid.CRDLabelKey] = binding
 	pod := &corev1.Pod{
 		ObjectMeta: v1.ObjectMeta{
diff --git a/pkg/stats/stats_test.go b/pkg/stats/stats_test.go
index c3c9f260..853257c9 100644
--- a/pkg/stats/stats_test.go
+++ b/pkg/stats/stats_test.go
@@ -11,7 +11,7 @@ import (
 
 func TestBasics(t *testing.T) {
 	glog.Infof("Test started")
-	validateMap := make(map[stats.StatsType]time.Duration, 0)
+	validateMap := make(map[stats.StatsType]time.Duration)
 	stats.Init()
 	stats.Put(stats.Total, time.Second*20)
 	validateMap[stats.Total] = time.Second * 20
diff --git a/test/e2e/identityvalidator/identityvalidator.go b/test/e2e/identityvalidator/identityvalidator.go
index 022e8ef6..a5f73ff7 100644
--- a/test/e2e/identityvalidator/identityvalidator.go
+++ b/test/e2e/identityvalidator/identityvalidator.go
@@ -79,7 +79,7 @@ func testClusterWideUserAssignedIdentity(logger *log.Entry, msiEndpoint, subscri
 		return errors.Wrapf(err, "Failed to verify cluster-wide user assigned identity")
 	}
 
-	logger.Infof("Succesfully verified cluster-wide user assigned identity. VM count: %d", len(vmlist.Values()))
+	logger.Infof("Successfully verified cluster-wide user assigned identity. VM count: %d", len(vmlist.Values()))
 	return nil
 }
 
@@ -97,7 +97,7 @@ func testUserAssignedIdentityOnPod(logger *log.Entry, msiEndpoint, identityClien
 		return errors.Wrapf(err, "Failed to verify user assigned identity on pod")
 	}
 
-	logger.Infof("Succesfully verified user assigned identity on pod")
+	logger.Infof("Successfully verified user assigned identity on pod")
 	return nil
 }
 
@@ -117,6 +117,6 @@ func testSystemAssignedIdentity(logger *log.Entry, msiEndpoint string) (*adal.To
 		return nil, errors.Errorf("No token found, MSI VM extension, msiEndpoint(%s)", msiEndpoint)
 	}
 
-	logger.Infof("Succesfully acquired a token using the MSI, msiEndpoint(%s)", msiEndpoint)
+	logger.Infof("Successfully acquired a token using the MSI, msiEndpoint(%s)", msiEndpoint)
 	return &token, nil
 }