Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Add AWSMachine fields to control vpc placement for the instance #4541

Merged
merged 2 commits into from
Jan 31, 2024
Merged

✨ Add AWSMachine fields to control vpc placement for the instance #4541

merged 2 commits into from
Jan 31, 2024

Conversation

cnmcavoy
Copy link
Contributor

@cnmcavoy cnmcavoy commented Oct 4, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds fields to the AWSMachine and AWSMachineTemplate resources to allow VPC selection, as well as overriding the security groups chosen for various infrastructure roles.

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

Special notes for your reviewer:

Checklist:

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

Release note:

 Added AWSMachine and AWSMachineTemplate fields to control vpc placement for individual instances

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 4, 2023
@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch from 5101fd6 to d7dad18 Compare October 4, 2023 22:50
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 4, 2023
@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch from d7dad18 to 0e9ae1f Compare October 4, 2023 22:59
@cnmcavoy
Copy link
Contributor Author

cnmcavoy commented Oct 5, 2023

Some verification of this I did with one of our clusters:
Screenshot 2023-10-05 at 12-56-19 Instances EC2 us-east-2
Screenshot 2023-10-05 at 12-57-49 awscmhinfra3 Clusters Elastic Kubernetes Service us-east-2
Screenshot 2023-10-05 at 12-58-46 awscmhinfra3 Clusters Elastic Kubernetes Service us-east-2

@Skarlso
Copy link
Contributor

Skarlso commented Oct 7, 2023

/test ?

@k8s-ci-robot
Copy link
Contributor

@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
Copy link
Contributor

Skarlso commented Oct 7, 2023

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

vincepri
vincepri reviewed Oct 9, 2023
View reviewed changes
api/v1beta2/awsmachine_types.go Outdated
Comment on lines 112 to 115
// VPC is a reference to the VPC to use when picking a subnet to use for this
// instance. Only valid if the subnet (id or filters) is also specified.
// +optional
VPC *AWSResourceReference `json:"vpc,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the related use case, I was wondering if we should be able to infer the VPC id from the subnet identifier?

What could confuse a bit users with the changes proposed here is that we have AdditionalSecurityGroups and now SecurityGroupOverrides, which might conflict with each other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the related use case, I was wondering if we should be able to infer the VPC id from the subnet identifier?

I think this is possible, it would require rewriting the existing logic to always look up the VPC, but it does have the advantage of simplifying the fields for the AWSMachine. I'll take a look at doing this.

What could confuse a bit users with the changes proposed here is that we have AdditionalSecurityGroups and now SecurityGroupOverrides, which might conflict with each other?

I agree there is some confusion because of the two fields for managing the same concept in AWS. The problem I tried to solve is that we need a different set of "default" SGs in the other VPC, because SGs are bound to a particular VPC, and we can't re-use the cluster default SGs. So I provided a way for the user to supply their own. AdditionalSecurityGroups is insufficient because we need default SGs that are present in the secondary VPC. I don't care about SecurityGroupOverrides as a field, except as a way to provide SGs for the secondary VPC.

So the other option is we could eliminate the SecurityGroupOverrides field and instead have the CAPA duplicate the SGs from the cluster VPC into any secondary VPC, and use those for these nodes. But there would also be no good way to tell when we could tear down these SGs, without tracking them at the cluster level too. We'd probably have to add them to the AWSCluster or AWSManagedCluster's status field to keep track. Otherwise they might get left dangling after a cluster is torn down. Can you think of a better way to make this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri updated to look up the VPC as needed, if you want to give it another look.

@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch from be505a6 to e4775a2 Compare October 12, 2023 20:55
@richardcase richardcase modified the milestones: v2.3.0, v2.4.0 Oct 16, 2023
@cnmcavoy cnmcavoy requested a review from vincepri October 23, 2023 18:14
@cnmcavoy cnmcavoy changed the title Add AWSMachine fields to control vpc placement for the instance ✨ Add AWSMachine fields to control vpc placement for the instance Nov 27, 2023
@Ankitasw
Copy link
Member

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

Ankitasw
Ankitasw reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@Ankitasw Ankitasw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cnmcavoy Would it be possible to add documentation for relevant changes in the CAPA book?

@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch 2 times, most recently from 62b8935 to f60d5b2 Compare January 12, 2024 20:44
@richardcase richardcase removed this from the v2.4.0 milestone Jan 23, 2024
@Ankitasw
Copy link
Member

Ankitasw commented Jan 25, 2024 •
edited
Loading

/approve

cc @vincepri for another look as you had comments

@k8s-ci-robot
Copy link
Contributor

[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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2024
vincepri
vincepri approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch from f60d5b2 to 78d5c86 Compare January 25, 2024 22:49
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@cnmcavoy
Copy link
Contributor Author

Rebased to fix tests.

@Ankitasw
Copy link
Member

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

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@cnmcavoy cnmcavoy force-pushed the awsmachine-vpc-selection branch from 78d5c86 to 0fa2337 Compare January 29, 2024 19:21
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@Ankitasw
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 1fd6d67 into kubernetes-sigs:main Jan 31, 2024
17 of 19 checks passed
@cnmcavoy cnmcavoy mentioned this pull request Sep 4, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankitasw Ankitasw Ankitasw left review comments

@vincepri vincepri vincepri approved these changes

@pydctw pydctw Awaiting requested review from pydctw

Assignees

@vincepri vincepri

@Ankitasw Ankitasw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to spawn machines in an external vpc to the cluster vpc
6 participants
@cnmcavoy @Skarlso @k8s-ci-robot @Ankitasw @vincepri @richardcase