diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee293d5a..bd07decb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -509,7 +509,6 @@ OpenSVC v3 is a major evolution, rebuilt in Go for performance, reliability, and ### Driver: ip * **Removed keywords:** - * `dns_name_suffix` * `provisioner` * `dns_update` @@ -520,6 +519,63 @@ OpenSVC v3 is a major evolution, rebuilt in Go for performance, reliability, and The `alias` keyword default value is now `true`, activating the ip stacking behaviour. Setting `dev=eth0:0` still forces the address labelling mode. +* **Changed default DNS search list:** + The fqdn of the object is no longer the first domain a container searches a + shortname in. The default list is now the domain of the object and each of + its parents: `.. . + `. + + In v2 the fqdn was first, which let a container reach a container of the + same object by its hostname alone. The record is still published as + `.`, so a configuration relying on the shortname must + now name that fqdn in the `dns_search` keyword of the container. + + Containers of an instance are expected to share a netns and to reach each + other over 127.0.0.1, which needs no name. One ip resource carries one + hostname, the one of the container its `netns` keyword points at, so a + shared netns has one name for the whole instance rather than one per + container. + + The `dns_name_suffix` keyword of the ip drivers is kept, and is how an + object with more than one address names them apart: the suffix is appended + to the hostname the record is published under, so a second address of the + same container answers to `.`. + +* **Changed keyword, `ip.netns.network`:** + It names the om network the address is drawn from, as it does on `ip.cni`. + An object attaches to a cluster network with one line: + + ``` + [ip#0] + type = netns + netns = container#0 + network = default + ``` + + The `dev`, `netmask` and `gateway` of the resource are read from that + network when the configuration does not set them, and an empty `name` has + the address allocated from it. An explicit value still wins, so a + configuration setting them keeps working unchanged. `dev` is no longer a + required keyword: a network names it. + + The keyword used to hold the address of the network, in dotted notation, + which set the destination of the route `del_net_route` removes. That + destination is the connected route the kernel adds along with the address, + which is the address masked, and the keyword could name no other: the + prefix length has always come from `netmask`. It is derived now. A value + still in that form is reported as obsolete and ignored, and a value that is + neither an address nor the name of a network is refused, naming the + networks that exist. + +* **Address allocation:** + om allocates the addresses of its `bridge` and `routed_bridge` networks + itself, rather than leaving them to the `host-local` cni plugin. A resource + draws the same address every time it starts, from a hash of the object and + the rid, so an object is not renumbered by a restart and the name it is + published under keeps resolving to the same place. `host-local` allocates + the first free address instead, which moves as the neighbours of an object + come and go. + * **Collector DNS zone:** This feature of the collector, used by the ip driver for one of its provisioning methods, is deprecated. @@ -741,6 +797,18 @@ Where the password is the value of the `þassword` key in `system/sec/relay-v3`. * Flush iptables rules created by om2. om3 now configures the firewall using nft only. +* The nft rules moved to the `osvc` table, one per address family. They used to + go in `nat` and `filter`, where the base chain om adds sits on a hook and a + priority the standard chain of those tables already uses, which makes them + unrepresentable to the iptables compatibility layer: `iptables -t nat -S` + answered ``table `nat' is incompatible, use 'nft' tool``, and every firewall + driver reaching the ruleset through iptables went blind on them. netavark is + one of those, so no podman container of a podman-built network could start on + a node `om net setup` had run on. + + A setup deletes the chains left in `nat` and `filter` by an earlier om, so a + node converts itself the first time it runs one. + * Change `ips_per_node` to `mask_per_node`. The former was inadequate for large subnets (ipv6). For example, `ips_per_node=18446744073709551616` is easier expressed as `mask_per_node=64`. Backward compatibility is maintained for this release. diff --git a/core/ipam/ipam.go b/core/ipam/ipam.go new file mode 100644 index 000000000..dc024e19f --- /dev/null +++ b/core/ipam/ipam.go @@ -0,0 +1,437 @@ +// Package ipam allocates the addresses of an om network on one node. +// +// The allocation is node local, which is what makes it safe without a lock +// held across the cluster. A routed_bridge network gives every node a subnet +// of its own, so two nodes never draw from the same addresses. A bridge +// network gives every node the whole subnet, but its addresses are node local +// and not routable, so the same address on two nodes never meets. +package ipam + +import ( + "fmt" + "hash/fnv" + "math/big" + "net" + "os" + "path/filepath" + "strings" + + "github.com/opensvc/om3/v3/core/naming" + "github.com/opensvc/om3/v3/core/rawconfig" +) + +// StoreDir returns where the reservations of a network are recorded. +// +// One place, so the allocator a resource builds and the adoption a network +// setup runs record in the same directory. +func StoreDir(name string) string { + return filepath.Join(rawconfig.Paths.Var, "ipam", name) +} + +type ( + // T allocates in one network, on one node. + T struct { + // Name is the network the addresses are drawn from. + Name string + + // Range is the addresses this node draws from. + Range *net.IPNet + + // Gateway is not allocated: it is the address of the bridge. + Gateway net.IP + + // Dir is where the reservations are recorded, one file per address. + Dir string + + // PeerDirs are the reservation stores of the allocators sharing this + // range, read and never written. While ip.cni is still served by the + // host-local plugin, its store is one of these: an address it handed + // out has no reservation here, and would be handed out twice. + PeerDirs []string + + // InUse reports the addresses the cluster says are taken. The daemon + // replicates the resource status of every instance, so this sees the + // addresses of objects whose reservation file this node cannot read. + // + // Neither network driver needs it: a routed_bridge gives this node a + // range no other node draws from, and the addresses of a bridge are + // node local and not routable. It is here for a network type that is + // neither. + InUse func() ([]net.IP, error) + } + + // Reservation is an address already held by a resource, which an adoption + // records so the resource keeps it. + Reservation struct { + IP net.IP + Key string + } +) + +// maxProbes bounds the walk over the candidates of a range. +// +// A range is walked from a candidate the key decides, and a full walk of an +// ipv6 range would not end. A network with more free addresses than this and +// none in the first probes is a network with a leak, not a network that is +// full. +const maxProbes = 4096 + +// Allocate returns the address reserved for key, reserving it when it has +// none. +// +// The same key draws the same address as long as it stays free, so an object +// keeps its address across restarts and its name keeps meaning what it meant. +// The reservation is a file created exclusively, which is the whole of the +// locking: one node draws from this range, so there is no other writer. +func (t *T) Allocate(key string) (net.IP, error) { + if t.Range == nil { + return nil, fmt.Errorf("network %s allocates no address on this node", t.Name) + } + if ip, err := t.Allocated(key); err != nil { + return nil, err + } else if ip != nil { + return ip, nil + } + taken, err := t.taken() + if err != nil { + return nil, err + } + ones, bits := t.Range.Mask.Size() + size := new(big.Int).Lsh(big.NewInt(1), uint(bits-ones)) + first := ipToInt(t.Range.IP) + offset := new(big.Int).Mod(keyOffset(key), size) + + probes := maxProbes + if size.IsInt64() && size.Int64() < int64(probes) { + probes = int(size.Int64()) + } + for i := 0; i < probes; i++ { + ip := intToIP(new(big.Int).Add(first, offset), t.Range.IP.To4() != nil) + if t.isAllocatable(ip, size) && !taken[ip.String()] { + if ok, err := t.reserve(ip, key); err != nil { + return nil, err + } else if ok { + return ip, nil + } + } + offset.Add(offset, big.NewInt(1)) + offset.Mod(offset, size) + } + return nil, fmt.Errorf("network %s: no free address in %s after %d probes", t.Name, t.Range, probes) +} + +// Adopt records the addresses resources already hold, so the allocator hands +// out the addresses that are free rather than the addresses that have no +// reservation yet. +// +// It is how the allocation of a network moves from an allocator that was +// keeping its own record. The record of the host-local plugin cannot serve: +// it names the holder of an address by the pid of a network namespace, which +// says nothing about which resource that is. The cluster status can, since it +// carries the object and the rid alongside the address. +// +// An address already reserved is left alone, whoever holds it, so an adoption +// run twice changes nothing and an adoption run late does not take an address +// from the resource that drew it. The count returned is of the addresses this +// call recorded, which is how a setup says whether it had anything to adopt. +func (t *T) Adopt(reservations []Reservation) (int, error) { + n := 0 + for _, reservation := range reservations { + if reservation.IP == nil || reservation.Key == "" { + continue + } + if t.Range != nil && !t.Range.Contains(reservation.IP) { + continue + } + ok, err := t.reserve(reservation.IP, reservation.Key) + if err != nil { + return n, err + } + if ok { + n++ + } + } + return n, nil +} + +// Allocated returns the address key holds, or nil when it holds none. +func (t *T) Allocated(key string) (net.IP, error) { + entries, err := os.ReadDir(t.Dir) + if os.IsNotExist(err) { + return nil, nil + } else if err != nil { + return nil, err + } + for _, entry := range entries { + ip := net.ParseIP(entry.Name()) + if ip == nil { + continue + } + if held, err := t.holder(entry.Name()); err != nil { + return nil, err + } else if held == key { + return ip, nil + } + } + return nil, nil +} + +// Free releases the address key holds, and does nothing when it holds none. +// +// Only the holder frees an address: the file names who took it, and a +// reservation another key made is left alone. +func (t *T) Free(key string) error { + ip, err := t.Allocated(key) + if err != nil { + return err + } + if ip == nil { + return nil + } + err = os.Remove(filepath.Join(t.Dir, ip.String())) + if os.IsNotExist(err) { + return nil + } + return err +} + +// reserve creates the reservation of an address, and reports whether this +// call is the one that created it. +func (t *T) reserve(ip net.IP, key string) (bool, error) { + if err := os.MkdirAll(t.Dir, 0755); err != nil { + return false, err + } + f, err := os.OpenFile(filepath.Join(t.Dir, ip.String()), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0644) + if os.IsExist(err) { + return false, nil + } else if err != nil { + return false, err + } + defer f.Close() + if _, err := f.WriteString(key + "\n"); err != nil { + return false, err + } + return true, nil +} + +// holder returns the key an address is reserved for. +func (t *T) holder(addr string) (string, error) { + b, err := os.ReadFile(filepath.Join(t.Dir, addr)) + if os.IsNotExist(err) { + return "", nil + } else if err != nil { + return "", err + } + return strings.TrimSpace(string(b)), nil +} + +// recorded returns the addresses a store holds. A store holds files that are +// not addresses, the bookkeeping of whoever writes it, and those are not ours +// to read or to remove. +func recorded(dir string) (map[string]bool, error) { + m := make(map[string]bool) + entries, err := os.ReadDir(dir) + if os.IsNotExist(err) { + return m, nil + } else if err != nil { + return nil, err + } + for _, entry := range entries { + if ip := net.ParseIP(entry.Name()); ip != nil { + m[ip.String()] = true + } + } + return m, nil +} + +// DrainPeers removes, from the stores of the allocators sharing this range, +// the record of every address this one now holds. +// +// A store is read to know what an allocator om is replacing handed out. Once +// om reserves an address, that record says nothing om does not know, and +// leaving it makes the address unusable for as long as the file lasts: the +// allocator that wrote it does not run any more, so nothing else will ever +// remove it. +// +// Only the addresses om holds are removed. An address in the store that om +// accounts for in no way is left, and stays excluded, because the reason it is +// there may be a resource whose status this node has not read. +func (t *T) DrainPeers() (int, int, error) { + held, err := recorded(t.Dir) + if err != nil { + return 0, 0, err + } + drained, left := 0, 0 + for _, dir := range t.PeerDirs { + peer, err := recorded(dir) + if err != nil { + return drained, left, err + } + for addr := range peer { + if !held[addr] { + left++ + continue + } + if err := os.Remove(filepath.Join(dir, addr)); err != nil && !os.IsNotExist(err) { + return drained, left, err + } + drained++ + } + } + return drained, left, nil +} + +// Reap releases the reservations held for keys that no longer exist. +// +// A resource releases its address when it stops, and a start that fails rolls +// the allocation back, so a reservation outliving its holder takes a crash at +// the wrong moment. It outlives it for good though: the key names an object +// that is gone, so nothing will ever come to release it, and the address is +// lost until someone reads the store and works out why. +// +// A key is kept unless it is known not to exist. Anything else would race a +// resource that has just reserved an address and not yet finished starting. +func (t *T) Reap(exists func(key string) bool) (int, error) { + entries, err := os.ReadDir(t.Dir) + if os.IsNotExist(err) { + return 0, nil + } else if err != nil { + return 0, err + } + n := 0 + for _, entry := range entries { + if net.ParseIP(entry.Name()) == nil { + continue + } + key, err := t.holder(entry.Name()) + if err != nil { + return n, err + } + if key == "" || exists(key) { + continue + } + if err := os.Remove(filepath.Join(t.Dir, entry.Name())); err != nil && !os.IsNotExist(err) { + return n, err + } + n++ + } + return n, nil +} + +// PathOfKey returns the object a key names, and whether it names one. +func PathOfKey(key string) (naming.Path, bool) { + s, _, found := strings.Cut(key, "!") + if !found { + return naming.Path{}, false + } + p, err := naming.ParsePath(s) + if err != nil { + return naming.Path{}, false + } + return p, true +} + +// taken returns the addresses no allocation may draw: the ones reserved here, +// the ones reserved by an allocator sharing the range, and the ones the +// cluster reports in use. +func (t *T) taken() (map[string]bool, error) { + m := make(map[string]bool) + for _, dir := range append([]string{t.Dir}, t.PeerDirs...) { + found, err := recorded(dir) + if err != nil { + return nil, err + } + for addr := range found { + m[addr] = true + } + } + if t.InUse != nil { + ips, err := t.InUse() + if err != nil { + return nil, err + } + for _, ip := range ips { + m[ip.String()] = true + } + } + return m, nil +} + +// isAllocatable reports whether an address of the range may be handed to an +// object. +// +// The first address of a range names the range, the last of an ipv4 range is +// its broadcast address, and the gateway answers for the bridge. +func (t *T) isAllocatable(ip net.IP, size *big.Int) bool { + if !t.Range.Contains(ip) { + return false + } + offset := new(big.Int).Sub(ipToInt(ip), ipToInt(t.Range.IP)) + if offset.Sign() == 0 { + return false + } + if ip.To4() != nil && offset.Cmp(new(big.Int).Sub(size, big.NewInt(1))) == 0 { + return false + } + if t.Gateway != nil && t.Gateway.Equal(ip) { + return false + } + return true +} + +// keyOffset returns where in a range the walk for a key starts. +// +// It is a hash of the key rather than the first free address, so the same +// object draws the same address every time without a record of what it drew +// last: a restart does not renumber it, and its name keeps resolving to the +// same place. +func keyOffset(key string) *big.Int { + h := fnv.New64a() + h.Write([]byte(key)) + return new(big.Int).SetUint64(h.Sum64()) +} + +func ipToInt(ip net.IP) *big.Int { + if v4 := ip.To4(); v4 != nil { + return new(big.Int).SetBytes(v4) + } + return new(big.Int).SetBytes(ip.To16()) +} + +func intToIP(i *big.Int, isV4 bool) net.IP { + size := 16 + if isV4 { + size = 4 + } + b := i.Bytes() + if len(b) > size { + b = b[len(b)-size:] + } + out := make([]byte, size) + copy(out[size-len(b):], b) + return net.IP(out) +} + +// Gateway returns the address the bridge of a range answers for, which is the +// first address of the range plus one. +// +// Both network drivers derive the address of their bridge that way, from the +// range they hand out: the bridge driver from the whole subnet, the +// routed_bridge driver from the subnet of the node. So the allocator has one +// rule to keep and no second place for it to drift from. +func Gateway(rng *net.IPNet) net.IP { + if rng == nil { + return nil + } + return intToIP(new(big.Int).Add(ipToInt(rng.IP), big.NewInt(1)), rng.IP.To4() != nil) +} + +// Key returns the reservation key of an ip resource. +// +// An address belongs to a resource, not to an object: an instance holds as +// many ip resources as it needs, several of them in one network, and each has +// an address of its own. Keying on the object alone would have ip#0 and ip#1 +// draw the same one. +func Key(p naming.Path, rid string) string { + return p.String() + "!" + rid +} diff --git a/core/ipam/ipam_test.go b/core/ipam/ipam_test.go new file mode 100644 index 000000000..683b246d5 --- /dev/null +++ b/core/ipam/ipam_test.go @@ -0,0 +1,489 @@ +package ipam + +import ( + "fmt" + "net" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opensvc/om3/v3/core/naming" +) + +func cidr(t *testing.T, s string) *net.IPNet { + t.Helper() + _, ipnet, err := net.ParseCIDR(s) + require.NoError(t, err) + return ipnet +} + +func newT(t *testing.T, network string) *T { + t.Helper() + return &T{ + Name: "backend3", + Range: cidr(t, network), + Gateway: net.ParseIP("10.100.0.1"), + Dir: filepath.Join(t.TempDir(), "backend3"), + } +} + +func TestAllocateStaysInTheRange(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + for _, key := range []string{"root/svc/pod1#ip#0", "root/svc/pod2#ip#0", "test/svc/db1#ip#1"} { + ip, err := ipam.Allocate(key) + require.NoErrorf(t, err, "allocate %s", key) + assert.Truef(t, ipam.Range.Contains(ip), "%s is outside %s", ip, ipam.Range) + } +} + +// TestAllocateIsStableForAKey pins that an object keeps its address across +// restarts, so its name keeps resolving to the same place. +func TestAllocateIsStableForAKey(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + first, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + + again, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + assert.Equal(t, first.String(), again.String(), "a second allocation must return the address already held") + + require.NoError(t, ipam.Free("root/svc/pod1#ip#0")) + after, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + assert.Equal(t, first.String(), after.String(), "an address freed and drawn again must be the same one") +} + +func TestAllocateHandsTheSameAddressOnce(t *testing.T) { + ipam := newT(t, "10.100.0.0/28") + seen := make(map[string]string) + for _, key := range []string{"a", "b", "c", "d", "e", "f", "g"} { + ip, err := ipam.Allocate(key) + require.NoErrorf(t, err, "allocate %s", key) + if other, ok := seen[ip.String()]; ok { + t.Fatalf("%s was given to %s and to %s", ip, other, key) + } + seen[ip.String()] = key + } +} + +// TestAllocateSkipsTheAddressesOfTheRangeItself pins the three an object never +// gets: the address naming the range, the broadcast address, and the gateway. +func TestAllocateSkipsTheAddressesOfTheRangeItself(t *testing.T) { + ipam := newT(t, "10.100.0.0/29") + ipam.Gateway = net.ParseIP("10.100.0.1") + for i := range 5 { + ip, err := ipam.Allocate(string(rune('a' + i))) + require.NoError(t, err) + assert.NotEqual(t, "10.100.0.0", ip.String(), "the address naming the range") + assert.NotEqual(t, "10.100.0.7", ip.String(), "the broadcast address") + assert.NotEqual(t, "10.100.0.1", ip.String(), "the gateway") + } +} + +// TestAllocateAvoidsWhatAnotherAllocatorHandedOut pins that om does not hand +// out an address the host-local plugin already gave, which it would while +// ip.cni is still served by it. +func TestAllocateAvoidsWhatAnotherAllocatorHandedOut(t *testing.T) { + ipam := newT(t, "10.100.0.0/29") + peer := t.TempDir() + ipam.PeerDirs = []string{peer} + // .0 names the range, .1 is the gateway and .7 is the broadcast address, + // so .2 to .6 are allocatable. Leave one. + for _, addr := range []string{"10.100.0.2", "10.100.0.3", "10.100.0.4", "10.100.0.5"} { + require.NoError(t, os.WriteFile(filepath.Join(peer, addr), []byte("2083975\n"), 0644)) + } + ip, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + assert.Equal(t, "10.100.0.6", ip.String(), "the only address left") +} + +// TestAllocateAvoidsWhatTheClusterReportsInUse pins the second source: the +// daemon sees the addresses of objects whose reservation file is on a node +// this one cannot read. +func TestAllocateAvoidsWhatTheClusterReportsInUse(t *testing.T) { + ipam := newT(t, "10.100.0.0/29") + ipam.InUse = func() ([]net.IP, error) { + return []net.IP{ + net.ParseIP("10.100.0.2"), + net.ParseIP("10.100.0.3"), + net.ParseIP("10.100.0.4"), + net.ParseIP("10.100.0.6"), + }, nil + } + ip, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + assert.Equal(t, "10.100.0.5", ip.String(), "the only address left") +} + +func TestAllocateReportsAFullRange(t *testing.T) { + // /30 holds 4 addresses: the range, the gateway, one host, the broadcast. + ipam := newT(t, "10.100.0.0/30") + first, err := ipam.Allocate("a") + require.NoError(t, err) + assert.Equal(t, "10.100.0.2", first.String()) + + _, err = ipam.Allocate("b") + require.Error(t, err) + assert.Contains(t, err.Error(), "no free address") +} + +func TestFreeLeavesTheReservationOfAnotherKey(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + ip, err := ipam.Allocate("root/svc/pod1#ip#0") + require.NoError(t, err) + + require.NoError(t, ipam.Free("root/svc/pod2#ip#0")) + held, err := ipam.Allocated("root/svc/pod1#ip#0") + require.NoError(t, err) + assert.Equal(t, ip.String(), held.String(), "freeing another key must leave this reservation") +} + +func TestFreeOfAnUnknownKeyIsNotAnError(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + require.NoError(t, ipam.Free("root/svc/never#ip#0")) +} + +// TestAllocateInAnIPV6Range pins that a range no walk could enumerate is +// allocated in all the same, the key deciding where the walk starts. +func TestAllocateInAnIPV6Range(t *testing.T) { + ipam := &T{ + Name: "backend1", + Range: cidr(t, "fdfe::/64"), + Dir: filepath.Join(t.TempDir(), "backend1"), + } + seen := make(map[string]bool) + for _, key := range []string{"a", "b", "c"} { + ip, err := ipam.Allocate(key) + require.NoErrorf(t, err, "allocate %s", key) + assert.Truef(t, ipam.Range.Contains(ip), "%s is outside %s", ip, ipam.Range) + assert.Nil(t, ip.To4(), "%s must be an ipv6 address", ip) + assert.Falsef(t, seen[ip.String()], "%s was handed out twice", ip) + seen[ip.String()] = true + } +} + +// TestAllocateRefusesANetworkWithNoRange pins that a network om allocates +// nothing in, the lo network among them, says so rather than answering with +// an address of some other range. +func TestAllocateRefusesANetworkWithNoRange(t *testing.T) { + ipam := &T{Name: "lo", Dir: t.TempDir()} + _, err := ipam.Allocate("root/svc/pod1#ip#0") + require.Error(t, err) + assert.Contains(t, err.Error(), "allocates no address") +} + +// TestGatewayIsTheFirstAddressPlusOne pins the rule both network drivers use +// to place their bridge, which is the address the allocator must not hand out. +func TestGatewayIsTheFirstAddressPlusOne(t *testing.T) { + for _, tc := range []struct{ network, want string }{ + {"10.100.0.0/24", "10.100.0.1"}, + {"10.100.1.0/24", "10.100.1.1"}, + {"10.22.0.0/16", "10.22.0.1"}, + {"fdfe::/114", "fdfe::1"}, + {"fdfe::4000/114", "fdfe::4001"}, + } { + assert.Equal(t, tc.want, Gateway(cidr(t, tc.network)).String(), tc.network) + } + assert.Nil(t, Gateway(nil)) +} + +// TestAllocateGivesEachResourceItsOwnAddress pins the case an instance with +// several ip resources in one network needs. om puts no limit on how many an +// object holds, nor on how many of them share a network, so the key names the +// resource: keying on the object alone would have ip#0 and ip#1 draw the same +// address. +func TestAllocateGivesEachResourceItsOwnAddress(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + p1, err := naming.ParsePath("root/svc/pod1") + require.NoError(t, err) + p2, err := naming.ParsePath("root/svc/pod2") + require.NoError(t, err) + + keys := []string{ + Key(p1, "ip#0"), Key(p1, "ip#1"), Key(p1, "ip#2"), + Key(p2, "ip#0"), Key(p2, "ip#1"), + } + held := make(map[string]string, len(keys)) + for _, key := range keys { + ip, err := ipam.Allocate(key) + require.NoErrorf(t, err, "allocate %s", key) + if other, ok := held[ip.String()]; ok { + t.Fatalf("%s was given to %s and to %s", ip, other, key) + } + held[ip.String()] = key + } + assert.Len(t, held, len(keys)) + + // And each keeps its own across a restart of the instance. + for addr, key := range held { + again, err := ipam.Allocate(key) + require.NoError(t, err) + assert.Equalf(t, addr, again.String(), "%s must keep its address", key) + } +} + +// TestAllocateResolvesTheCollisionsOfASmallRange pins that the walk finds the +// free addresses of a range too small for the hashes to spread over. Filling a +// range is where two keys landing on one candidate stops being unlikely. +func TestAllocateResolvesTheCollisionsOfASmallRange(t *testing.T) { + // .0 names the range, .1 is the gateway, .15 is the broadcast address: + // 13 addresses for 13 resources. + ipam := newT(t, "10.100.0.0/28") + p, err := naming.ParsePath("root/svc/pod1") + require.NoError(t, err) + + held := make(map[string]string) + for i := range 13 { + key := Key(p, fmt.Sprintf("ip#%d", i)) + ip, err := ipam.Allocate(key) + require.NoErrorf(t, err, "allocate %s of 13 in a /28", key) + if other, ok := held[ip.String()]; ok { + t.Fatalf("%s was given to %s and to %s", ip, other, key) + } + held[ip.String()] = key + } + assert.Len(t, held, 13, "every allocatable address of the range") + + _, err = ipam.Allocate(Key(p, "ip#13")) + require.Error(t, err, "the range is full") +} + +// TestAdoptKeepsTheAddressAResourceHolds pins the move from another +// allocator: a resource that already has an address keeps it, rather than +// being handed a new one and renumbered. +func TestAdoptKeepsTheAddressAResourceHolds(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + p, err := naming.ParsePath("root/svc/pod1") + require.NoError(t, err) + key := Key(p, "ip#0") + + _, err = ipam.Adopt([]Reservation{{IP: net.ParseIP("10.100.0.22"), Key: key}}) + require.NoError(t, err) + + ip, err := ipam.Allocate(key) + require.NoError(t, err) + assert.Equal(t, "10.100.0.22", ip.String(), "the address the resource already holds") +} + +// TestAdoptDoesNotHandOutAnAdoptedAddress pins the other half: an address +// recorded for one resource is not drawn by another. +func TestAdoptDoesNotHandOutAnAdoptedAddress(t *testing.T) { + // .0 names the range, .1 is the gateway, .7 is the broadcast address. + ipam := newT(t, "10.100.0.0/29") + adopted := []Reservation{ + {IP: net.ParseIP("10.100.0.2"), Key: "a"}, + {IP: net.ParseIP("10.100.0.3"), Key: "b"}, + {IP: net.ParseIP("10.100.0.4"), Key: "c"}, + {IP: net.ParseIP("10.100.0.5"), Key: "d"}, + } + n, err := ipam.Adopt(adopted) + require.NoError(t, err) + assert.Equal(t, 4, n, "four addresses recorded") + + ip, err := ipam.Allocate("e") + require.NoError(t, err) + assert.Equal(t, "10.100.0.6", ip.String(), "the only address left") +} + +// TestAdoptIsIdempotentAndLate pins that an adoption run twice changes +// nothing, and that one run after an allocation does not take an address from +// the resource that drew it. +func TestAdoptIsIdempotentAndLate(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + drawn, err := ipam.Allocate("first") + require.NoError(t, err) + + adopted := []Reservation{{IP: drawn, Key: "late"}} + n, err := ipam.Adopt(adopted) + require.NoError(t, err) + assert.Equal(t, 0, n, "the address is already reserved by the resource that drew it") + _, err = ipam.Adopt(adopted) + require.NoError(t, err) + + held, err := ipam.Allocated("first") + require.NoError(t, err) + assert.Equal(t, drawn.String(), held.String(), "the resource that drew it keeps it") + + other, err := ipam.Allocate("late") + require.NoError(t, err) + assert.NotEqual(t, drawn.String(), other.String(), "the late adoption gets an address of its own") +} + +// TestAdoptSkipsWhatIsNotThisNodeRange pins that an address of another node, +// which the cluster status reports alongside this node's, is not recorded +// here. +func TestAdoptSkipsWhatIsNotThisNodeRange(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + n, err := ipam.Adopt([]Reservation{ + {IP: net.ParseIP("10.100.1.22"), Key: "on another node"}, + {IP: nil, Key: "no address"}, + {IP: net.ParseIP("10.100.0.22"), Key: ""}, + }) + require.NoError(t, err) + assert.Equal(t, 0, n) + entries, err := os.ReadDir(ipam.Dir) + if !os.IsNotExist(err) { + require.NoError(t, err) + assert.Empty(t, entries, "nothing of another range is recorded") + } +} + +// TestDrainPeersRemovesWhatThisAllocatorHolds pins the tidying of the store an +// allocator om replaced was writing. An address om reserves is one that record +// says nothing about, and the allocator that wrote it no longer runs, so +// nothing else would ever remove it. +func TestDrainPeersRemovesWhatThisAllocatorHolds(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + peer := t.TempDir() + ipam.PeerDirs = []string{peer} + + for _, addr := range []string{"10.100.0.2", "10.100.0.22", "10.100.0.24"} { + require.NoError(t, os.WriteFile(filepath.Join(peer, addr), []byte("2083975\neth0\n"), 0644)) + } + // The bookkeeping of whoever wrote the store is not ours to remove. + require.NoError(t, os.WriteFile(filepath.Join(peer, "last_reserved_ip.0"), []byte("10.100.0.24\n"), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(peer, "lock"), nil, 0644)) + + _, err := ipam.Adopt([]Reservation{ + {IP: net.ParseIP("10.100.0.2"), Key: "a"}, + {IP: net.ParseIP("10.100.0.22"), Key: "b"}, + }) + require.NoError(t, err) + + drained, left, err := ipam.DrainPeers() + require.NoError(t, err) + assert.Equal(t, 2, drained, "the addresses om now holds") + assert.Equal(t, 1, left, "the address om accounts for in no way") + + for _, addr := range []string{"10.100.0.2", "10.100.0.22"} { + assert.NoFileExistsf(t, filepath.Join(peer, addr), "%s is om's now", addr) + } + assert.FileExists(t, filepath.Join(peer, "10.100.0.24"), "an unaccounted address is left alone") + assert.FileExists(t, filepath.Join(peer, "last_reserved_ip.0"), "not an address, not ours") + assert.FileExists(t, filepath.Join(peer, "lock"), "not an address, not ours") +} + +// TestDrainPeersKeepsAnUnaccountedAddressExcluded pins that what is left +// behind still blocks an allocation, since the reason it is there may be a +// resource whose status this node has not read. +func TestDrainPeersKeepsAnUnaccountedAddressExcluded(t *testing.T) { + ipam := newT(t, "10.100.0.0/29") + peer := t.TempDir() + ipam.PeerDirs = []string{peer} + for _, addr := range []string{"10.100.0.2", "10.100.0.3", "10.100.0.4", "10.100.0.5"} { + require.NoError(t, os.WriteFile(filepath.Join(peer, addr), []byte("2083975\n"), 0644)) + } + _, _, err := ipam.DrainPeers() + require.NoError(t, err) + + ip, err := ipam.Allocate("late") + require.NoError(t, err) + assert.Equal(t, "10.100.0.6", ip.String(), "the addresses left in the record are still excluded") +} + +// TestAllocateIsSafeUnderContention pins that the exclusive create is the +// whole of the locking. The range is small enough that the keys collide on +// their first candidate, so the walk and the reservation race for real. +func TestAllocateIsSafeUnderContention(t *testing.T) { + // A /27 holds 32 addresses, 29 of them allocatable. + ipam := newT(t, "10.100.0.0/27") + + const workers = 29 + var ( + wg sync.WaitGroup + mu sync.Mutex + held = make(map[string]string, workers) + errs []error + ) + for i := range workers { + wg.Add(1) + go func(i int) { + defer wg.Done() + key := fmt.Sprintf("root/svc/obj%d!ip#0", i) + ip, err := ipam.Allocate(key) + mu.Lock() + defer mu.Unlock() + if err != nil { + errs = append(errs, err) + return + } + if other, ok := held[ip.String()]; ok { + errs = append(errs, fmt.Errorf("%s given to %s and to %s", ip, other, key)) + return + } + held[ip.String()] = key + }(i) + } + wg.Wait() + + assert.Empty(t, errs) + assert.Len(t, held, workers, "every allocatable address of the range, each to one key") +} + +// TestReapReleasesWhatNoObjectHolds pins the safety net for a reservation that +// outlived its holder: a crash between the reservation and the start leaves an +// address held for an object that is then deleted, and nothing would ever come +// to release it. +func TestReapReleasesWhatNoObjectHolds(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + p, err := naming.ParsePath("root/svc/alive") + require.NoError(t, err) + gone, err := naming.ParsePath("root/svc/gone") + require.NoError(t, err) + + aliveIP, err := ipam.Allocate(Key(p, "ip#0")) + require.NoError(t, err) + goneIP, err := ipam.Allocate(Key(gone, "ip#0")) + require.NoError(t, err) + + n, err := ipam.Reap(func(key string) bool { + path, ok := PathOfKey(key) + require.True(t, ok) + return path.String() == "alive" + }) + require.NoError(t, err) + assert.Equal(t, 1, n) + + held, err := ipam.Allocated(Key(p, "ip#0")) + require.NoError(t, err) + assert.Equal(t, aliveIP.String(), held.String(), "the object that exists keeps its address") + + released, err := ipam.Allocated(Key(gone, "ip#0")) + require.NoError(t, err) + assert.Nil(t, released, "the object that is gone holds nothing") + assert.NoFileExists(t, filepath.Join(ipam.Dir, goneIP.String())) +} + +// TestReapKeepsWhatItCannotJudge pins that a reap only releases what it knows +// to be gone. Anything else would race a resource that has just reserved an +// address and not yet finished starting. +func TestReapKeepsWhatItCannotJudge(t *testing.T) { + ipam := newT(t, "10.100.0.0/24") + ip, err := ipam.Allocate("a key of another shape") + require.NoError(t, err) + + n, err := ipam.Reap(func(key string) bool { + _, ok := PathOfKey(key) + return !ok + }) + require.NoError(t, err) + assert.Equal(t, 0, n) + assert.FileExists(t, filepath.Join(ipam.Dir, ip.String())) +} + +func TestPathOfKey(t *testing.T) { + p, ok := PathOfKey("root/svc/pod1!ip#0") + require.True(t, ok) + assert.Equal(t, "pod1", p.String()) + + p, ok = PathOfKey("test/vol/data1!ip#2") + require.True(t, ok) + assert.Equal(t, "test/vol/data1", p.String()) + + _, ok = PathOfKey("no separator") + assert.False(t, ok) +} diff --git a/core/network/fw.go b/core/network/fw.go index 51547224e..0cb52db56 100644 --- a/core/network/fw.go +++ b/core/network/fw.go @@ -5,7 +5,7 @@ package network import ( "fmt" "net" - "reflect" + "strings" "github.com/google/nftables" "github.com/rs/zerolog" @@ -26,6 +26,47 @@ type ( } ) +// fwTableName is the table om adds its rules to. +// +// They used to go in "nat" and "filter", which are two of the five tables the +// iptables compatibility layer owns. The base chain om added there sits on a +// hook and a priority the standard chain of that table already uses, and that +// makes the whole table unrepresentable in iptables terms: "iptables -t nat -S" +// answers "table `nat' is incompatible, use 'nft' tool". Every firewall driver +// going through iptables is blind to that table from then on. netavark is one +// of them: it stops seeing the chains it created, creates them again, and fails +// with "Chain already exists", so no podman container needing a podman-built +// network starts on a node om has set a network up on. +// +// nftables evaluates the base chains of every table registered on a hook, so om +// keeps its rules by owning a table of its own, and stops standing in the way of +// the tables it does not own. +const ( + fwTableName = "osvc" + + fwChainPostrouting = "osvc-postrouting" + fwChainMasq = "osvc-masq" + fwChainForward = "osvc-forward" +) + +var ( + // fwFamilies are the address families om adds its rules for. + fwFamilies = []nftables.TableFamily{ + nftables.TableFamilyIPv4, + nftables.TableFamilyIPv6, + } + + // legacyFWChains are the chains om used to add to the tables the iptables + // compatibility layer owns. A setup deletes them, so a node upgrading stops + // hiding those tables from iptables. The chain holding the jumps comes + // first: nft refuses to delete a chain another one still jumps to. + legacyFWChains = []legacyChain{ + {Table: "nat", Chain: fwChainPostrouting}, + {Table: "nat", Chain: fwChainMasq}, + {Table: "filter", Chain: fwChainForward}, + } +) + func newNFTHandle() *nftHandle { h := &nftHandle{ conn: &nftables.Conn{}, @@ -94,29 +135,6 @@ func (t *nftHandle) GetTable(family nftables.TableFamily, tableName string) (*nf return nil, nil } -func (t *nftHandle) AddTable(family nftables.TableFamily, tableName string) (*nftables.Table, error) { - table, err := t.GetTable(family, tableName) - if err != nil { - return nil, err - } - if table != nil { - return table, nil - } - table = &nftables.Table{ - Family: family, - Name: tableName, - } - if err := t.Run([]string{"nft", "add", "table", fmtFamily(family), tableName}); err != nil { - return nil, err - } - table, err = t.GetTable(family, tableName) - if err != nil { - return nil, err - } - t.tables = append(t.tables, table) - return table, nil -} - func (t *nftHandle) GetChain(family nftables.TableFamily, tableName, chainName string) (*nftables.Chain, error) { chains, err := t.Chains() if err != nil { @@ -137,157 +155,6 @@ func (t *nftHandle) GetChain(family nftables.TableFamily, tableName, chainName s return nil, nil } -func (t *nftHandle) AddForwardChain(table *nftables.Table, chainName string) (*nftables.Chain, error) { - chain := &nftables.Chain{ - Name: chainName, - Table: table, - Hooknum: nftables.ChainHookForward, - Priority: nftables.ChainPriorityFilter, - Type: nftables.ChainTypeFilter, - } - return t.addChain(chain) -} - -func (t *nftHandle) AddPostRoutingChain(table *nftables.Table, chainName string) (*nftables.Chain, error) { - chain := &nftables.Chain{ - Name: chainName, - Table: table, - Hooknum: nftables.ChainHookPostrouting, - Priority: nftables.ChainPriorityNATSource, - Type: nftables.ChainTypeNAT, - } - return t.addChain(chain) -} - -func (t *nftHandle) AddChain(table *nftables.Table, chainName string) (*nftables.Chain, error) { - chain := &nftables.Chain{ - Name: chainName, - Table: table, - } - return t.addRegularChain(chain) -} - -func fmtRegularChain(chain *nftables.Chain) []string { - l := []string{"nft", "add", "chain", fmtFamily(chain.Table.Family), chain.Table.Name, chain.Name} - return l -} - -func fmtChain(chain *nftables.Chain) []string { - l := []string{"nft", "add", "chain", fmtFamily(chain.Table.Family), chain.Table.Name, chain.Name} - - s := "{ type " + string(chain.Type) - switch chain.Hooknum { - case nftables.ChainHookPrerouting: - s += " hook prerouting" - case nftables.ChainHookInput: - s += " hook input" - case nftables.ChainHookForward: - s += " hook forward" - case nftables.ChainHookOutput: - s += " hook output" - case nftables.ChainHookPostrouting: - s += " hook postrouting" - } - - s += fmt.Sprintf(" priority %d", chain.Priority) - - if chain.Policy != nil { - switch *chain.Policy { - case nftables.ChainPolicyAccept: - s += " policy accept" - case nftables.ChainPolicyDrop: - s += " policy drop" - } - } - s += "; }" - return append(l, s) -} - -func (t *nftHandle) addRegularChain(chain *nftables.Chain) (*nftables.Chain, error) { - cachedChain, err := t.GetChain(chain.Table.Family, chain.Table.Name, chain.Name) - if err != nil { - return nil, err - } - if cachedChain != nil { - return cachedChain, nil - } - l := fmtRegularChain(chain) - if err := t.Run(l); err != nil { - return nil, err - } - t.chains = append(t.chains, chain) - return chain, nil -} - -func (t *nftHandle) addChain(chain *nftables.Chain) (*nftables.Chain, error) { - cachedChain, err := t.GetChain(chain.Table.Family, chain.Table.Name, chain.Name) - if err != nil { - return nil, err - } - if cachedChain != nil { - return cachedChain, nil - } - l := fmtChain(chain) - if err := t.Run(l); err != nil { - return nil, err - } - t.chains = append(t.chains, chain) - return chain, nil -} - -func debugRules() error { - h := newNFTHandle() - family := nftables.TableFamilyIPv4 - table, err := h.AddTable(family, "nat") - if err != nil { - return err - } - chain, err := h.AddChain(table, "osvc-networks") - if err != nil { - return err - } - rules, err := h.Conn().GetRule(table, chain) - if err != nil { - return err - } - for _, rule := range rules { - _ = rule - fmt.Printf("%+v\n", rule) - for _, e := range rule.Exprs { - fmt.Printf(" %s %+v\n", reflect.TypeOf(e), e) - } - } - return nil -} - -func setupFW(n logger, nws []Networker) error { - h := newNFTHandle() - h.SetLogger(n.Log()) - if err := h.FlushChains(); err != nil { - return err - } - for _, other := range nws { - cidr := other.Network() - if err := h.AddRuleDestinationReturn(cidr); err != nil { - return err - } - if i, ok := other.(backendDevNamer); ok { - dev := i.BackendDevName() - if dev != "" { - if err := h.AddRuleSourceJump(cidr); err != nil { - return err - } - if err := h.AddRuleForwardAccept(cidr, dev); err != nil { - return err - } - } - } - } - h.AddRuleDestinationReturn("224.0.0.0/8") - h.AddRuleMasq() - return nil -} - func fmtFamily(family nftables.TableFamily) string { switch family { case nftables.TableFamilyIPv4: @@ -299,14 +166,6 @@ func fmtFamily(family nftables.TableFamily) string { } } -func networkFamily(nw Networker) nftables.TableFamily { - if nw.IsIP6() { - return nftables.TableFamilyIPv6 - } else { - return nftables.TableFamilyIPv4 - } -} - func ipFamily(ip net.IP) nftables.TableFamily { if ip.To4() == nil { return nftables.TableFamilyIPv6 @@ -315,132 +174,210 @@ func ipFamily(ip net.IP) nftables.TableFamily { } } -func (t *nftHandle) FlushChains() error { - families := []nftables.TableFamily{ - nftables.TableFamilyIPv4, - nftables.TableFamilyIPv6, - } - chainNames := []struct { - Table string - Chain string - }{ - {"nat", "osvc-masq"}, - {"nat", "osvc-postrouting"}, - {"filter", "osvc-forward"}, - } - for _, family := range families { - for _, data := range chainNames { - if chain, _ := t.GetChain(family, data.Table, data.Chain); chain != nil { - l := []string{"nft", "flush", "chain", fmtFamily(family), data.Table, data.Chain} - if err := t.Run(l); err != nil { - return err - } +// fwNetwork is what the ruleset needs to know about a network: the addresses +// it holds, and the device its backend traffic goes through when it has one. +type fwNetwork struct { + CIDR string + Dev string +} + +// legacyChain names a chain om left in a table it no longer writes to. +type legacyChain struct { + Family string + Table string + Chain string +} + +// multicastCIDR is returned from the masquerade chain rather than translated. +// There is no ipv6 counterpart because there never was one. +const multicastCIDR = "224.0.0.0/8" + +// maxDevNameLen is the size of a device name the kernel accepts, the +// terminating nul included, which nft refuses to write a rule past. +const maxDevNameLen = 16 + +// isDevNameValid reports whether a rule may name a device. +// +// A network with no backend device is masqueraded by nobody, which is how a +// public network is left alone. A device named past what the kernel accepts +// cannot exist, so it is treated the same rather than rendered into a rule nft +// would refuse, and the ruleset being one transaction, refuse the rest with +// it. +func isDevNameValid(dev string) bool { + return dev != "" && len(dev) < maxDevNameLen +} + +// fwNetworks returns what the ruleset is rendered from. +// +// A device whose name is longer than the kernel accepts is left out. Such a +// device cannot exist, so no rule of it would ever match, and one rule nft +// refuses is now the whole ruleset refused: a single network named too long +// would leave the node with no firewall at all. +func fwNetworks(nws []Networker) []fwNetwork { + l := make([]fwNetwork, 0, len(nws)) + for _, nw := range nws { + n := fwNetwork{CIDR: nw.Network()} + if i, ok := nw.(backendDevNamer); ok { + n.Dev = i.BackendDevName() + if n.Dev != "" && !isDevNameValid(n.Dev) { + nw.Log().Warnf("device %s is named past the %d characters the kernel accepts, so it cannot exist and this network is left out of the firewall rules", n.Dev, maxDevNameLen-1) } } + l = append(l, n) } - return nil + return l } -func (t *nftHandle) AddRuleMasq() error { - families := []nftables.TableFamily{ - nftables.TableFamilyIPv4, - nftables.TableFamilyIPv6, - } - for _, family := range families { - table, _ := t.GetTable(family, "nat") - if table != nil { - if err := t.addRuleMasq(table); err != nil { - return err - } +// fwRuleset renders the nft document a setup applies. +// +// The whole document is one transaction. The table is deleted and defined +// again in full, so the rules are never half there: the kernel swaps the table +// in one step, and a rule nft refuses leaves the ruleset as it was rather than +// partly rebuilt. Adding a rule at a time left the masquerade and the forward +// accepts absent for as long as the setup ran, a quarter of a second of new +// connections leaving a container unmasqueraded. +// +// The "table" line before the "delete" is what makes the delete safe to write +// unconditionally: it creates the table when it is absent, and says nothing +// when it is not. +func fwRuleset(networks []fwNetwork, legacy []legacyChain) (string, error) { + var sb strings.Builder + for _, chain := range legacy { + fmt.Fprintf(&sb, "flush chain %s %s %s\n", chain.Family, chain.Table, chain.Chain) + } + for _, chain := range legacy { + fmt.Fprintf(&sb, "delete chain %s %s %s\n", chain.Family, chain.Table, chain.Chain) + } + for _, family := range fwFamilies { + body, err := fwTable(family, networks) + if err != nil { + return "", err + } + if body == "" { + continue } + name := fmtFamily(family) + fmt.Fprintf(&sb, "table %s %s { }\n", name, fwTableName) + fmt.Fprintf(&sb, "delete table %s %s\n", name, fwTableName) + sb.WriteString(body) } - return nil + return sb.String(), nil } -func (t *nftHandle) addRuleMasq(table *nftables.Table) error { - chain, err := t.AddChain(table, "osvc-masq") - if err != nil { - return err +// fwTable renders the table of one address family, or an empty string when no +// network of that family is configured. +// +// The devices are matched by name rather than by index. An index is resolved +// when the rule is loaded, so a bridge that does not exist yet makes nft +// refuse the rule, and the whole ruleset with it now that it is one +// transaction: a single network whose setup failed would leave the node with +// no firewall at all. An index also goes stale when a bridge is recreated, +// where a name does not. +func fwTable(family nftables.TableFamily, networks []fwNetwork) (string, error) { + var returns, jumps, devs []string + name := fmtFamily(family) + for _, nw := range networks { + ip, ipnet, err := net.ParseCIDR(nw.CIDR) + if err != nil { + return "", fmt.Errorf("network %s: %w", nw.CIDR, err) + } + if ipFamily(ip) != family { + continue + } + returns = append(returns, fmt.Sprintf("\t\t%s daddr %s counter return\n", name, ipnet)) + if !isDevNameValid(nw.Dev) { + continue + } + jumps = append(jumps, fmt.Sprintf("\t\t%s saddr %s counter jump %s\n", name, ipnet, fwChainMasq)) + devs = append(devs, nw.Dev) } - l := []string{"nft", "add", "rule", fmtFamily(table.Family), table.Name, chain.Name, "masquerade"} - if err := t.Run(l); err != nil { - return err + if len(returns) == 0 { + return "", nil } - return nil -} - -func (t *nftHandle) AddRuleDestinationReturn(cidr string) error { - ip, ipnet, err := net.ParseCIDR(cidr) - if err != nil { - return err + if family == nftables.TableFamilyIPv4 { + returns = append(returns, fmt.Sprintf("\t\tip daddr %s counter return\n", multicastCIDR)) } - family := ipFamily(ip) - table, err := t.AddTable(family, "nat") - if err != nil { - return err + + var sb strings.Builder + fmt.Fprintf(&sb, "table %s %s {\n", name, fwTableName) + + fmt.Fprintf(&sb, "\tchain %s {\n", fwChainMasq) + for _, rule := range returns { + sb.WriteString(rule) } - chain, err := t.AddChain(table, "osvc-masq") - if err != nil { - return err + sb.WriteString("\t\tmasquerade\n\t}\n") + + fmt.Fprintf(&sb, "\tchain %s {\n", fwChainPostrouting) + sb.WriteString("\t\ttype nat hook postrouting priority srcnat; policy accept;\n") + for _, rule := range jumps { + sb.WriteString(rule) } - l := []string{"nft", "insert", "rule", fmtFamily(family), table.Name, chain.Name} - if ip.To4() == nil { - l = append(l, "ip6") - } else { - l = append(l, "ip") + sb.WriteString("\t}\n") + + fmt.Fprintf(&sb, "\tchain %s {\n", fwChainForward) + sb.WriteString("\t\ttype filter hook forward priority filter; policy accept;\n") + for _, dev := range devs { + fmt.Fprintf(&sb, "\t\tiifname \"%s\" counter accept\n", dev) + fmt.Fprintf(&sb, "\t\toifname \"%s\" counter accept\n", dev) } - l = append(l, "daddr", ipnet.String(), "counter", "return") - return t.Run(l) + sb.WriteString("\t}\n}\n") + + return sb.String(), nil } -func (t *nftHandle) AddRuleSourceJump(cidr string) error { - ip, ipnet, err := net.ParseCIDR(cidr) - if err != nil { - return err - } - family := ipFamily(ip) - table, err := t.AddTable(family, "nat") - if err != nil { - return err +// legacyChains returns the chains om left in the tables it no longer writes +// to, so the document deletes the ones this node still has. +// +// They cannot be deleted unconditionally: nft aborts a transaction on a delete +// of an absent chain, and not half applying is the point of the transaction. +func (t *nftHandle) legacyChains() ([]legacyChain, error) { + l := make([]legacyChain, 0) + for _, family := range fwFamilies { + for _, data := range legacyFWChains { + chain, err := t.GetChain(family, data.Table, data.Chain) + if err != nil { + return nil, err + } + if chain == nil { + continue + } + l = append(l, legacyChain{Family: fmtFamily(family), Table: data.Table, Chain: data.Chain}) + } } - chain, err := t.AddPostRoutingChain(table, "osvc-postrouting") - if err != nil { - return err + return l, nil +} + +// apply hands the document to nft, which reads a ruleset from stdin and +// applies it as one transaction. +func (t *nftHandle) apply(ruleset string) error { + if ruleset == "" { + return nil } - l := []string{"nft", "add", "rule", fmtFamily(family), table.Name, chain.Name} - if ip.To4() == nil { - l = append(l, "ip6") - } else { - l = append(l, "ip") + cmd := command.New( + command.WithName("nft"), + command.WithVarArgs("-f", "-"), + command.WithLogger(t.log), + command.WithCommandLogLevel(zerolog.InfoLevel), + command.WithStdoutLogLevel(zerolog.InfoLevel), + command.WithStderrLogLevel(zerolog.ErrorLevel), + ) + cmd.Cmd().Stdin = strings.NewReader(ruleset) + if t.log != nil { + t.log.Attr("ruleset", ruleset).Infof("apply the nft ruleset of the om networks") } - l = append(l, "saddr", ipnet.String(), "counter", "jump", "osvc-masq") - return t.Run(l) + return cmd.Run() } -func (t *nftHandle) AddRuleForwardAccept(cidr, dev string) error { - ip, _, err := net.ParseCIDR(cidr) - if err != nil { - return err - } - family := ipFamily(ip) - table, err := t.AddTable(family, "filter") +func setupFW(n logger, nws []Networker) error { + h := newNFTHandle() + h.SetLogger(n.Log()) + legacy, err := h.legacyChains() if err != nil { return err } - chain, err := t.AddForwardChain(table, "osvc-forward") + ruleset, err := fwRuleset(fwNetworks(nws), legacy) if err != nil { return err } - - l := []string{"nft", "add", "rule", fmtFamily(family), table.Name, chain.Name, "iif", dev, "counter", "accept"} - if err := t.Run(l); err != nil { - return err - } - - l = []string{"nft", "add", "rule", fmtFamily(family), table.Name, chain.Name, "oif", dev, "counter", "accept"} - if err := t.Run(l); err != nil { - return err - } - - return nil + return h.apply(ruleset) } diff --git a/core/network/fw_test.go b/core/network/fw_test.go new file mode 100644 index 000000000..a1b3de31f --- /dev/null +++ b/core/network/fw_test.go @@ -0,0 +1,197 @@ +//go:build linux + +package network + +import ( + "os/exec" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// iptablesTables are the tables the iptables compatibility layer owns. A base +// chain om adds to one of them, on a hook and priority its standard chain +// already uses, makes the whole table unrepresentable in iptables terms, and +// every firewall driver going through iptables goes blind on it. +var iptablesTables = []string{"filter", "nat", "mangle", "raw", "security"} + +// TestFWTableIsNotOneIptablesOwns pins the rule this module broke: om adds its +// rules to a table of its own. +// +// It used to add them to "nat" and "filter", which left "iptables -t nat -S" +// answering "table `nat' is incompatible, use 'nft' tool" and netavark failing +// every container start with "Chain already exists". +func TestFWTableIsNotOneIptablesOwns(t *testing.T) { + assert.NotContainsf(t, iptablesTables, fwTableName, + "%s is a table the iptables compatibility layer owns", fwTableName) +} + +// TestLegacyFWChainsCoverTheOwnedChains pins that a setup cleans up every chain +// om used to leave elsewhere. A chain missing from the legacy list is one an +// upgraded node keeps in a table it no longer writes to, where it goes on +// hiding that table from iptables forever. +func TestLegacyFWChainsCoverTheOwnedChains(t *testing.T) { + legacy := make([]string, 0, len(legacyFWChains)) + for _, data := range legacyFWChains { + assert.Containsf(t, iptablesTables, data.Table, + "%s is not a table om ever added a chain to", data.Table) + legacy = append(legacy, data.Chain) + } + for _, chainName := range []string{fwChainPostrouting, fwChainMasq, fwChainForward} { + assert.Containsf(t, legacy, chainName, + "%s is not deleted from where om used to put it", chainName) + } +} + +// TestLegacyFWChainsDeleteTheJumperFirst pins the deletion order. nft refuses +// to delete a chain a rule still jumps to, and osvc-postrouting is what jumps +// to osvc-masq. +func TestLegacyFWChainsDeleteTheJumperFirst(t *testing.T) { + index := func(name string) int { + return slices.IndexFunc(legacyFWChains, func(data legacyChain) bool { + return data.Chain == name + }) + } + jumper, target := index("osvc-postrouting"), index("osvc-masq") + require.NotEqual(t, -1, jumper) + require.NotEqual(t, -1, target) + assert.Less(t, jumper, target, "the chain holding the jumps must be deleted first") +} + +// TestFWRulesetIsOneTransaction pins the shape that makes a setup atomic: the +// table is created, deleted and defined again, all in one document. +// +// Adding a rule at a time left the masquerade and the forward accepts absent +// for as long as the setup ran. +func TestFWRulesetIsOneTransaction(t *testing.T) { + got, err := fwRuleset([]fwNetwork{ + {CIDR: "10.100.0.0/22", Dev: "obr_backend3"}, + {CIDR: "127.0.0.1/32"}, + {CIDR: "fdfe::/112", Dev: "obr_backend1"}, + }, nil) + require.NoError(t, err) + + assert.Equal(t, `table ip osvc { } +delete table ip osvc +table ip osvc { + chain osvc-masq { + ip daddr 10.100.0.0/22 counter return + ip daddr 127.0.0.1/32 counter return + ip daddr 224.0.0.0/8 counter return + masquerade + } + chain osvc-postrouting { + type nat hook postrouting priority srcnat; policy accept; + ip saddr 10.100.0.0/22 counter jump osvc-masq + } + chain osvc-forward { + type filter hook forward priority filter; policy accept; + iifname "obr_backend3" counter accept + oifname "obr_backend3" counter accept + } +} +table ip6 osvc { } +delete table ip6 osvc +table ip6 osvc { + chain osvc-masq { + ip6 daddr fdfe::/112 counter return + masquerade + } + chain osvc-postrouting { + type nat hook postrouting priority srcnat; policy accept; + ip6 saddr fdfe::/112 counter jump osvc-masq + } + chain osvc-forward { + type filter hook forward priority filter; policy accept; + iifname "obr_backend1" counter accept + oifname "obr_backend1" counter accept + } +} +`, got) +} + +// TestFWRulesetSkipsAFamilyWithNoNetwork pins that no table is written for an +// address family nothing is configured in. +func TestFWRulesetSkipsAFamilyWithNoNetwork(t *testing.T) { + got, err := fwRuleset([]fwNetwork{{CIDR: "10.22.0.0/16", Dev: "obr_default"}}, nil) + require.NoError(t, err) + assert.Contains(t, got, "table ip osvc {") + assert.NotContains(t, got, "ip6 osvc") +} + +// TestFWRulesetDeletesTheLegacyChainsFirst pins that a chain is emptied before +// any is deleted: nft refuses to delete a chain a rule still jumps to, and the +// jumps live in a chain of the same list. +func TestFWRulesetDeletesTheLegacyChainsFirst(t *testing.T) { + got, err := fwRuleset(nil, []legacyChain{ + {Family: "ip", Table: "nat", Chain: "osvc-postrouting"}, + {Family: "ip", Table: "nat", Chain: "osvc-masq"}, + }) + require.NoError(t, err) + assert.Equal(t, `flush chain ip nat osvc-postrouting +flush chain ip nat osvc-masq +delete chain ip nat osvc-postrouting +delete chain ip nat osvc-masq +`, got) +} + +func TestFWRulesetRefusesABadCIDR(t *testing.T) { + _, err := fwRuleset([]fwNetwork{{CIDR: "not-a-cidr"}}, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "not-a-cidr") +} + +// TestFWRulesetIsAcceptedByNft parses the rendered document with the nft +// binary, which reports what it would refuse without applying anything. +// +// The devices named here do not exist on the machine running this, and must +// not need to: a rule naming an interface by index is refused when that +// interface is absent, which is why the devices are matched by name. +func TestFWRulesetIsAcceptedByNft(t *testing.T) { + if _, err := exec.LookPath("nft"); err != nil { + t.Skip("nft is not installed") + } + ruleset, err := fwRuleset([]fwNetwork{ + {CIDR: "10.100.0.0/22", Dev: "obr_backend3"}, + {CIDR: "127.0.0.1/32"}, + {CIDR: "fdfe::/112", Dev: "obr_backend1"}, + }, nil) + require.NoError(t, err) + + cmd := exec.Command("nft", "--check", "-f", "-") + cmd.Stdin = strings.NewReader(ruleset) + b, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "nft refused the ruleset: %s", b) +} + +// TestFWRulesetSkipsADeviceNamedPastTheKernelLimit pins that one network +// named too long does not cost the node its whole firewall. The rules are one +// transaction, so a rule nft refuses is a ruleset nft refuses, and a device +// whose name cannot exist would have no rule matching it anyway. +func TestFWRulesetSkipsADeviceNamedPastTheKernelLimit(t *testing.T) { + got, err := fwRuleset([]fwNetwork{ + {CIDR: "10.100.0.0/22", Dev: "obr_backendlong1"}, + {CIDR: "10.22.0.0/16", Dev: "obr_default"}, + }, nil) + require.NoError(t, err) + + // A device that cannot exist is treated as no device at all, which is + // how a public network is already left alone: no forward accept and no + // masquerade jump. The network is still returned from the masquerade of + // the others, which needs no device. + assert.NotContains(t, got, "obr_backendlong1") + assert.Contains(t, got, `iifname "obr_default"`) + assert.Contains(t, got, "ip daddr 10.100.0.0/22 counter return") + assert.NotContains(t, got, "ip saddr 10.100.0.0/22 counter jump osvc-masq") + + if _, err := exec.LookPath("nft"); err != nil { + t.Skip("nft is not installed") + } + cmd := exec.Command("nft", "--check", "-f", "-") + cmd.Stdin = strings.NewReader(got) + b, err := cmd.CombinedOutput() + require.NoErrorf(t, err, "nft refused the ruleset: %s", b) +} diff --git a/core/network/ipam.go b/core/network/ipam.go new file mode 100644 index 000000000..ceeb20790 --- /dev/null +++ b/core/network/ipam.go @@ -0,0 +1,194 @@ +package network + +import ( + "encoding/json" + "fmt" + "net" + "os" + "path/filepath" + + "github.com/opensvc/om3/v3/core/instance" + "github.com/opensvc/om3/v3/core/ipam" + "github.com/opensvc/om3/v3/core/naming" + "github.com/opensvc/om3/v3/core/object" + "github.com/opensvc/om3/v3/util/hostname" +) + +// ipAddrInfoKey is the resource status info an ip resource publishes its +// address under. +const ipAddrInfoKey = "ipaddr" + +// NewAllocator returns the allocator of a network on a node, or nil when the +// network is one om allocates no address in. +func NewAllocator(nw Networker, nodename string) (*ipam.T, error) { + i, ok := nw.(IPAMer) + if !ok { + return nil, nil + } + rng, err := i.AllocatableRange(nodename) + if err != nil { + return nil, fmt.Errorf("network %s: %w", nw.Name(), err) + } + if rng == nil { + return nil, nil + } + return &ipam.T{ + Name: nw.Name(), + Range: rng, + Gateway: ipam.Gateway(rng), + Dir: ipam.StoreDir(nw.Name()), + // The record of the host-local plugin is read while it still hands + // out addresses of this network, and never written: an address it + // gave has no reservation of om's until a setup adopts it. + PeerDirs: []string{filepath.Join(cniCacheDir, nw.Name())}, + }, nil +} + +// Lookup returns the network of a name, or nil when no network has it. +func Lookup(name string) (Networker, []string, error) { + node, err := object.NewNode(object.WithVolatile(true)) + if err != nil { + return nil, nil, err + } + names := make([]string, 0) + for _, nw := range Networks(node) { + if nw.Name() == name { + return nw, nil, nil + } + names = append(names, nw.Name()) + } + return nil, names, nil +} + +// cniCacheDir is where the host-local plugin records the addresses it hands +// out. +const cniCacheDir = "/var/lib/cni/networks" + +// setupIPAM records the addresses the resources of this node already hold, so +// the allocator hands out the addresses that are free rather than the ones it +// has no reservation for yet. +// +// It runs here rather than when a resource starts, because it has to be done +// once for a whole network and before the first allocation in it. A setup is +// where the once-per-network work of a node already happens, and it runs on +// daemon start and on every cluster configuration change. +// +// This matters most the first time a node allocates in a network whose +// addresses another allocator was handing out. The host-local plugin stops +// running then, so it stops releasing what it gave: every address it holds +// would be blocked for as long as its record lasts, and every address it holds +// would be free to hand out twice if that record were ignored. Neither, once +// the addresses in use are reservations of om's. +func setupIPAM(nws []Networker) error { + reservations, err := localReservations() + if err != nil { + return err + } + installed, err := installedPaths() + if err != nil { + return err + } + nodename := hostname.Hostname() + for _, nw := range nws { + a, err := NewAllocator(nw, nodename) + if err != nil { + nw.Log().Warnf("ipam: %s", err) + continue + } + if a == nil { + continue + } + adopted, err := a.Adopt(reservations) + if err != nil { + return err + } + if adopted > 0 { + nw.Log().Infof("ipam: adopted %d address(es) already held in this network", adopted) + } + reaped, err := a.Reap(func(key string) bool { + p, ok := ipam.PathOfKey(key) + if !ok { + // A key of a shape this om does not write is not one it may + // decide is gone. + return true + } + return installed[p.String()] + }) + if err != nil { + return err + } + if reaped > 0 { + nw.Log().Infof("ipam: released %d address(es) held for an object that no longer exists", reaped) + } + drained, left, err := a.DrainPeers() + if err != nil { + return err + } + if drained > 0 { + nw.Log().Infof("ipam: dropped %d address(es) from the record of the plugin that used to allocate them", drained) + } + if left > 0 { + nw.Log().Warnf("ipam: the record of the plugin that used to allocate in this network still holds %d address(es) om accounts for in no way, and they stay excluded. Remove them from %s once nothing uses them", left, filepath.Join(cniCacheDir, nw.Name())) + } + } + return nil +} + +// localReservations returns the addresses the ip resources of this node hold, +// read from the status every object caches locally. +// +// The cache of the host-local plugin cannot serve: it names the holder of an +// address by the pid of a network namespace, which says nothing about which +// resource that is, so an address adopted from it could never be released by +// the resource that stops. The object status names the resource, and it is on +// this node, which is the only node whose addresses matter here. +func localReservations() ([]ipam.Reservation, error) { + paths, err := naming.InstalledPaths() + if err != nil { + return nil, err + } + l := make([]ipam.Reservation, 0) + for _, p := range paths { + status, err := loadInstanceStatus(p) + if err != nil { + // An object with no status yet holds no address yet. + continue + } + for rid, rstat := range status.Resources { + s, ok := rstat.Info[ipAddrInfoKey].(string) + if !ok || s == "" { + continue + } + ip := net.ParseIP(s) + if ip == nil { + continue + } + l = append(l, ipam.Reservation{IP: ip, Key: ipam.Key(p, rid)}) + } + } + return l, nil +} + +// loadInstanceStatus reads the status an object cached, without evaluating it. +func loadInstanceStatus(p naming.Path) (instance.Status, error) { + var data instance.Status + b, err := os.ReadFile(filepath.Join(p.VarDir(), "status.json")) + if err != nil { + return data, err + } + err = json.Unmarshal(b, &data) + return data, err +} + +// installedPaths returns the objects configured on this node, by path. +func installedPaths() (map[string]bool, error) { + paths, err := naming.InstalledPaths() + if err != nil { + return nil, err + } + m := make(map[string]bool, len(paths)) + for _, p := range paths { + m[p.String()] = true + } + return m, nil +} diff --git a/core/network/main.go b/core/network/main.go index b8a7e6876..7c132514f 100644 --- a/core/network/main.go +++ b/core/network/main.go @@ -114,6 +114,15 @@ type ( Setuper interface { Setup() error } + // IPAMer is implemented by the network drivers whose addresses om + // allocates. A driver that does not implement it has om allocate nothing + // in it, which is what the lo network wants. + IPAMer interface { + // AllocatableRange returns the addresses a node draws from in this + // network. The gateway of that range is its first address plus one, + // which is how both drivers derive the address of their bridge. + AllocatableRange(nodename string) (*net.IPNet, error) + } CNIer interface { CNIConfigData() (interface{}, error) } diff --git a/core/network/setup.go b/core/network/setup.go index cda952e22..469b8919a 100644 --- a/core/network/setup.go +++ b/core/network/setup.go @@ -56,6 +56,9 @@ func Setup(n *object.Node, names ...string) error { n.Log().Infof("network setup: commit config changes on %s", strings.Join(needCommit, ",")) cluster.Config().Set(kops...) } + if err := setupIPAM(nws); err != nil { + errs = append(errs, err) + } if err := setupFW(n, nws); err != nil { errs = append(errs, err) } diff --git a/drivers/networkbridge/main.go b/drivers/networkbridge/main.go index 66d16bc8a..19c9186e9 100644 --- a/drivers/networkbridge/main.go +++ b/drivers/networkbridge/main.go @@ -107,6 +107,17 @@ func (t *T) CNIConfigData() (interface{}, error) { return m, nil } +// AllocatableRange returns the whole subnet of the network. +// +// A bridge network is node local and its addresses are not routable: the +// bridge is per node, nothing carries the subnet between nodes, and the same +// address on two nodes never meets. So every node draws from all of it, and +// the slicing a routed_bridge needs would only make the range smaller for no +// gain. +func (t *T) AllocatableRange(_ string) (*net.IPNet, error) { + return t.IPNet() +} + func (t *T) bridgeIP() (net.IP, error) { subnetStr := t.Network() if subnetStr == "" { diff --git a/drivers/networkroutedbridge/main.go b/drivers/networkroutedbridge/main.go index c9ba67801..ae74f9fe9 100644 --- a/drivers/networkroutedbridge/main.go +++ b/drivers/networkroutedbridge/main.go @@ -276,6 +276,15 @@ func (t *T) checkMaxIpsPerNode(network *net.IPNet, maskPerNode int, nodes []stri return nil } +// AllocatableRange returns the subnet this node was assigned in the network. +// +// A routed_bridge slices its network, one subnet per node, and routes between +// them, so a node draws from its own slice and two nodes never draw the same +// address. +func (t *T) AllocatableRange(nodename string) (*net.IPNet, error) { + return t.NodeSubnet(nodename) +} + func (t *T) Setup() error { var ( localIP net.IP @@ -302,7 +311,13 @@ func (t *T) Setup() error { return fmt.Errorf("link up: %w", err) } if localIP, err = t.getLocalIP(); err != nil { - return fmt.Errorf("get local ip: %w", err) + // Say what the failure costs. The setup stops here, past the bridge + // and before the peers, so the network is left half made: the bridge + // is up with its gateway address, no tunnel or route reaches a peer, + // and setupNetwork returns before writing the cni configuration. An + // object using this network does not start on this node. + t.Log().Errorf("no %s address for this node, so this network is left with its bridge up, no tunnel or route to the peer nodes, and no cni configuration written: an object using it will not start on this node. Set the addr keyword scoped to this node, or give the node an address of that family", t.getAF()) + return fmt.Errorf("get local %s address of this node: %w", t.getAF(), err) } nodes, err := t.Nodes() if err != nil { diff --git a/drivers/rescontainer/resolv.go b/drivers/rescontainer/resolv.go new file mode 100644 index 000000000..475485e3c --- /dev/null +++ b/drivers/rescontainer/resolv.go @@ -0,0 +1,93 @@ +package rescontainer + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +type ( + // ResolvConf is the resolver configuration handed to a container. + ResolvConf struct { + // Nameservers are written one "nameserver" line each, in order. + Nameservers []string + + // Searches are written as the single "search" line, in order. + Searches []string + + // Options are written as the single "options" line, in order. + Options []string + } +) + +// ResolvConfOptions are the resolver options a container is given, the same +// ones the "--dns-opt" arguments used to carry. +var ResolvConfOptions = []string{"ndots:2", "edns0", "use-vc"} + +// MaxNameservers is the number of nameservers a resolver reads. glibc and musl +// both stop at MAXNS, and the lines past it are dead weight in the file. +// +// The search list is not capped the same way: the implementations disagree on +// its limit, glibc counting domains and musl counting bytes, so the file says +// what was asked and each resolver applies its own rule. +const MaxNameservers = 3 + +// SearchDomains returns the search list of an object, which is its domain and +// each of the parents of that domain. +// +// The domain of "root/svc/svc1" in cluster "clu" is "root.svc.clu", so a name +// is looked up in the namespace of the object, then in its kind, then in the +// cluster. The extra domains come first, so a configured one wins. +func SearchDomains(objectDomain string, extra []string) []string { + l := make([]string, 0, len(extra)+3) + l = append(l, extra...) + for domain := objectDomain; domain != ""; { + l = append(l, domain) + _, parent, found := strings.Cut(domain, ".") + if !found { + break + } + domain = parent + } + return l +} + +// String returns the file content. +func (t ResolvConf) String() string { + var sb strings.Builder + if len(t.Searches) > 0 { + sb.WriteString("search " + strings.Join(t.Searches, " ") + "\n") + } + for i, nameserver := range t.Nameservers { + if i >= MaxNameservers { + break + } + sb.WriteString("nameserver " + nameserver + "\n") + } + if len(t.Options) > 0 { + sb.WriteString("options " + strings.Join(t.Options, " ") + "\n") + } + return sb.String() +} + +// IsZero returns true when the resolver configuration would say nothing, and +// the container is better left with the one its image carries. +func (t ResolvConf) IsZero() bool { + return len(t.Nameservers) == 0 && len(t.Searches) == 0 +} + +// WriteResolvConf writes the resolver configuration to path and returns it. +// +// The file is written where a container mounts it from, and the container +// reads it for as long as it runs: a container adapts to a cluster layout +// change by being restarted, which is when this is written again. +func WriteResolvConf(path string, resolvConf ResolvConf) (string, error) { + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + return "", fmt.Errorf("resolv.conf dir: %w", err) + } + if err := os.WriteFile(path, []byte(resolvConf.String()), 0644); err != nil { + return "", fmt.Errorf("resolv.conf: %w", err) + } + return path, nil +} diff --git a/drivers/rescontainer/resolv_test.go b/drivers/rescontainer/resolv_test.go new file mode 100644 index 000000000..03eeadb8d --- /dev/null +++ b/drivers/rescontainer/resolv_test.go @@ -0,0 +1,126 @@ +package rescontainer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSearchDomains pins the list a container looks a short name up in: its +// own domain first, then each parent of it, so "pod2" reaches +// "pod2.root.svc.cluster1" from an object of the root namespace. +func TestSearchDomains(t *testing.T) { + for _, tc := range []struct { + name string + domain string + extra []string + want []string + }{ + { + name: "an object domain walks up to the cluster", + domain: "root.svc.cluster1", + want: []string{"root.svc.cluster1", "svc.cluster1", "cluster1"}, + }, + { + name: "another namespace searches its own first", + domain: "test.vol.cluster1", + want: []string{"test.vol.cluster1", "vol.cluster1", "cluster1"}, + }, + { + name: "a configured domain comes first", + domain: "root.svc.cluster1", + extra: []string{"corp.example"}, + want: []string{"corp.example", "root.svc.cluster1", "svc.cluster1", "cluster1"}, + }, + { + name: "no object domain leaves the configured ones", + domain: "", + extra: []string{"corp.example"}, + want: []string{"corp.example"}, + }, + { + name: "a domain with no parent is the whole list", + domain: "cluster1", + want: []string{"cluster1"}, + }, + { + name: "nothing to search", + domain: "", + want: []string{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, SearchDomains(tc.domain, tc.extra)) + }) + } +} + +func TestResolvConfString(t *testing.T) { + resolvConf := ResolvConf{ + Nameservers: []string{"10.29.0.11", "10.29.0.12"}, + Searches: []string{"root.svc.cluster1", "svc.cluster1"}, + Options: ResolvConfOptions, + } + assert.Equal(t, `search root.svc.cluster1 svc.cluster1 +nameserver 10.29.0.11 +nameserver 10.29.0.12 +options ndots:2 edns0 use-vc +`, resolvConf.String()) +} + +// TestResolvConfStopsAtTheNameserversAResolverReads pins that the file holds +// no line a resolver would not read. glibc stops at MAXNS, and a cluster can +// name more servers than that. +func TestResolvConfStopsAtTheNameserversAResolverReads(t *testing.T) { + resolvConf := ResolvConf{ + Nameservers: []string{"10.29.0.11", "10.29.0.12", "10.29.0.13", "1.2.2.4"}, + } + assert.Equal(t, `nameserver 10.29.0.11 +nameserver 10.29.0.12 +nameserver 10.29.0.13 +`, resolvConf.String()) +} + +// TestResolvConfIsZero pins that a container is left with the resolver of its +// image rather than an empty file, when the cluster names none. +func TestResolvConfIsZero(t *testing.T) { + assert.True(t, ResolvConf{Options: ResolvConfOptions}.IsZero()) + assert.False(t, ResolvConf{Nameservers: []string{"10.29.0.11"}}.IsZero()) + assert.False(t, ResolvConf{Searches: []string{"svc.cluster1"}}.IsZero()) +} + +func TestWriteResolvConfCreatesItsDir(t *testing.T) { + path := filepath.Join(t.TempDir(), "container#1", "resolv.conf") + got, err := WriteResolvConf(path, ResolvConf{Nameservers: []string{"10.29.0.11"}}) + require.NoError(t, err) + assert.Equal(t, path, got) + b, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "nameserver 10.29.0.11\n", string(b)) +} + +// TestWriteResolvConfRewritesInPlace pins that a rewrite keeps the inode. +// +// A container bind mounts this file, and a bind mount follows the inode: a +// write to a new file renamed over this one would leave every running +// container reading the file it was started with, with nothing to say so. +func TestWriteResolvConfRewritesInPlace(t *testing.T) { + path := filepath.Join(t.TempDir(), "resolv.conf") + _, err := WriteResolvConf(path, ResolvConf{Nameservers: []string{"10.29.0.11"}}) + require.NoError(t, err) + before, err := os.Stat(path) + require.NoError(t, err) + + _, err = WriteResolvConf(path, ResolvConf{Nameservers: []string{"10.29.0.99"}}) + require.NoError(t, err) + after, err := os.Stat(path) + require.NoError(t, err) + + assert.True(t, os.SameFile(before, after), "the rewrite must keep the inode a mount follows") + b, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "nameserver 10.29.0.99\n", string(b)) +} diff --git a/drivers/rescontainerocibase/executor_args.go b/drivers/rescontainerocibase/executor_args.go index 505b83a3c..f97b5c455 100644 --- a/drivers/rescontainerocibase/executor_args.go +++ b/drivers/rescontainerocibase/executor_args.go @@ -7,6 +7,7 @@ import ( "os" "strings" + "github.com/opensvc/om3/v3/drivers/rescontainer" "github.com/opensvc/om3/v3/util/args" "github.com/opensvc/om3/v3/util/file" "github.com/opensvc/om3/v3/util/plog" @@ -291,26 +292,9 @@ func (ea *ExecutorArg) runArgsDNSSearch() []string { return nil } var a []string - for _, s := range ea.BT.DNSSearch { + for _, s := range rescontainer.SearchDomains(ea.BT.ObjectDomain, ea.BT.DNSSearch) { a = append(a, "--dns-search", s) } - - dom0 := ea.BT.ObjectDomain - if len(dom0) > 0 { - a = append(a, "--dns-search", dom0) - dom0S := strings.SplitN(dom0, ".", 2) - if len(dom0S) > 1 { - dom1 := dom0S[1] - if len(dom1) > 0 { - a = append(a, "--dns-search", dom1) - dom1S := strings.SplitN(dom1, ".", 2) - if len(dom1S) > 1 { - dom2 := dom1S[1] - a = append(a, "--dns-search", dom2) - } - } - } - } return a } diff --git a/drivers/rescontainerocibase/text/kw/dns_search b/drivers/rescontainerocibase/text/kw/dns_search index 465df0573..410710502 100644 --- a/drivers/rescontainerocibase/text/kw/dns_search +++ b/drivers/rescontainerocibase/text/kw/dns_search @@ -1,4 +1,13 @@ The whitespace-separated list of DNS domains to search for shortname lookups. -If empty or not set, the list will be `..svc. - .svc. svc.`. +If empty or not set, the list is the domain of the object and each of the +parents of that domain: `.. +. `. A shortname is therefore looked up in +the namespace of the object first, then in its kind, then in the cluster. + +The fqdn of the object is not in that list. The hostname of a container is +published in the cluster zone as `.`, so reaching a +container of the same object by its hostname alone takes naming that fqdn +here. Containers of an instance are expected to share a netns, which is the +layout the ip drivers are written for, and to reach each other over +127.0.0.1, which needs no name at all. diff --git a/drivers/rescontainerpodman/executor_args.go b/drivers/rescontainerpodman/executor_args.go index bd514d87c..dc10aa3b2 100644 --- a/drivers/rescontainerpodman/executor_args.go +++ b/drivers/rescontainerpodman/executor_args.go @@ -3,8 +3,10 @@ package rescontainerpodman import ( "context" "os/exec" + "path/filepath" "strings" + "github.com/opensvc/om3/v3/drivers/rescontainer" "github.com/opensvc/om3/v3/util/args" ) @@ -36,14 +38,57 @@ func (ea *ExecutorArg) RunArgsBase(ctx context.Context) (*args.T, error) { } if a.HasOptionAndMatchingValue("--net", "(^none$|^container:.*$)") || a.HasOptionAndMatchingValue("--network", "(^none$|^container:.*$)") { + // podman refuses the dns options in these two network modes: + // + // conflicting options: dns and the network mode: none + // conflicting options: dns and the network mode: container + // + // and writes no /etc/resolv.conf of its own for them either, so + // dropping the options left the container of an object using the + // pause container model with no resolver at all: it could not resolve + // a name of its own cluster. Hand it the file the options would have + // produced instead. a.DropOptionAndAnyValue("--dns") a.DropOptionAndAnyValue("--dns-opt") a.DropOptionAndAnyValue("--dns-option") a.DropOptionAndAnyValue("--dns-search") + if mount, err := ea.resolvConfMount(); err != nil { + return nil, err + } else if mount != "" { + a.DropOptionAndExactValue("-v", mount) + a.DropOptionAndExactValue("--volume", mount) + a.Append("-v", mount) + } } return a, nil } +// resolvConfMount writes the resolver configuration of the container and +// returns the option mounting it, or an empty string when there is nothing to +// say to the container. +// +// The file is written on every start, and a container reads it for as long as +// it runs: a container adapts to a cluster layout change by being restarted. +func (ea *ExecutorArg) resolvConfMount() (string, error) { + resolvConf := rescontainer.ResolvConf{ + Nameservers: ea.BT.DNS, + Searches: rescontainer.SearchDomains(ea.BT.ObjectDomain, ea.BT.DNSSearch), + Options: rescontainer.ResolvConfOptions, + } + if resolvConf.IsZero() { + return "", nil + } + if n := len(resolvConf.Nameservers); n > rescontainer.MaxNameservers { + ea.BT.Log().Warnf("cluster.dns names %d nameservers, a resolver reads the first %d: %s is not written to the container resolv.conf", + n, rescontainer.MaxNameservers, strings.Join(resolvConf.Nameservers[rescontainer.MaxNameservers:], ", ")) + } + path, err := rescontainer.WriteResolvConf(filepath.Join(ea.BT.VarDir(), "resolv.conf"), resolvConf) + if err != nil { + return "", err + } + return path + ":/etc/resolv.conf:ro", nil +} + func (ea *ExecutorArg) WaitRemoved(ctx context.Context) error { return ea.wait(ctx, "container", "wait", "--ignore", "--condition", "removing", ea.BT.ContainerName()) } diff --git a/drivers/rescontainerpodman/main.go b/drivers/rescontainerpodman/main.go index 55d7eedb2..ae2c78ffd 100644 --- a/drivers/rescontainerpodman/main.go +++ b/drivers/rescontainerpodman/main.go @@ -8,8 +8,6 @@ import ( type ( T struct { rescontainerocibase.BT - - CNIConfig string } ExecutorArg struct { @@ -33,11 +31,16 @@ func (t *T) configure(ea *ExecutorArg) { _ = t.WithExecuter(executor) } +// executorArg returns the executor of the podman commands. +// +// Its base args are empty: this driver never asks podman to build a network. +// The netns keyword resolves to "host", to a private namespace, or to the one +// of another container, and the addresses are configured by the ip drivers, +// from outside. Podman therefore never reads a network configuration, and the +// "--cni-config-dir" this used to pass is both inert and, since podman 5 +// dropped the cni backend, an unknown flag. func (t *T) executorArg() *ExecutorArg { var baseArgs []string - if t.CNIConfig != "" { - baseArgs = append(baseArgs, "--cni-config-dir", t.CNIConfig) - } return &ExecutorArg{ ExecutorArg: &rescontainerocibase.ExecutorArg{ diff --git a/drivers/rescontainerpodman/main_test.go b/drivers/rescontainerpodman/main_test.go index f9706b0ac..f6615b4c9 100644 --- a/drivers/rescontainerpodman/main_test.go +++ b/drivers/rescontainerpodman/main_test.go @@ -8,8 +8,16 @@ import ( "github.com/opensvc/om3/v3/drivers/rescontainerocibase" ) +// Test_ExecBaseArgs pins that no base argument is passed to podman. +// +// The driver used to pass "--cni-config-dir", from a node keyword with a +// default, so every podman command carried it. Podman never read it: the +// netns keyword resolves to a host, a private or another container's +// namespace, and the ip drivers configure the addresses from outside. Podman +// 5 dropped the cni backend and the flag with it, where it stopped being +// inert and became an unknown flag. func Test_ExecBaseArgs(t *testing.T) { - d := &T{CNIConfig: "/test-cni-config.d"} + d := &T{} if err := d.Configure(); err != nil { require.NoError(t, err) @@ -20,6 +28,5 @@ func Test_ExecBaseArgs(t *testing.T) { ExecutorArgser().(rescontainerocibase.ExecutorBaseArgser). ExecBaseArgs() - expectedBaseArgs := []string{"--cni-config-dir", "/test-cni-config.d"} - require.ElementsMatchf(t, expectedBaseArgs, baseArgs, "want: %s\ngot: %s", expectedBaseArgs, baseArgs) + require.Emptyf(t, baseArgs, "podman is passed no base argument, got: %s", baseArgs) } diff --git a/drivers/rescontainerpodman/manifest.go b/drivers/rescontainerpodman/manifest.go index 5d953453e..30492981f 100644 --- a/drivers/rescontainerpodman/manifest.go +++ b/drivers/rescontainerpodman/manifest.go @@ -38,9 +38,6 @@ func (t *T) DriverID() driver.ID { // Manifest exposes to the core the input expected by the driver. func (t *T) Manifest() *manifest.T { m := t.BT.ManifestWithID(DrvID) - m.Add( - manifest.ContextCNIConfig, - ) m.AddKeywords(kws...) return m } diff --git a/drivers/resipcni/ipam.go b/drivers/resipcni/ipam.go new file mode 100644 index 000000000..19f63ef03 --- /dev/null +++ b/drivers/resipcni/ipam.go @@ -0,0 +1,158 @@ +//go:build linux + +package resipcni + +import ( + "encoding/json" + "fmt" + "net" + "os" + "strings" + + "github.com/opensvc/om3/v3/core/ipam" + "github.com/opensvc/om3/v3/core/network" + "github.com/opensvc/om3/v3/util/hostname" +) + +// ipam returns the allocator of the network this resource plugs into, or nil +// when om allocates no address in it. +// +// A network keyword names a cni configuration, which om writes for the +// networks of the cluster and an administrator may write by hand for a plugin +// om knows nothing about. The second keeps the ipam of its own configuration: +// om allocates in the networks it owns, and leaves the others alone. +func (t *T) ipam() (*ipam.T, error) { + nw, _, err := network.Lookup(t.Network) + if err != nil { + return nil, err + } + if nw == nil { + return nil, nil + } + return network.NewAllocator(nw, hostname.Hostname()) +} + +// Configure reports a network this resource could never plug into, when the +// resource is loaded rather than when it is started. +// +// A name is good when a cni configuration of that name exists, whoever wrote +// it, or when it is a network of the cluster, whose configuration a network +// setup writes. A name that is neither is a renamed network or a typo. +func (t *T) Configure() error { + if _, err := os.Stat(t.netConfFile()); err == nil { + return nil + } + nw, names, err := network.Lookup(t.Network) + if err != nil { + return err + } + if nw != nil { + return nil + } + return fmt.Errorf("unknown network %s: no %s, and no cluster network of that name, expected one of %s", + t.Network, t.netConfFile(), strings.Join(names, ", ")) +} + +// ipamKey names the reservation of this resource. An instance holds as many ip +// resources as it needs, several of them in one network, so the address +// belongs to the resource rather than to the object. +func (t *T) ipamKey() string { + return ipam.Key(t.Path, t.RID()) +} + +// allocateIP reserves the address of this resource, and returns the one it +// already holds when it holds one. +func (t *T) allocateIP() (net.IP, error) { + i, err := t.ipam() + if err != nil { + return nil, err + } + if i == nil { + return nil, nil + } + ip, err := i.Allocate(t.ipamKey()) + if err != nil { + return nil, err + } + t.Log().Infof("allocated %s in network %s", ip, i.Name) + return ip, nil +} + +// freeIP releases the address of this resource. +func (t *T) freeIP() error { + i, err := t.ipam() + if err != nil { + return err + } + if i == nil { + return nil + } + return i.Free(t.ipamKey()) +} + +// staticIPAM rewrites the ipam section of a plugin configuration so the plugin +// is told the address rather than asked to pick one. +// +// The plugin still does the wiring, which is what a cni plugin is for. It no +// longer does the addressing, which om does for every network it owns, so one +// network can serve this driver and ip.netns without two allocators handing +// out the same address. +// +// The routes of the configuration are carried over untouched: they are the +// network's, and the static plugin takes the same ones. The gateway is the +// first address of the range, which is where both network drivers put their +// bridge, and which host-local defaulted to. +func staticIPAM(conf []byte, ip net.IP, rng *net.IPNet, gateway net.IP) ([]byte, error) { + var m map[string]any + if err := json.Unmarshal(conf, &m); err != nil { + return nil, err + } + address := (&net.IPNet{IP: ip, Mask: rng.Mask}).String() + static := map[string]any{ + "type": "static", + "addresses": []map[string]any{{"address": address, "gateway": gateway.String()}}, + } + if previous, ok := m["ipam"].(map[string]any); ok { + if routes, ok := previous["routes"]; ok { + static["routes"] = routes + } + } + m["ipam"] = static + return json.Marshal(m) +} + +// netConfBytesFor returns the configuration handed to the plugin, with the +// address om allocated written into it. +// +// A network om allocates no address in is handed its configuration as it is on +// disk, so a third party plugin keeps the ipam it was given. +func (t *T) netConfBytesFor(ip net.IP) ([]byte, error) { + b, err := t.netConfBytes() + if err != nil { + return nil, err + } + if ip == nil { + return b, nil + } + i, err := t.ipam() + if err != nil { + return nil, err + } + if i == nil || i.Range == nil { + return b, nil + } + return staticIPAM(b, ip, i.Range, i.Gateway) +} + +// allocatedIP returns the address reserved for this resource, or nil when it +// has none. +func (t *T) allocatedIP() (net.IP, error) { + i, err := t.ipam() + if err != nil { + return nil, err + } + if i == nil { + return nil, nil + } + return i.Allocated(t.ipamKey()) +} diff --git a/drivers/resipcni/ipam_test.go b/drivers/resipcni/ipam_test.go new file mode 100644 index 000000000..f4deaa7a4 --- /dev/null +++ b/drivers/resipcni/ipam_test.go @@ -0,0 +1,88 @@ +//go:build linux + +package resipcni + +import ( + "encoding/json" + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestStaticIPAMTellsThePluginTheAddress pins the rewrite that moves the +// addressing from the plugin to om: the plugin keeps the wiring and the +// routes of the network, and is told which address to configure. +func TestStaticIPAMTellsThePluginTheAddress(t *testing.T) { + conf := []byte(`{ + "cniVersion": "0.3.0", + "name": "backend3", + "type": "bridge", + "bridge": "obr_backend3", + "isGateway": true, + "ipMasq": false, + "ipam": { + "type": "host-local", + "routes": [{"dst": "0.0.0.0/0"}, {"dst": "10.100.0.0/22", "gw": "10.100.0.1"}], + "subnet": "10.100.0.0/24" + } + }`) + _, rng, err := net.ParseCIDR("10.100.0.0/24") + require.NoError(t, err) + + b, err := staticIPAM(conf, net.ParseIP("10.100.0.24"), rng, net.ParseIP("10.100.0.1")) + require.NoError(t, err) + + var m map[string]any + require.NoError(t, json.Unmarshal(b, &m)) + + // What the plugin does is untouched. + assert.Equal(t, "bridge", m["type"]) + assert.Equal(t, "obr_backend3", m["bridge"]) + assert.Equal(t, true, m["isGateway"]) + + ipam, ok := m["ipam"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "static", ipam["type"], "om picks the address, not the plugin") + assert.Nil(t, ipam["subnet"], "a range to pick from is not the plugin's business any more") + + addresses, ok := ipam["addresses"].([]any) + require.True(t, ok) + require.Len(t, addresses, 1) + address := addresses[0].(map[string]any) + assert.Equal(t, "10.100.0.24/24", address["address"], "the mask is the range's") + assert.Equal(t, "10.100.0.1", address["gateway"]) + + // The routes of the network are carried over, or the container loses its + // default route. + routes, ok := ipam["routes"].([]any) + require.True(t, ok) + require.Len(t, routes, 2) + assert.Equal(t, "0.0.0.0/0", routes[0].(map[string]any)["dst"]) + assert.Equal(t, "10.100.0.0/22", routes[1].(map[string]any)["dst"]) + assert.Equal(t, "10.100.0.1", routes[1].(map[string]any)["gw"]) +} + +// TestStaticIPAMOfAConfWithNoRoutes pins that a configuration naming no route +// is rewritten all the same. +func TestStaticIPAMOfAConfWithNoRoutes(t *testing.T) { + _, rng, err := net.ParseCIDR("fdfe::/114") + require.NoError(t, err) + b, err := staticIPAM([]byte(`{"name": "backend1", "type": "bridge"}`), net.ParseIP("fdfe::12"), rng, net.ParseIP("fdfe::1")) + require.NoError(t, err) + + var m map[string]any + require.NoError(t, json.Unmarshal(b, &m)) + ipam := m["ipam"].(map[string]any) + assert.Nil(t, ipam["routes"]) + address := ipam["addresses"].([]any)[0].(map[string]any) + assert.Equal(t, "fdfe::12/114", address["address"]) + assert.Equal(t, "fdfe::1", address["gateway"]) +} + +func TestStaticIPAMRefusesAConfItCannotRead(t *testing.T) { + _, rng, _ := net.ParseCIDR("10.100.0.0/24") + _, err := staticIPAM([]byte("not json"), net.ParseIP("10.100.0.24"), rng, net.ParseIP("10.100.0.1")) + require.Error(t, err) +} diff --git a/drivers/resipcni/main.go b/drivers/resipcni/main.go index 5670e8bce..1e0ad670b 100644 --- a/drivers/resipcni/main.go +++ b/drivers/resipcni/main.go @@ -6,14 +6,12 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "math" "net" "os" "path/filepath" "slices" - "strconv" "strings" "time" @@ -69,11 +67,6 @@ const ( netnsRunDir = "/run/netns" ) -var ( - ErrNoIPAddrAvail = errors.New("no ip address available") - ErrDupIPAlloc = errors.New("duplicate ip allocation") -) - func New() resource.Driver { t := &T{} return t @@ -206,42 +199,6 @@ func (t *T) hasNetNS() bool { return true } -func (t *T) purgeCNIVarWithNetNS(ns string) error { - pattern := fmt.Sprintf("/var/lib/cni/networks/%s/{*.*.*.*,*:*}", t.Network) - paths, err := filepath.Glob(pattern) - if err != nil { - return err - } - for _, p := range paths { - buff, err := os.ReadFile(p) - if err != nil { - return err - } - var ( - line string - wantRemove bool - ) - if len(buff) != 0 { - line = strings.Fields(string(buff))[0] - } - if line == "" { - if file.ModTime(p).Add(5 * time.Second).Before(time.Now()) { - t.Log().Infof("remove empty %s (and more than 5s old)", p) - wantRemove = true - } - } else if line == ns { - t.Log().Infof("remove leftover %s", p) - wantRemove = true - } - if wantRemove { - if err := os.Remove(p); err != nil { - return err - } - } - } - return nil -} - func (t *T) addObjectNetNS() error { if t.NetNS != "" { // the container is expected to already have a netns. don't even care to log info. @@ -252,9 +209,6 @@ func (t *T) addObjectNetNS() error { t.Log().Infof("netns %s already added", nsPID) return nil } - if err := t.purgeCNIVarWithNetNS(nsPID); err != nil { - return err - } cmd := command.New( command.WithName("ip"), command.WithVarArgs("netns", "add", nsPID), @@ -287,63 +241,6 @@ func (t *T) delObjectNetNS() error { return cmd.Run() } -func (t *T) purgeCNIVarDir() error { - pattern := fmt.Sprintf("/var/lib/cni/networks/%s/{*.*.*.*,*:*}", t.Network) - paths, err := filepath.Glob(pattern) - if err != nil { - return err - } - for _, p := range paths { - if err := t.purgeCNIVarFile(p); err != nil { - return err - } - } - return nil -} - -func (t *T) purgeCNIVarFile(p string) error { - buff, err := os.ReadFile(p) - if err != nil { - return err - } - - line := strings.Fields(string(buff))[0] - _, err = strconv.Atoi(line) - if _, err := strconv.Atoi(line); err != nil { - runNetNSFile := filepath.Join(netnsRunDir, line) - if _, err := os.Stat(runNetNSFile); err == nil || !errors.Is(err, os.ErrNotExist) { - // the process is still alive, don't remove - return nil - } - } else { - pidFile := fmt.Sprintf("/proc/%s", line) - if _, err := os.Stat(pidFile); err == nil || !errors.Is(err, os.ErrNotExist) { - // the process is still alive, don't remove - return nil - } - } - if err = os.Remove(p); err == nil { - t.Log().Infof("removed %s: %s no longer exist", p, line) - } else if err != nil { - return err - } - return nil -} - -func (t *T) purgeCNIVarFileWithIP(ip net.IP) error { - p := fmt.Sprintf("/var/lib/cni/networks/%s/%s", t.Network, ip) - err := os.Remove(p) - switch { - case err == nil: - t.Log().Infof("removed %s", p) - return nil - case errors.Is(err, os.ErrNotExist): - return nil - default: - return err - } -} - // StatusInfo implements resource.StatusInfoer func (t *T) StatusInfo(ctx context.Context) map[string]interface{} { data := make(map[string]interface{}) @@ -403,7 +300,10 @@ func (t *T) Stop(ctx context.Context) error { if err := t.delObjectNetNS(); err != nil { return err } - return nil + // The reservation is released last: an address still configured in a + // namespace this stop failed to clean is an address om must not hand to + // another resource. + return t.freeIP() } func (t *T) Status(ctx context.Context) status.T { @@ -560,7 +460,6 @@ func (t *T) stop(ctx context.Context) error { // TODO: introduce t.StopTimeout and use context.WithTimeout ? ctx = context.Background() } - ip, _, _ := t.ipNet(ctx) netConf, err := t.netConf() if err != nil { return err @@ -601,8 +500,13 @@ func (t *T) stop(ctx context.Context) error { command.WithBufferedStderr(), ) + reserved, err := t.allocatedIP() + if err != nil { + return err + } + // {"name": "noop-test", "cniVersion": "0.3.1", ...} - stdinData, err := t.netConfBytes() + stdinData, err := t.netConfBytesFor(reserved) if err != nil { return err } @@ -627,9 +531,6 @@ func (t *T) stop(ctx context.Context) error { if err != nil { return err } - if t.purgeCNIVarFileWithIP(ip); err != nil { - return err - } return nil } @@ -667,8 +568,22 @@ func (t *T) start(ctx context.Context) error { fmt.Sprintf("CNI_PATH=%s", filepath.Dir(plugin)), } + ip, err := t.allocateIP() + if err != nil { + return err + } + if ip != nil { + // A start that fails past this point rolls back, and the address goes + // with it. Without this an object whose start failed holds an address + // while it is down, and deleting it without a stop leaves a + // reservation nothing will ever release. + actionrollback.Register(ctx, func(ctx context.Context) error { + return t.freeIP() + }) + } + // {"name": "noop-test", "cniVersion": "0.3.1", ...} - stdinData, err := t.netConfBytes() + stdinData, err := t.netConfBytesFor(ip) if err != nil { return err } @@ -704,30 +619,14 @@ func (t *T) start(ctx context.Context) error { if resp.Code == 0 { return nil } - if strings.Contains(resp.Msg, "no IP addresses available") { - return ErrNoIPAddrAvail - } - if strings.Contains(resp.Msg, "duplicate allocation") { - return ErrDupIPAlloc - } return fmt.Errorf("cni error code %d msg %s: %w", resp.Code, resp.Msg, err) } - err = run() - switch { - case err == nil: - case errors.Is(err, ErrNoIPAddrAvail), errors.Is(err, ErrDupIPAlloc): - t.Log().Infof("clean allocations and retry: %s", err) - t.purgeCNIVarDir() - - // clean run leftovers (container veth name provided (eth12) already exists) - // use nil context, start context may be deadlined - t.stop(nil) // clean run leftovers (container veth name provided (eth12) already exists) - err = run() - default: + if err := run(); err != nil { t.Log().Errorf("%s", err) + return err } - return err + return nil } func getInterfaceAndAddr(ref *net.IPNet) (net.Interface, net.Addr, error) { diff --git a/drivers/resipnetns/ipam.go b/drivers/resipnetns/ipam.go new file mode 100644 index 000000000..47a3ff4d3 --- /dev/null +++ b/drivers/resipnetns/ipam.go @@ -0,0 +1,179 @@ +//go:build linux + +package resipnetns + +import ( + "fmt" + "net" + "strings" + + "github.com/opensvc/om3/v3/core/ipam" + "github.com/opensvc/om3/v3/core/network" + "github.com/opensvc/om3/v3/util/hostname" +) + +// resolveNetwork returns the om network the network keyword names. +// +// The keyword used to hold the address of the network in dotted notation, +// which set the destination of the route del_net_route removes. That +// destination is the connected route the kernel adds along with the address, +// so it is derived from the address and the mask now, and the keyword names +// the network the address is drawn from, as it does on ip.cni. +// +// A value that is still an address is therefore obsolete rather than wrong: +// it is reported and ignored. A value that is neither an address nor a +// network is a mistake worth stopping for, a renamed network or a typo. +func (t *T) resolveNetwork() (network.Networker, error) { + if t._networkResolved { + return t._network, nil + } + t._networkResolved = true + if t.Network == "" { + return nil, nil + } + nw, names, err := network.Lookup(t.Network) + if err != nil { + return nil, err + } + if nw != nil { + t._network = nw + return nw, nil + } + if isAddr(t.Network) { + t.Log().Warnf("the network keyword holds the address %s, which is obsolete and ignored: the route del_net_route removes is derived from the address and the netmask. The keyword names the network the address is drawn from now", t.Network) + return nil, nil + } + return nil, fmt.Errorf("unknown network %s, expected one of %s", t.Network, strings.Join(names, ", ")) +} + +// isAddr reports whether a value is an address or a subnet, which is what the +// network keyword used to hold. +func isAddr(s string) bool { + if net.ParseIP(s) != nil { + return true + } + _, _, err := net.ParseCIDR(s) + return err == nil +} + +// ipam returns the allocator of the network this resource draws from, or nil +// when it draws from none. +// +// The addresses the cluster holds on its other nodes are not consulted: a +// routed_bridge gives this node a range of its own, and the addresses of a +// bridge are node local and not routable, so an address in use elsewhere is +// never one this node could hand out by mistake. +func (t *T) ipam() (*ipam.T, error) { + nw, err := t.resolveNetwork() + if err != nil { + return nil, err + } + if nw == nil { + return nil, nil + } + return network.NewAllocator(nw, hostname.Hostname()) +} + +// ipamKey names the reservation of this resource. An instance holds as many ip +// resources as it needs, several of them in one network, so the address +// belongs to the resource rather than to the object. +func (t *T) ipamKey() string { + return ipam.Key(t.Path, t.RID()) +} + +// allocateIP reserves the address of this resource, and returns the one it +// already holds when it holds one. +func (t *T) allocateIP() (net.IP, error) { + i, err := t.ipam() + if err != nil { + return nil, err + } + if i == nil { + return nil, nil + } + ip, err := i.Allocate(t.ipamKey()) + if err != nil { + return nil, err + } + t.Log().Infof("allocated %s in network %s", ip, i.Name) + return ip, nil +} + +// allocatedIP returns the address reserved for this resource, or nil when it +// has none. It never reserves one: reading a status must not take an address. +func (t *T) allocatedIP() (net.IP, error) { + i, err := t.ipam() + if err != nil { + return nil, err + } + if i == nil { + return nil, nil + } + return i.Allocated(t.ipamKey()) +} + +// freeIP releases the address of this resource. +func (t *T) freeIP() error { + i, err := t.ipam() + if err != nil { + return err + } + if i == nil { + return nil + } + return i.Free(t.ipamKey()) +} + +// networkDev returns the device of the network this resource draws from. +func (t *T) networkDev() string { + nw, err := t.resolveNetwork() + if err != nil || nw == nil { + return "" + } + if i, ok := nw.(interface{ BackendDevName() string }); ok { + return i.BackendDevName() + } + return "" +} + +// Configure fills from the network what the configuration did not say. +// +// A resource drawing its address from a network needs the device, the netmask +// and the gateway of that network, and they are the network's to know: naming +// the network is enough, and repeating them in the object configuration is a +// second copy to keep in step. An explicit value always wins. +func (t *T) Configure() error { + nw, err := t.resolveNetwork() + if err != nil { + return err + } + if nw == nil { + return nil + } + if t.Dev == "" { + t.Dev = t.networkDev() + } + i, err := t.ipam() + if err != nil { + return err + } + if i == nil || i.Range == nil { + // A network om draws no address from, the lo network among them, has + // nothing this resource can be built out of. Saying so here beats the + // device lookup failing later on an empty name. + if t.Dev == "" && t.Name == "" { + return fmt.Errorf("network %s provides neither a device nor an address to draw from: name a dev and a name, or name a network om allocates in", nw.Name()) + } + return nil + } + if t.Netmask == "" { + ones, _ := i.Range.Mask.Size() + t.Netmask = fmt.Sprintf("%d", ones) + } + if t.Gateway == "" { + if gw := ipam.Gateway(i.Range); gw != nil { + t.Gateway = gw.String() + } + } + return nil +} diff --git a/drivers/resipnetns/main.go b/drivers/resipnetns/main.go index b5e62a5d4..bc00f1b96 100644 --- a/drivers/resipnetns/main.go +++ b/drivers/resipnetns/main.go @@ -16,6 +16,7 @@ import ( "github.com/opensvc/om3/v3/core/actionresdeps" "github.com/opensvc/om3/v3/core/actionrollback" "github.com/opensvc/om3/v3/core/naming" + "github.com/opensvc/om3/v3/core/network" "github.com/opensvc/om3/v3/core/provisioned" "github.com/opensvc/om3/v3/core/resource" "github.com/opensvc/om3/v3/core/status" @@ -64,10 +65,12 @@ type ( Expose []string `json:"expose"` // cache - _ipaddr net.IP - _ipaddrAge time.Duration - _ipmask net.IPMask - _ipnet *net.IPNet + _network network.Networker + _networkResolved bool + _ipaddr net.IP + _ipaddrAge time.Duration + _ipmask net.IPMask + _ipnet *net.IPNet } Addrs []net.Addr @@ -148,6 +151,19 @@ func (t *T) Start(ctx context.Context) error { } else if t._ipaddrAge > 0 { t.Log().Warnf("ip %s lookup issue, cache valid (%s old)", t.Name, duration.FmtShortDuration(t._ipaddrAge)) } + allocated, err := t.allocateIP() + if err != nil { + return err + } + if allocated != nil { + // A start that fails past this point rolls back, and the address goes + // with it. Without this an object whose start failed holds an address + // while it is down, and deleting it without a stop leaves a + // reservation nothing will ever release. + actionrollback.Register(ctx, func(ctx context.Context) error { + return t.freeIP() + }) + } if err := t.startMode(ctx); err != nil { return err } @@ -255,16 +271,20 @@ func (t *T) startRoutesDel(ctx context.Context, netns ns.NetNS, guestDev string) if !t.DelNetRoute { return nil } - if t.Network == "" { - return nil - } if v, err := t.hasLinkIn(guestDev, netns.Path()); err != nil { return err } else if !v { return nil } - ones, _ := t.ipmask().Size() - dest := fmt.Sprintf("%s/%d", t.Network, ones) + // The route the kernel adds along with the address is the address + // masked, so that is the one to remove. The network keyword used to hold + // its base, which could name no other route: the length has always come + // from the netmask. + ipnet := t.ipnet() + if ipnet == nil { + return nil + } + dest := (&net.IPNet{IP: ipnet.IP.Mask(ipnet.Mask), Mask: ipnet.Mask}).String() if err := t.routeDelDevIn(dest, guestDev, netns.Path()); err != nil { return err } @@ -286,6 +306,16 @@ func (t *T) Stop(ctx context.Context) error { } else if t._ipaddrAge > 0 { t.Log().Warnf("ip %s lookup issue, cache valid (%s old)", t.Name, duration.FmtShortDuration(t._ipaddrAge)) } + if err := t.stopMode(ctx); err != nil { + return err + } + // The reservation is released last: an address still configured in a + // namespace this stop failed to clean is an address om must not hand to + // another resource. + return t.freeIP() +} + +func (t *T) stopMode(ctx context.Context) error { if t.Tags.Has(tagDedicated) { return t.stopDedicated(ctx) } @@ -328,9 +358,16 @@ func (t *T) statusWithIPAddrCacheTrust(ctx context.Context) status.T { err error carrier bool ) + // An empty name is how a resource says its address is the network's to + // choose. Saying nothing at all is the mistake: no address, and no + // network to draw one from. The network is the resolved one, so a + // keyword still holding the address of a network, which is obsolete and + // ignored, reads here as the absence it is. if t.Name == "" { - t.StatusLog().Warn("name not set") - return status.NotApplicable + if nw, _ := t.resolveNetwork(); nw == nil { + t.StatusLog().Warn("no name, and no network to draw an address from") + return status.NotApplicable + } } if t.Dev == "" { t.StatusLog().Warn("dev not set") @@ -355,6 +392,12 @@ func (t *T) statusWithIPAddrCacheTrust(ctx context.Context) status.T { ip := t.ipaddr() if ip == nil { + if t.Name == "" { + // The allocation happens on start, so no address here means + // the resource has not been started rather than that a lookup + // failed. + return status.Down + } t.StatusLog().Error("ip %s lookup issue, cache miss", t.Name) return status.Undef } else if t._ipaddrAge > maxIPAddrAge { @@ -393,7 +436,15 @@ func (t *T) Label(_ context.Context) string { dev = "@" + t.NSDev } ones, _ := t.ipmask().Size() - return fmt.Sprintf("%s/%d%s in %s", t.ipaddr(), ones, dev, t.NetNS) + s := fmt.Sprintf("%s/%d%s in %s", t.ipaddr(), ones, dev, t.NetNS) + // Name the network the address was drawn from, as the cni label does. The + // address alone does not say where it came from, and a reader comparing + // the instances of an object wants to see it. A keyword still holding the + // address of a network resolves to none, so it does not reach the label. + if nw, _ := t.resolveNetwork(); nw != nil { + s = nw.Name() + " " + s + } + return s } func (t *T) Provision(ctx context.Context) error { @@ -461,6 +512,16 @@ func (t *T) ipaddr() net.IP { if t._ipaddr != nil { return t._ipaddr } + if t.Name == "" { + // The address is drawn from a network, so it is the one the + // allocator holds. Reading it never takes one: only a start does. + ip, err := t.allocatedIP() + if err != nil { + t.StatusLog().Warn("%s", err) + } + t._ipaddr = ip + return t._ipaddr + } ip, age, err := getaddr.Lookup(t.Name) if getaddr.IsErrManyAddr(err) { t.StatusLog().Warn("%s", err) @@ -494,6 +555,10 @@ func (t *T) getIPMask() net.IPMask { if m, err := parseDottedMask(t.Netmask); err == nil { return m } + // the mask of the range the address is drawn from + if i, err := t.ipam(); err == nil && i != nil && i.Range != nil { + return i.Range.Mask + } // fallback to the mask of the first found ip on the intf if m, err := t.defaultMask(); err == nil { return m diff --git a/drivers/resipnetns/manifest.go b/drivers/resipnetns/manifest.go index ba3c17f22..0a4a7b98c 100644 --- a/drivers/resipnetns/manifest.go +++ b/drivers/resipnetns/manifest.go @@ -115,7 +115,6 @@ var ( Attr: "Dev", Example: "br-prd", Option: "dev", - Required: true, Scopable: true, Text: keywords.NewText(fs, "text/kw/dev"), }, @@ -135,12 +134,11 @@ var ( Text: keywords.NewText(fs, "text/kw/gateway"), }, { - Attr: "Network", - Example: "10.0.0.0/16", - Option: "network", - Provisioning: true, - Scopable: true, - Text: keywords.NewText(fs, "text/kw/network"), + Attr: "Network", + Example: "default", + Option: "network", + Scopable: true, + Text: keywords.NewText(fs, "text/kw/network"), }, { Attr: "CheckCarrier", diff --git a/drivers/resipnetns/text/kw/network b/drivers/resipnetns/text/kw/network index 383b51538..8c8063d64 100644 --- a/drivers/resipnetns/text/kw/network +++ b/drivers/resipnetns/text/kw/network @@ -1,3 +1,14 @@ -The ip address network, in dotted notation. +The name of the om network the address is drawn from. -Used to set the network route if `del_net_route=true`. +The `default` network exists on every node of a fresh installation, so an +object needs nothing more than `network=default` to receive an address. + +Naming a network is enough: the `dev`, `netmask` and `gateway` of the resource +are the network's to know, and are taken from it when the configuration does +not set them. Leaving `name` empty has the address allocated from the network +too, and the same resource keeps the same address across restarts. + +This keyword used to hold the address of the network, in dotted notation, to +set the destination of the route removed by `del_net_route`. That destination +is derived from the address and the netmask now. A value in that form is +reported as obsolete and ignored. diff --git a/drivers/restaskpodman/main.go b/drivers/restaskpodman/main.go index 01907fcc8..90d91016f 100644 --- a/drivers/restaskpodman/main.go +++ b/drivers/restaskpodman/main.go @@ -17,8 +17,6 @@ type ( // T is the driver structure. T struct { restaskocibase.T - - CNIConfig string } ) @@ -84,7 +82,6 @@ func (t *T) GetContainerDetached() restaskocibase.ContainerTasker { StartTimeout: startTimeout, LogOutputs: t.LogOutputs, }, - CNIConfig: t.CNIConfig, } if err := ct.Configure(); err != nil { t.Log().Errorf("unable to configure podman task container") diff --git a/drivers/restaskpodman/manifest.go b/drivers/restaskpodman/manifest.go index de29c5cc3..c745afaf8 100644 --- a/drivers/restaskpodman/manifest.go +++ b/drivers/restaskpodman/manifest.go @@ -51,7 +51,6 @@ func (t *T) Manifest() *manifest.T { manifest.ContextObjectID, manifest.ContextObjectID, manifest.ContextDNS, - manifest.ContextCNIConfig, ) m.AddKeywords(restask.Keywords...) m.AddKeywords(restaskocibase.Keywords...)