✨ feat: create vpc objects in explicitly provided availability zones #4543

Skarlso · 2023-10-05T09:55:51Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

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 #4333

Special notes for your reviewer:

Checklist:

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

Release note:

 Ability to specify Availability Zones from the VPCSpec object.

Skarlso · 2023-10-05T10:03:35Z

Oh totally forgot all the conversion shenanigans.

Skarlso · 2023-10-06T05:30:22Z

/test ?

k8s-ci-robot · 2023-10-06T05:30:23Z

@Skarlso: 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.

Skarlso · 2023-10-06T05:30:33Z

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

vincepri · 2023-10-09T03:05:48Z

api/v1beta2/network_types.go

+	// AvailabilityZoneList defines a list of Availability Zones in which to create network resources in.
+	// If defined, both AvailabilityZoneUsageLimit and AvailabilityZoneSelection are ignored.
+	// +optional
+	AvailabilityZoneList []string `json:"availabilityZoneList,omitempty"`


Suggested change

// AvailabilityZoneList defines a list of Availability Zones in which to create network resources in.

// If defined, both AvailabilityZoneUsageLimit and AvailabilityZoneSelection are ignored.

// +optional

AvailabilityZoneList []string `json:"availabilityZoneList,omitempty"`

// AvailabilityZoneList defines a list of Availability Zones in which to create network resources in.

// Cannot be defined alongside AvailabilityZoneUsageLimit and AvailabilityZoneSelection.

// +optional

AvailabilityZones []string `json:"availabilityZones,omitempty"`

We should probably refuse usage when both fields are set, that can cause confusion. We could check in a validation webhook, and given that this field is new, the change is backward compatible.

Sounds good. 👍

Actually, both fields have mandatory defaults. They are always defined. Meaning we cannot differentiate between deliberately set or the default value defined. Which means we cannot have a validation against them.

We could move the defaults in the defaulting webhook instead of openapi schema? That way, the defaulting logic can be conditional.

I'm not a 100% sure how that works. It wouldn't break existing clusters, right? Like, the default in the kubebuilder results in a different yaml, while the defaulting webhook... doesn't? Or does it?

Also... Okay. So... Kubebuilder Markers GENERATE default values so the generated output will be different. But mutating admission webhooks change values in flight so the output OBJECT's value will be different.

Unless I'm super mistaken these two scenarios ARE different.

The yaml is going to be different, although the defaulting logic moves to the webhook, which historically we have not treated as a breaking change, but as a compatible one.

That said, what I was suggesting is to validate this new field in the webhook, and reject it when the other two colliding fields are set.

Yes, but validating it would require removing the defaults from here. And that's what started this conversation. :)

Yeah that's not a breaking change, @fabriziopandini @sbueringer from the upstream project can keep me honest though.

richardcase · 2023-10-26T10:39:19Z

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

richardcase · 2023-10-26T10:45:47Z

#4575 is probably needed for the eks e2e to pass.

k8s-ci-robot · 2023-11-16T16:05:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Skarlso · 2023-11-16T16:05:25Z

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

cnmcavoy · 2023-11-20T19:19:35Z

/retest

cnmcavoy · 2023-11-20T19:23:28Z

/lgtm

AndiDog

LGTM except one minor remark

AndiDog · 2023-11-23T09:10:00Z

pkg/cloud/services/network/subnets_test.go

+						AvailabilityZone:    aws.String("us-east-1b"),
+						MapPublicIpOnLaunch: aws.Bool(false),
+					},
+				}, nil).After(zone1PublicSubnet)


[minor]

Isn't it an implementation detail in which order private and public subnets are created i.e. we shouldn't check for private.After(public) here?

I don't think it matters that much here? I copied this entire section from previous tests. :) So whatever they do this one does as well. :)

Yeah, not a blocker at all

AndiDog · 2023-11-23T10:03:39Z

/lgtm

synthe102 · 2024-04-02T21:47:11Z

Hey !
Thanks a lot for doing the implementation of this feature. Could it be possible to have it merged in the coming month?

Skarlso · 2024-04-03T05:39:30Z

I can rebase and the rest is up to the capa team. :)

k8s-ci-robot · 2024-04-03T06:01:43Z

New changes are detected. LGTM label has been removed.

Skarlso · 2024-04-03T06:32:19Z

Interesting, I ran generate

