pkg/storage/azure: support vnet and subnet discovery

Author: flavianmissi

pkg/storage/azure/azure.go

@@ -550,22 +550,11 @@ func (d *driver) assurePrivateAccount(cfg *Azure, infra *configv1.Infrastructure
 		// user did not request private storage account setup - skip.
 		return "", nil
 	}
-	if d.Config.NetworkAccess.Internal.VNetName == "" || d.Config.NetworkAccess.Internal.SubnetName == "" {
-		return "", fmt.Errorf("both vnetName and subnetName are required to setup a private storage account")
-	}
-
-	var privateEndpointName string
-	if d.Config.NetworkAccess.Internal != nil && d.Config.NetworkAccess.Internal.PrivateEndpointName != "" {
-		privateEndpointName = d.Config.NetworkAccess.Internal.PrivateEndpointName
-	} else {
-		privateEndpointName = generateAccountName(infra.Status.InfrastructureName)
-	}
 
 	environment, err := getEnvironmentByName(d.Config.CloudName)
 	if err != nil {
 		return "", err
 	}
-
 	azclient, err := azureclient.New(&azureclient.Options{
 		Environment:        environment,
 		TenantID:           cfg.TenantID,
@@ -580,20 +569,49 @@ func (d *driver) assurePrivateAccount(cfg *Azure, infra *configv1.Infrastructure
 		return "", err
 	}
 
+	internalConfig := d.Config.NetworkAccess.Internal
+	if internalConfig == nil {
+		// avoid nil pointer checks everywhere - this will happen when the user
+		// wants the operator to discover vnet and subnet names.
+		internalConfig = &imageregistryv1.AzureNetworkAccessInternal{}
+	}
+
+	privateEndpointName := internalConfig.PrivateEndpointName
+	if privateEndpointName == "" {
+		privateEndpointName = generateAccountName(infra.Status.InfrastructureName)
+	}
+
 	// the last step in this function is to disable public network for the
 	// storage account - if we already did that, then none of the steps
 	// below need to be executed.
 	if azclient.IsStorageAccountPrivate(d.Context, accountName) {
 		return privateEndpointName, nil
 	}
 
+	if internalConfig.VNetName == "" {
+		tagKey := fmt.Sprintf("kubernetes.io_cluster.%s", infra.Status.InfrastructureName)
+		vnet, err := azclient.GetVNetByTag(d.Context, tagKey, "owned", "shared")
+		if err != nil {
+			return "", fmt.Errorf("failed to discover vnet name, please provide network details manually: %q", err)
+		}
+		internalConfig.VNetName = *vnet.Name
+	}
+
+	if internalConfig.SubnetName == "" {
+		subnet, err := azclient.GetSubnetsByVNet(d.Context, internalConfig.VNetName)
+		if err != nil {
+			return "", fmt.Errorf("failed to discover subnet name, please provide network details manually: %q", err)
+		}
+		internalConfig.SubnetName = *subnet.Name
+	}
+
 	klog.V(3).Infof("configuring private endpoint %q for storage account...", privateEndpointName)
 	pe, err := azclient.CreatePrivateEndpoint(
 		d.Context,
 		&azureclient.PrivateEndpointCreateOptions{
 			Location:            cfg.Region,
-			VNetName:            d.Config.NetworkAccess.Internal.VNetName,
-			SubnetName:          d.Config.NetworkAccess.Internal.SubnetName,
+			VNetName:            internalConfig.VNetName,
+			SubnetName:          internalConfig.SubnetName,
 			PrivateEndpointName: privateEndpointName,
 			StorageAccountName:  accountName,
 		},
@@ -605,7 +623,7 @@ func (d *driver) assurePrivateAccount(cfg *Azure, infra *configv1.Infrastructure
 
 	klog.V(3).Info("configuring private DNS...")
 	if err := azclient.ConfigurePrivateDNS(
-		d.Context, pe, d.Config.NetworkAccess.Internal.VNetName, accountName,
+		d.Context, pe, internalConfig.VNetName, accountName,
 	); err != nil {
 		return privateEndpointName, err
 	}
@@ -621,6 +639,8 @@ func (d *driver) assurePrivateAccount(cfg *Azure, infra *configv1.Infrastructure
 		accountName, privateEndpointName,
 	)
 
+	d.Config.NetworkAccess.Internal = internalConfig
+
 	return privateEndpointName, nil
 }
 

pkg/storage/azure/azureclient/azureclient.go

@@ -138,6 +138,59 @@ func (c *Client) getStorageAccount(ctx context.Context, accountName string) (arm
 	return resp.Account, nil
 }
 
+func (c *Client) GetVNetByTag(ctx context.Context, tagKey string, tagValues ...string) (armnetwork.VirtualNetwork, error) {
+	client, err := armnetwork.NewVirtualNetworksClient(c.subscriptionID, c.creds, c.clientOpts)
+	if err != nil {
+		return armnetwork.VirtualNetwork{}, fmt.Errorf("failed to create accounts client: %q", err)
+	}
+
+	pager := client.NewListPager(c.resourceGroupName, nil)
+	for pager.More() {
+		page, err := pager.NextPage(ctx)
+		if err != nil {
+			return armnetwork.VirtualNetwork{}, err
+		}
+		for _, vnet := range page.Value {
+			tag, ok := vnet.Tags[tagKey]
+			if !ok {
+				continue
+			}
+			for _, tagValue := range tagValues {
+				if *tag == tagValue {
+					return *vnet, nil
+				}
+			}
+		}
+	}
+
+	return armnetwork.VirtualNetwork{}, fmt.Errorf("vnet with tag '%s: %v' not found", tagKey, tagValues)
+}
+
+func (c *Client) GetSubnetsByVNet(ctx context.Context, vnetName string) (armnetwork.Subnet, error) {
+	client, err := armnetwork.NewSubnetsClient(c.subscriptionID, c.creds, c.clientOpts)
+	if err != nil {
+		return armnetwork.Subnet{}, fmt.Errorf("failed to create subnets client: %q", err)
+	}
+
+	pager := client.NewListPager(c.resourceGroupName, vnetName, nil)
+	for pager.More() {
+		page, err := pager.NextPage(ctx)
+		if err != nil {
+			return armnetwork.Subnet{}, err
+		}
+		for _, subnet := range page.Value {
+			// should we match the subnet name with the string "worker"?
+			// does it even matter? (don't think so)
+			//
+			// return the first subnet.
+			// unless each subnet in the cluster has strict access groups it
+			// doesn't matter which subnet we choose (worker/master).
+			return *subnet, nil
+		}
+	}
+	return armnetwork.Subnet{}, fmt.Errorf("no subnets found on vnet %q", vnetName)
+}
+
 func (c *Client) UpdateStorageAccountNetworkAccess(ctx context.Context, accountName string, allowPublicAccess bool) error {
 	client, err := armstorage.NewAccountsClient(c.subscriptionID, c.creds, c.clientOpts)
 	if err != nil {

test/e2e/azure_test.go

@@ -237,6 +237,94 @@ func TestAzurePrivateStorageAccount(t *testing.T) {
 	}
 }
 
+func TestPrivateStorageAccountVNetSubnetDiscovery(t *testing.T) {
+	ctx := context.Background()
+
+	kcfg, err := regopclient.GetConfig()
+	if err != nil {
+		t.Fatalf("Error building kubeconfig: %s", err)
+	}
+
+	newMockLister, err := listers.NewMockLister(kcfg)
+	if err != nil {
+		t.Fatalf("unable to create mock lister: %v", err)
+	}
+
+	mockLister, err := newMockLister.GetListers()
+	if err != nil {
+		t.Fatalf("unable to get listers from mock lister: %v", err)
+	}
+
+	infra, err := util.GetInfrastructure(mockLister.StorageListers.Infrastructures)
+	if err != nil {
+		t.Fatalf("unable to get install configuration: %v", err)
+	}
+
+	if infra.Status.PlatformStatus.Type != configapiv1.AzurePlatformType {
+		t.Skip("skipping on non-Azure platform")
+	}
+
+	te := framework.Setup(t)
+	defer framework.TeardownImageRegistry(te)
+
+	framework.DeployImageRegistry(te, nil)
+	framework.WaitUntilImageRegistryIsAvailable(te)
+	framework.EnsureInternalRegistryHostnameIsSet(te)
+	framework.EnsureClusterOperatorStatusIsSet(te)
+	framework.EnsureOperatorIsNotHotLooping(te)
+
+	patch := fmt.Sprintf(
+		`{"spec": {"storage": {"azure": {"networkAccess": {"type": "%s"}}}}}`,
+		imageregistryv1.AzureNetworkAccessTypeInternal,
+	)
+	if _, err = te.Client().Configs().Patch(
+		ctx,
+		defaults.ImageRegistryResourceName,
+		types.MergePatchType,
+		[]byte(patch),
+		metav1.PatchOptions{},
+	); err != nil {
+		t.Errorf("unable to patch image registry custom resource: %#v", err)
+	}
+
+	azureConfig := &imageregistryv1.ImageRegistryConfigStorageAzure{}
+	err = wait.PollUntilContextTimeout(ctx, time.Second, framework.AsyncOperationTimeout, true,
+		func(ctx context.Context) (stop bool, err error) {
+			cr, err := te.Client().Configs().Get(
+				ctx,
+				defaults.ImageRegistryResourceName,
+				metav1.GetOptions{},
+			)
+			if err != nil {
+				t.Logf(
+					"unable to get custom resource %s/%s: %#v",
+					defaults.ImageRegistryOperatorNamespace,
+					defaults.ImageRegistryResourceName,
+					err,
+				)
+				return false, nil
+			}
+			if cr.Spec.Storage.Azure.NetworkAccess.Internal == nil || cr.Spec.Storage.Azure.NetworkAccess.Internal.PrivateEndpointName == "" {
+				// operator has not yet set the name - keep waiting
+				return false, nil
+			}
+			azureConfig = cr.Spec.Storage.Azure
+			t.Logf("PrivateEndpointName is %q", azureConfig.NetworkAccess.Internal.PrivateEndpointName)
+			return true, nil
+		},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if azureConfig.NetworkAccess.Internal.VNetName == "" {
+		t.Fatal("expected vnet name to be set in operator config but it was empty")
+	}
+	if azureConfig.NetworkAccess.Internal.SubnetName == "" {
+		t.Fatal("expected subnet name to be set in operator config but it was empty")
+	}
+}
+
 func getEnvironmentByName(name string) (autorestazure.Environment, error) {
 	if name == "" {
 		return autorestazure.PublicCloud, nil