Added validations and templates for multiple networks configuration #453
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @harikrishna-patnala. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/cloud/instance.go
Outdated
| func (c *client) isIpAvailableInNetwork(ip, networkID string) (bool, error) { | ||
| params := c.cs.Address.NewListPublicIpAddressesParams() | ||
| params.SetNetworkid(networkID) | ||
| params.SetIpaddress(ip) | ||
| params.SetAllocatedonly(false) | ||
| params.SetForvirtualnetwork(false) | ||
| params.SetListall(true) | ||
|
|
||
| resp, err := c.cs.Address.ListPublicIpAddresses(params) | ||
| if err != nil { | ||
| return false, errors.Wrapf(err, "failed to list public IP addresses for network %q", networkID) | ||
| } | ||
|
|
||
| for _, addr := range resp.PublicIpAddresses { | ||
| if addr.State == "Free" { | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| func (c *client) hasFreeIPInNetwork(resolvedNet *cloudstack.Network) (bool, error) { | ||
| params := c.cs.Address.NewListPublicIpAddressesParams() | ||
| params.SetNetworkid(resolvedNet.Id) | ||
| params.SetAllocatedonly(false) | ||
| params.SetForvirtualnetwork(false) | ||
| params.SetListall(true) | ||
|
|
||
| resp, err := c.cs.Address.ListPublicIpAddresses(params) | ||
| if err != nil { | ||
| return false, errors.Wrapf(err, "failed to check free IPs for network %q", resolvedNet.Id) | ||
| } | ||
|
|
||
| for _, addr := range resp.PublicIpAddresses { | ||
| if addr.State == "Free" { | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| func (c *client) buildStaticIPEntry(ip, networkID string, resolvedNet *cloudstack.Network) (map[string]string, error) { | ||
| if err := c.validateIPInCIDR(ip, resolvedNet, networkID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if resolvedNet.Type == "Shared" { | ||
| isAvailable, err := c.isIpAvailableInNetwork(ip, networkID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !isAvailable { | ||
| return nil, errors.Errorf("IP %q is already allocated in network %q or out of range", ip, networkID) | ||
| } | ||
| } | ||
|
|
||
| return map[string]string{ | ||
| "networkid": networkID, | ||
| "ip": ip, | ||
| }, nil | ||
| } | ||
|
|
||
| func (c *client) buildDynamicIPEntry(resolvedNet *cloudstack.Network) (map[string]string, error) { | ||
| if resolvedNet.Type == "Shared" { | ||
| freeIPExists, err := c.hasFreeIPInNetwork(resolvedNet) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !freeIPExists { | ||
| return nil, errors.Errorf("no free IPs available in network %q", resolvedNet.Id) | ||
| } | ||
| } | ||
|
|
||
| return map[string]string{ | ||
| "networkid": resolvedNet.Id, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
I think we can merge these 4 methods.
There was a problem hiding this comment.
I am not sure if we have covered each and every type of network possible with cloudstack here. So, this might require more checks as well.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
- Coverage 25.66% 25.21% -0.46%
==========================================
Files 59 72 +13
Lines 5563 6941 +1378
==========================================
+ Hits 1428 1750 +322
- Misses 3996 5022 +1026
- Partials 139 169 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Setting up environment failed |
1 similar comment
|
Setting up environment failed |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances multi-network support by adding validation logic, status updates, and configuration templates for assigning multiple NICs and IPs in CloudStack.
- Iterate over all NICs to populate
csMachine.Status.Addresses - Introduce helper methods for network resolution, IP-in-CIDR validation, and free-IP checks
- Update deployment logic and documentation to support multiple networks
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/cloud/instance.go | Populate multiple NIC addresses; add network/IP helper methods and integrate into VM deploy |
| docs/book/src/clustercloudstack/configuration.md | Add “Option for Multiple Networks” section with template example |
Comments suppressed due to low confidence (1)
pkg/cloud/instance.go:339
- New functions like
buildIPEntry,validateIPInCIDR,isFreeIPAvailable, andconfigureNetworkParamsintroduce nontrivial logic but lack unit tests. Adding tests for valid/invalid IPs, network mismatches, and no-free-IP scenarios will improve reliability.
func (c *client) buildIPEntry(resolvedNet *cloudstack.Network, ip string) (map[string]string, error) {
|
Setting up environment failed |
1 similar comment
|
Setting up environment failed |
|
Test Results : (tid-883) |
|
Test Results : (tid-881) |
|
Test Results : (tid-885) |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harikrishna-patnala, vishesh92 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 |
k8s-ci-robot
added
the
approved
Description of changes:
This PR related to #448, here we are adding multiple validations, csMachine status updates for the network and IPs, and a template for explaining configuration for multiple nics.
Testing performed:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.