libnetwork/portallocator: assorted cleanups by thaJeztah · Pull Request #48373 · moby/moby

thaJeztah · 2024-08-26T11:02:00Z

portallocator: RequestPortInRange: fix doc-link in godoc

The doc-link was not formatted correctly and didn't work. While updating
also slightly touch-up the description to explain "defaultIP".

portallocator: ReleaseAll: remove unused error-return

portallocator: use new instance in tests

Test the functionality in isolation instead of using the singleton that's
returned by the GET function; this makes sure tests don't affect each other,
and doesn't require resetting the singleton in between tests, potentially
allowing these tests to eb run in parallel.

portallocator: use net.IPv4zero for defaultIP, and make it a property

Use the variable that's provided by the net package, and make the defaultIP
a property of the allocator instead of a package variable.

portallocator: RequestPort: skip RequestPortInRange as intermediate

RequestPortInRange is a wrapper for RequestPortsInRange; skip it as
intermediate function.

portallocator: make newPortMap a regular constructor

It was a method on PortAllocator, but not really related, other than
reading the default values.

portallocator: RequestPortsInRange: validate range once

RequestPortsInRange calls portMap.getPortRange() in a loop, but the given
port-range is always the same. Perform validation once instead of for each
iteration.

portallocator: un-export PortAllocator.Begin, PortAllocator.End

These values are configured when instantiating the allocator, and not
intended to be mutated externally. They're only used internally with
the exception of a test in the bridge driver that uses it to pick a
port that can be used for testing.

This patch:

un-exports the Begin and End fields
introduces a GetPortRange() utility to allow the bridge driver
to get the port, but marking it as a function for internal use.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

robmry

LGTM

akerouanton · 2024-08-26T14:28:23Z

-		Begin: start,
-		End:   end,
+		ipMap:     ipMapping{},
+		defaultIP: net.IPv4zero,


I don't see why it'd be better to turn defaultIP into a PortAllocator property. However, I agree that tests shouldn't rely on this var, and I think we can just ditch that defaultIP altogether ⬇️

I'm not really tied to this; I initially kept it as a package variable, but decided to move it internal to the PortAllocator, mostly because that's already a singleton and to make it more clear it's internal to that instance, but also (slightly) considering if this would be something configurable (i.e. when instancing the allocator).

akerouanton · 2024-08-27T09:03:23Z

