[release-v1.19] Automated cherry pick of #331: Fix config validation of NAT IP addresses

pkg/controller/infrastructure/configvalidator.go

@@ -17,6 +17,7 @@ package infrastructure
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	api "github.com/gardener/gardener-extension-provider-gcp/pkg/apis/gcp"
 	"github.com/gardener/gardener-extension-provider-gcp/pkg/apis/gcp/helper"
@@ -66,35 +67,39 @@ func (c *configValidator) Validate(ctx context.Context, infra *extensionsv1alpha
 	}
 
 	// Validate infrastructure config
-	if config.Networks.CloudNAT != nil {
-		logger.Info("Validating infrastructure networks.cloudNAT configuration")
-		allErrs = append(allErrs, c.validateCloudNAT(ctx, computeClient, infra.Spec.Region, config.Networks.CloudNAT, field.NewPath("networks", "cloudNAT"))...)
-	}
+	logger.Info("Validating infrastructure networks configuration")
+	allErrs = append(allErrs, c.validateNetworks(ctx, computeClient, infra.Namespace, infra.Spec.Region, config.Networks, field.NewPath("networks"))...)
 
 	return allErrs
 }
 
-func (c *configValidator) validateCloudNAT(ctx context.Context, computeClient gcpclient.ComputeClient, region string, cloudNAT *api.CloudNAT, fldPath *field.Path) field.ErrorList {
+func (c *configValidator) validateNetworks(ctx context.Context, computeClient gcpclient.ComputeClient, clusterName, region string, networks api.NetworkConfig, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if len(cloudNAT.NatIPNames) == 0 {
+	if networks.CloudNAT == nil || len(networks.CloudNAT.NatIPNames) == 0 {
 		return allErrs
 	}
 
-	// Get external IP addresses mapped to whether they are available or not
+	// Get external IP addresses mapped to the names of their users
 	externalAddresses, err := computeClient.GetExternalAddresses(ctx, region)
 	if err != nil {
 		allErrs = append(allErrs, field.InternalError(fldPath, fmt.Errorf("could not get external IP addresses: %w", err)))
 		return allErrs
 	}
 
+	cloudRouterName := clusterName + "-cloud-router"
+	if networks.VPC != nil && networks.VPC.CloudRouter != nil && len(networks.VPC.CloudRouter.Name) > 0 {
+		cloudRouterName = networks.VPC.CloudRouter.Name
+	}
+
 	// Check whether each specified NAT IP name exists and is available
-	for i, natIP := range cloudNAT.NatIPNames {
-		natIPNamePath := fldPath.Child("natIPNames").Index(i).Child("name")
-		if available, ok := externalAddresses[natIP.Name]; !ok {
+	for i, natIP := range networks.CloudNAT.NatIPNames {
+		natIPNamePath := fldPath.Child("cloudNAT", "natIPNames").Index(i).Child("name")
+		if userNames, ok := externalAddresses[natIP.Name]; !ok {
 			allErrs = append(allErrs, field.NotFound(natIPNamePath, natIP.Name))
-		} else if !available {
-			allErrs = append(allErrs, field.Invalid(natIPNamePath, natIP.Name, "external IP address is already in use"))
+		} else if len(userNames) > 1 || len(userNames) == 1 && userNames[0] != cloudRouterName {
+			allErrs = append(allErrs, field.Invalid(natIPNamePath, natIP.Name,
+				fmt.Sprintf("external IP address is already in use by %s", strings.Join(userNames, ","))))
 		}
 	}
 

pkg/controller/infrastructure/configvalidator_test.go

@@ -112,9 +112,18 @@ var _ = Describe("ConfigValidator", func() {
 			gcpClientFactory.EXPECT().NewComputeClient(ctx, c, infra.Spec.SecretRef).Return(gcpComputeClient, nil)
 		})
 
+		It("should succeed if there are no NAT IP names", func() {
+			infra.Spec.ProviderConfig.Raw = encode(&apisgcp.InfrastructureConfig{
+				Networks: apisgcp.NetworkConfig{},
+			})
+
+			errorList := cv.Validate(ctx, infra)
+			Expect(errorList).To(BeEmpty())
+		})
+
 		It("should forbid NAT IP names that don't exist or are not available", func() {
-			gcpComputeClient.EXPECT().GetExternalAddresses(ctx, region).Return(map[string]bool{
-				"test2": false,
+			gcpComputeClient.EXPECT().GetExternalAddresses(ctx, region).Return(map[string][]string{
+				"test2": {"foo"},
 			}, nil)
 
 			errorList := cv.Validate(ctx, infra)
@@ -124,14 +133,38 @@ var _ = Describe("ConfigValidator", func() {
 			}, Fields{
 				"Type":   Equal(field.ErrorTypeInvalid),
 				"Field":  Equal("networks.cloudNAT.natIPNames[1].name"),
-				"Detail": Equal("external IP address is already in use"),
+				"Detail": Equal("external IP address is already in use by foo"),
 			}))
 		})
 
-		It("should allow NAT IP names that exist and are available", func() {
-			gcpComputeClient.EXPECT().GetExternalAddresses(ctx, region).Return(map[string]bool{
-				"test1": true,
-				"test2": true,
+		It("should allow NAT IP names that exist and are available, or in use by the default cloud router", func() {
+			gcpComputeClient.EXPECT().GetExternalAddresses(ctx, region).Return(map[string][]string{
+				"test1": nil,
+				"test2": {namespace + "-cloud-router"},
+			}, nil)
+
+			errorList := cv.Validate(ctx, infra)
+			Expect(errorList).To(BeEmpty())
+		})
+
+		It("should allow NAT IP names that exist and are in use by the configured cloud router", func() {
+			infra.Spec.ProviderConfig.Raw = encode(&apisgcp.InfrastructureConfig{
+				Networks: apisgcp.NetworkConfig{
+					VPC: &apisgcp.VPC{
+						Name: "test-vpc",
+						CloudRouter: &apisgcp.CloudRouter{
+							Name: "test-cloud-router",
+						},
+					},
+					CloudNAT: &apisgcp.CloudNAT{
+						NatIPNames: []apisgcp.NatIPName{
+							{Name: "test1"},
+						},
+					},
+				},
+			})
+			gcpComputeClient.EXPECT().GetExternalAddresses(ctx, region).Return(map[string][]string{
+				"test1": {"test-cloud-router"},
 			}, nil)
 
 			errorList := cv.Validate(ctx, infra)
@@ -144,7 +177,7 @@ var _ = Describe("ConfigValidator", func() {
 			errorList := cv.Validate(ctx, infra)
 			Expect(errorList).To(ConsistOfFields(Fields{
 				"Type":   Equal(field.ErrorTypeInternal),
-				"Field":  Equal("networks.cloudNAT"),
+				"Field":  Equal("networks"),
 				"Detail": Equal("could not get external IP addresses: test"),
 			}))
 		})

pkg/gcp/client/compute.go

@@ -16,6 +16,7 @@ package client
 
 import (
 	"context"
+	"strings"
 
 	"github.com/gardener/gardener-extension-provider-gcp/pkg/gcp"
 
@@ -29,8 +30,8 @@ import (
 
 // ComputeClient is an interface which must be implemented by GCP compute clients.
 type ComputeClient interface {
-	// GetExternalAddresses returns a list of all external IP addresses mapped to whether they are available for use or not.
-	GetExternalAddresses(ctx context.Context, region string) (map[string]bool, error)
+	// GetExternalAddresses returns a list of all external IP addresses mapped to the names of their users.
+	GetExternalAddresses(ctx context.Context, region string) (map[string][]string, error)
 }
 
 type computeClient struct {
@@ -65,13 +66,20 @@ func NewComputeClientFromSecretRef(ctx context.Context, c client.Client, secretR
 	return newComputeClient(ctx, serviceAccount)
 }
 
-// GetExternalAddresses returns a list of all external IP addresses mapped to whether they are available for use or not.
-func (s *computeClient) GetExternalAddresses(ctx context.Context, region string) (map[string]bool, error) {
-	addresses := make(map[string]bool)
+// GetExternalAddresses returns a list of all external IP addresses mapped to the names of their users.
+func (s *computeClient) GetExternalAddresses(ctx context.Context, region string) (map[string][]string, error) {
+	addresses := make(map[string][]string)
 	if err := s.service.Addresses.List(s.projectID, region).Pages(ctx, func(resp *compute.AddressList) error {
 		for _, address := range resp.Items {
 			if address.AddressType == "EXTERNAL" {
-				addresses[address.Name] = address.Status == "RESERVED"
+				var userNames []string
+				if address.Status == "IN_USE" {
+					for _, user := range address.Users {
+						parts := strings.Split(user, "/")
+						userNames = append(userNames, parts[len(parts)-1])
+					}
+				}
+				addresses[address.Name] = userNames
 			}
 		}
 		return nil