diff --git a/config/crd/bases/airship.airshipit.org_ippools.yaml b/config/crd/bases/airship.airshipit.org_ippools.yaml index ecbe830..f49553c 100644 --- a/config/crd/bases/airship.airshipit.org_ippools.yaml +++ b/config/crd/bases/airship.airshipit.org_ippools.yaml @@ -32,14 +32,23 @@ spec: metadata: type: object spec: - description: IPPool tracks allocation ranges and statuses within a specific + description: IPPoolSpec tracks allocation ranges and statuses within a specific subnet IPv4 or IPv6 subnet. It has a set of ranges of IPs within the subnet from which IPs can be allocated by IPAM, and a set of IPs that are currently allocated already. properties: allocatedIPs: items: - type: string + description: AllocatedIP Allocates an IP to an entity + properties: + allocatedTo: + type: string + ip: + type: string + required: + - allocatedTo + - ip + type: object type: array ranges: items: diff --git a/config/samples/ippool.yaml b/config/samples/ippool.yaml index 742bac0..591134e 100644 --- a/config/samples/ippool.yaml +++ b/config/samples/ippool.yaml @@ -10,6 +10,9 @@ spec: - start: 10.0.1.1 stop: 10.0.1.9 allocatedIPs: - - 10.0.0.1 - - 10.0.0.2 - - 10.0.1.1 + - allocatedTo: default-vino-test-cr-leviathan-worker-0 + ip: 10.0.0.1 + - allocatedTo: default-vino-test-cr-leviathan-worker-1 + ip: 10.0.0.2 + - allocatedTo: default-vino-test-cr-leviathan-worker-2 + ip: 10.0.1.1 diff --git a/docs/api/vino.md b/docs/api/vino.md index bbd6968..dfd0c34 100644 --- a/docs/api/vino.md +++ b/docs/api/vino.md @@ -9,6 +9,47 @@

Package v1 contains API Schema definitions for the airship v1 API group

Resource Types: +

AllocatedIP +

+

+(Appears on: +IPPoolSpec) +

+

AllocatedIP Allocates an IP to an entity

+
+
+ + + + + + + + + + + + + + + + + +
FieldDescription
+ip
+ +string + +		 +
+allocatedTo
+ +string + +		 +
+
+

BMCCredentials

@@ -326,7 +367,9 @@ string allocatedIPs
-[]string + +[]AllocatedIP + @@ -357,7 +400,7 @@ IPPoolStatus (Appears on: IPPool)

-

IPPool tracks allocation ranges and statuses within a specific +

IPPoolSpec tracks allocation ranges and statuses within a specific subnet IPv4 or IPv6 subnet. It has a set of ranges of IPs within the subnet from which IPs can be allocated by IPAM, and a set of IPs that are currently allocated already.

