🐛 Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails #4723

AndiDog · 2024-01-05T09:22:59Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In case of a repeating storage error, for example

"Reconciler error" err="failed to reconcile network for AWSManagedControlPlane mynamespace/mycluster: failed to patch conditions: AWSManagedControlPlane.controlplane.cluster.x-k8s.io \"mycluster\" is invalid: spec.network.subnets[7]: Duplicate value: map[string]interface {}{\"id\":\"\"}" [...]

which was caused by something totally different, CAPA would create an indefinite number of VPCs (and potentially other resources depending how far reconciliation succeeds!), filling up the AWS account to its limits and requiring manual cleanup of the whole mess (the one case I had wasn't fun 😆).

This is terrifying but based on a very simple, horrible bug: VPC creation wasn't idempotent. This PR introduces the typical look-up-else-create logic.

Unfortunately, VPC creation tests were faulty because the mockCtrl object was shared across all test cases, so the expected mock calls of test case A could be "used" by a unrelated test case B, potentially making a test case pass that normally should fail. Or in short, we weren't testing what we thought we were. Also, some test cases had a description differing from the test, and made no sense overall, so I removed those. The tests now describe that VPC creation should happen only once. On top, since some tests were related to IPv6, I added a check to see if the AWS VPC IPv6 support matches our spec, or fail on mismatch. This made all tests green again. To ensure that we test the right thing in the future, test cases now check for specific errors.

Checklist:

Release note:

Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails

AndiDog · 2024-01-05T09:41:18Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

AndiDog · 2024-01-08T09:18:34Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

vincepri · 2024-01-08T16:40:35Z

pkg/cloud/services/network/vpc.go

+		if !vpc.Tags.HasOwned(s.scope.Name()) {
+			return errors.Errorf("an unmanaged VPC %s already exists, refusing to create another managed VPC", vpc.ID)
 		}


The VPC today is created with tags within the same call, the error message is confusing to users given that if there are no tags, there isn't another managed VPC

Suggested change

if !vpc.Tags.HasOwned(s.scope.Name()) {

return errors.Errorf("an unmanaged VPC %s already exists, refusing to create another managed VPC", vpc.ID)

}

if !vpc.Tags.HasOwned(s.scope.Name()) {

return errors.Errorf("found VPC named %s which cannot be managed by CAPA due to lack of tags, either tag the VPC manually [add tag needed here], or provide the `vpc.id` field instead if you wish to bring your own VPC (link to the doc?)", vpc.ID)

}

vincepri · 2024-01-08T16:41:59Z

pkg/cloud/services/network/vpc.go

 		}
