Merge pull request #2261 from k8s-infra-cherrypick-robot/cherry-pick-…

…2248-to-release-1.1

[release-1.1] feat: add support for network resource in a different subscription when using MSI

pkg/auth/azure_auth.go

@@ -248,12 +248,18 @@ func ParseAzureEnvironment(cloudName, resourceManagerEndpoint, identitySystem st
 	return &env, err
 }
 
-// UsesNetworkResourceInDifferentTenantOrSubscription determines whether the AzureAuthConfig indicates to use network resources in different AAD Tenant and Subscription than those for the cluster
-// Return true when one of NetworkResourceTenantID and NetworkResourceSubscriptionID are specified
-// and equal to one defined in global configs
-func (config *AzureAuthConfig) UsesNetworkResourceInDifferentTenantOrSubscription() bool {
-	return (len(config.NetworkResourceTenantID) > 0 && !strings.EqualFold(config.NetworkResourceTenantID, config.TenantID)) ||
-		(len(config.NetworkResourceSubscriptionID) > 0 && !strings.EqualFold(config.NetworkResourceSubscriptionID, config.SubscriptionID))
+// UsesNetworkResourceInDifferentTenant determines whether the AzureAuthConfig indicates to use network resources in
+// different AAD Tenant than those for the cluster. Return true when NetworkResourceTenantID is specified  and not equal
+// to one defined in global configs
+func (config *AzureAuthConfig) UsesNetworkResourceInDifferentTenant() bool {
+	return len(config.NetworkResourceTenantID) > 0 && !strings.EqualFold(config.NetworkResourceTenantID, config.TenantID)
+}
+
+// UsesNetworkResourceInDifferentSubscription determines whether the AzureAuthConfig indicates to use network resources
+// in different Subscription than those for the cluster. Return true when NetworkResourceSubscriptionID is specified
+// and not equal to one defined in global configs
+func (config *AzureAuthConfig) UsesNetworkResourceInDifferentSubscription() bool {
+	return len(config.NetworkResourceSubscriptionID) > 0 && !strings.EqualFold(config.NetworkResourceSubscriptionID, config.SubscriptionID)
 }
 
 // decodePkcs12 decodes a PKCS#12 client certificate by extracting the public certificate and
@@ -285,8 +291,8 @@ func azureStackOverrides(env *azure.Environment, resourceManagerEndpoint, identi
 
 // checkConfigWhenNetworkResourceInDifferentTenant checks configuration for the scenario of using network resource in different tenant
 func (config *AzureAuthConfig) checkConfigWhenNetworkResourceInDifferentTenant() error {
-	if !config.UsesNetworkResourceInDifferentTenantOrSubscription() {
-		return fmt.Errorf("NetworkResourceTenantID and NetworkResourceSubscriptionID must be configured")
+	if !config.UsesNetworkResourceInDifferentTenant() {
+		return fmt.Errorf("NetworkResourceTenantID must be configured")
 	}
 
 	if strings.EqualFold(config.IdentitySystem, consts.ADFSIdentitySystem) {

pkg/auth/azure_auth_test.go

@@ -374,3 +374,21 @@ func TestAzureStackOverrides(t *testing.T) {
 	assert.Equal(t, env.ResourceManagerVMDNSSuffix, "cloudapp.test.com")
 	assert.Equal(t, env.ActiveDirectoryEndpoint, "https://login.microsoftonline.com")
 }
+
+func TestUsesNetworkResourceInDifferentTenant(t *testing.T) {
+	config := &AzureAuthConfig{
+		TenantID:                      "TenantID",
+		AADClientID:                   "AADClientID",
+		AADClientSecret:               "AADClientSecret",
+		NetworkResourceTenantID:       "NetworkTenantID",
+		NetworkResourceSubscriptionID: "NetworkResourceSubscriptionID",
+	}
+
+	assert.Equal(t, config.UsesNetworkResourceInDifferentTenant(), true)
+	assert.Equal(t, config.UsesNetworkResourceInDifferentSubscription(), true)
+
+	config.NetworkResourceTenantID = ""
+	config.NetworkResourceSubscriptionID = ""
+	assert.Equal(t, config.UsesNetworkResourceInDifferentTenant(), false)
+	assert.Equal(t, config.UsesNetworkResourceInDifferentSubscription(), false)
+}

pkg/provider/azure.go

@@ -681,7 +681,7 @@ func (az *Cloud) configureMultiTenantClients(servicePrincipalToken *adal.Service
 	var err error
 	var multiTenantServicePrincipalToken *adal.MultiTenantServicePrincipalToken
 	var networkResourceServicePrincipalToken *adal.ServicePrincipalToken
-	if az.Config.UsesNetworkResourceInDifferentTenantOrSubscription() {
+	if az.Config.UsesNetworkResourceInDifferentTenant() {
 		multiTenantServicePrincipalToken, err = auth.GetMultiTenantServicePrincipalToken(&az.Config.AzureAuthConfig, &az.Environment)
 		if err != nil {
 			return err
@@ -783,7 +783,9 @@ func (az *Cloud) configAzureClients(
 		loadBalancerClientConfig.Authorizer = networkResourceServicePrincipalTokenAuthorizer
 		securityGroupClientConfig.Authorizer = networkResourceServicePrincipalTokenAuthorizer
 		publicIPClientConfig.Authorizer = networkResourceServicePrincipalTokenAuthorizer
+	}
 
+	if az.UsesNetworkResourceInDifferentSubscription() {
 		routeClientConfig.SubscriptionID = az.Config.NetworkResourceSubscriptionID
 		subnetClientConfig.SubscriptionID = az.Config.NetworkResourceSubscriptionID
 		routeTableClientConfig.SubscriptionID = az.Config.NetworkResourceSubscriptionID

pkg/provider/azure_loadbalancer.go

@@ -188,7 +188,7 @@ func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, ser
 	// Here we'll firstly ensure service do not lie in the opposite LB.
 	var err error
 	serviceName := getServiceName(service)
-	mc := metrics.NewMetricContext("services", "ensure_loadbalancer", az.ResourceGroup, az.SubscriptionID, serviceName)
+	mc := metrics.NewMetricContext("services", "ensure_loadbalancer", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
 	klog.V(5).InfoS("EnsureLoadBalancer Start", "service", serviceName, "cluster", clusterName, "service_spec", service)
 
 	isOperationSucceeded := false
@@ -210,7 +210,7 @@ func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, ser
 func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) error {
 	var err error
 	serviceName := getServiceName(service)
-	mc := metrics.NewMetricContext("services", "update_loadbalancer", az.ResourceGroup, az.SubscriptionID, serviceName)
+	mc := metrics.NewMetricContext("services", "update_loadbalancer", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
 	klog.V(5).InfoS("UpdateLoadBalancer Start", "service", serviceName, "cluster", clusterName, "service_spec", service)
 	isOperationSucceeded := false
 	defer func() {
@@ -244,7 +244,7 @@ func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName stri
 	var err error
 	isInternal := requiresInternalLoadBalancer(service)
 	serviceName := getServiceName(service)
-	mc := metrics.NewMetricContext("services", "ensure_loadbalancer_deleted", az.ResourceGroup, az.SubscriptionID, serviceName)
+	mc := metrics.NewMetricContext("services", "ensure_loadbalancer_deleted", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
 	klog.V(5).InfoS("EnsureLoadBalancerDeleted Start", "service", serviceName, "cluster", clusterName, "service_spec", service)
 	isOperationSucceeded := false
 	defer func() {
@@ -1965,7 +1965,7 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
 
 			newConfig := network.FrontendIPConfiguration{
 				Name:                                    to.StringPtr(defaultLBFrontendIPConfigName),
-				ID:                                      to.StringPtr(fmt.Sprintf(consts.FrontendIPConfigIDTemplate, az.SubscriptionID, az.ResourceGroup, *lb.Name, defaultLBFrontendIPConfigName)),
+				ID:                                      to.StringPtr(fmt.Sprintf(consts.FrontendIPConfigIDTemplate, az.getNetworkResourceSubscriptionID(), az.ResourceGroup, *lb.Name, defaultLBFrontendIPConfigName)),
 				FrontendIPConfigurationPropertiesFormat: fipConfigurationProperties,
 			}
 

pkg/provider/azure_routes.go

@@ -369,7 +369,7 @@ func (az *Cloud) createRouteTable() error {
 // route.Name will be ignored, although the cloud-provider may use nameHint
 // to create a more user-meaningful name.
 func (az *Cloud) CreateRoute(ctx context.Context, clusterName string, nameHint string, kubeRoute *cloudprovider.Route) error {
-	mc := metrics.NewMetricContext("routes", "create_route", az.ResourceGroup, az.SubscriptionID, string(kubeRoute.TargetNode))
+	mc := metrics.NewMetricContext("routes", "create_route", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), string(kubeRoute.TargetNode))
 	isOperationSucceeded := false
 	defer func() {
 		mc.ObserveOperationWithResult(isOperationSucceeded)
@@ -448,7 +448,7 @@ func (az *Cloud) CreateRoute(ctx context.Context, clusterName string, nameHint s
 // DeleteRoute deletes the specified managed route
 // Route should be as returned by ListRoutes
 func (az *Cloud) DeleteRoute(ctx context.Context, clusterName string, kubeRoute *cloudprovider.Route) error {
-	mc := metrics.NewMetricContext("routes", "delete_route", az.ResourceGroup, az.SubscriptionID, string(kubeRoute.TargetNode))
+	mc := metrics.NewMetricContext("routes", "delete_route", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), string(kubeRoute.TargetNode))
 	isOperationSucceeded := false
 	defer func() {
 		mc.ObserveOperationWithResult(isOperationSucceeded)

pkg/provider/azure_standard.go

@@ -107,7 +107,7 @@ func (az *Cloud) getLoadBalancerProbeID(lbName, rgName, lbRuleName string) strin
 
 // getNetworkResourceSubscriptionID returns the subscription id which hosts network resources
 func (az *Cloud) getNetworkResourceSubscriptionID() string {
-	if az.Config.UsesNetworkResourceInDifferentTenantOrSubscription() {
+	if az.Config.UsesNetworkResourceInDifferentSubscription() {
 		return az.NetworkResourceSubscriptionID
 	}
 	return az.SubscriptionID

pkg/provider/azure_standard_test.go

@@ -545,44 +545,54 @@ func testGetLoadBalancerSubResourceID(
 		resourceGroupName                   string
 		subResourceName                     string
 		useNetworkResourceInDifferentTenant bool
+		useNetworkResourceInDifferentSub    bool
 		expected                            string
 	}{
 		{
-			description:                         "resource id should contain NetworkResourceSubscriptionID when using network resources in different subscription",
+			description:                         "resource id should contain NetworkResourceSubscriptionID when using network resources in different tenant and subscription",
 			loadBalancerName:                    "lbName",
 			resourceGroupName:                   "rgName",
 			subResourceName:                     "subResourceName",
 			useNetworkResourceInDifferentTenant: true,
+			useNetworkResourceInDifferentSub:    true,
+		},
+		{
+			description:                         "resource id should contain NetworkResourceSubscriptionID when using network resources in different subscription",
+			loadBalancerName:                    "lbName",
+			resourceGroupName:                   "rgName",
+			subResourceName:                     "subResourceName",
+			useNetworkResourceInDifferentTenant: false,
+			useNetworkResourceInDifferentSub:    true,
 		},
 		{
 			description:                         "resource id should contain SubscriptionID when not using network resources in different subscription",
 			loadBalancerName:                    "lbName",
 			resourceGroupName:                   "rgName",
 			subResourceName:                     "subResourceName",
 			useNetworkResourceInDifferentTenant: false,
+			useNetworkResourceInDifferentSub:    false,
 		},
 	}
 
 	for _, c := range cases {
+		subscriptionID := az.SubscriptionID
 		if c.useNetworkResourceInDifferentTenant {
 			az.NetworkResourceTenantID = networkResourceTenantID
-			az.NetworkResourceSubscriptionID = networkResourceSubscriptionID
-			c.expected = fmt.Sprintf(
-				expectedResourceIDTemplate,
-				az.NetworkResourceSubscriptionID,
-				c.resourceGroupName,
-				c.loadBalancerName,
-				c.subResourceName)
 		} else {
 			az.NetworkResourceTenantID = ""
+		}
+		if c.useNetworkResourceInDifferentSub {
+			az.NetworkResourceSubscriptionID = networkResourceSubscriptionID
+			subscriptionID = networkResourceSubscriptionID
+		} else {
 			az.NetworkResourceSubscriptionID = ""
-			c.expected = fmt.Sprintf(
-				expectedResourceIDTemplate,
-				az.SubscriptionID,
-				c.resourceGroupName,
-				c.loadBalancerName,
-				c.subResourceName)
 		}
+		c.expected = fmt.Sprintf(
+			expectedResourceIDTemplate,
+			subscriptionID,
+			c.resourceGroupName,
+			c.loadBalancerName,
+			c.subResourceName)
 		subResourceID := getLoadBalancerSubResourceID(c.loadBalancerName, c.resourceGroupName, c.subResourceName)
 		assert.Equal(t, c.expected, subResourceID, c.description)
 	}