retry: keep retrying on AppGW until access is granted (#891)

* retry: keep retrying on AppGW until access is granted

* add e2e tests

* change authorizer

* change to settings.GetAuthorizer; change wait 5 seconds

* change environment variable names

* moved role assignment creaton to a seaprate step in E2E

* correct function name

* update summary; role cmd; function name

* add resource id func

Author: akshaysngupta

cmd/appgw-ingress/main.go

@@ -108,7 +108,7 @@ func main() {
 		glog.Fatal(errorLine)
 	}
 
-	azClient := azure.NewAzClient(azure.SubscriptionID(env.SubscriptionID), azure.ResourceGroup(env.ResourceGroupName), azure.ResourceName(env.AppGwName))
+	azClient := azure.NewAzClient(azure.SubscriptionID(env.SubscriptionID), azure.ResourceGroup(env.ResourceGroupName), azure.ResourceName(env.AppGwName), env.ClientID)
 	appGwIdentifier := appgw.Identifier{
 		SubscriptionID: env.SubscriptionID,
 		ResourceGroup:  env.ResourceGroupName,
@@ -125,7 +125,7 @@ func main() {
 		env.HTTPServicePort)
 	httpServer.Start()
 
-	glog.V(3).Infof("App Gateway Details: Subscription: %s, Resource Group: %s, Name: %s", env.SubscriptionID, env.ResourceGroupName, env.AppGwName)
+	glog.V(3).Infof("Appication Gateway Details: Subscription=\"%s\" Resource Group=\"%s\" Name=\"%s\"", env.SubscriptionID, env.ResourceGroupName, env.AppGwName)
 
 	var authorizer autorest.Authorizer
 	if authorizer, err = azure.GetAuthorizerWithRetry(env.AuthLocation, env.UseManagedIdentityForPod, cpConfig, maxRetryCount, retryPause); err != nil {
@@ -138,24 +138,28 @@ func main() {
 		azClient.SetAuthorizer(authorizer)
 	}
 
-	if _, err = azClient.GetGateway(); err != nil {
-		if controllererrors.IsErrorCode(err, controllererrors.ErrorApplicationGatewayNotFound) &&
-			env.EnableDeployAppGateway {
+	// Check if Application Gateway exists/have get access
+	// If AGIC's service principal or managed identity doesn't have read access to the Application Gateway's resource group,
+	// then AGIC can't read it's role assignments to look for the needed permission.
+	// Instead we perform a simple GET request to check both that the Application Gateway exists as well as implicitly make sure that AGIC has read access to it.
+	err = azClient.WaitForGetAccessOnGateway()
+	if err != nil {
+		if controllererrors.IsErrorCode(err, controllererrors.ErrorApplicationGatewayNotFound) && env.EnableDeployAppGateway {
 			if env.AppGwSubnetID != "" {
 				err = azClient.DeployGatewayWithSubnet(env.AppGwSubnetID)
 			} else if cpConfig != nil {
 				err = azClient.DeployGatewayWithVnet(azure.ResourceGroup(cpConfig.VNetResourceGroup), azure.ResourceName(cpConfig.VNetName), azure.ResourceName(env.AppGwSubnetName), env.AppGwSubnetPrefix)
 			}
 
 			if err != nil {
-				errorLine := fmt.Sprint("Failed in deploying App gateway", err)
+				errorLine := fmt.Sprint("Failed in deploying Application Gateway", err)
 				if agicPod != nil {
 					recorder.Event(agicPod, v1.EventTypeWarning, events.ReasonFailedDeployingAppGw, errorLine)
 				}
 				glog.Fatal(errorLine)
 			}
 		} else {
-			errorLine := fmt.Sprint("Failed authenticating with Azure Resource Manager: ", err)
+			errorLine := fmt.Sprint("Failed getting Application Gateway: ", err)
 			if agicPod != nil {
 				recorder.Event(agicPod, v1.EventTypeWarning, events.ReasonARMAuthFailure, errorLine)
 			}

helm/ingress-azure/templates/configmap.yaml

@@ -70,6 +70,7 @@ data:
 
 {{- if .Values.armAuth -}}
 {{- if eq .Values.armAuth.type "aadPodIdentity"}}
+  AZURE_CLIENT_ID: "{{ .Values.armAuth.identityClientID }}"
   USE_MANAGED_IDENTITY_FOR_POD: "true"
 {{- end }}
 {{- end }}

pkg/azure/azure.go

@@ -55,6 +55,16 @@ func RouteTableID(subscriptionID SubscriptionID, resourceGroup ResourceGroup, ro
 	return ResourceID(subscriptionID, resourceGroup, "Microsoft.Network", "routeTables", string(routeTableName))
 }
 
+// ApplicationGatewayID generates a application gateway resource id
+func ApplicationGatewayID(subscriptionID SubscriptionID, resourceGroup ResourceGroup, applicationGatewayName ResourceName) string {
+	return ResourceID(subscriptionID, resourceGroup, "Microsoft.Network", "applicationGateways", string(applicationGatewayName))
+}
+
+// ResourceGroupID generates a resource group resource id
+func ResourceGroupID(subscriptionID SubscriptionID, resourceGroup ResourceGroup) string {
+	return fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscriptionID, resourceGroup)
+}
+
 // ConvertToClusterResourceGroup converts infra resource group to aks cluster ID
 func ConvertToClusterResourceGroup(subscriptionID SubscriptionID, resourceGroup ResourceGroup, err error) (string, error) {
 	if err != nil {

pkg/azure/azure_test.go

@@ -124,6 +124,20 @@ var _ = Describe("Azure", func() {
 			})
 		})
 
+		Context("test ApplicationGatewayID func", func() {
+			It("generate correct application gateway ID", func() {
+				expectedGatewayID := "/subscriptions/subID/resourceGroups/resGp/providers/Microsoft.Network/applicationGateways/gateway"
+				Expect(ApplicationGatewayID(SubscriptionID("subID"), ResourceGroup("resGp"), ResourceName("gateway"))).To(Equal(expectedGatewayID))
+			})
+		})
+
+		Context("test ResourceGroupID func", func() {
+			It("generate correct resource group ID", func() {
+				expectedGroupID := "/subscriptions/subID/resourceGroups/resGp"
+				Expect(ResourceGroupID(SubscriptionID("subID"), ResourceGroup("resGp"))).To(Equal(expectedGroupID))
+			})
+		})
+
 		Context("test ParseSubResourceID func", func() {
 			It("parses sub resource ID correctly", func() {
 				subResourceID := "/subscriptions/subID/resourceGroups/resGp/providers/Microsoft.Network/applicationGateways/appgw/sslCertificates/cert"

pkg/azure/client.go

@@ -9,7 +9,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"time"
 
 	r "github.com/Azure/azure-sdk-for-go/profiles/latest/resources/mgmt/resources"
 	n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-09-01/network"
@@ -22,17 +21,12 @@ import (
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/version"
 )
 
-const (
-	retryPause         = 10 * time.Second
-	retryCount         = 3
-	extendedRetryCount = 60
-)
-
 // AzClient is an interface for client to Azure
 type AzClient interface {
 	SetAuthorizer(authorizer autorest.Authorizer)
 
 	ApplyRouteTable(string, string) error
+	WaitForGetAccessOnGateway() error
 	GetGateway() (n.ApplicationGateway, error)
 	UpdateGateway(*n.ApplicationGateway) error
 	DeployGatewayWithVnet(ResourceGroup, ResourceName, ResourceName, string) error
@@ -49,6 +43,7 @@ type azClient struct {
 	routeTablesClient     n.RouteTablesClient
 	groupsClient          r.GroupsClient
 	deploymentsClient     r.DeploymentsClient
+	clientID              string
 
 	subscriptionID    SubscriptionID
 	resourceGroupName ResourceGroup
@@ -59,7 +54,7 @@ type azClient struct {
 }
 
 // NewAzClient returns an Azure Client
-func NewAzClient(subscriptionID SubscriptionID, resourceGroupName ResourceGroup, appGwName ResourceName) AzClient {
+func NewAzClient(subscriptionID SubscriptionID, resourceGroupName ResourceGroup, appGwName ResourceName, clientID string) AzClient {
 	settings, err := auth.GetSettingsFromEnvironment()
 	if err != nil {
 		return nil
@@ -74,6 +69,7 @@ func NewAzClient(subscriptionID SubscriptionID, resourceGroupName ResourceGroup,
 		routeTablesClient:     n.NewRouteTablesClientWithBaseURI(settings.Environment.ResourceManagerEndpoint, string(subscriptionID)),
 		groupsClient:          r.NewGroupsClientWithBaseURI(settings.Environment.ResourceManagerEndpoint, string(subscriptionID)),
 		deploymentsClient:     r.NewDeploymentsClientWithBaseURI(settings.Environment.ResourceManagerEndpoint, string(subscriptionID)),
+		clientID:              clientID,
 
 		subscriptionID:    subscriptionID,
 		resourceGroupName: resourceGroupName,
@@ -118,50 +114,80 @@ func (az *azClient) SetAuthorizer(authorizer autorest.Authorizer) {
 	az.deploymentsClient.Authorizer = authorizer
 }
 
-func (az *azClient) GetGateway() (response n.ApplicationGateway, err error) {
-	err = utils.Retry(retryCount, retryPause,
+func (az *azClient) WaitForGetAccessOnGateway() (err error) {
+	glog.V(5).Info("Getting Application Gateway configuration.")
+	err = utils.Retry(-1, retryPause,
 		func() (utils.Retriable, error) {
-			response, err = az.appGatewaysClient.Get(az.ctx, string(az.resourceGroupName), string(az.appGwName))
+			response, err := az.appGatewaysClient.Get(az.ctx, string(az.resourceGroupName), string(az.appGwName))
 			if err == nil {
-				return utils.Retriable(false), nil
+				return utils.Retriable(true), nil
 			}
 
-			// Reasons for 403 errors
-			if response.Response.Response != nil && response.Response.StatusCode == 403 {
-				glog.Error("Following might be potential reasons:\n" +
-					" AKS Service Principal requires 'Managed Identity Operator' access on Controller Identity\n" +
-					" 'identityResourceID' and/or 'identityClientID' are incorrect in the Helm config\n" +
-					" AGIC Identity requires 'Contributor' access on Application Gateway and 'Reader' access on Application Gateway's Resource Group\n" +
-					" Please check the AAD Pod Identity mni and nmi pod logs to find potential issues.")
-			}
+			e := controllererrors.NewErrorWithInnerErrorf(
+				controllererrors.ErrorGetApplicationGatewayError,
+				err,
+				"Failed fetching configuration for Application Gateway. Will retry in %v.", retryPause,
+			)
 
-			if response.Response.Response != nil && response.Response.StatusCode == 404 {
-				err := controllererrors.NewErrorWithInnerError(
-					controllererrors.ErrorApplicationGatewayNotFound,
+			if response.Response.Response != nil {
+				e = controllererrors.NewErrorWithInnerErrorf(
+					controllererrors.ErrorApplicationGatewayUnexpectedStatusCode,
 					err,
-					"received 404 NOT FOUND status code on getting Application Gateway from ARM.",
+					"Unexpected status code '%d' while performing a GET on Application Gateway.", response.Response.StatusCode,
 				)
-				glog.Error(err.Error())
-				return utils.Retriable(false), err
+
+				if response.Response.StatusCode == 404 {
+					e.Code = controllererrors.ErrorApplicationGatewayNotFound
+				}
+
+				if response.Response.StatusCode == 403 {
+					e.Code = controllererrors.ErrorApplicationGatewayForbidden
+
+					clientID := "<agic-client-id>"
+					if az.clientID != "" {
+						clientID = az.clientID
+					}
+
+					groupID := ResourceGroupID(az.subscriptionID, az.resourceGroupName)
+					applicationGatewayID := ApplicationGatewayID(az.subscriptionID, az.resourceGroupName, az.appGwName)
+					roleAssignmentCmd := fmt.Sprintf("az role assignment create --role Reader --scope %s --assignee %s;"+
+						" az role assignment create --role Contributor --scope %s --assignee %s",
+						groupID,
+						clientID,
+						applicationGatewayID,
+						clientID,
+					)
+
+					e.Message += fmt.Sprintf(" You can use '%s' to assign permissions."+
+						" AGIC Identity needs atleast has 'Contributor' access to Application Gateway '%s' and 'Reader' access to Application Gateway's Resource Group '%s'.",
+						roleAssignmentCmd,
+						string(az.appGwName),
+						string(az.resourceGroupName),
+					)
+				}
 			}
 
-			if response.Response.Response != nil && response.Response.StatusCode != 200 {
-				// for example, getting 401. This is not expected as we are getting a token before making the call.
-				glog.Error("unexpected ARM status code on GET existing App Gateway config: ", response.Response.StatusCode)
+			glog.Errorf(e.Error())
+
+			if controllererrors.IsErrorCode(e, controllererrors.ErrorApplicationGatewayNotFound) {
+				return utils.Retriable(false), e
 			}
 
-			err := controllererrors.NewErrorWithInnerErrorf(
-				controllererrors.ErrorGetApplicationGatewayError,
-				err,
-				"failed fetching config for App Gateway instance. Will retry in %v.", retryPause,
-			)
-			glog.Errorf(err.Error())
-			return utils.Retriable(true), err
+			return utils.Retriable(true), e
 		})
 
-	if err != nil && !controllererrors.IsErrorCode(err, controllererrors.ErrorApplicationGatewayNotFound) {
-		glog.Errorf("Tried %d times to authenticate with ARM; Error: %s", retryCount, err)
-	}
+	return
+}
+
+func (az *azClient) GetGateway() (gateway n.ApplicationGateway, err error) {
+	err = utils.Retry(retryCount, retryPause,
+		func() (utils.Retriable, error) {
+			gateway, err = az.appGatewaysClient.Get(az.ctx, string(az.resourceGroupName), string(az.appGwName))
+			if err != nil {
+				glog.Errorf("Error while getting application gateway '%s': %s", string(az.appGwName), err)
+			}
+			return utils.Retriable(true), err
+		})
 	return
 }
 
@@ -459,7 +485,8 @@ func getTemplate() map[string]interface{} {
 				"apiVersion": "2018-08-01",
 				"location": "[resourceGroup().location]",
 				"tags": {
-					"managed-by-k8s-ingress": "true"
+					"managed-by-k8s-ingress": "true",
+					"created-by": "ingress-appgw"
 				},
 				"properties": {
 					"sku": {

pkg/azure/consts.go

@@ -0,0 +1,15 @@
+// -------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+// --------------------------------------------------------------------------------------------
+
+package azure
+
+import "time"
+
+const (
+	retryPause         = 10 * time.Second
+	retryCount         = 3
+	maxAuthRetryCount  = 10
+	extendedRetryCount = 60
+)

pkg/azure/fake.go

@@ -52,6 +52,20 @@ func (az *FakeAzClient) GetGateway() (n.ApplicationGateway, error) {
 	return n.ApplicationGateway{}, nil
 }
 
+// WaitForGetAccessOnGateway runs GetGatewayFunc until it returns a gateway
+func (az *FakeAzClient) WaitForGetAccessOnGateway() error {
+	if az.GetGatewayFunc != nil {
+		for {
+			_, err := az.GetGatewayFunc()
+			if err == nil {
+				return nil
+			}
+		}
+	}
+
+	return nil
+}
+
 // UpdateGateway runs UpdateGatewayFunc and return a gateway
 func (az *FakeAzClient) UpdateGateway(appGwObj *n.ApplicationGateway) (err error) {
 	if az.UpdateGatewayFunc != nil {

pkg/controllererrors/errorcodes.go

@@ -59,10 +59,12 @@ const (
 	ErrorInvalidContent    ErrorCode = "ErrorInvalidContent"
 
 	// azure package
-	ErrorGetApplicationGatewayError ErrorCode = "ErrorGetApplicationGatewayError"
-	ErrorApplicationGatewayNotFound ErrorCode = "ErrorApplicationGatewayNotFound"
-	ErrorSubnetNotFound             ErrorCode = "ErrorSubnetNotFound"
-	ErrorMissingResourceGroup       ErrorCode = "ErrorMissingResourceGroup"
+	ErrorGetApplicationGatewayError             ErrorCode = "ErrorGetApplicationGatewayError"
+	ErrorApplicationGatewayNotFound             ErrorCode = "ErrorApplicationGatewayNotFound"
+	ErrorApplicationGatewayForbidden            ErrorCode = "ErrorApplicationGatewayForbidden"
+	ErrorApplicationGatewayUnexpectedStatusCode ErrorCode = "ErrorApplicationGatewayUnexpectedStatusCode"
+	ErrorSubnetNotFound                         ErrorCode = "ErrorSubnetNotFound"
+	ErrorMissingResourceGroup                   ErrorCode = "ErrorMissingResourceGroup"
 
 	// main package
 	ErrorNoSuchNamespace ErrorCode = "ErrorNoSuchNamespace"

pkg/controllererrors/types.go

@@ -56,10 +56,10 @@ func NewErrorWithInnerErrorf(code ErrorCode, innerError error, message string, a
 // Error implements error interface to return error
 func (e *Error) Error() string {
 	if e.InnerError != nil {
-		return fmt.Sprintf("Code: %s, Message: %s, InnerError: %s", e.Code, e.Message, e.InnerError.Error())
+		return fmt.Sprintf("Code=\"%s\" Message=\"%s\" InnerError=\"%s\"", e.Code, e.Message, e.InnerError.Error())
 	}
 
-	return fmt.Sprintf("Code: %s, Message: %s", e.Code, e.Message)
+	return fmt.Sprintf("Code=\"%s\" Message=\"%s\"", e.Code, e.Message)
 }
 
 // IsErrorCode matches error code to the error

pkg/environment/environment.go

@@ -20,6 +20,9 @@ const (
 	// CloudProviderConfigLocationVarName is an environment variable name. This file is available on azure cluster.
 	CloudProviderConfigLocationVarName = "AZURE_CLOUD_PROVIDER_LOCATION"
 
+	// ClientIDVarName is an environment variable which stores the client id provided through user assigned identity
+	ClientIDVarName = "AZURE_CLIENT_ID"
+
 	// SubscriptionIDVarName is the name of the APPGW_SUBSCRIPTION_ID
 	SubscriptionIDVarName = "APPGW_SUBSCRIPTION_ID"
 
@@ -101,6 +104,7 @@ var (
 // EnvVariables is a struct storing values for environment variables.
 type EnvVariables struct {
 	CloudProviderConfigLocation string
+	ClientID                    string
 	SubscriptionID              string
 	ResourceGroupName           string
 	AppGwName                   string
@@ -158,6 +162,7 @@ func (env *EnvVariables) Consolidate(cpConfig *azure.CloudProviderConfig) {
 func GetEnv() EnvVariables {
 	env := EnvVariables{
 		CloudProviderConfigLocation: os.Getenv(CloudProviderConfigLocationVarName),
+		ClientID:                    os.Getenv(ClientIDVarName),
 		SubscriptionID:              os.Getenv(SubscriptionIDVarName),
 		ResourceGroupName:           os.Getenv(ResourceGroupNameVarName),
 		AppGwName:                   os.Getenv(AppGwNameVarName),