-
Notifications
You must be signed in to change notification settings - Fork 96
Add FrontendVPCs support to NodeBalancers #857
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
base: proj/frontend-vpcs
Are you sure you want to change the base?
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I didn't see this field used in your CCM PR, is this field necessary?
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.
These field a primarily used for returned response body!
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.
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.
* Support for Entities and IAM endpoints (linode#754) ## 📝 Description **What does this PR do and why is this change necessary?** Implements: - **GET** /entities - **GET** /iam/role-permissions - **GET** /iam/users/{username}/role-permissions - **PUT** /iam/users/{username}/role-permissions ## ✔️ How to Test **How do I run the relevant unit/integration tests?** ```bash make TEST_ARGS='-run TestIAM' test-unit ``` ```bash make TEST_ARGS='-run TestEntities' test-unit ``` * IO Ready and new IAM Endpoints (linode#864) ## 📝 Description Adds `io_ready` field to Volumes and 2 new endpoints for IAM ## ✔️ How to Test ``` make test-unit ``` * pr feedback --------- Co-authored-by: Zhiwei Liang <[email protected]>
* Add `premium_40GB` NodeBalancer plan type * typo fix --------- Co-authored-by: Zhiwei Liang <[email protected]>
* DI-27070 Add support for ACLP alerting * Fix lint * Fix deprecation formatting * Fix lint * fixed review comments * fixed review comments * Delete request_helpers.go * fixed review comments * fixed review comments * fix format * formatting issues fixed * Fixed test failures * reverted change to go.mod * fixed review comments * Added new test cases * Fixed review comments * Ran make tidy * fixed review comments from ye-chen and copilot * Apply suggestion from @yec-akamai Co-authored-by: Ye Chen <[email protected]> * fixed fixtures and other review comments * fixed comments and lint errors * fix fixtures * fix alert channel type * import resort --------- Co-authored-by: Lena Garber <[email protected]> Co-authored-by: ezilber-akamai <[email protected]> Co-authored-by: Ye Chen <[email protected]> Co-authored-by: Ye Chen <[email protected]>
* ACLP stuff * revert not needed changes to unit test base * test fixes * fixtures fix
* Re-enable passed linters * Refresh fixtures for TestInstance_GetMonthlyTransfer
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.