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.

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

✨ Support adding custom secondary VPC CIDR blocks in AWSCluster #4898

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

AndiDog
Copy link
Contributor

@AndiDog AndiDog commented Mar 27, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

For Cilium in AWS ENI mode, or other CNIs that can use a separate CIDR block e.g. for pod IPs, we need to support creating those CIDRs like we do for EKS.

I kept IPv6 out for now since most users won't be able to use it, but kept the structure field naming flexible to accommodate for it later.

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

n/a

Checklist:

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

Release note:

Support adding custom secondary VPC CIDR blocks in `AWSCluster`

@k8s-ci-robot 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. labels Mar 27, 2024
@k8s-ci-robot k8s-ci-robot requested review from fiunchinho and nrb March 27, 2024 12:57
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Mar 27, 2024

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

@nrb
Copy link
Contributor

nrb commented Apr 3, 2024

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

@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from e14cc80 to 6caacd3 Compare April 4, 2024 14:49
@AndiDog AndiDog marked this pull request as draft April 9, 2024 15:28
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 9, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Apr 9, 2024

Marking as draft again since there seem to be some implications and assumptions regarding subnets that CAPA has. I'll first dig into that and update the PR.

@AndiDog
Copy link
Contributor Author

AndiDog commented Apr 15, 2024

Working now. I included and extended the fix https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4800/files since CAPA isn't supposed to choose any secondary CIDR subnet for creating nodes.

@AndiDog AndiDog marked this pull request as ready for review April 15, 2024 13:15
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2024
@AndiDog AndiDog requested a review from nrb April 15, 2024 13:16
@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from 92e4be4 to 9b917e8 Compare April 22, 2024 07:54
@nrb
Copy link
Contributor

nrb commented Apr 23, 2024

/lgtm

The verify job just needs a make generate run, and the API diff doesn't look concerning.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Apr 25, 2024

I ran make generate and my commit is up to date with that. But it seems the YAML output is slightly different on my machine compared to CI. I'll check the tool versions and update here.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Apr 30, 2024

@nrb Looks fine now after I rebased onto main

@nrb
Copy link
Contributor

nrb commented Apr 30, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 30, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented May 2, 2024

I added the two permissions

ec2:AssociateVpcCidrBlock
ec2:DisassociateVpcCidrBlock

to func (t Template) ControllersPolicy() *iamv1.PolicyDocument because they were only listed for EKS until now.

@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from 1df6444 to 266368b Compare May 2, 2024 07:55
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2024
@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from 266368b to 8ea9435 Compare May 2, 2024 16:20
@AndiDog
Copy link
Contributor Author

AndiDog commented May 3, 2024

make test works for me locally, let's try again

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

Copy link
Contributor

@cnmcavoy cnmcavoy 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 May 8, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2024
@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from 8ea9435 to 108127e Compare July 1, 2024 08:24
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Jul 1, 2024

Rebased. @cnmcavoy can you take a look again (was already LGTM)? And @Ankitasw @richardcase I need an approval as well.

@cnmcavoy
Copy link
Contributor

cnmcavoy commented Jul 1, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2024
@AndiDog AndiDog force-pushed the secondary-vpc-cidr branch from 108127e to 05d6299 Compare July 26, 2024 12:31
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 26, 2024
@AndiDog
Copy link
Contributor Author

AndiDog commented Jul 26, 2024

I had to rebase this once again due to the new field SubnetSchema which gave easily resolvable merge conflicts.

@richardcase @Ankitasw do you have time to look into this PR? It was already reviewed, but not approved.

@nrb
Copy link
Contributor

nrb commented Jul 30, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nrb

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 Jul 30, 2024
@k8s-ci-robot k8s-ci-robot merged commit f99a356 into kubernetes-sigs:main Jul 30, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants