Add FrontendVPCs support to NodeBalancers #857
Conversation
- Add `FrontendAddressType` and `FrontendVPCSubnetID` fields to `NodeBalancer` struct - Add `FrontendVPCs` field to `NodeBalancerCreateOptions` struct - Add clarifying comment to `IPv4RangeAutoAssign` field in `NodeBalancerVPCOptions`
komer3
requested review from
Copilot,
yec-akamai and
zliang-akamai
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR adds support for frontend VPC configuration to NodeBalancers, enabling VPC-based frontend addressing alongside the existing backend VPC support. The changes introduce new fields to track frontend VPC subnet information and allow frontend VPC specification during NodeBalancer creation.
Key changes:
- Added
FrontendAddressTypeandFrontendVPCSubnetIDfields to track frontend VPC configuration - Added
FrontendVPCsoption to enable frontend VPC specification during creation - Clarified that
IPv4RangeAutoAssignapplies only to backend VPC configuration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Tags []string `json:"tags"` | ||
| FirewallID int `json:"firewall_id,omitempty"` | ||
| Type NodeBalancerPlanType `json:"type,omitempty"` | ||
| VPCs []NodeBalancerVPCOptions `json:"vpcs,omitempty"` |
There was a problem hiding this comment.
Thoughts on adding a new BackendVPCs field and marking the VPCs field as deprecated in favor of the BackendVPCs field?
There was a problem hiding this comment.
That is a good idea and makes things less ambiguous, but I'm leaning towards keeping the field naming the same as what the API returns.
| // Frontend address type (e.g., "vpc") | ||
| FrontendAddressType *string `json:"frontend_address_type,omitempty"` | ||
| // Frontend VPC subnet ID when using VPC addressing | ||
| FrontendVPCSubnetID *int `json:"frontend_vpc_subnet_id,omitempty"` |
There was a problem hiding this comment.
I know this PR is for adding frontendVPC support, but why don't we already have (Backend)VPC fields?
There was a problem hiding this comment.
Also, I didn't see this field used in your CCM PR, is this field necessary?
There was a problem hiding this comment.
These field a primarily used for returned response body!
There was a problem hiding this comment.
Ended up using these field in the CCM implementation for filtering NBs from the list of NBs. There could be many NBs with the same IPv4 address but have different subnetIDs.
zliang-akamai
force-pushed
the
proj/frontend-vpcs
branch
from
fb0a239 to
8a0a88c
Compare
| FrontendAddressType *string `json:"frontend_address_type,omitempty"` | ||
| // Frontend VPC subnet ID when using VPC addressing | ||
| FrontendVPCSubnetID *int `json:"frontend_vpc_subnet_id,omitempty"` |
There was a problem hiding this comment.
If they are not nullable, can we have them as non-pointer types?
| SubnetID int `json:"subnet_id"` | ||
| // IPv4RangeAutoAssign is only used for backend VPC configuration. | ||
| // For frontend VPCs, this field is ignored. | ||
| IPv4RangeAutoAssign bool `json:"ipv4_range_auto_assign,omitempty"` |
There was a problem hiding this comment.
Couldn't find this attribute in the API spec doc. Is the doc outdated?
| IPv4Range string `json:"ipv4_range,omitempty"` | ||
| IPv6Range string `json:"ipv6_range,omitempty"` |
There was a problem hiding this comment.
If these fields are optional, can we have them as pointer types?
3 participants
FrontendAddressTypeandFrontendVPCSubnetIDfields toNodeBalancerstructFrontendVPCsfield toNodeBalancerCreateOptionsstructIPv4RangeAutoAssignfield inNodeBalancerVPCOptions📝 Description
What does this PR do and why is this change necessary?
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
How do I run the relevant unit/integration tests?
📷 Preview
If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.