feat: validation enforcing vnet guid is set for overlay cni clusters (#…

…675)

* feat: validation enforcing vnet guid is set for overlay cni clusters

* fix: deps

* test: adding test for the azurecni with overlay guid validation

* test: adding validation to validate shape looks like a guid, leveraged google uuid lib based on this benchmark test https://gist.github.com/mattes/69a4ab7027b9e8ee952b5843e7ca6955

* ci: golangci-lint run

* deps: moving google uuid from an indirect dependency to a direct one

* refactor: updating options to use a proper guid

* test: network-plugin-mode falls back to overlay when unset which triggers vnet guid validation, this modifies that test to pass in an empty string

* ci: golangci-lint run

* test: testing happy path of properly configured vnet guid

* fix: validate all vnet guids that are passed in even if we don't use the value

* test: resolve merge conflic

---------

Co-authored-by: Alex Leites <18728999+tallaxes@users.noreply.github.com>

go.mod

@@ -29,6 +29,7 @@ require (
 	github.com/go-openapi/validate v0.24.0
 	github.com/go-playground/validator/v10 v10.25.0
 	github.com/google/go-cmp v0.6.0
+	github.com/google/uuid v1.6.0
 	github.com/imdario/mergo v0.3.16
 	github.com/jongio/azidext/go/azidext v0.5.0
 	github.com/mitchellh/hashstructure/v2 v2.0.2
@@ -107,7 +108,6 @@ require (
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20241210010833-40e02aabc2ad // indirect
-	github.com/google/uuid v1.6.0 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.24.0 // indirect
 	github.com/hashicorp/golang-lru v1.0.2 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect

pkg/operator/operator.go

@@ -40,13 +40,9 @@ import (
 	"sigs.k8s.io/karpenter/pkg/operator/injection"
 	karpenteroptions "sigs.k8s.io/karpenter/pkg/operator/options"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork"
-
-	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	webhooksalt "github.com/Azure/karpenter-provider-azure/pkg/alt/karpenter-core/pkg/webhooks"
 	"github.com/Azure/karpenter-provider-azure/pkg/auth"
 	azurecache "github.com/Azure/karpenter-provider-azure/pkg/cache"
-	"github.com/Azure/karpenter-provider-azure/pkg/consts"
 
 	"github.com/Azure/karpenter-provider-azure/pkg/operator/options"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/imagefamily"
@@ -55,8 +51,6 @@ import (
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/launchtemplate"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/loadbalancer"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/pricing"
-	"github.com/Azure/karpenter-provider-azure/pkg/utils"
-	armopts "github.com/Azure/karpenter-provider-azure/pkg/utils/opts"
 	"sigs.k8s.io/karpenter/pkg/operator"
 )
 
@@ -89,12 +83,6 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont
 	azClient, err := instance.CreateAZClient(ctx, azConfig)
 	lo.Must0(err, "creating Azure client")
 
-	if options.FromContext(ctx).VnetGUID == "" && options.FromContext(ctx).NetworkPluginMode == consts.NetworkPluginModeOverlay {
-		vnetGUID, err := getVnetGUID(azConfig, options.FromContext(ctx).SubnetID)
-		lo.Must0(err, "getting VNET GUID")
-		options.FromContext(ctx).VnetGUID = vnetGUID
-	}
-
 	unavailableOfferingsCache := azurecache.NewUnavailableOfferings()
 	pricingProvider := pricing.NewProvider(
 		ctx,
@@ -235,28 +223,3 @@ func getCABundle(restConfig *rest.Config) (*string, error) {
 	}
 	return ptr.String(base64.StdEncoding.EncodeToString(transportConfig.TLS.CAData)), nil
 }
-
-func getVnetGUID(cfg *auth.Config, subnetID string) (string, error) {
-	creds, err := azidentity.NewDefaultAzureCredential(nil)
-	if err != nil {
-		return "", err
-	}
-	opts := armopts.DefaultArmOpts()
-	vnetClient, err := armnetwork.NewVirtualNetworksClient(cfg.SubscriptionID, creds, opts)
-	if err != nil {
-		return "", err
-	}
-
-	subnetParts, err := utils.GetVnetSubnetIDComponents(subnetID)
-	if err != nil {
-		return "", err
-	}
-	vnet, err := vnetClient.Get(context.Background(), subnetParts.ResourceGroupName, subnetParts.VNetName, nil)
-	if err != nil {
-		return "", err
-	}
-	if vnet.Properties == nil || vnet.Properties.ResourceGUID == nil {
-		return "", fmt.Errorf("vnet %s does not have a resource GUID", subnetParts.VNetName)
-	}
-	return *vnet.Properties.ResourceGUID, nil
-}

pkg/operator/options/options_validation.go

@@ -23,13 +23,15 @@ import (
 	"github.com/Azure/karpenter-provider-azure/pkg/consts"
 	"github.com/Azure/karpenter-provider-azure/pkg/utils"
 	"github.com/go-playground/validator/v10"
+	"github.com/google/uuid"
 	"go.uber.org/multierr"
 )
 
 func (o Options) Validate() error {
 	validate := validator.New()
 	return multierr.Combine(
 		o.validateRequiredFields(),
+		o.validateVNETGUID(),
 		o.validateEndpoint(),
 		o.validateNetworkingOptions(),
 		o.validateVMMemoryOverheadPercent(),
@@ -39,6 +41,16 @@ func (o Options) Validate() error {
 	)
 }
 
+func (o Options) validateVNETGUID() error {
+	if o.VnetGUID != "" && uuid.Validate(o.VnetGUID) != nil {
+		return fmt.Errorf("vnet-guid %s is malformed", o.VnetGUID)
+	}
+	if o.isAzureCNIWithOverlay() && o.VnetGUID == "" {
+		return fmt.Errorf("vnet-guid cannot be empty for AzureCNI clusters with networkPluginMode overlay")
+	}
+	return nil
+}
+
 func (o Options) validateNetworkingOptions() error {
 	if o.NetworkPlugin != consts.NetworkPluginAzure && o.NetworkPlugin != consts.NetworkPluginNone {
 		return fmt.Errorf("network-plugin %v is invalid. network-plugin must equal 'azure' or 'none'", o.NetworkPlugin)
@@ -56,6 +68,10 @@ func (o Options) validateNetworkingOptions() error {
 	return nil
 }
 
+func (o Options) isAzureCNIWithOverlay() bool {
+	return o.NetworkPlugin == consts.NetworkPluginAzure && o.NetworkPluginMode == consts.NetworkPluginModeOverlay
+}
+
 func (o Options) validateVnetSubnetID() error {
 	_, err := utils.GetVnetSubnetIDComponents(o.SubnetID)
 	if err != nil {

pkg/operator/options/suite_test.go

@@ -55,6 +55,7 @@ var _ = Describe("Options", func() {
 		"NODE_IDENTITIES",
 		"PROVISION_MODE",
 		"NODEBOOTSTRAPPING_SERVER_URL",
+		"VNET_GUID",
 	}
 
 	var fs *coreoptions.FlagSet
@@ -99,6 +100,7 @@ var _ = Describe("Options", func() {
 			os.Setenv("VNET_SUBNET_ID", "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/sillygeese/providers/Microsoft.Network/virtualNetworks/karpentervnet/subnets/karpentersub")
 			os.Setenv("PROVISION_MODE", "bootstrappingclient")
 			os.Setenv("NODEBOOTSTRAPPING_SERVER_URL", "https://nodebootstrapping-server-url")
+			os.Setenv("VNET_GUID", "a519e60a-cac0-40b2-b883-084477fe6f5c")
 			fs = &coreoptions.FlagSet{
 				FlagSet: flag.NewFlagSet("karpenter", flag.ContinueOnError),
 			}
@@ -118,10 +120,41 @@ var _ = Describe("Options", func() {
 				NodeIdentities:                 []string{"/subscriptions/1234/resourceGroups/mcrg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/envid1", "/subscriptions/1234/resourceGroups/mcrg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/envid2"},
 				ProvisionMode:                  lo.ToPtr("bootstrappingclient"),
 				NodeBootstrappingServerURL:     lo.ToPtr("https://nodebootstrapping-server-url"),
+				VnetGUID:                       lo.ToPtr("a519e60a-cac0-40b2-b883-084477fe6f5c"),
 			}))
 		})
 	})
 	Context("Validation", func() {
+		It("should fail when vnet guid is not a uuid", func() {
+			errMsg := "vnet-guid null is malformed"
+			err := opts.Parse(
+				fs,
+				"--cluster-name", "my-name",
+				"--cluster-endpoint", "https://karpenter-000000000000.hcp.westus2.staging.azmk8s.io",
+				"--kubelet-bootstrap-token", "flag-bootstrap-token",
+				"--ssh-public-key", "flag-ssh-public-key",
+				"--vm-memory-overhead-percent", "-0.01",
+				"--network-plugin", "azure",
+				"--network-plugin-mode", "overlay",
+				"--vnet-guid", "null", // sometimes output of jq can produce null or some other data, we should enforce that the vnet guid passed in at least looks like a uuid
+			)
+			Expect(err).To(MatchError(ContainSubstring(errMsg)))
+		})
+
+		It("should fail when vnet guid is empty for azure cni with overlay clusters", func() {
+			errMsg := "vnet-guid cannot be empty for AzureCNI clusters with networkPluginMode overlay"
+			err := opts.Parse(
+				fs,
+				"--cluster-name", "my-name",
+				"--cluster-endpoint", "https://karpenter-000000000000.hcp.westus2.staging.azmk8s.io",
+				"--kubelet-bootstrap-token", "flag-bootstrap-token",
+				"--ssh-public-key", "flag-ssh-public-key",
+				"--vm-memory-overhead-percent", "-0.01",
+				"--network-plugin", "azure",
+				"--network-plugin-mode", "overlay",
+			)
+			Expect(err).To(MatchError(ContainSubstring(errMsg)))
+		})
 		It("should fail when network-plugin-mode is invalid", func() {
 			typo := "overlaay"
 			errMsg := fmt.Sprintf("network-plugin-mode %v is invalid. network-plugin-mode must equal 'overlay' or ''", typo)
@@ -263,6 +296,7 @@ var _ = Describe("Options", func() {
 				"--ssh-public-key", "flag-ssh-public-key",
 				"--vnet-subnet-id", "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/sillygeese/providers/Microsoft.Network/virtualNetworks/karpentervnet/subnets/karpentersub",
 				"--network-plugin", "azure",
+				"--network-plugin-mode", "",
 			)
 			Expect(err).ToNot(HaveOccurred())
 		})
@@ -279,6 +313,21 @@ var _ = Describe("Options", func() {
 			)
 			Expect(err).ToNot(HaveOccurred())
 		})
+		It("should succeed when azure-cni with overlay is configured with the right options", func() {
+			err := opts.Parse(
+				fs,
+				"--cluster-name", "my-name",
+				"--cluster-endpoint", "https://karpenter-000000000000.hcp.westus2.staging.azmk8s.io",
+				"--kubelet-bootstrap-token", "flag-bootstrap-token",
+				"--ssh-public-key", "flag-ssh-public-key",
+				"--vnet-subnet-id", "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/sillygeese/providers/Microsoft.Network/virtualNetworks/karpentervnet/subnets/karpentersub",
+				"--network-plugin", "azure",
+				"--network-plugin-mode", "overlay",
+				"--vnet-guid", "a519e60a-cac0-40b2-b883-084477fe6f5c",
+			)
+			Expect(err).ToNot(HaveOccurred())
+
+		})
 		It("should fail validation when ProvisionMode is not valid", func() {
 			err := opts.Parse(
 				fs,

pkg/providers/instancetype/suite_test.go

@@ -161,7 +161,7 @@ var _ = Describe("InstanceType Provider", func() {
 			Expect(decodedString).To(SatisfyAll(
 				ContainSubstring("kubernetes.azure.com/ebpf-dataplane=cilium"),
 				ContainSubstring("kubernetes.azure.com/network-subnet=karpentersub"),
-				ContainSubstring("kubernetes.azure.com/nodenetwork-vnetguid=test-vnet-guid"),
+				ContainSubstring("kubernetes.azure.com/nodenetwork-vnetguid=a519e60a-cac0-40b2-b883-084477fe6f5c"),
 				ContainSubstring("kubernetes.azure.com/podnetwork-type=overlay"),
 				ContainSubstring("kubernetes.azure.com/azure-cni-overlay=true"),
 			))
@@ -522,7 +522,7 @@ var _ = Describe("InstanceType Provider", func() {
 			// Since the network plugin is not "azure" it should not include the following kubeletLabels
 			Expect(customData).To(Not(SatisfyAny(
 				ContainSubstring("kubernetes.azure.com/network-subnet=karpentersub"),
-				ContainSubstring("kubernetes.azure.com/nodenetwork-vnetguid=test-vnet-guid"),
+				ContainSubstring("kubernetes.azure.com/nodenetwork-vnetguid=a519e60a-cac0-40b2-b883-084477fe6f5c"),
 				ContainSubstring("kubernetes.azure.com/podnetwork-type=overlay"),
 			)))
 		})
@@ -1255,7 +1255,7 @@ var _ = Describe("InstanceType Provider", func() {
 			sets.New(
 				"kubernetes.azure.com/azure-cni-overlay=true",
 				"kubernetes.azure.com/network-subnet=karpentersub",
-				"kubernetes.azure.com/nodenetwork-vnetguid=test-vnet-guid",
+				"kubernetes.azure.com/nodenetwork-vnetguid=a519e60a-cac0-40b2-b883-084477fe6f5c",
 				"kubernetes.azure.com/podnetwork-type=overlay",
 			)),
 		Entry("Azure CNI w Overlay w Cilium",
@@ -1264,7 +1264,7 @@ var _ = Describe("InstanceType Provider", func() {
 			sets.New(
 				"kubernetes.azure.com/azure-cni-overlay=true",
 				"kubernetes.azure.com/network-subnet=karpentersub",
-				"kubernetes.azure.com/nodenetwork-vnetguid=test-vnet-guid",
+				"kubernetes.azure.com/nodenetwork-vnetguid=a519e60a-cac0-40b2-b883-084477fe6f5c",
 				"kubernetes.azure.com/podnetwork-type=overlay",
 				"kubernetes.azure.com/ebpf-dataplane=cilium",
 			)),

pkg/test/options.go

@@ -64,7 +64,7 @@ func Options(overrides ...OptionsFields) *azoptions.Options {
 		NetworkPlugin:                  lo.FromPtrOr(options.NetworkPlugin, "azure"),
 		NetworkPluginMode:              lo.FromPtrOr(options.NetworkPluginMode, "overlay"),
 		NetworkPolicy:                  lo.FromPtrOr(options.NetworkPolicy, "cilium"),
-		VnetGUID:                       lo.FromPtrOr(options.VnetGUID, "test-vnet-guid"),
+		VnetGUID:                       lo.FromPtrOr(options.VnetGUID, "a519e60a-cac0-40b2-b883-084477fe6f5c"),
 		NetworkDataplane:               lo.FromPtrOr(options.NetworkDataplane, "cilium"),
 		VMMemoryOverheadPercent:        lo.FromPtrOr(options.VMMemoryOverheadPercent, 0.075),
 		NodeIdentities:                 options.NodeIdentities,