@@ -114,7 +115,7 @@ func (p *PortAllocator) RequestPort(ip net.IP, proto string, port int) (int, err
 // default IP (0.0.0.0).
 func (p *PortAllocator) RequestPortInRange(ip net.IP, proto string, portStart, portEnd int) (int, error) {


Excluding tests, RequestPortInRange is only called by RequestPort. RequestPort is only called from allocateDaemonPort in a loop that already prevents ip to be nil.

I think we can just drop that defaulting behavior from this method, but it's probably worth taking a defensive stance by returning an error if ip is nil.

I think I had a patch stashed that removed that loop, which I left out of this PR for now, but, yes, also agreed that we could look at reducing the signature, and remove what we don't need.

akerouanton · 2024-08-27T09:29:41Z


 	if ip == nil {
-		ip = defaultIP
+		ip = p.defaultIP // FIXME(thaJeztah): consider making this a required argument and producing an error instead, or set default when constructing.


For this one, there's a bit more code involved but I think we're all good too (ie. we can remove this defaulting).

ReleasePorts is called from two places:

attemptBindHostPorts, in a defer while iterating on the addrs slice, which is built from portBindingReq.childHostIP. This field is set in setChildHostIP. This method either re-use PBs HostIP as-is (they already get a default value applied), or uses what the portDriverClient.ChildHostIP() returns (which is a netip.Addr).

moby/libnetwork/drivers/bridge/port_mapping_linux.go

Lines 373 to 386 in 609c698

if len(bnd.HostIP) == 0 {

if defHostIP.To4() == nil {

// The default binding address is IPv6.

return portBindingReq{}, false

}

bnd.HostIP = defHostIP

}

// Unmap the addresses if they're IPv4-mapped IPv6.

bnd.HostIP = bnd.HostIP.To4()

bnd.IP = containerIPv4.To4()

return setChildHostIP(pdc, portBindingReq{

PortBinding: bnd,

disableNAT: disableNAT,

}), true

moby/libnetwork/drivers/bridge/port_mapping_linux.go

Lines 400 to 421 in 609c698

// If there's no host address, use the default.

if len(bnd.HostIP) == 0 {

if defHostIP.Equal(net.IPv4zero) {

if !netutils.IsV6Listenable() {

// No implicit binding if the host has no IPv6 support.

return portBindingReq{}, false

}

// Implicit binding to "::", no explicit HostIP and the default is 0.0.0.0

bnd.HostIP = net.IPv6zero

} else if defHostIP.To4() == nil {

// The default binding IP is an IPv6 address, use it.

bnd.HostIP = defHostIP

} else {

// The default binding IP is an IPv4 address, nothing to do here.

return portBindingReq{}, false

}

}

bnd.IP = containerIP

return setChildHostIP(pdc, portBindingReq{

PortBinding: bnd,

disableNAT: disableNAT,

}), true

moby/libnetwork/drivers/bridge/port_mapping_linux.go

Lines 424 to 432 in 609c698

func setChildHostIP(pdc portDriverClient, req portBindingReq) portBindingReq {

if pdc == nil {

req.childHostIP = req.HostIP

return req

}

hip, _ := netip.AddrFromSlice(req.HostIP)

req.childHostIP = pdc.ChildHostIP(hip).AsSlice()

return req

}

releasePortBindings, which is called:
a. in a defer in addPortMappings with the result of bindHostPorts() -> attemptBindHostPorts().
b. from releasePorts with the list of port mappings that were effectively created.

Maybe it's worth changing the signature of ReleasePorts to make sure no nil net.IP get passed.

I think in the future we should also consider replacing net.IP with netip.Addr.

Yup; I'd be +1 for sure to make it a hard failure. I was on the fence changing it already, but went looking where the value came from, and there were quite some places where it could be set, so I decided to (for now) leave a TODO here and to leave it for a follow-up.

(Also because I didn't want to shove such a change in a general "cleanup").

akerouanton · 2024-08-27T09:42:19Z

With the recent changes made by @robmry to our portmapper, I really wonder how useful this whole portallocator is. We're now using bind syscalls to reserve 'OS-level' ports (and thus confirm that they're free). Before that change, there was a risk of port clashes being detected after inserting iptables rules, but not anymore.

Maybe we should just ditch this package altogether? WDYT @robmry?

robmry · 2024-08-27T09:56:32Z

Maybe we should just ditch this package altogether? WDYT @robmry?

I think it's still useful to track where to start the search for the next free port (to avoid reallocating the same port for short-lived mappings) - but it could be an internal bridge driver package, and possibly simplified.

The doc-link was not formatted correctly and didn't work. While updating also slightly touch-up the description to explain "defaultIP". Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Test the functionality in isolation instead of using the singleton that's returned by the `GET` function; this makes sure tests don't affect each other, and doesn't require resetting the singleton in between tests, potentially allowing these tests to eb run in parallel. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Use the variable that's provided by the net package, and make the defaultIP a property of the allocator instead of a package variable. Signed-off-by: Sebastiaan van Stijn <[email protected]>

RequestPortInRange is a wrapper for RequestPortsInRange; skip it as intermediate function. Signed-off-by: Sebastiaan van Stijn <[email protected]>

It was a method on PortAllocator, but not really related, other than reading the default values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

RequestPortsInRange calls portMap.getPortRange() in a loop, but the given port-range is always the same. Perform validation once instead of for each iteration. Signed-off-by: Sebastiaan van Stijn <[email protected]>

These values are configured when instantiating the allocator, and not intended to be mutated externally. They're only used internally with the exception of a test in the bridge driver that uses it to pick a port that can be used for testing. This patch: - un-exports the Begin and End fields - introduces a GetPortRange() utility to allow the bridge driver to get the port, but marking it as a function for internal use. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added status/2-code-review area/networking Networking kind/refactor PR's that refactor, or clean-up code area/networking/portmapping Networking labels Aug 26, 2024

thaJeztah added this to the 28.0.0 milestone Aug 26, 2024

thaJeztah self-assigned this Aug 26, 2024

thaJeztah force-pushed the cleanup_portallocator branch 2 times, most recently from 20c88d4 to 12f54ea Compare August 26, 2024 15:06

thaJeztah marked this pull request as ready for review August 26, 2024 19:32

thaJeztah requested review from akerouanton and robmry August 26, 2024 19:32

robmry approved these changes Aug 27, 2024

View reviewed changes

akerouanton requested changes Aug 27, 2024

View reviewed changes

thaJeztah added 8 commits August 27, 2024 17:07

portallocator: RequestPortInRange: fix doc-link in godoc

2a2176f

The doc-link was not formatted correctly and didn't work. While updating also slightly touch-up the description to explain "defaultIP". Signed-off-by: Sebastiaan van Stijn <[email protected]>

portallocator: ReleaseAll: remove unused error-return

1897a21

Signed-off-by: Sebastiaan van Stijn <[email protected]>

portallocator: use net.IPv4zero for defaultIP, and make it a property

78d88d0

Use the variable that's provided by the net package, and make the defaultIP a property of the allocator instead of a package variable. Signed-off-by: Sebastiaan van Stijn <[email protected]>

portallocator: RequestPort: skip RequestPortInRange as intermediate

c00f628

RequestPortInRange is a wrapper for RequestPortsInRange; skip it as intermediate function. Signed-off-by: Sebastiaan van Stijn <[email protected]>

portallocator: make newPortMap a regular constructor

05d784d

It was a method on PortAllocator, but not really related, other than reading the default values. Signed-off-by: Sebastiaan van Stijn <[email protected]>

portallocator: RequestPortsInRange: validate range once

fb1ae4b

RequestPortsInRange calls portMap.getPortRange() in a loop, but the given port-range is always the same. Perform validation once instead of for each iteration. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the cleanup_portallocator branch from 12f54ea to 8e580ef Compare August 27, 2024 15:15

akerouanton approved these changes Aug 28, 2024

View reviewed changes

thaJeztah merged commit 623e717 into moby:master Aug 29, 2024

thaJeztah deleted the cleanup_portallocator branch August 29, 2024 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnetwork/portallocator: assorted cleanups#48373

libnetwork/portallocator: assorted cleanups#48373
thaJeztah merged 8 commits intomoby:masterfrom
thaJeztah:cleanup_portallocator

thaJeztah commented Aug 26, 2024 •

edited

Loading

Uh oh!

robmry left a comment

Uh oh!

akerouanton Aug 26, 2024

Uh oh!

thaJeztah Aug 27, 2024

Uh oh!

akerouanton Aug 27, 2024

Uh oh!

thaJeztah Aug 27, 2024

Uh oh!

akerouanton Aug 27, 2024

Uh oh!

thaJeztah Aug 27, 2024

Uh oh!

Uh oh!

akerouanton commented Aug 27, 2024

Uh oh!

robmry commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -114,7 +115,7 @@ func (p *PortAllocator) RequestPort(ip net.IP, proto string, port int) (int, err
		// default IP (0.0.0.0).
		func (p *PortAllocator) RequestPortInRange(ip net.IP, proto string, portStart, portEnd int) (int, error) {

	if len(bnd.HostIP) == 0 {
	if defHostIP.To4() == nil {
	// The default binding address is IPv6.
	return portBindingReq{}, false
	}
	bnd.HostIP = defHostIP
	}
	// Unmap the addresses if they're IPv4-mapped IPv6.
	bnd.HostIP = bnd.HostIP.To4()
	bnd.IP = containerIPv4.To4()
	return setChildHostIP(pdc, portBindingReq{
	PortBinding: bnd,
	disableNAT: disableNAT,
	}), true

	// If there's no host address, use the default.
	if len(bnd.HostIP) == 0 {
	if defHostIP.Equal(net.IPv4zero) {
	if !netutils.IsV6Listenable() {
	// No implicit binding if the host has no IPv6 support.
	return portBindingReq{}, false
	}
	// Implicit binding to "::", no explicit HostIP and the default is 0.0.0.0
	bnd.HostIP = net.IPv6zero
	} else if defHostIP.To4() == nil {
	// The default binding IP is an IPv6 address, use it.
	bnd.HostIP = defHostIP
	} else {
	// The default binding IP is an IPv4 address, nothing to do here.
	return portBindingReq{}, false
	}
	}
	bnd.IP = containerIP
	return setChildHostIP(pdc, portBindingReq{
	PortBinding: bnd,
	disableNAT: disableNAT,
	}), true

	func setChildHostIP(pdc portDriverClient, req portBindingReq) portBindingReq {
	if pdc == nil {
	req.childHostIP = req.HostIP
	return req
	}
	hip, _ := netip.AddrFromSlice(req.HostIP)
	req.childHostIP = pdc.ChildHostIP(hip).AsSlice()
	return req
	}

Conversation

thaJeztah commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

portallocator: RequestPortInRange: fix doc-link in godoc

portallocator: ReleaseAll: remove unused error-return

portallocator: use new instance in tests

portallocator: use net.IPv4zero for defaultIP, and make it a property

portallocator: RequestPort: skip RequestPortInRange as intermediate

portallocator: make newPortMap a regular constructor

portallocator: RequestPortsInRange: validate range once

portallocator: un-export PortAllocator.Begin, PortAllocator.End

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akerouanton commented Aug 27, 2024

Uh oh!

robmry commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Aug 26, 2024 •

edited

Loading