Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions pkg/allocate/allocate.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func AssignIP(ipamConf types.RangeConfiguration, reservelist []types.IPReservati
}
}

newip, updatedreservelist, err := IterateForAssignment(*ipnet, ipamConf.RangeStart, ipamConf.RangeEnd, reservelist, ipamConf.OmitRanges, containerID, podRef, ifName)
newip, updatedreservelist, err := IterateForAssignment(*ipnet, ipamConf.RangeStart, ipamConf.RangeEnd, ipamConf.PickAddresses, reservelist, ipamConf.OmitRanges, containerID, podRef, ifName)
if err != nil {
return net.IPNet{}, nil, err
}
Expand Down Expand Up @@ -83,15 +83,15 @@ func removeIdxFromSlice(s []types.IPReservation, i int) []types.IPReservation {
// If rangeEnd is specified, it is respected if it lies within the ipnet and if it is >= rangeStart.
// reserveList holds a list of reserved IPs.
// excludeRanges holds a list of subnets to be excluded (meaning the full subnet, including the network and broadcast IP).
func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, reserveList []types.IPReservation, excludeRanges []string, containerID, podRef, ifName string) (net.IP, []types.IPReservation, error) {
func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, pickAddr []net.IP, reserveList []types.IPReservation, excludeRanges []string, containerID, podRef, ifName string) (net.IP, []types.IPReservation, error) {
// Get the valid range, delimited by the ipnet's first and last usable IP as well as the rangeStart and rangeEnd.
firstIP, lastIP, err := iphelpers.GetIPRange(ipnet, rangeStart, rangeEnd)
if err != nil {
logging.Errorf("GetIPRange request failed with: %v", err)
return net.IP{}, reserveList, err
}
logging.Debugf("IterateForAssignment input >> range_start: %v | range_end: %v | ipnet: %v | first IP: %v | last IP: %v",
rangeStart, rangeEnd, ipnet.String(), firstIP, lastIP)
logging.Debugf("IterateForAssignment input >> range_start: %v | range_end: %v | pick_list_len: %d | ipnet: %v | first IP: %v | last IP: %v",
rangeStart, rangeEnd, len(pickAddr), ipnet.String(), firstIP, lastIP)

// Build reserved map.
reserved := make(map[string]bool)
Expand All @@ -109,6 +109,46 @@ func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, r
excluded = append(excluded, subnet)
}

// If pickAddr is provided, try to allocate using that list instead of iterating the full range.
if len(pickAddr) > 0 {
for _, candidate := range pickAddr {
if candidate == nil {
continue
}
// Ensure canonical form for comparisons/logging
if v4 := candidate.To4(); v4 != nil {
candidate = v4
} else if v6 := candidate.To16(); v6 != nil {
candidate = v6
}
// Must be inside the primary ipnet
if !ipnet.Contains(candidate) {
continue
}
// Must not be reserved already
if reserved[candidate.String()] {
continue
}
// Must not be within any excluded subnet
skip := false
for _, subnet := range excluded {
if subnet.Contains(candidate) {
skip = true
break
}
}
if skip {
continue
}
// Found a valid candidate, reserve and return
logging.Debugf("Reserving IP from pick list: %q - container ID %q - podRef: %q - ifName: %q", candidate.String(), containerID, podRef, ifName)
reserveList = append(reserveList, types.IPReservation{IP: candidate, ContainerID: containerID, PodRef: podRef, IfName: ifName})
return candidate, reserveList, nil
}
// No valid IPs in pick list; return regular assignment error
return net.IP{}, reserveList, AssignmentError{firstIP, lastIP, ipnet, excludeRanges}
Comment on lines +148 to +149
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error message returned when no valid IPs are found in the pickAddr list could be more informative. Consider including details about why the IPs were invalid (e.g., out of range, reserved, excluded) to aid in debugging.

Also, consider returning a wrapped error that includes the original AssignmentError for better context.

Suggested change
// No valid IPs in pick list; return regular assignment error
return net.IP{}, reserveList, AssignmentError{firstIP, lastIP, ipnet, excludeRanges}
return net.IP{}, reserveList, fmt.Errorf("no valid IP addresses found in pick list: %w", AssignmentError{firstIP, lastIP, ipnet, excludeRanges})

}

// Iterate over every IP address in the range, accounting for reserved IPs and exclude ranges. Make sure that ip is
// within ipnet, and make sure that ip is smaller than lastIP.
for ip := firstIP; ipnet.Contains(ip) && iphelpers.CompareIPs(ip, lastIP) <= 0; ip = iphelpers.IncIP(ip) {
Expand Down
114 changes: 91 additions & 23 deletions pkg/allocate/allocate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,80 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
var exrange []string
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("192.168.1.1"))

})

Context("pickAddr selection", func() {
It("selects the first valid IP from the pick list", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

pick := []net.IP{net.ParseIP("10.0.0.50"), net.ParseIP("10.0.0.60")}
newip, _, err := IterateForAssignment(*ipnet, nil, nil, pick, nil, nil, "cid-1", "pod/ns", "eth0")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("10.0.0.50"))
})

It("skips pick IPs outside of the pool CIDR", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

pick := []net.IP{net.ParseIP("192.168.1.10"), net.ParseIP("10.0.0.5")}
newip, _, err := IterateForAssignment(*ipnet, nil, nil, pick, nil, nil, "cid-2", "pod/ns", "eth0")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("10.0.0.5"))
})

It("skips already reserved pick IPs and uses the next candidate", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

ipres := []types.IPReservation{{IP: net.ParseIP("10.0.0.5"), PodRef: "default/pod1"}}
pick := []net.IP{net.ParseIP("10.0.0.5"), net.ParseIP("10.0.0.6")}
newip, _, err := IterateForAssignment(*ipnet, nil, nil, pick, ipres, nil, "cid-3", "pod/ns", "eth0")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("10.0.0.6"))
})

It("honors exclude ranges when evaluating pick list (single IP and CIDR)", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

exrange := []string{"10.0.0.5", "10.0.0.6/31"} // excludes .5 and {.6,.7}
pick := []net.IP{net.ParseIP("10.0.0.5"), net.ParseIP("10.0.0.6"), net.ParseIP("10.0.0.8")}
newip, _, err := IterateForAssignment(*ipnet, nil, nil, pick, nil, exrange, "cid-4", "pod/ns", "eth0")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("10.0.0.8"))
})

It("returns an error when all pick candidates are invalid", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

// .1 is within CIDR but reserved; 192.168.1.10 is out of CIDR
ipres := []types.IPReservation{{IP: net.ParseIP("10.0.0.1"), PodRef: "default/pod1"}}
pick := []net.IP{net.ParseIP("192.168.1.10"), net.ParseIP("10.0.0.1")}
_, _, err = IterateForAssignment(*ipnet, nil, nil, pick, ipres, nil, "cid-5", "pod/ns", "eth0")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Could not allocate IP in range"))
Comment on lines +87 to +88
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error message assertion ContainSubstring("Could not allocate IP in range") is too generic. It would be better to assert against a more specific error message that includes details about the pickAddr list and why the allocation failed. This will make the test more robust and easier to debug.

})

It("allocates a pick candidate even if outside rangeStart/rangeEnd but inside CIDR", func() {
_, ipnet, err := net.ParseCIDR("10.0.0.0/24")
Expect(err).NotTo(HaveOccurred())

rangeStart := net.ParseIP("10.0.0.100")
rangeEnd := net.ParseIP("10.0.0.150")
pick := []net.IP{net.ParseIP("10.0.0.10")}
newip, _, err := IterateForAssignment(*ipnet, rangeStart, rangeEnd, pick, nil, nil, "cid-6", "pod/ns", "eth0")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("10.0.0.10"))
})
})

It("can IterateForAssignment on an IPv6 address when the first hextet has NO leading zeroes", func() {

firstip, ipnet, err := net.ParseCIDR("caa5::0/112")
Expand All @@ -43,7 +111,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
var exrange []string
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("caa5::1"))

Expand All @@ -59,7 +127,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
var exrange []string
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("::1"))

Expand All @@ -77,7 +145,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
var exrange []string
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("fd::1"))

Expand All @@ -93,7 +161,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
var exrange []string
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("100::2:1"))
})
Expand All @@ -108,7 +176,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"192.168.0.0/30"}
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.4"))

})
Expand All @@ -122,7 +190,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"192.168.0.1"}
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.2"))
})
Expand All @@ -136,7 +204,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"192.168.0.1/123"}
_, _, err = IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("could not parse exclude range")))
})

Expand All @@ -150,7 +218,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"100::2:1/126"}
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("100::2:4"))

})
Expand All @@ -164,7 +232,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"100::2:1"}
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("100::2:2"))
})

Expand All @@ -177,7 +245,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"100::2::1"}
_, _, err = IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("could not parse exclude range")))
})

Expand All @@ -191,7 +259,7 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"2001:db8::0/32"}
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("2001:db9::"))

})
Expand All @@ -206,11 +274,11 @@ var _ = Describe("Allocation operations", func() {

var ipres []types.IPReservation
exrange := []string{"192.168.0.0/30", "192.168.0.6/31", "192.168.0.8/31", "192.168.0.4/30"}
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ := IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.10"))

exrange = []string{"192.168.0.0/30", "192.168.0.14/31", "192.168.0.4/30", "192.168.0.6/31", "192.168.0.8/31"}
newip, _, _ = IterateForAssignment(*ipnet, calculatedrangestart, nil, ipres, exrange, "0xdeadbeef", "", "")
newip, _, _ = IterateForAssignment(*ipnet, calculatedrangestart, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.10"))
})

Expand All @@ -234,7 +302,7 @@ var _ = Describe("Allocation operations", func() {
},
}
exrange := []string{"192.168.0.0/30"}
_, _, err = IterateForAssignment(*ipnet, firstip, nil, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, firstip, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("Could not allocate IP in range")))

})
Expand All @@ -258,7 +326,7 @@ var _ = Describe("Allocation operations", func() {
},
}
exrange := []string{"192.168.0.4/30"}
_, _, err = IterateForAssignment(*ipnet, firstip, nil, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, firstip, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("Could not allocate IP in range")))

})
Expand All @@ -284,7 +352,7 @@ var _ = Describe("Allocation operations", func() {
}

exrange := []string{"100::2:4/126"}
_, _, err = IterateForAssignment(*ipnet, firstip, nil, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, firstip, nil, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("Could not allocate IP in range")))

})
Expand All @@ -297,7 +365,7 @@ var _ = Describe("Allocation operations", func() {
_, ipnet, err := net.ParseCIDR("192.168.0.0/29")
Expect(err).NotTo(HaveOccurred())
rangeStart := net.ParseIP("192.168.0.0") // Network address, out of bounds.
newip, _, err := IterateForAssignment(*ipnet, rangeStart, nil, nil, nil, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, rangeStart, nil, nil, nil, nil, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.1"))
})
Expand All @@ -309,7 +377,7 @@ var _ = Describe("Allocation operations", func() {
Expect(err).NotTo(HaveOccurred())
rangeStart := net.ParseIP("192.168.0.0") // Network address, out of bounds.
rangeEnd := net.ParseIP("192.168.0.8") // Broadcast address, out of bounds.
newip, _, err := IterateForAssignment(*ipnet, rangeStart, rangeEnd, nil, nil, "0xdeadbeef", "", "")
newip, _, err := IterateForAssignment(*ipnet, rangeStart, rangeEnd, nil, nil, nil, "0xdeadbeef", "", "")
Expect(err).NotTo(HaveOccurred())
Expect(fmt.Sprint(newip)).To(Equal("192.168.0.1"))
})
Expand Down Expand Up @@ -337,7 +405,7 @@ var _ = Describe("Allocation operations", func() {
},
}
exrange := []string{"192.168.0.4/30"}
_, _, err = IterateForAssignment(*ipnet, startip, lastip, ipres, exrange, "0xdeadbeef", "", "")
_, _, err = IterateForAssignment(*ipnet, startip, lastip, nil, ipres, exrange, "0xdeadbeef", "", "")
Expect(err).To(MatchError(HavePrefix("Could not allocate IP in range")))
})

Expand All @@ -350,7 +418,7 @@ var _ = Describe("Allocation operations", func() {
lastip := net.ParseIP("192.168.0.6")

ipres := []types.IPReservation{}
_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, ipres, nil, "0xdeadbeef", "dummy-0", "")
_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, nil, ipres, nil, "0xdeadbeef", "dummy-0", "")
Expect(err).NotTo(HaveOccurred())
Expect(len(ipres)).To(Equal(1))
Expect(fmt.Sprint(ipres[0].IP)).To(Equal("192.168.0.1"))
Expand Down Expand Up @@ -379,7 +447,7 @@ var _ = Describe("Allocation operations", func() {
},
}

_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, ipres, nil, "0xdeadbeef", "dummy-0", "")
_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, nil, ipres, nil, "0xdeadbeef", "dummy-0", "")
Expect(err).NotTo(HaveOccurred())
Expect(len(ipres)).To(Equal(4))
Expect(fmt.Sprint(ipres[3].IP)).To(Equal("192.168.0.4"))
Expand Down Expand Up @@ -408,7 +476,7 @@ var _ = Describe("Allocation operations", func() {
},
}

_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, ipres, nil, "0xdeadbeef", "dummy-0", "")
_, ipres, err = IterateForAssignment(*ipnet, startip, lastip, nil, ipres, nil, "0xdeadbeef", "dummy-0", "")
Expect(err).NotTo(HaveOccurred())
Expect(len(ipres)).To(Equal(4))
Expect(fmt.Sprint(ipres[3].IP)).To(Equal("192.168.0.3"))
Expand Down
9 changes: 6 additions & 3 deletions pkg/storage/kubernetes/ipam.go
Original file line number Diff line number Diff line change
Expand Up @@ -612,10 +612,13 @@ func IPManagementKubernetesUpdate(ctx context.Context, mode int, ipam *Kubernete
logging.Errorf("Error parsing node slice cidr to range start: %v", err)
return newips, err
}
// Preserve additional range configuration (e.g., omit ranges, pick addresses) while overriding start/end
ipRange = whereaboutstypes.RangeConfiguration{
Range: ipRange.Range,
RangeStart: rangeStart,
RangeEnd: rangeEnd,
OmitRanges: ipRange.OmitRanges,
Range: ipRange.Range,
RangeStart: rangeStart,
RangeEnd: rangeEnd,
PickAddresses: ipRange.PickAddresses,
}
}
logging.Debugf("using pool identifier: %v", poolIdentifier)
Expand Down
9 changes: 5 additions & 4 deletions pkg/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ type NetConfList struct {
}

type RangeConfiguration struct {
OmitRanges []string `json:"exclude,omitempty"`
Range string `json:"range"`
RangeStart net.IP `json:"range_start,omitempty"`
RangeEnd net.IP `json:"range_end,omitempty"`
OmitRanges []string `json:"exclude,omitempty"`
Range string `json:"range"`
RangeStart net.IP `json:"range_start,omitempty"`
RangeEnd net.IP `json:"range_end,omitempty"`
PickAddresses []net.IP `json:"pick_addresses,omitempty"`
}

// IPAMConfig describes the expected json configuration for this plugin
Expand Down