vincepri · 2024-04-17T15:52:46Z

The only thing left in this PR is to adjust the webhook validation like mentioned above

Skarlso · 2024-04-23T08:08:06Z

Yeah, not gonna do that. Someone else can pick this up.

synthe102 · 2024-04-23T16:18:48Z

I picked up where @Skarlso stopped and added the validating and mutating webhook changes @vincepri requested here: #4950

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 5, 2023

k8s-ci-robot requested review from Ankitasw and shivi28 October 5, 2023 09:55

k8s-ci-robot added needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2023

Skarlso force-pushed the availability-zone-list branch from 695994c to 8151645 Compare October 5, 2023 10:24

vincepri reviewed Oct 9, 2023

View reviewed changes

Skarlso force-pushed the availability-zone-list branch 2 times, most recently from 545ecd3 to 67ad355 Compare October 9, 2023 08:04

Skarlso requested a review from vincepri October 11, 2023 08:23

cnmcavoy approved these changes Oct 13, 2023

View reviewed changes

Skarlso force-pushed the availability-zone-list branch from 67ad355 to f6ed45c Compare November 16, 2023 16:05

k8s-ci-robot assigned cnmcavoy Nov 20, 2023

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

AndiDog reviewed Nov 23, 2023

View reviewed changes

k8s-ci-robot assigned AndiDog Nov 23, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2024

Skarlso force-pushed the availability-zone-list branch from f6ed45c to 2b8faee Compare April 3, 2024 06:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

Skarlso changed the title ~~feat: create vpc objects in explicitly provided availability zones~~ ✨ feat: create vpc objects in explicitly provided availability zones Apr 3, 2024

feat: create vpc objects in explicitly provided availability zones

c426f23

Skarlso force-pushed the availability-zone-list branch from 2b8faee to c426f23 Compare April 3, 2024 06:42

Skarlso closed this Apr 23, 2024

Skarlso deleted the availability-zone-list branch April 23, 2024 08:08

synthe102 mentioned this pull request Apr 23, 2024

✨ feat: create vpc objects in explicitly provided availability zones #4950

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feat: create vpc objects in explicitly provided availability zones #4543

✨ feat: create vpc objects in explicitly provided availability zones #4543

Skarlso commented Oct 5, 2023 •

edited

Loading

Skarlso commented Oct 5, 2023

Skarlso commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

Skarlso commented Oct 6, 2023

vincepri Oct 9, 2023

Skarlso Oct 9, 2023

Skarlso Oct 9, 2023

vincepri Oct 9, 2023

Skarlso Oct 9, 2023

Skarlso Oct 17, 2023

vincepri Oct 17, 2023

vincepri Oct 17, 2023

Skarlso Oct 17, 2023

vincepri Oct 17, 2023

richardcase commented Oct 26, 2023

richardcase commented Oct 26, 2023

k8s-ci-robot commented Nov 16, 2023

Skarlso commented Nov 16, 2023

cnmcavoy commented Nov 20, 2023

cnmcavoy commented Nov 20, 2023

AndiDog left a comment

AndiDog Nov 23, 2023

Skarlso Nov 23, 2023

AndiDog Nov 23, 2023

AndiDog commented Nov 23, 2023

synthe102 commented Apr 2, 2024

Skarlso commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

Skarlso commented Apr 3, 2024

vincepri commented Apr 17, 2024

Skarlso commented Apr 23, 2024

synthe102 commented Apr 23, 2024

✨ feat: create vpc objects in explicitly provided availability zones #4543

✨ feat: create vpc objects in explicitly provided availability zones #4543

Conversation

Skarlso commented Oct 5, 2023 • edited Loading

Skarlso commented Oct 5, 2023

Skarlso commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

Skarlso commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Oct 26, 2023

richardcase commented Oct 26, 2023

k8s-ci-robot commented Nov 16, 2023

Skarlso commented Nov 16, 2023

cnmcavoy commented Nov 20, 2023

cnmcavoy commented Nov 20, 2023

AndiDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndiDog commented Nov 23, 2023

synthe102 commented Apr 2, 2024

Skarlso commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

Skarlso commented Apr 3, 2024

vincepri commented Apr 17, 2024

Skarlso commented Apr 23, 2024

synthe102 commented Apr 23, 2024

Skarlso commented Oct 5, 2023 •

edited

Loading