+
+		if s.scope.VPC().IsIPv6Enabled() != (vpc.IPv6 != nil) {


split this in two lines for readability?

done in both locations

vincepri · 2024-01-18T16:38:29Z

pkg/cloud/services/network/vpc.go

+		actualIPv6Enabled := (vpc.IPv6 != nil)
+		if s.scope.VPC().IsIPv6Enabled() != actualIPv6Enabled {
+			return errors.Errorf("IPv6 support of found unmanaged VPC %s differs from desired spec. Changing IP family is currently not supported.", vpc.ID)
+		}


This seems a net-new check (repeated in the other function as well); should we remove this for now and add it in a different PR? Generally these checks would be best in a webhook at Update time

The webhook already checks that IPv6 can't be toggled later:

cluster-api-provider-aws/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go

Lines 162 to 165 in a1918aa

if oldAWSManagedControlplane.Spec.NetworkSpec.VPC.IsIPv6Enabled() != r.Spec.NetworkSpec.VPC.IsIPv6Enabled() {

allErrs = append(allErrs,

field.Invalid(field.NewPath("spec", "networkSpec", "vpc", "enableIPv6"), r.Spec.NetworkSpec.VPC.IsIPv6Enabled(), "changing IP family is not allowed after it has been set"))

}

IIRC, my change was to get a clear error message in unit tests, since they can simulate mismatching situations such as someone manually having turned on IPv6 via AWS Console. In such scenarios, the webhook wouldn't be called, but we still want an error instead of being silent about not reconciling the difference. I don't think this would be a user-facing breakage since normally, the reconciled state (here: IPv6 support on/off in the VPC) matches the spec.

Agree with @vincepri here, it's better to do it in separate PR.

I removed the 2 occurrences of the check, and the new test case for that. I'll take note to re-add this in a separate PR, potentially targeting a later milestone.

richardcase · 2024-01-23T14:47:05Z

/milestone v2.4.0

AndiDog · 2024-01-24T07:24:40Z

E2E tests didn't work some weeks ago, it seems. Rebased onto main to see if that fixes the problem.

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

Ankitasw · 2024-02-01T06:18:52Z

/test pull-cluster-api-provider-aws-e2e

… if storage of the ID fails

vincepri · 2024-02-01T15:30:31Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

vincepri

/lgtm
/assign @Ankitasw

Ankitasw · 2024-02-02T08:21:24Z

/approve

k8s-ci-robot · 2024-02-02T08:21:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Ankitasw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from cnmcavoy and vincepri January 5, 2024 09:23

AndiDog mentioned this pull request Jan 5, 2024

latest capa-controller is not working properly with our cluster-eks and spawns infinite number of VPC due some error giantswarm/roadmap#3048

Closed

vincepri reviewed Jan 8, 2024

View reviewed changes

AndiDog force-pushed the vpc-recon-idempotence branch from 2b579fc to 811ef3e Compare January 10, 2024 15:57

AndiDog requested a review from vincepri January 18, 2024 08:11

vincepri reviewed Jan 18, 2024

View reviewed changes

k8s-ci-robot added this to the v2.4.0 milestone Jan 23, 2024

AndiDog force-pushed the vpc-recon-idempotence branch from 811ef3e to 0e78bb0 Compare January 24, 2024 07:24

Make VPC creation idempotent to avoid indefinite creation of new VPCs…

af9ffa5

… if storage of the ID fails

AndiDog force-pushed the vpc-recon-idempotence branch from 0e78bb0 to af9ffa5 Compare February 1, 2024 12:55

vincepri approved these changes Feb 1, 2024

View reviewed changes

k8s-ci-robot assigned Ankitasw and vincepri Feb 1, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2024

k8s-ci-robot merged commit 030638a into kubernetes-sigs:main Feb 2, 2024
19 checks passed

AndiDog deleted the vpc-recon-idempotence branch February 13, 2024 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails #4723

🐛 Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails #4723

AndiDog commented Jan 5, 2024

AndiDog commented Jan 5, 2024

AndiDog commented Jan 8, 2024

vincepri Jan 8, 2024 •

edited

Loading

AndiDog Jan 10, 2024

vincepri Jan 8, 2024

AndiDog Jan 10, 2024

vincepri Jan 18, 2024

AndiDog Jan 19, 2024

Ankitasw Jan 23, 2024

AndiDog Feb 1, 2024

richardcase commented Jan 23, 2024

AndiDog commented Jan 24, 2024

Ankitasw commented Feb 1, 2024

vincepri commented Feb 1, 2024

vincepri left a comment

Ankitasw commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

	if oldAWSManagedControlplane.Spec.NetworkSpec.VPC.IsIPv6Enabled() != r.Spec.NetworkSpec.VPC.IsIPv6Enabled() {
	allErrs = append(allErrs,
	field.Invalid(field.NewPath("spec", "networkSpec", "vpc", "enableIPv6"), r.Spec.NetworkSpec.VPC.IsIPv6Enabled(), "changing IP family is not allowed after it has been set"))
	}

🐛 Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails #4723

🐛 Make VPC creation idempotent to avoid indefinite creation of new VPCs if storage of the ID fails #4723

Conversation

AndiDog commented Jan 5, 2024

AndiDog commented Jan 5, 2024

AndiDog commented Jan 8, 2024

vincepri Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

AndiDog Jan 10, 2024

Choose a reason for hiding this comment

vincepri Jan 8, 2024

Choose a reason for hiding this comment

AndiDog Jan 10, 2024

Choose a reason for hiding this comment

vincepri Jan 18, 2024

Choose a reason for hiding this comment

AndiDog Jan 19, 2024

Choose a reason for hiding this comment

Ankitasw Jan 23, 2024

Choose a reason for hiding this comment

AndiDog Feb 1, 2024

Choose a reason for hiding this comment

richardcase commented Jan 23, 2024

AndiDog commented Jan 24, 2024

Ankitasw commented Feb 1, 2024

vincepri commented Feb 1, 2024

vincepri left a comment

Choose a reason for hiding this comment

Ankitasw commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

vincepri Jan 8, 2024 •

edited

Loading