Merge pull request #1233 from jwmay2012/skip-custom-subnet-delete

Skip deletion of Subnet when created ouside CAPG.

Author: k8s-ci-robot

cloud/services/compute/subnets/reconcile.go

@@ -19,14 +19,16 @@ package subnets
 import (
 	"context"
 
+	infrav1 "sigs.k8s.io/cluster-api-provider-gcp/api/v1beta1"
+
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"google.golang.org/api/compute/v1"
 
 	"sigs.k8s.io/cluster-api-provider-gcp/cloud/gcperrors"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-// Reconcile reconcile cluster network components.
+// Reconcile reconciles cluster network components.
 func (s *Service) Reconcile(ctx context.Context) error {
 	logger := log.FromContext(ctx)
 	logger.Info("Reconciling subnetwork resources")
@@ -43,13 +45,31 @@ func (s *Service) Reconcile(ctx context.Context) error {
 func (s *Service) Delete(ctx context.Context) error {
 	logger := log.FromContext(ctx)
 	for _, subnetSpec := range s.scope.SubnetSpecs() {
-		logger.V(2).Info("Deleting a subnet", "name", subnetSpec.Name)
 		subnetKey := meta.RegionalKey(subnetSpec.Name, s.getSubnetRegion(subnetSpec))
-		err := s.subnets.Delete(ctx, subnetKey)
-		if err != nil && !gcperrors.IsNotFound(err) {
-			logger.Error(err, "Error deleting subnet", "name", subnetSpec.Name)
+		logger.V(2).Info("Looking for subnet before deleting it", "name", subnetSpec.Name)
+		subnet, err := s.subnets.Get(ctx, subnetKey)
+		if err != nil {
+			if gcperrors.IsNotFound(err) {
+				continue
+			}
+			logger.Error(err, "Error getting subnet", "name", subnetSpec.Name)
 			return err
 		}
+
+		// Skip delete if subnet was not created by CAPG.
+		// If subnet description is not set by the Spec, or by our default value, then assume it was created externally.
+		if subnet.Description != infrav1.ClusterTagKey(s.scope.Name()) && (subnetSpec.Description == "" || subnet.Description != subnetSpec.Description) {
+			logger.V(2).Info("Skipping subnet deletion as it was created outside of Cluster API", "name", subnetSpec.Name)
+			return nil
+		}
+
+		logger.V(2).Info("Deleting a subnet", "name", subnetSpec.Name)
+		if err := s.subnets.Delete(ctx, subnetKey); err != nil {
+			if !gcperrors.IsNotFound(err) {
+				logger.Error(err, "Error deleting subnet", "name", subnetSpec.Name)
+				return err
+			}
+		}
 	}
 
 	return nil

cloud/services/compute/subnets/reconcile_test.go

@@ -61,10 +61,11 @@ var fakeGCPCluster = &infrav1.GCPCluster{
 		Network: infrav1.NetworkSpec{
 			Subnets: infrav1.Subnets{
 				infrav1.SubnetSpec{
-					Name:      "workers",
-					CidrBlock: "10.0.0.1/28",
-					Region:    "us-central1",
-					Purpose:   ptr.To[string]("INTERNAL_HTTPS_LOAD_BALANCER"),
+					Name:        "workers",
+					CidrBlock:   "10.0.0.1/28",
+					Region:      "us-central1",
+					Purpose:     ptr.To[string]("INTERNAL_HTTPS_LOAD_BALANCER"),
+					Description: ptr.To[string](infrav1.ClusterTagKey(fakeCluster.Name)),
 				},
 			},
 		},
@@ -212,9 +213,26 @@ func TestService_Delete(t *testing.T) {
 				DeleteError: map[meta.Key]error{
 					*meta.RegionalKey(fakeGCPCluster.Spec.Network.Subnets[0].Name, fakeGCPCluster.Spec.Region): &googleapi.Error{Code: http.StatusBadRequest},
 				},
+				Objects: map[meta.Key]*cloud.MockSubnetworksObj{
+					*meta.RegionalKey(fakeGCPCluster.Spec.Network.Subnets[0].Name, fakeGCPCluster.Spec.Region): {Obj: compute.Subnetwork{Description: infrav1.ClusterTagKey(fakeCluster.Name)}},
+				},
 			},
 			wantErr: true,
 		},
+		{
+			name:  "subnet not created by CAPI, should not try to delete it",
+			scope: func() Scope { return clusterScope },
+			mockSubnetworks: &cloud.MockSubnetworks{
+				ProjectRouter: &cloud.SingleProjectRouter{ID: "my-proj"},
+				DeleteError: map[meta.Key]error{
+					*meta.RegionalKey(fakeGCPCluster.Spec.Network.Subnets[0].Name, fakeGCPCluster.Spec.Region): &googleapi.Error{Code: http.StatusBadRequest},
+				},
+				Objects: map[meta.Key]*cloud.MockSubnetworksObj{
+					*meta.RegionalKey(fakeGCPCluster.Spec.Network.Subnets[0].Name, fakeGCPCluster.Spec.Region): {Obj: compute.Subnetwork{Description: "my-custom-subnet-description"}},
+				},
+			},
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

test/e2e/data/cni/calico/calico.yaml

@@ -468,7 +468,7 @@ spec:
               numAllowedLocalASNumbers:
                 description: Maximum number of local AS numbers that are allowed in
                   the AS path for received routes. This removes BGP loop prevention
-                  and should only be used if absolutely necesssary.
+                  and should only be used if absolutely necessary.
                 format: int32
                 type: integer
               password: