[NCP] Add features to find and apply available CIDR for LB type subnet #1606
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cb-spider
approved these changes
Nov 4, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves the NLB (Network Load Balancer) subnet creation logic for the NCP (Naver Cloud Platform) driver by implementing dynamic CIDR allocation to prevent subnet conflicts. Previously, the code used a hardcoded CIDR suffix (.240/28) which could cause conflicts with existing subnets. The new implementation scans existing subnets and allocates an available CIDR block dynamically.
- Implements conflict-free CIDR allocation algorithm for NLB subnets
- Refactors constant names for better consistency (e.g.,
NcpLbType→NcpNetworkLbType) - Removes hardcoded CIDR suffix and adds intelligent CIDR selection logic
Comment on lines
+125
to
+132
| // Find available CIDR for NLB subnet | ||
| availableCIDR, err := nlbHandler.findAvailableCIDRForNLBSubnet(*ncpVPCInfo.VpcNo, *ncpVPCInfo.Ipv4CidrBlock) | ||
| if err != nil { | ||
| newErr := fmt.Errorf("Failed to find available CIDR for NLB subnet: [%v]", err) | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return irs.NLBInfo{}, newErr | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: lines 125-132 use spaces while the rest of the file uses tabs. Please convert spaces to tabs to maintain consistency with the codebase style.
Suggested change
| // Find available CIDR for NLB subnet | |
| availableCIDR, err := nlbHandler.findAvailableCIDRForNLBSubnet(*ncpVPCInfo.VpcNo, *ncpVPCInfo.Ipv4CidrBlock) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to find available CIDR for NLB subnet: [%v]", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return irs.NLBInfo{}, newErr | |
| } | |
| // Find available CIDR for NLB subnet | |
| availableCIDR, err := nlbHandler.findAvailableCIDRForNLBSubnet(*ncpVPCInfo.VpcNo, *ncpVPCInfo.Ipv4CidrBlock) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to find available CIDR for NLB subnet: [%v]", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return irs.NLBInfo{}, newErr | |
| } |
| NameId: lbTypeSubnetName, | ||
| }, | ||
| IPv4_CIDR: cidrForNlbSubnet, | ||
| IPv4_CIDR: availableCIDR, |
There was a problem hiding this comment.
Inconsistent whitespace: there's a tab character between the colon and value. Please use consistent spacing to align with other struct field assignments.
Suggested change
| IPv4_CIDR: availableCIDR, | |
| IPv4_CIDR: availableCIDR, |
Comment on lines
+1665
to
+1727
| cblogger.Info("NCP VPC Cloud Driver: called findAvailableCIDRForNLBSubnet()") | ||
| InitLog() | ||
| callLogInfo := GetCallLogScheme(nlbHandler.RegionInfo.Region, "NETWORKLOADBALANCE", vpcId, "findAvailableCIDRForNLBSubnet()") | ||
|
|
||
| if strings.EqualFold(vpcId, "") { | ||
| newErr := fmt.Errorf("Invalid VPC ID") | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return "", newErr | ||
| } | ||
|
|
||
| // Get existing subnet list | ||
| vpcHandler := NcpVpcVPCHandler{ | ||
| RegionInfo: nlbHandler.RegionInfo, | ||
| VPCClient: nlbHandler.VPCClient, | ||
| } | ||
| subnetInfoList, err := vpcHandler.ListSubnet(&vpcId) | ||
| if err != nil { | ||
| newErr := fmt.Errorf("Failed to Get Subnet List: %v", err) | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return "", newErr | ||
| } | ||
|
|
||
| // Collect existing subnet CIDRs | ||
| existingCIDRs := make([]string, 0) | ||
| for _, subnet := range subnetInfoList { | ||
| existingCIDRs = append(existingCIDRs, subnet.IPv4_CIDR) | ||
| } | ||
|
|
||
| // Parse VPC CIDR | ||
| _, vpcNet, err := net.ParseCIDR(vpcCIDR) | ||
| if err != nil { | ||
| newErr := fmt.Errorf("Failed to Parse VPC CIDR [%s]: %v", vpcCIDR, err) | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return "", newErr | ||
| } | ||
|
|
||
| // NLB subnet requires /28 (16 IP addresses) | ||
| nlbSubnetMask := 28 | ||
|
|
||
| // Generate candidate CIDR blocks within the VPC range | ||
| candidateCIDRs, err := nlbHandler.generateCandidateCIDRs(vpcNet, nlbSubnetMask) | ||
| if err != nil { | ||
| newErr := fmt.Errorf("Failed to Generate Candidate CIDRs: %v", err) | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return "", newErr | ||
| } | ||
|
|
||
| // Find first available CIDR that doesn't conflict with existing subnets | ||
| for _, candidateCIDR := range candidateCIDRs { | ||
| if !nlbHandler.isCIDRConflict(candidateCIDR, existingCIDRs) { | ||
| cblogger.Infof("Found available CIDR for NLB subnet: %s", candidateCIDR) | ||
| return candidateCIDR, nil | ||
| } | ||
| } | ||
|
|
||
| newErr := fmt.Errorf("No available CIDR block found for NLB subnet in VPC %s", vpcCIDR) | ||
| cblogger.Error(newErr.Error()) | ||
| LoggingError(callLogInfo, newErr) | ||
| return "", newErr |
There was a problem hiding this comment.
Inconsistent indentation: lines 1665-1727 use spaces while the rest of the file uses tabs. Please convert all spaces to tabs to maintain consistency with the codebase.
Suggested change
| cblogger.Info("NCP VPC Cloud Driver: called findAvailableCIDRForNLBSubnet()") | |
| InitLog() | |
| callLogInfo := GetCallLogScheme(nlbHandler.RegionInfo.Region, "NETWORKLOADBALANCE", vpcId, "findAvailableCIDRForNLBSubnet()") | |
| if strings.EqualFold(vpcId, "") { | |
| newErr := fmt.Errorf("Invalid VPC ID") | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Get existing subnet list | |
| vpcHandler := NcpVpcVPCHandler{ | |
| RegionInfo: nlbHandler.RegionInfo, | |
| VPCClient: nlbHandler.VPCClient, | |
| } | |
| subnetInfoList, err := vpcHandler.ListSubnet(&vpcId) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Get Subnet List: %v", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Collect existing subnet CIDRs | |
| existingCIDRs := make([]string, 0) | |
| for _, subnet := range subnetInfoList { | |
| existingCIDRs = append(existingCIDRs, subnet.IPv4_CIDR) | |
| } | |
| // Parse VPC CIDR | |
| _, vpcNet, err := net.ParseCIDR(vpcCIDR) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Parse VPC CIDR [%s]: %v", vpcCIDR, err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // NLB subnet requires /28 (16 IP addresses) | |
| nlbSubnetMask := 28 | |
| // Generate candidate CIDR blocks within the VPC range | |
| candidateCIDRs, err := nlbHandler.generateCandidateCIDRs(vpcNet, nlbSubnetMask) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Generate Candidate CIDRs: %v", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Find first available CIDR that doesn't conflict with existing subnets | |
| for _, candidateCIDR := range candidateCIDRs { | |
| if !nlbHandler.isCIDRConflict(candidateCIDR, existingCIDRs) { | |
| cblogger.Infof("Found available CIDR for NLB subnet: %s", candidateCIDR) | |
| return candidateCIDR, nil | |
| } | |
| } | |
| newErr := fmt.Errorf("No available CIDR block found for NLB subnet in VPC %s", vpcCIDR) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| cblogger.Info("NCP VPC Cloud Driver: called findAvailableCIDRForNLBSubnet()") | |
| InitLog() | |
| callLogInfo := GetCallLogScheme(nlbHandler.RegionInfo.Region, "NETWORKLOADBALANCE", vpcId, "findAvailableCIDRForNLBSubnet()") | |
| if strings.EqualFold(vpcId, "") { | |
| newErr := fmt.Errorf("Invalid VPC ID") | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Get existing subnet list | |
| vpcHandler := NcpVpcVPCHandler{ | |
| RegionInfo: nlbHandler.RegionInfo, | |
| VPCClient: nlbHandler.VPCClient, | |
| } | |
| subnetInfoList, err := vpcHandler.ListSubnet(&vpcId) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Get Subnet List: %v", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Collect existing subnet CIDRs | |
| existingCIDRs := make([]string, 0) | |
| for _, subnet := range subnetInfoList { | |
| existingCIDRs = append(existingCIDRs, subnet.IPv4_CIDR) | |
| } | |
| // Parse VPC CIDR | |
| _, vpcNet, err := net.ParseCIDR(vpcCIDR) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Parse VPC CIDR [%s]: %v", vpcCIDR, err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // NLB subnet requires /28 (16 IP addresses) | |
| nlbSubnetMask := 28 | |
| // Generate candidate CIDR blocks within the VPC range | |
| candidateCIDRs, err := nlbHandler.generateCandidateCIDRs(vpcNet, nlbSubnetMask) | |
| if err != nil { | |
| newErr := fmt.Errorf("Failed to Generate Candidate CIDRs: %v", err) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr | |
| } | |
| // Find first available CIDR that doesn't conflict with existing subnets | |
| for _, candidateCIDR := range candidateCIDRs { | |
| if !nlbHandler.isCIDRConflict(candidateCIDR, existingCIDRs) { | |
| cblogger.Infof("Found available CIDR for NLB subnet: %s", candidateCIDR) | |
| return candidateCIDR, nil | |
| } | |
| } | |
| newErr := fmt.Errorf("No available CIDR block found for NLB subnet in VPC %s", vpcCIDR) | |
| cblogger.Error(newErr.Error()) | |
| LoggingError(callLogInfo, newErr) | |
| return "", newErr |
Comment on lines
+1732
to
+1767
| cblogger.Info("NCP VPC Cloud Driver: called generateCandidateCIDRs()") | ||
|
|
||
| candidates := make([]string, 0) | ||
|
|
||
| // Get VPC network size | ||
| vpcOnes, vpcBits := vpcNet.Mask.Size() | ||
| if vpcOnes > subnetMask { | ||
| return nil, fmt.Errorf("VPC mask /%d is larger than required subnet mask /%d", vpcOnes, subnetMask) | ||
| } | ||
|
|
||
| // Calculate number of possible subnets | ||
| subnetCount := 1 << uint(subnetMask-vpcOnes) | ||
|
|
||
| // Calculate subnet size | ||
| subnetSize := 1 << uint(vpcBits-subnetMask) | ||
|
|
||
| // Generate candidate CIDRs | ||
| baseIP := vpcNet.IP | ||
| for i := 0; i < subnetCount; i++ { | ||
| // Calculate subnet IP | ||
| subnetIP := make(net.IP, len(baseIP)) | ||
| copy(subnetIP, baseIP) | ||
|
|
||
| // Add offset | ||
| offset := i * subnetSize | ||
| addOffset(subnetIP, offset) | ||
|
|
||
| // Check if subnet IP is within VPC range | ||
| if vpcNet.Contains(subnetIP) { | ||
| candidateCIDR := fmt.Sprintf("%s/%d", subnetIP.String(), subnetMask) | ||
| candidates = append(candidates, candidateCIDR) | ||
| } | ||
| } | ||
|
|
||
| cblogger.Infof("Generated %d candidate CIDRs", len(candidates)) | ||
| return candidates, nil |
There was a problem hiding this comment.
Inconsistent indentation: lines 1732-1767 use spaces while the rest of the file uses tabs. Please convert all spaces to tabs to maintain consistency with the codebase.
Suggested change
| cblogger.Info("NCP VPC Cloud Driver: called generateCandidateCIDRs()") | |
| candidates := make([]string, 0) | |
| // Get VPC network size | |
| vpcOnes, vpcBits := vpcNet.Mask.Size() | |
| if vpcOnes > subnetMask { | |
| return nil, fmt.Errorf("VPC mask /%d is larger than required subnet mask /%d", vpcOnes, subnetMask) | |
| } | |
| // Calculate number of possible subnets | |
| subnetCount := 1 << uint(subnetMask-vpcOnes) | |
| // Calculate subnet size | |
| subnetSize := 1 << uint(vpcBits-subnetMask) | |
| // Generate candidate CIDRs | |
| baseIP := vpcNet.IP | |
| for i := 0; i < subnetCount; i++ { | |
| // Calculate subnet IP | |
| subnetIP := make(net.IP, len(baseIP)) | |
| copy(subnetIP, baseIP) | |
| // Add offset | |
| offset := i * subnetSize | |
| addOffset(subnetIP, offset) | |
| // Check if subnet IP is within VPC range | |
| if vpcNet.Contains(subnetIP) { | |
| candidateCIDR := fmt.Sprintf("%s/%d", subnetIP.String(), subnetMask) | |
| candidates = append(candidates, candidateCIDR) | |
| } | |
| } | |
| cblogger.Infof("Generated %d candidate CIDRs", len(candidates)) | |
| return candidates, nil | |
| cblogger.Info("NCP VPC Cloud Driver: called generateCandidateCIDRs()") | |
| candidates := make([]string, 0) | |
| // Get VPC network size | |
| vpcOnes, vpcBits := vpcNet.Mask.Size() | |
| if vpcOnes > subnetMask { | |
| return nil, fmt.Errorf("VPC mask /%d is larger than required subnet mask /%d", vpcOnes, subnetMask) | |
| } | |
| // Calculate number of possible subnets | |
| subnetCount := 1 << uint(subnetMask-vpcOnes) | |
| // Calculate subnet size | |
| subnetSize := 1 << uint(vpcBits-subnetMask) | |
| // Generate candidate CIDRs | |
| baseIP := vpcNet.IP | |
| for i := 0; i < subnetCount; i++ { | |
| // Calculate subnet IP | |
| subnetIP := make(net.IP, len(baseIP)) | |
| copy(subnetIP, baseIP) | |
| // Add offset | |
| offset := i * subnetSize | |
| addOffset(subnetIP, offset) | |
| // Check if subnet IP is within VPC range | |
| if vpcNet.Contains(subnetIP) { | |
| candidateCIDR := fmt.Sprintf("%s/%d", subnetIP.String(), subnetMask) | |
| candidates = append(candidates, candidateCIDR) | |
| } | |
| } | |
| cblogger.Infof("Generated %d candidate CIDRs", len(candidates)) | |
| return candidates, nil |
Comment on lines
+1772
to
+1776
| for i := len(ip) - 1; i >= 0 && offset > 0; i-- { | ||
| current := int(ip[i]) + offset | ||
| ip[i] = byte(current & 0xFF) | ||
| offset = current >> 8 | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: lines 1772-1776 use spaces while the rest of the file uses tabs. Please convert all spaces to tabs to maintain consistency with the codebase.
Suggested change
| for i := len(ip) - 1; i >= 0 && offset > 0; i-- { | |
| current := int(ip[i]) + offset | |
| ip[i] = byte(current & 0xFF) | |
| offset = current >> 8 | |
| } | |
| for i := len(ip) - 1; i >= 0 && offset > 0; i-- { | |
| current := int(ip[i]) + offset | |
| ip[i] = byte(current & 0xFF) | |
| offset = current >> 8 | |
| } |
| // Note : Create a 'LOADB' type of subnet ('LB Type' subnet for LB Only) | ||
|
|
||
| // Create a 'LOADB' type of subnet ('LB Type' subnet for LB Only) | ||
| ncpNlbSubnetInfo, err := nlbHandler.creatNcpSubnetForNlbOnly(irs.IID{SystemId: *ncpVPCInfo.VpcNo}, subnetReqInfo) // Waitting time Included |
There was a problem hiding this comment.
Corrected spelling of 'Waitting' to 'Waiting'.
Suggested change
| ncpNlbSubnetInfo, err := nlbHandler.creatNcpSubnetForNlbOnly(irs.IID{SystemId: *ncpVPCInfo.VpcNo}, subnetReqInfo) // Waitting time Included | |
| ncpNlbSubnetInfo, err := nlbHandler.creatNcpSubnetForNlbOnly(irs.IID{SystemId: *ncpVPCInfo.VpcNo}, subnetReqInfo) // Waiting time Included |
| } | ||
|
|
||
| // NLB subnet requires /28 (16 IP addresses) | ||
| nlbSubnetMask := 28 |
There was a problem hiding this comment.
The hardcoded subnet mask value (28) should be defined as a constant at the package level for maintainability. This follows the pattern used for other NLB-related constants like DefaultThroughputType and LbTypeSubnetDefaultName.
|
|
||
| // Checks if candidate CIDR conflicts with any existing CIDRs | ||
| func (nlbHandler *NcpVpcNLBHandler) isCIDRConflict(candidateCIDR string, existingCIDRs []string) bool { | ||
| cblogger.Debug("Checking CIDR conflict for:", candidateCIDR) |
There was a problem hiding this comment.
Logging format is inconsistent with cb-log best practices. Use structured logging with proper format string: cblogger.Debugf(\"Checking CIDR conflict for: %s\", candidateCIDR) instead of passing multiple arguments to Debug.
Suggested change
| cblogger.Debug("Checking CIDR conflict for:", candidateCIDR) | |
| cblogger.Debugf("Checking CIDR conflict for: %s", candidateCIDR) |
|
|
||
| _, candidateNet, err := net.ParseCIDR(candidateCIDR) | ||
| if err != nil { | ||
| cblogger.Error("Failed to parse candidate CIDR:", err) |
There was a problem hiding this comment.
Logging format is inconsistent with cb-log best practices. Use structured logging with proper format string: cblogger.Errorf(\"Failed to parse candidate CIDR: %v\", err) instead of passing multiple arguments to Error.
Suggested change
| cblogger.Error("Failed to parse candidate CIDR:", err) | |
| cblogger.Errorf("Failed to parse candidate CIDR: %v", err) |
| for _, existingCIDR := range existingCIDRs { | ||
| _, existingNet, err := net.ParseCIDR(existingCIDR) | ||
| if err != nil { | ||
| cblogger.Warn("Failed to parse existing CIDR:", existingCIDR, err) |
There was a problem hiding this comment.
Logging format is inconsistent with cb-log best practices. Use structured logging with proper format string: cblogger.Warnf(\"Failed to parse existing CIDR %s: %v\", existingCIDR, err) instead of passing multiple arguments to Warn.
Suggested change
| cblogger.Warn("Failed to parse existing CIDR:", existingCIDR, err) | |
| cblogger.Warnf("Failed to parse existing CIDR %s: %v", existingCIDR, err) |
Member
Author
|
Since the above PR has already been merged, inconsistent indentations will be addressed in the next PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[ CB-Tumblebug error message that occurred before modifying ]
[ Example of dynamically assigned LB type subnet range (After modifying) ]