@@ -397,7 +440,9 @@ string allocatedIPs
-[]string + +[]AllocatedIP + diff --git a/pkg/api/v1/ippool_types.go b/pkg/api/v1/ippool_types.go index 628bede..c867e84 100644 --- a/pkg/api/v1/ippool_types.go +++ b/pkg/api/v1/ippool_types.go @@ -21,14 +21,20 @@ import ( // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. -// IPPool tracks allocation ranges and statuses within a specific +// IPPoolSpec tracks allocation ranges and statuses within a specific // subnet IPv4 or IPv6 subnet. It has a set of ranges of IPs // within the subnet from which IPs can be allocated by IPAM, // and a set of IPs that are currently allocated already. type IPPoolSpec struct { - Subnet string `json:"subnet"` - Ranges []Range `json:"ranges"` - AllocatedIPs []string `json:"allocatedIPs"` + Subnet string `json:"subnet"` + Ranges []Range `json:"ranges"` + AllocatedIPs []AllocatedIP `json:"allocatedIPs"` +} + +// AllocatedIP Allocates an IP to an entity +type AllocatedIP struct { + IP string `json:"ip"` + AllocatedTo string `json:"allocatedTo"` } // Range has (inclusive) bounds within a subnet from which IPs can be allocated diff --git a/pkg/api/v1/zz_generated.deepcopy.go b/pkg/api/v1/zz_generated.deepcopy.go index b3aef28..dd1c83a 100644 --- a/pkg/api/v1/zz_generated.deepcopy.go +++ b/pkg/api/v1/zz_generated.deepcopy.go @@ -25,6 +25,21 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AllocatedIP) DeepCopyInto(out *AllocatedIP) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AllocatedIP. +func (in *AllocatedIP) DeepCopy() *AllocatedIP { + if in == nil { + return nil + } + out := new(AllocatedIP) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BMCCredentials) DeepCopyInto(out *BMCCredentials) { *out = *in @@ -175,7 +190,7 @@ func (in *IPPoolSpec) DeepCopyInto(out *IPPoolSpec) { } if in.AllocatedIPs != nil { in, out := &in.AllocatedIPs, &out.AllocatedIPs - *out = make([]string, len(*in)) + *out = make([]AllocatedIP, len(*in)) copy(*out, *in) } } diff --git a/pkg/ipam/ipam.go b/pkg/ipam/ipam.go index 6ab79c1..4e1fc62 100644 --- a/pkg/ipam/ipam.go +++ b/pkg/ipam/ipam.go @@ -65,6 +65,9 @@ func NewRange(start string, stop string) (vinov1.Range, error) { // AddSubnetRange adds a range within a subnet for IP allocation // TODO error: range overlaps with existing range or subnet overlaps with existing subnet +// NOTE: the above should only be an error if a subnet is re-added with a *different* +// subnet range than what is already allocated -- i.e. this function should be idempotent +// against allocating the exact same subnet+range multiple times. // TODO error: invalid range for subnet func (i *Ipam) AddSubnetRange(ctx context.Context, subnet string, subnetRange vinov1.Range) error { logger := i.Log.WithValues("subnet", subnet, "subnetRange", subnetRange) @@ -73,35 +76,43 @@ func (i *Ipam) AddSubnetRange(ctx context.Context, subnet string, subnetRange vi if err != nil { return err } + // Add the IPAM subnet if it doesn't exist already ippool, exists := ippools[subnet] if !exists { - logger.Info("IPAM creating subnet and adding range") + logger.Info("IPAM creating subnet") ippool = &vinov1.IPPoolSpec{ Subnet: subnet, Ranges: []vinov1.Range{}, - AllocatedIPs: []string{}, + AllocatedIPs: []vinov1.AllocatedIP{}, } ippools[subnet] = ippool - } else { - logger.Info("IPAM subnet already exists; adding range") - // Does the subnet's requested range already exist? (this should fail) - for _, r := range ippool.Ranges { - if r == subnetRange { - return ErrSubnetRangeOverlapsWithExistingRange{Subnet: subnet, SubnetRange: subnetRange} - } + } + // Add the IPAM range to the subnet if it doesn't exist already + exists = false + for _, existingSubnetRange := range ippools[subnet].Ranges { + if existingSubnetRange == subnetRange { + exists = true + break } } - ippool.Ranges = append(ippool.Ranges, subnetRange) - err = i.applyIPPool(ctx, *ippool) - if err != nil { - return err + if !exists { + logger.Info("IPAM creating subnet") + ippool.Ranges = append(ippool.Ranges, subnetRange) + err = i.applyIPPool(ctx, *ippool) + if err != nil { + return err + } } return nil } // AllocateIP allocates an IP from a range and return it -func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1.Range) (string, error) { +// allocatedTo: a unique identifier for the entity that is requesting / will own the +// allocated IP. If the same entity requests another IP, it will be given +// the same one. I.e. this function is idempotent for the same allocatedTo. +func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1.Range, + allocatedTo string) (string, error) { ippools, err := i.getIPPools(ctx) if err != nil { return "", err @@ -122,20 +133,38 @@ func (i *Ipam) AllocateIP(ctx context.Context, subnet string, subnetRange vinov1 return "", ErrSubnetRangeNotAllocated{Subnet: subnet, SubnetRange: subnetRange} } - ip, err := findFreeIPInRange(ippool, subnetRange) - if err != nil { - return "", err - } - i.Log.Info("Allocating IP", "ip", ip, "subnet", subnet, "subnetRange", subnetRange) - ippool.AllocatedIPs = append(ippool.AllocatedIPs, ip) - err = i.applyIPPool(ctx, *ippool) - if err != nil { - return "", err + // If an IP has already been allocated to this entity, return it + ip := findAlreadyAllocatedIP(ippool, allocatedTo) + + // No IP already allocated, so allocate a new IP + if ip == "" { + ip, err = findFreeIPInRange(ippool, subnetRange) + if err != nil { + return "", err + } + i.Log.Info("Allocating IP", "ip", ip, "subnet", subnet, "subnetRange", subnetRange) + ippool.AllocatedIPs = append(ippool.AllocatedIPs, vinov1.AllocatedIP{IP: ip, AllocatedTo: allocatedTo}) + err = i.applyIPPool(ctx, *ippool) + if err != nil { + return "", err + } } return ip, nil } +// This returns an IP already allocated to the entity specified by `allocatedTo` +// if it exists within the requested ippool/subnet, and a blank string +// if no IP is already allocated. +func findAlreadyAllocatedIP(ippool *vinov1.IPPoolSpec, allocatedTo string) string { + for _, allocatedIP := range ippool.AllocatedIPs { + if allocatedIP.AllocatedTo == allocatedTo { + return allocatedIP.IP + } + } + return "" +} + // This converts IP ranges/addresses into iterable ints, // steps through them looking for one that that is not already // in use, converts it back to a string and returns it. @@ -170,12 +199,12 @@ func findFreeIPInRange(ippool *vinov1.IPPoolSpec, subnetRange vinov1.Range) (str return "", ErrSubnetRangeExhausted{ippool.Subnet, subnetRange} } -// Create a map[uint64]struct{} representation of a string slice, +// Create a map[uint64]struct{} representation of an AllocatedIP slice, // for efficient set lookups -func sliceToMap(slice []string) (map[uint64]struct{}, error) { +func sliceToMap(slice []vinov1.AllocatedIP) (map[uint64]struct{}, error) { m := map[uint64]struct{}{} for _, s := range slice { - i, err := ipStringToInt(s) + i, err := ipStringToInt(s.IP) if err != nil { return m, err } diff --git a/pkg/ipam/ipam_test.go b/pkg/ipam/ipam_test.go index 29617a8..dda445f 100644 --- a/pkg/ipam/ipam_test.go +++ b/pkg/ipam/ipam_test.go @@ -59,7 +59,9 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli Ranges: []vinov1.Range{ {Start: "192.168.0.0", Stop: "192.168.0.0"}, }, - AllocatedIPs: []string{"192.168.0.0"}, + AllocatedIPs: []vinov1.AllocatedIP{ + {IP: "192.168.0.0", AllocatedTo: "old-vm-name"}, + }, }, }, { @@ -68,7 +70,9 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli Ranges: []vinov1.Range{ {Start: "2600:1700:b031:0000::", Stop: "2600:1700:b031:0000::"}, }, - AllocatedIPs: []string{"2600:1700:b031:0000::"}, + AllocatedIPs: []vinov1.AllocatedIP{ + {IP: "2600:1700:b031:0000::", AllocatedTo: "old-vm-name"}, + }, }, }, }, @@ -83,36 +87,41 @@ func SetUpMockClient(ctx context.Context, ctrl *gomock.Controller) *test.MockCli func TestAllocateIP(t *testing.T) { tests := []struct { - name, subnet, expectedErr string - subnetRange vinov1.Range + name, subnet, allocatedTo, expectedErr string + subnetRange vinov1.Range }{ { name: "success ipv4", subnet: "10.0.0.0/16", subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"}, + allocatedTo: "new-vm-name", }, { name: "success ipv6", subnet: "2600:1700:b030:0000::/72", subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:0009::"}, + allocatedTo: "new-vm-name", }, { name: "error subnet not allocated ipv4", subnet: "10.0.0.0/20", subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"}, expectedErr: "IPAM subnet 10.0.0.0/20 not allocated", + allocatedTo: "new-vm-name", }, { name: "error subnet not allocated ipv6", subnet: "2600:1700:b030:0000::/80", subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:0009::"}, expectedErr: "IPAM subnet 2600:1700:b030:0000::/80 not allocated", + allocatedTo: "new-vm-name", }, { name: "error range not allocated ipv4", subnet: "10.0.0.0/16", subnetRange: vinov1.Range{Start: "10.0.2.0", Stop: "10.0.2.9"}, expectedErr: "IPAM range [10.0.2.0,10.0.2.9] in subnet 10.0.0.0/16 is not allocated", + allocatedTo: "new-vm-name", }, { name: "error range not allocated ipv6", @@ -120,12 +129,20 @@ func TestAllocateIP(t *testing.T) { subnetRange: vinov1.Range{Start: "2600:1700:b030:0000::", Stop: "2600:1700:b030:1111::"}, expectedErr: "IPAM range [2600:1700:b030:0000::,2600:1700:b030:1111::] " + "in subnet 2600:1700:b030:0000::/72 is not allocated", + allocatedTo: "new-vm-name", + }, + { + name: "success idempotency ipv4", + subnet: "192.168.0.0/1", + subnetRange: vinov1.Range{Start: "192.168.0.0", Stop: "192.168.0.0"}, + allocatedTo: "old-vm-name", }, { name: "error range exhausted ipv4", subnet: "192.168.0.0/1", subnetRange: vinov1.Range{Start: "192.168.0.0", Stop: "192.168.0.0"}, expectedErr: "IPAM range [192.168.0.0,192.168.0.0] in subnet 192.168.0.0/1 is exhausted", + allocatedTo: "new-vm-name", }, { name: "error range exhausted ipv6", @@ -133,6 +150,7 @@ func TestAllocateIP(t *testing.T) { subnetRange: vinov1.Range{Start: "2600:1700:b031:0000::", Stop: "2600:1700:b031:0000::"}, expectedErr: "IPAM range [2600:1700:b031:0000::,2600:1700:b031:0000::] " + "in subnet 2600:1700:b031:0000::/64 is exhausted", + allocatedTo: "new-vm-name", }, } @@ -147,7 +165,7 @@ func TestAllocateIP(t *testing.T) { ipammer := NewIpam(log.Log, m, "vino-system") ipammer.Log = log.Log - ip, err := ipammer.AllocateIP(ctx, tt.subnet, tt.subnetRange) + ip, err := ipammer.AllocateIP(ctx, tt.subnet, tt.subnetRange, tt.allocatedTo) if tt.expectedErr != "" { require.Error(t, err) assert.Equal(t, "", ip) @@ -217,12 +235,6 @@ func TestAddSubnetRange(t *testing.T) { subnetRange: vinov1.Range{Start: "10.0.2.0", Stop: "10.0.2.9"}, expectedErr: "", }, - { - name: "error range already exists", - subnet: "10.0.0.0/16", - subnetRange: vinov1.Range{Start: "10.0.1.0", Stop: "10.0.1.9"}, - expectedErr: "IPAM range [10.0.1.0,10.0.1.9] in subnet 10.0.0.0/16 overlaps", - }, // TODO: check for partially overlapping ranges and subnets } @@ -294,7 +306,10 @@ func TestFindFreeIPInRange(t *testing.T) { {Start: "2600:1700:b030:1001::", Stop: "2600:1700:b030:1009::"}, {Start: "2600:1700:b031::", Stop: "2600:1700:b031::"}, }, - AllocatedIPs: []string{"10.0.2.0", "2600:1700:b031::"}, + AllocatedIPs: []vinov1.AllocatedIP{ + {IP: "10.0.2.0", AllocatedTo: "old-vm-name"}, + {IP: "2600:1700:b031::", AllocatedTo: "old-vm-name"}, + }, } actual, err := findFreeIPInRange(&ippool, tt.subnetRange) if tt.expectedErr != "" { @@ -311,23 +326,28 @@ func TestFindFreeIPInRange(t *testing.T) { func TestSliceToMap(t *testing.T) { tests := []struct { name string - in []string + in []vinov1.AllocatedIP out map[uint64]struct{} }{ { name: "empty slice", - in: []string{}, + in: []vinov1.AllocatedIP{}, out: map[uint64]struct{}{}, }, { name: "one-element slice", - in: []string{"0.0.0.1"}, - out: map[uint64]struct{}{1: {}}, + in: []vinov1.AllocatedIP{ + {IP: "0.0.0.1", AllocatedTo: "old-vm-name"}, + }, + out: map[uint64]struct{}{1: {}}, }, { name: "two-element slice", - in: []string{"0.0.0.1", "0.0.0.2"}, - out: map[uint64]struct{}{1: {}, 2: {}}, + in: []vinov1.AllocatedIP{ + {IP: "0.0.0.1", AllocatedTo: "old-vm-name"}, + {IP: "0.0.0.2", AllocatedTo: "old-vm-name"}, + }, + out: map[uint64]struct{}{1: {}, 2: {}}, }, }