Skip to content

[feat] : add ipv6 ipam to CCM #369

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[feat] : add ipv6 ipam to CCM #369

wants to merge 7 commits into from

Conversation

rahulait
Copy link
Collaborator

@rahulait rahulait commented Apr 4, 2025

General:

This PR enables ipv6 ipam allocation to nodes. If CCM is started with --allocate-node-cidrs, it will allocate both ipv4 and ipv6 pod CIDRs to nodes.

This PR adds IPv6 IP Address Management (IPAM) support to the cloud controller manager by extending the node IPAM allocator to compute and assign unique /112 IPv6 ranges from each node’s /64, updating Linode client interfaces and tests accordingly.

Introduce a new cloud/nodeipam controller and ipam package that allocates both IPv4 and IPv6 PodCIDRs.
Extend Linode client API (ListInstanceConfigs) and update mocks, metrics, and go.mod deps to support IPv6 range retrieval.
Update the Linode-specific nodeipamcontroller to pass IPv6 mask defaults and enforce only one IPv4 cluster CIDR.

Note
In this PR, we are adding a function to calculate unique /112 ipv6 cidr range for each node from the /64 cidr allocated to it. Dependencies for starting CCM with --allocate-node-cidrs flag:

  1. CCM must be running within the VPC
  2. VPC must be configured to have ipv6 range allocated.
  3. Subnet to which linode is attached should also have ipv6 range allocated.
  4. Linode must request for auto-allocation of ipv6 range.
  5. We currently support legacy interfaces. We will add support for newer interfaces in future PRs.

Sample CAPL PR which does the same: linode/cluster-api-provider-linode#773

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

@github-actions github-actions bot added the new-feature for new features in the changelog. label Apr 4, 2025
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

Attention: Patch coverage is 51.23596% with 217 lines in your changes missing coverage. Please review.

Project coverage is 72.10%. Comparing base (280f94b) to head (4ca7bff).

Files with missing lines Patch % Lines
cloud/nodeipam/ipam/cloud_allocator.go 45.82% 145 Missing and 30 partials ⚠️
cloud/nodeipam/node_ipam_controller.go 63.33% 17 Missing and 5 partials ⚠️
cloud/nodeipam/ipam/cidr_allocator.go 60.86% 15 Missing and 3 partials ⚠️
cloud/linode/nodeipamcontroller.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   75.39%   72.10%   -3.29%     
==========================================
  Files          16       19       +3     
  Lines        2983     3402     +419     
==========================================
+ Hits         2249     2453     +204     
- Misses        545      722     +177     
- Partials      189      227      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported
Comments suppressed due to low confidence (1)

cloud/linode/nodeipamcontroller.go:45

  • The default IPv6 node CIDR mask size has been changed from 64 to 112. Please confirm that this new value aligns with the desired network design and update any related documentation if necessary.
defaultNodeMaskCIDRIPv6 = 112

@rahulait rahulait requested a review from Copilot April 7, 2025 14:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported

@rahulait rahulait requested a review from Copilot April 7, 2025 16:07
Copilot

This comment was marked as outdated.

@rahulait rahulait force-pushed the add-ipv6-ipam-support branch from 9a369d3 to 3d7f16a Compare July 2, 2025 04:10
@rahulait rahulait requested a review from Copilot July 2, 2025 04:15
@rahulait rahulait changed the title [feat][wip] : add ipv6 ipam to CCM [feat] : add ipv6 ipam to CCM Jul 2, 2025
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds IPv6 IPAM support to the Linode cloud controller manager by introducing a new node IPAM controller and extending the existing Linode client interfaces to retrieve IPv6 configuration.

  • Introduce cloud/nodeipam package with a dual-stack allocator that assigns both IPv4 and IPv6 PodCIDRs.
  • Extend Client interface with ListInstanceConfigs, update mocks, metrics, and go.mod dependencies for IPv6 support.
  • Refactor cloud/linode/nodeipamcontroller.go to enforce a single IPv4 cluster CIDR, set default IPv6 mask to /112, and wire in the new allocator.

Reviewed Changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
go.mod Bumped indirect dependencies, added Gomega, replaced LinodeGo fork
cloud/nodeipam/node_ipam_controller.go New top-level IPAM controller coordinating IPv4/IPv6 allocators
cloud/nodeipam/ipam/ New IPAM package (allocators, utils, tests, docs)
cloud/linode/nodeipamcontroller.go Refactored startup, processCIDRs, default masks, signature change
cloud/linode/client/client.go Added ListInstanceConfigs to the Client interface
cloud/linode/client/client_with_metrics.go Instrumented ListInstanceConfigs
cloud/linode/client/mocks/mock_client.go Added mock methods for ListInstanceConfigs
Makefile Expanded test coverage to include IPAM packages
Comments suppressed due to low confidence (3)

cloud/nodeipam/ipam/cloud_allocator.go:335

  • [nitpick] Consider adding unit tests for allocateIPv6CIDR to verify correct IPv6 parsing and that each node receives a unique /112 range.
		return nil, fmt.Errorf("node %s has invalid ProviderID %s, expected prefix '%s'", node.Name, node.Spec.ProviderID, providerIDPrefix)

cloud/linode/nodeipamcontroller.go:47

  • [nitpick] The signature of startNodeIpamController changed from the generic cloudprovider.Interface to a concrete *linodeCloud type. This may reduce flexibility; consider keeping the interface abstraction if other clouds or unit tests rely on the original interface.
func startNodeIpamController(stopCh <-chan struct{}, cloud *linodeCloud, nodeInformer v1.NodeInformer, kubeclient kubernetes.Interface) error {

cloud/linode/nodeipamcontroller_test.go:33

  • [nitpick] Tests for processCIDRs and setNodeCIDRMaskSizes no longer cover IPv6 or dual-stack scenarios. Consider adding tests to validate IPv6 parsing and ensure mask sizes are applied correctly in both single- and dual-stack configurations.
func Test_setNodeCIDRMaskSizes(t *testing.T) {

return fmt.Errorf("failed to allocate cidr from cluster cidr: %w", err)
}
allocatedCIDRs[0] = podCIDR
if allocatedCIDRs[1], err = c.allocateIPv6CIDR(ctx, node); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add logic to enable/disable ipv6 cidr allocation via flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature for new features in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant