-
Notifications
You must be signed in to change notification settings - Fork 539
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 BYO Public IPv4 Pool when provision infrastructure #4905
base: main
Are you sure you want to change the base?
Conversation
Hi @mtulio. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary changes to test locally as it has been addressed in #4892 . It will be removed once merged/rebased;
dad9f3c
to
994196a
Compare
prevent WIP notifications |
994196a
to
fa1f162
Compare
/test pull-cluster-api-provider-aws-e2e |
@mtulio: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
fa1f162
to
4c41955
Compare
/ok-to-test |
d1af290
to
73a2e51
Compare
Hello @damdo , thanks a lot for your review, good suggestions. All applied. I am leaving only two comments "unresolved" to further discussion. Let me know your thoughts. |
/lgtm |
@vr4manta: changing LGTM is restricted to collaborators In response to this:
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-sigs/prow repository. |
Hi CAPA maintainers, I will be OoO in the next three weeks, please also ping @rvanderp3 to align/address items in your review in this PR. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing feedback @mtulio
Changes
/lgtm
Let's get an approver review. Thanks! |
if !strings.HasPrefix(*eipp.PublicIpv4Pool, awsPublicIpv4PoolPrefix) { | ||
allErrs = append(allErrs, field.Invalid(field.NewPath("spec.elasticIpPool.publicIpv4Pool"), r.Spec.ElasticIPPool, fmt.Sprintf("publicIpv4Pool must start with %s.", awsPublicIpv4PoolPrefix))) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the eipp.PublicIpv4Pool == nil && eipp.PublicIpv4PoolFallBackOrder != nil
case here?
api/v1beta2/network_types.go
Outdated
@@ -477,6 +483,25 @@ func (v *VPCSpec) IsIPv6Enabled() bool { | |||
return v.IPv6 != nil | |||
} | |||
|
|||
// GetElasticIPPool returns the custom Elastic IP Pool configuration when present. | |||
func (v *VPCSpec) GetElasticIPPool() *ElasticIPPool { | |||
if v.ElasticIPPool == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just return v.ElasticIPPool
here, and eliminate the if statement?
api/v1beta2/network_types.go
Outdated
// resource ID starts with 'ipv4pool-ec2'. | ||
// | ||
// +kubebuilder:validation:MaxLength=30 | ||
// +kubebuilder:validation:XValidation:rule="self.startsWith('ipv4pool-ec2-')",message="publicIpv4Pool must start with 'ipv4pool-ec2-'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is present as an XValidation rule and in the webhook; do we need both?
Bringing your own Public IPv4 Pool (BYOIPv4) can be used as an alternative to buying Public IPs from AWS, also considering the changes in charging for this since February 2024[2]. | ||
|
||
Supported resources to BYO Public IPv4 Pool (`BYO Public IPv4`): | ||
- Nat Gateways |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Nat Gateways | |
- NAT Gateways |
@@ -2141,7 +2141,7 @@ func TestCreateInstance(t *testing.T) { | |||
AvailabilityZone: aws.String("us-east-1b"), | |||
MapPublicIpOnLaunch: aws.Bool(true), | |||
}}, | |||
}, nil) | |||
}, nil).MaxTimes(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set this max? To fail earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the mock was failing due to it being called more than once. I believe the intent here was address that since it is supposed to be called twice.
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4bba41a
to
3648331
Compare
Introducing support of BYO Public IPv4 Pool to allow CAPA allocate IPv4 Elastic IPs from user-provided IPv4 pools that was brought to AWS when provisioning base cluster infrastructure. This change introduce the API fields: - AWSCLuster NetworkSpec.ElasticIPPool: allowing the controllers to consume from user-provided public pools when provisioning core components from the infrastructure, like Nat Gateways and public Network Load Balancer (API server only) - AWSMachine ElasticIPPool: allowing the machine to consume from BYO Public IPv4 pool when the instance is deployed in the public subnets. The ElasticIPPool structure defines a custom IPv4 Pool (previously created in the AWS Account) to teach controllers to set the pool when creating public ip addresses (Elastic IPs) for components which requires it, such as Nat Gateways and NLBs.
/test pull-cluster-api-provider-aws-test |
3648331
to
a62b90d
Compare
What type of PR is this?
/kind feature
/kind api-change
/kind documentation
What this PR does / why we need it:
Introducing support of PublicIpv4Pool to provision base cluster infrastructure consuming public IPv4 (EIP) from a custom Public IPv4 pool brought to AWS.
A subset of changes of this PR is isolated into those PRs:
Which issue(s) this PR fixes
Fixes #4887
Special notes for your reviewer:
The changes proposes to create Elastic IPs for each resource and zone claiming public IP address when creating the core infrastructure components and Machines in public subnets. The components supported for core infrastructure are Nat Gateways and Network Load Balancer for API server.
Checklist:
Release note: