🐛 Restore subnet management functionality #4474

vincepri · 2023-08-31T21:11:50Z

/kind bug
/kind api-change
/kind regression

What this PR does / why we need it:

This PR reverts some commits originally introduced in #3748 to support ClusterClass with SSA and proposes a different approach that's backward compatible with that functionality.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4026

Special notes for your reviewer:

Checklist:

squashed commits
includes documentation
adds unit tests
adds or updates e2e tests

Release note:

action required
When the VPC is managed, the `SubnetSpec.ID` field can be used now to offload subnet management to the AWSCluster controller. The `id` should be set to a string that DOES NOT start with `subnet-`; the value is used to set the `Name` tag in the subnet resource and the actual AWS resource ID is stored in `SubnetSpec.ResourceID`.

vincepri · 2023-08-31T21:12:12Z

/priority critical-urgent

vincepri · 2023-08-31T21:12:53Z

/assign @richardcase

/cc @fabriziopandini @sbueringer

vincepri · 2023-08-31T21:13:36Z

/cc @AverageMarcus @AndiDog

Might also be good to incorporate giantswarm@407bd98

k8s-ci-robot · 2023-08-31T21:13:39Z

@vincepri: GitHub didn't allow me to request PR reviews from the following users: AndiDog.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @AverageMarcus @AndiDog

Might also be good to incorporate giantswarm@407bd98

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

AndiDog · 2023-08-31T21:22:51Z

/cc @AverageMarcus @AndiDog

Might also be good to incorporate giantswarm@407bd98

I'd be happy if you cherry-pick that commit. That's exactly the workaround we have in our fork for the moment. It applies almost cleanly to v2.2.1, so I'm sure the patch is still fine.

What do maintainers say about reverting to the original behavior for now? (CC @richardcase who was also involved in network discussions apart from the people already assigned on this PR)

vincepri · 2023-08-31T22:59:49Z

/test ?

k8s-ci-robot · 2023-08-31T22:59:50Z

@vincepri: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-aws-build
/test pull-cluster-api-provider-aws-test
/test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e-eks-gc
/test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vincepri · 2023-08-31T23:00:12Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance

vincepri · 2023-08-31T23:08:59Z

From a project and SIG perspective, we should strive to retain key features and behavior that made this project successful. With Terraform licensing change, I suspect more and more users would prefer Cluster API to manage infrastructure required to have a functional cluster. Subnets are definitely in that mix.

The reversal of these commits it's not a breaking change, but rather a regression fix; going from required to optional is fine from an API perspective.

Regarding ClusterClass and SSA, as mentioned in the PR description we should find alternative solutions rather than breaking existing functionality.

AndiDog

/hold

(due to review comments)

api/v1beta1/awscluster_conversion.go

pkg/cloud/services/network/subnets.go

api/v1beta2/network_types.go

pkg/cloud/services/network/subnets.go

richardcase · 2023-10-04T14:26:43Z

As discussed just now on slack, to allow more time for testing and review:

/milestone v2.3.0

vincepri · 2023-10-04T16:06:01Z

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

vincepri · 2023-10-04T16:33:08Z

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

vincepri · 2023-10-04T18:36:31Z

/retest

k8s-ci-robot · 2023-10-04T20:08:40Z

@vincepri: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-e2e-eks-gc	112640dc71609532d4d550330d8e009ffbfdde97	link	false	`/test pull-cluster-api-provider-aws-e2e-eks-gc`
pull-cluster-api-provider-aws-e2e-eks	112640dc71609532d4d550330d8e009ffbfdde97	link	false	`/test pull-cluster-api-provider-aws-e2e-eks`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vincepri · 2023-10-04T20:28:09Z

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

AndiDog · 2023-10-06T08:32:05Z

Looks good so far. Which cases did you test manually so far? Should we help testing this change before merging?

I think we should fix the single-case switch statement – see the remaining comment. Then all comments seem to be addressed.

vincepri · 2023-10-06T20:41:41Z

@AndiDog Only tested without ClusterClass, creating subnets managed by CAPA in different AZs. I'll fix the switch statement now; given the e2e are passing we should be good to merge and follow-up with the other tests/bits later?

This reverts commit 1bc6ce8.

This reverts commit 39f088b.

Signed-off-by: Vince Prignano <vincepri@redhat.com>

AndiDog

/lgtm
/unhold

k8s-infra-cherrypick-robot · 2023-10-08T17:11:15Z

@richardcase: #4474 failed to apply on top of branch "release-2.2":

