Merge pull request #2587 from randomvariable/reconcileVPC

Cleanup ReconcileVPC() and set id early in reconciliation.

Author: k8s-ci-robot

api/v1alpha4/awscluster_webhook.go

@@ -109,6 +109,19 @@ func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
 		)
 	}
 
+	// Modifying VPC id is not allowed because it will cause a new VPC creation if set to nil.
+	if !reflect.DeepEqual(oldC.Spec.NetworkSpec, NetworkSpec{}) &&
+		!reflect.DeepEqual(oldC.Spec.NetworkSpec.VPC, VPCSpec{}) &&
+		oldC.Spec.NetworkSpec.VPC.ID != "" {
+		if reflect.DeepEqual(r.Spec.NetworkSpec, NetworkSpec{}) ||
+			reflect.DeepEqual(r.Spec.NetworkSpec.VPC, VPCSpec{}) ||
+			oldC.Spec.NetworkSpec.VPC.ID != r.Spec.NetworkSpec.VPC.ID {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "networkSpec", "vpc", "id"),
+					r.Spec.IdentityRef, "field cannot be modified once set"))
+		}
+	}
+
 	// If a identityRef is already set, do not allow removal of it.
 	if oldC.Spec.IdentityRef != nil && r.Spec.IdentityRef == nil {
 		allErrs = append(allErrs,

api/v1alpha4/awscluster_webhook_test.go

@@ -200,6 +200,38 @@ func TestAWSCluster_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "VPC id is immutable cannot be emptied once set",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					NetworkSpec: NetworkSpec{
+						VPC: VPCSpec{ID: "managed-or-unmanaged-vpc"},
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{},
+			},
+			wantErr: true,
+		},
+		{
+			name: "VPC id is immutable, cannot be set to a different value once set",
+			oldCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					NetworkSpec: NetworkSpec{
+						VPC: VPCSpec{ID: "managed-or-unmanaged-vpc"},
+					},
+				},
+			},
+			newCluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					NetworkSpec: NetworkSpec{
+						VPC: VPCSpec{ID: "a-new-vpc"},
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/cloud/services/network/network.go

@@ -72,15 +72,22 @@ func (s *Service) ReconcileNetwork() (err error) {
 func (s *Service) DeleteNetwork() (err error) {
 	s.scope.V(2).Info("Deleting network")
 
-	// Search for a previously created and tagged VPC
-	vpc, err := s.describeVPC()
-	if err != nil {
-		if awserrors.IsNotFound(err) {
-			// If the VPC does not exist, nothing to do
-			return nil
+	vpc := &infrav1.VPCSpec{}
+	// Get VPC used for the cluster
+	if s.scope.VPC().ID != "" {
+		var err error
+		vpc, err = s.describeVPCByID()
+		if err != nil {
+			if awserrors.IsNotFound(err) {
+				// If the VPC does not exist, nothing to do
+				return nil
+			}
+			return err
 		}
-		return err
+	} else {
+		s.scope.Error(err, "non-fatal: VPC ID is missing, ")
 	}
+
 	vpc.DeepCopyInto(s.scope.VPC())
 
 	// Secondary CIDR

pkg/cloud/services/network/vpc.go

@@ -19,17 +19,16 @@ package network
 import (
 	"fmt"
 
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
-	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services"
-	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/wait"
-
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/pkg/errors"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha4"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/awserrors"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/converters"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/filter"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/wait"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/tags"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/record"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
@@ -43,55 +42,55 @@ const (
 func (s *Service) reconcileVPC() error {
 	s.scope.V(2).Info("Reconciling VPC")
 
-	vpc, err := s.describeVPC()
-	if awserrors.IsNotFound(err) { // nolint:nestif
-		// Create a new managed vpc.
-		if !conditions.Has(s.scope.InfraCluster(), infrav1.VpcReadyCondition) {
-			conditions.MarkFalse(s.scope.InfraCluster(), infrav1.VpcReadyCondition, infrav1.VpcCreationStartedReason, clusterv1.ConditionSeverityInfo, "")
+	// If the ID is not nil, VPC is either managed or unmanaged but should exist in the AWS.
+	if s.scope.VPC().ID != "" {
+		vpc, err := s.describeVPCByID()
+		if err != nil {
+			return errors.Wrap(err, ".spec.vpc.id is set but VPC resource is missing in AWS; failed to describe VPC resources. (might be in creation process)")
+		}
+
+		s.scope.VPC().CidrBlock = vpc.CidrBlock
+		s.scope.VPC().Tags = vpc.Tags
+
+		// If VPC is unmanaged, return early.
+		if vpc.IsUnmanaged(s.scope.Name()) {
+			s.scope.V(2).Info("Working on unmanaged VPC", "vpc-id", vpc.ID)
 			if err := s.scope.PatchObject(); err != nil {
-				return errors.Wrap(err, "failed to patch conditions")
+				return errors.Wrap(err, "failed to patch unmanaged VPC fields")
 			}
+			record.Eventf(s.scope.InfraCluster(), "SuccessfulSetVPCAttributes", "Set managed VPC attributes for %q", vpc.ID)
+			return nil
 		}
-		vpc, err = s.createVPC()
-		if err != nil {
-			return errors.Wrap(err, "failed to create new vpc")
-		}
-	} else if err != nil {
-		return errors.Wrap(err, "failed to describe VPCs")
-	}
 
-	// This function creates a new infrav1.VPCSpec, populates it with data from AWS, and then deep copies into the
-	// AWSCluster's VPC spec (see the DeepCopyInto lines below). This is potentially problematic, as it completely
-	// overwrites the data for the VPC spec as retrieved from the apiserver. This is a temporary band-aid to restore
-	// recently-added fields that descripe user intent and do not come from AWS resource descriptions.
-	//
-	// FIXME(ncdc): rather than copying these values from the scope to vpc, find a better way to merge AWS information
-	// with data in the scope retrieved from the apiserver. Could use something like mergo.
-	//
-	// NOTE: it may look like we are losing InternetGatewayID because it's not populated by describeVPC/createVPC or
-	// restored here, but that's ok. It is restored by reconcileInternetGateways, which is invoked after this.
-	vpc.AvailabilityZoneSelection = s.scope.VPC().AvailabilityZoneSelection
-	vpc.AvailabilityZoneUsageLimit = s.scope.VPC().AvailabilityZoneUsageLimit
+		// if the VPC is managed, make managed sure attributes are configured.
+		if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
+			if err := s.ensureManagedVPCAttributes(vpc); err != nil {
+				return false, err
+			}
+			return true, nil
+		}, awserrors.VPCNotFound); err != nil {
+			return errors.Wrapf(err, "failed to to set vpc attributes for %q", vpc.ID)
+		}
 
-	if vpc.IsUnmanaged(s.scope.Name()) {
-		vpc.DeepCopyInto(s.scope.VPC())
-		s.scope.V(2).Info("Working on unmanaged VPC", "vpc-id", vpc.ID)
 		return nil
 	}
 
-	// Make sure attributes are configured
-	if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
-		buildParams := s.getVPCTagParams(vpc.ID)
-		tagsBuilder := tags.New(&buildParams, tags.WithEC2(s.EC2Client))
-		if err := tagsBuilder.Ensure(vpc.Tags); err != nil {
-			return false, err
+	// .spec.vpc.id is nil, Create a new managed vpc.
+	if !conditions.Has(s.scope.InfraCluster(), infrav1.VpcReadyCondition) {
+		conditions.MarkFalse(s.scope.InfraCluster(), infrav1.VpcReadyCondition, infrav1.VpcCreationStartedReason, clusterv1.ConditionSeverityInfo, "")
+		if err := s.scope.PatchObject(); err != nil {
+			return errors.Wrap(err, "failed to patch conditions")
 		}
-		return true, nil
-	}, awserrors.VPCNotFound); err != nil {
-		record.Warnf(s.scope.InfraCluster(), "FailedTagVPC", "Failed to tag managed VPC %q: %v", vpc.ID, err)
-		return errors.Wrapf(err, "failed to tag vpc %q", vpc.ID)
+	}
+	vpc, err := s.createVPC()
+	if err != nil {
+		return errors.Wrap(err, "failed to create new vpc")
 	}
 
+	s.scope.VPC().CidrBlock = vpc.CidrBlock
+	s.scope.VPC().Tags = vpc.Tags
+	s.scope.VPC().ID = vpc.ID
+
 	// Make sure attributes are configured
 	if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
 		if err := s.ensureManagedVPCAttributes(vpc); err != nil {
@@ -102,8 +101,6 @@ func (s *Service) reconcileVPC() error {
 		return errors.Wrapf(err, "failed to to set vpc attributes for %q", vpc.ID)
 	}
 
-	vpc.DeepCopyInto(s.scope.VPC())
-	s.scope.V(2).Info("Working on managed VPC", "vpc-id", vpc.ID)
 	return nil
 }
 
@@ -165,10 +162,6 @@ func (s *Service) ensureManagedVPCAttributes(vpc *infrav1.VPCSpec) error {
 }
 
 func (s *Service) createVPC() (*infrav1.VPCSpec, error) {
-	if s.scope.VPC().IsUnmanaged(s.scope.Name()) {
-		return nil, errors.Errorf("cannot create a managed vpc in unmanaged mode")
-	}
-
 	if s.scope.VPC().CidrBlock == "" {
 		s.scope.VPC().CidrBlock = defaultVPCCidr
 	}
@@ -189,15 +182,6 @@ func (s *Service) createVPC() (*infrav1.VPCSpec, error) {
 	record.Eventf(s.scope.InfraCluster(), "SuccessfulCreateVPC", "Created new managed VPC %q", *out.Vpc.VpcId)
 	s.scope.V(2).Info("Created new VPC with cidr", "vpc-id", *out.Vpc.VpcId, "cidr-block", *out.Vpc.CidrBlock)
 
-	// TODO: we should attempt to record the VPC ID as soon as possible by setting s.scope.VPC().ID
-	// however, the logic used for determining managed vs unmanaged VPCs relies on the tags and will
-	// need to be updated to accommodate for the recording of the VPC ID prior to the tagging.
-
-	wReq := &ec2.DescribeVpcsInput{VpcIds: []*string{out.Vpc.VpcId}}
-	if err := s.EC2Client.WaitUntilVpcAvailable(wReq); err != nil {
-		return nil, errors.Wrapf(err, "failed to wait for vpc %q", *out.Vpc.VpcId)
-	}
-
 	return &infrav1.VPCSpec{
 		ID:        *out.Vpc.VpcId,
 		CidrBlock: *out.Vpc.CidrBlock,
@@ -232,19 +216,18 @@ func (s *Service) deleteVPC() error {
 	return nil
 }
 
-func (s *Service) describeVPC() (*infrav1.VPCSpec, error) {
+func (s *Service) describeVPCByID() (*infrav1.VPCSpec, error) {
+	if s.scope.VPC().ID == "" {
+		return nil, errors.New("VPC ID is not set, failed to describe VPCs by ID")
+	}
+
 	input := &ec2.DescribeVpcsInput{
 		Filters: []*ec2.Filter{
 			filter.EC2.VPCStates(ec2.VpcStatePending, ec2.VpcStateAvailable),
 		},
 	}
 
-	if s.scope.VPC().ID == "" {
-		// Try to find a previously created and tagged VPC
-		input.Filters = append(input.Filters, filter.EC2.Cluster(s.scope.Name()))
-	} else {
-		input.VpcIds = []*string{aws.String(s.scope.VPC().ID)}
-	}
+	input.VpcIds = []*string{aws.String(s.scope.VPC().ID)}
 
 	out, err := s.EC2Client.DescribeVpcs(input)
 	if err != nil {

pkg/cloud/services/network/vpc_test.go

@@ -21,9 +21,13 @@ import (
 	"reflect"
 	"testing"
 
+	. "github.com/onsi/gomega"
+
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
+	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/diff"
@@ -68,13 +72,14 @@ func TestReconcileVPC(t *testing.T) {
 	selection := infrav1.AZSelectionSchemeOrdered
 
 	testCases := []struct {
-		name     string
-		input    *infrav1.VPCSpec
-		expected *infrav1.VPCSpec
-		expect   func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		name        string
+		input       *infrav1.VPCSpec
+		expected    *infrav1.VPCSpec
+		expect      func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		expectError bool
 	}{
 		{
-			name:  "managed vpc exists",
+			name:  "if unmanaged vpc exists, updates tags with aws VPC resource tags",
 			input: &infrav1.VPCSpec{ID: "vpc-exists", AvailabilityZoneUsageLimit: &usageLimit, AvailabilityZoneSelection: &selection},
 			expected: &infrav1.VPCSpec{
 				ID:        "vpc-exists",
@@ -87,6 +92,7 @@ func TestReconcileVPC(t *testing.T) {
 				AvailabilityZoneUsageLimit: &usageLimit,
 				AvailabilityZoneSelection:  &selection,
 			},
+			expectError: false,
 			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
 				m.DescribeVpcs(gomock.Eq(&ec2.DescribeVpcsInput{
 					VpcIds: []*string{
@@ -128,8 +134,9 @@ func TestReconcileVPC(t *testing.T) {
 			},
 		},
 		{
-			name:  "managed vpc does not exist",
-			input: &infrav1.VPCSpec{AvailabilityZoneUsageLimit: &usageLimit, AvailabilityZoneSelection: &selection},
+			name:        "if managed vpc does not exist, creates a new VPC",
+			input:       &infrav1.VPCSpec{AvailabilityZoneUsageLimit: &usageLimit, AvailabilityZoneSelection: &selection},
+			expectError: false,
 			expected: &infrav1.VPCSpec{
 				ID:        "vpc-new",
 				CidrBlock: "10.1.0.0/16",
@@ -142,20 +149,6 @@ func TestReconcileVPC(t *testing.T) {
 				AvailabilityZoneSelection:  &selection,
 			},
 			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
-				m.DescribeVpcs(gomock.Eq(&ec2.DescribeVpcsInput{
-					Filters: []*ec2.Filter{
-						{
-							Name:   aws.String("state"),
-							Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}),
-						},
-						{
-							Name:   aws.String("tag-key"),
-							Values: aws.StringSlice([]string{"sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"}),
-						},
-					},
-				})).
-					Return(&ec2.DescribeVpcsOutput{}, nil)
-
 				m.CreateVpc(gomock.AssignableToTypeOf(&ec2.CreateVpcInput{})).
 					Return(&ec2.CreateVpcOutput{
 						Vpc: &ec2.Vpc{
@@ -184,11 +177,25 @@ func TestReconcileVPC(t *testing.T) {
 
 				m.ModifyVpcAttribute(gomock.AssignableToTypeOf(&ec2.ModifyVpcAttributeInput{})).
 					Return(&ec2.ModifyVpcAttributeOutput{}, nil).Times(2)
-
-				m.WaitUntilVpcAvailable(gomock.Eq(&ec2.DescribeVpcsInput{
-					VpcIds: []*string{aws.String("vpc-new")},
+			},
+		},
+		{
+			name:        "managed vpc id exists, but vpc resource is missing",
+			input:       &infrav1.VPCSpec{ID: "vpc-exists", AvailabilityZoneUsageLimit: &usageLimit, AvailabilityZoneSelection: &selection},
+			expectError: true,
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeVpcs(gomock.Eq(&ec2.DescribeVpcsInput{
+					VpcIds: []*string{
+						aws.String("vpc-exists"),
+					},
+					Filters: []*ec2.Filter{
+						{
+							Name:   aws.String("state"),
+							Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}),
+						},
+					},
 				})).
-					Return(nil)
+					Return(nil, awserr.New("404", "http not found err", errors.New("err")))
 			},
 		},
 	}
@@ -226,9 +233,14 @@ func TestReconcileVPC(t *testing.T) {
 
 			s := NewService(clusterScope)
 			s.EC2Client = ec2Mock
-
-			if err := s.reconcileVPC(); err != nil {
-				t.Fatalf("got an unexpected error: %v", err)
+			g := NewWithT(t)
+
+			err = s.reconcileVPC()
+			if tc.expectError {
+				g.Expect(err).ToNot(BeNil())
+				return
+			} else {
+				g.Expect(err).To(BeNil())
 			}
 
 			if !reflect.DeepEqual(tc.expected, &clusterScope.AWSCluster.Spec.NetworkSpec.VPC) {