diff --git a/api/v1alpha3/awscluster_webhook.go b/api/v1alpha3/awscluster_webhook.go index 8e9f87f622..bace3c0b41 100644 --- a/api/v1alpha3/awscluster_webhook.go +++ b/api/v1alpha3/awscluster_webhook.go @@ -96,6 +96,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, diff --git a/api/v1alpha3/awscluster_webhook_test.go b/api/v1alpha3/awscluster_webhook_test.go index 79f860870d..2326293182 100644 --- a/api/v1alpha3/awscluster_webhook_test.go +++ b/api/v1alpha3/awscluster_webhook_test.go @@ -143,6 +143,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) { diff --git a/pkg/cloud/services/network/network.go b/pkg/cloud/services/network/network.go index 695d328975..fb7196f7e6 100644 --- a/pkg/cloud/services/network/network.go +++ b/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 diff --git a/pkg/cloud/services/network/vpc.go b/pkg/cloud/services/network/vpc.go index bd4b57216f..d0bd2f721b 100644 --- a/pkg/cloud/services/network/vpc.go +++ b/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/v1alpha3" "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/v1alpha3" @@ -43,56 +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 err := s.scope.PatchObject(); err != nil { - return errors.Wrap(err, "failed to patch conditions") - } - } - vpc, err = s.createVPC() + // If the ID is not nil, VPC is either managed or unmanaged but should exist in the AWS. + if s.scope.VPC().ID != "" { // nolint:nestif + vpc, err := s.describeVPCByID() if err != nil { - return errors.Wrap(err, "failed to create new vpc") + 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)") } - } else if err != nil { - return errors.Wrap(err, "failed to describe VPCs") - } + s.scope.VPC().CidrBlock = vpc.CidrBlock + s.scope.VPC().Tags = vpc.Tags - // 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 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 unmanaged VPC fields") + } + record.Eventf(s.scope.InfraCluster(), "SuccessfulSetVPCAttributes", "Set managed VPC attributes for %q", vpc.ID) + return nil + } + + // 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 { @@ -103,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 } @@ -166,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 } @@ -190,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, @@ -233,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 { diff --git a/pkg/cloud/services/network/vpc_test.go b/pkg/cloud/services/network/vpc_test.go index e913bf330a..0edf8e1237 100644 --- a/pkg/cloud/services/network/vpc_test.go +++ b/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"))) }, }, } @@ -225,9 +232,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) {