Applying: Revert "Make subnet spec id field required for SSA to work with CC"
Applying: Revert "Remove multi-az e2e test (it is no longer supported)"
Applying: Fix SSA support by adding Subnet.ResourceID field
Using index info to reconstruct a base tree...
M	api/v1beta1/awscluster_conversion.go
M	api/v1beta1/zz_generated.conversion.go
M	api/v1beta2/network_types.go
M	config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
M	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
M	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
M	pkg/cloud/scope/shared_test.go
M	pkg/cloud/services/ec2/bastion.go
M	pkg/cloud/services/ec2/instances.go
M	pkg/cloud/services/elb/loadbalancer.go
M	pkg/cloud/services/network/natgateways.go
M	pkg/cloud/services/network/routetables.go
M	pkg/cloud/services/network/subnets.go
M	pkg/cloud/services/network/subnets_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/services/network/subnets_test.go
Auto-merging pkg/cloud/services/network/subnets.go
Auto-merging pkg/cloud/services/network/routetables.go
Auto-merging pkg/cloud/services/network/natgateways.go
Auto-merging pkg/cloud/services/elb/loadbalancer.go
Auto-merging pkg/cloud/services/ec2/instances.go
Auto-merging pkg/cloud/services/ec2/bastion.go
Auto-merging pkg/cloud/scope/shared_test.go
CONFLICT (content): Merge conflict in pkg/cloud/scope/shared_test.go
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
Auto-merging config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
Auto-merging api/v1beta2/network_types.go
Auto-merging api/v1beta1/zz_generated.conversion.go
CONFLICT (content): Merge conflict in api/v1beta1/zz_generated.conversion.go
Auto-merging api/v1beta1/awscluster_conversion.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Fix SSA support by adding Subnet.ResourceID field
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-2.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot requested review from shivi28 and Skarlso August 31, 2023 21:12

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority labels Aug 31, 2023

k8s-ci-robot assigned richardcase Aug 31, 2023

k8s-ci-robot requested review from fabriziopandini and sbueringer August 31, 2023 21:12

k8s-ci-robot requested a review from AverageMarcus August 31, 2023 21:13

vincepri force-pushed the revert-subnet-id-required branch from ebb6ac5 to 2dfc9bd Compare August 31, 2023 22:57

vincepri force-pushed the revert-subnet-id-required branch from 2dfc9bd to b78118f Compare August 31, 2023 23:03

vincepri force-pushed the revert-subnet-id-required branch from b78118f to 214db67 Compare August 31, 2023 23:09

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2023

vincepri force-pushed the revert-subnet-id-required branch from cfe1665 to 3b77e28 Compare September 1, 2023 04:51

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2023

AndiDog reviewed Oct 4, 2023

View reviewed changes

k8s-ci-robot modified the milestones: v2.2.3, v2.3.0 Oct 4, 2023

vincepri force-pushed the revert-subnet-id-required branch 3 times, most recently from 9c9eb0c to 112640d Compare October 4, 2023 15:50

vincepri force-pushed the revert-subnet-id-required branch from 112640d to ca671fb Compare October 4, 2023 20:26

vincepri force-pushed the revert-subnet-id-required branch from ca671fb to 66eb159 Compare October 6, 2023 16:50

vincepri added 3 commits October 6, 2023 13:48

Revert "Make subnet spec id field required for SSA to work with CC"

391452a

This reverts commit 1bc6ce8.

Revert "Remove multi-az e2e test (it is no longer supported)"

63f22ec

This reverts commit 39f088b.

Fix SSA support by adding Subnet.ResourceID field

703ec18

Signed-off-by: Vince Prignano <vincepri@redhat.com>

vincepri force-pushed the revert-subnet-id-required branch from 66eb159 to 703ec18 Compare October 6, 2023 20:49

AndiDog reviewed Oct 8, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2023

k8s-ci-robot assigned AndiDog Oct 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2023

k8s-ci-robot merged commit 54bfea1 into kubernetes-sigs:main Oct 8, 2023
13 checks passed

AndiDog mentioned this pull request Oct 8, 2023

cluster-aws: Adapt to CAPA subnet spec changes giantswarm/roadmap#2870

Closed

richardcase mentioned this pull request Oct 12, 2023

[E2E] EKS tests are failing #4574

Closed

AndiDog mentioned this pull request Nov 15, 2023

✨ Add test for provider-managed infrastructure, fix writing latest status back into ClusterScope #4637

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Restore subnet management functionality #4474

🐛 Restore subnet management functionality #4474

vincepri commented Aug 31, 2023 •

edited

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

AndiDog commented Aug 31, 2023

vincepri commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

AndiDog left a comment

richardcase commented Oct 4, 2023

vincepri commented Oct 4, 2023

vincepri commented Oct 4, 2023

vincepri commented Oct 4, 2023

k8s-ci-robot commented Oct 4, 2023 •

edited

vincepri commented Oct 4, 2023

AndiDog commented Oct 6, 2023

vincepri commented Oct 6, 2023 •

edited

AndiDog left a comment

k8s-infra-cherrypick-robot commented Oct 8, 2023

🐛 Restore subnet management functionality #4474

🐛 Restore subnet management functionality #4474

Conversation

vincepri commented Aug 31, 2023 • edited

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

AndiDog commented Aug 31, 2023

vincepri commented Aug 31, 2023

k8s-ci-robot commented Aug 31, 2023

vincepri commented Aug 31, 2023

vincepri commented Aug 31, 2023

AndiDog left a comment

Choose a reason for hiding this comment

richardcase commented Oct 4, 2023

vincepri commented Oct 4, 2023

vincepri commented Oct 4, 2023

vincepri commented Oct 4, 2023

k8s-ci-robot commented Oct 4, 2023 • edited

vincepri commented Oct 4, 2023

AndiDog commented Oct 6, 2023

vincepri commented Oct 6, 2023 • edited

AndiDog left a comment

Choose a reason for hiding this comment

k8s-infra-cherrypick-robot commented Oct 8, 2023

vincepri commented Aug 31, 2023 •

edited

k8s-ci-robot commented Oct 4, 2023 •

edited

vincepri commented Oct 6, 2023 •

edited