Re: [PATCH v2] conf: Unify several paths in conf_ports()

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 18 Mar 2025 18:15:27 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> On 18/03/2025 17:54, Stefano Brivio wrote:
> > On Mon, 17 Mar 2025 12:50:36 +0100
> > Paul Holzinger <pholzing@redhat.com> wrote:
> >  
> >> On 15/03/2025 00:50, Stefano Brivio wrote:  
> >>> On Wed, 12 Mar 2025 14:43:59 +1100
> >>> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>     
> >>>> In conf_ports() we have three different paths which actually do the setup
> >>>> of an individual forwarded port: one for the "all" case, one for the
> >>>> exclusions only case and one for the range of ports with possible
> >>>> exclusions case.
> >>>>
> >>>> We can unify those cases using a new helper which handles a single range
> >>>> of ports, with a bitmap of exclusions.  Although this is slightly longer
> >>>> (largely due to the new helpers function comment), it reduces duplicated
> >>>> logic.  It will also make future improvements to the tracking of port
> >>>> forwards easier.
> >>>>
> >>>> The new conf_ports_range_except() function has a pretty prodigious
> >>>> parameter list, but I still think it's an overall improvement in conceptual
> >>>> complexity.
> >>>>
> >>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>>> ---
> >>>>    conf.c | 173 ++++++++++++++++++++++++++++++---------------------------
> >>>>    1 file changed, 90 insertions(+), 83 deletions(-)
> >>>>
> >>>> v2:
> >>>>    * Commit message updated slightly, but otherwise unmodified.
> >>>>
> >>>>
> >>>> diff --git a/conf.c b/conf.c
> >>>> index 065e7201..4e0099ba 100644
> >>>> --- a/conf.c
> >>>> +++ b/conf.c
> >>>> @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **endptr,
> >>>>    	return 0;
> >>>>    }
> >>>>    
> >>>> +/**
> >>>> + * conf_ports_range_except() - Set up forwarding for a range of ports minus a
> >>>> + *                             bitmap of exclusions
> >>>> + * @c:		Execution context
> >>>> + * @optname:	Short option name, t, T, u, or U
> >>>> + * @optarg:	Option argument (port specification)
> >>>> + * @fwd:	Pointer to @fwd_ports to be updated
> >>>> + * @addr:	Listening address
> >>>> + * @ifname:	Listening interface
> >>>> + * @first:	First port to forward
> >>>> + * @last:	Last port to forward
> >>>> + * @exclude:	Bitmap of ports to exclude
> >>>> + * @to:		Port to translate @first to when forwarding
> >>>> + * @weak:	Ignore errors, as long as at least one port is mapped
> >>>> + */
> >>>> +static void conf_ports_range_except(const struct ctx *c, char optname,
> >>>> +				    const char *optarg, struct fwd_ports *fwd,
> >>>> +				    const union inany_addr *addr,
> >>>> +				    const char *ifname,
> >>>> +				    uint16_t first, uint16_t last,
> >>>> +				    const uint8_t *exclude, uint16_t to,
> >>>> +				    bool weak)
> >>>> +{
> >>>> +	bool bound_one = false;
> >>>> +	unsigned i;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (first == 0) {
> >>>> +		die("Can't forward port 0 for option '-%c %s'",
> >>>> +		    optname, optarg);
> >>>> +	}  
> >>> This introduces two subtle functional changes that are a bit unexpected
> >>> given the commit message. Before:
> >>>
> >>> $ ./pasta -t 0
> >>> $
> >>>
> >>> $ ./pasta -t 0-1025
> >>> Failed to bind port 1 (Permission denied) for option '-t 0-1025', exiting
> >>>
> >>> After:
> >>>
> >>> $ ./pasta -t 0
> >>> Can't forward port 0 for option '-t 0'
> >>>
> >>> $ ./pasta -t 0-1025
> >>> Can't forward port 0 for option '-t 0-1025'
> >>>
> >>> ...anyway, I doubt anybody would use -t 0 on purpose (to get a port
> >>> automatically assigned), and while it probably works for TCP (check
> >>> bound ports after starting pasta, use the assigned one), it wouldn't
> >>> necessarily work as expected for UDP if the application relies on our
> >>> flow tracking.  
> >> Why would this not work for UDP? bind() wise you can still bind 0 fine
> >> and get a free port assigned?  
> > The bind() part itself would work, but with the current implementation
> > we wouldn't be able to track flows corresponding to this specific port
> > forwarding, so I expect that the "return" (outbound) traffic won't
> > work.
> >
> > It's a matter of implementation (or lack thereof), we could get it to
> > work with a getsockname() after bind().
> >
> > Before this change, it happened to work *by mistake* for TCP, not for
> > UDP. With this change, it doesn't work for TCP. We can add it back with
> > a proper syntax (-t ...any?), as David mentioned.
> >  
> >>> For TCP, actually, -t 0 might be useful, see e.g. random_free_port() in
> >>> Podman tests (/test/system/helpers.network.bash). We should print the
> >>> port number that was bound, though, and document the feature.
> >>>
> >>> More than that: that could actually be the only race-free possibility
> >>> of picking and forwarding a port where the number doesn't matter.  
> >> Yes it could be useful for podman but then it should also work with udp.  
> > We can get it to work if needed. We would need, I guess:
> >
> > - that getsockname() for UDP, whatever is missing for the UDP case
> >
> > - a new configuration sub-option
> >
> > - documentation
> >  
> >> I am less worried about the tests, this issue is in podman proper as you
> >> can do "-p 80", then podman assigns a free host port. Except that this
> >> is super broken in podman because we do this once when we create the
> >> container so this is totally racy and non conflict free[1].  
> > Ouch, I wasn't aware of that. For pasta it should be relatively easy to
> > do that in a race-free way, because the kernel guarantees that.
> >  
> >> The thing of
> >> course is for podman we have to deal with like 4 other port forwarder
> >> implementations that we would need to support. As such I don't see us
> >> ever finding time to properly fix it unless it magically gets a ton of
> >> priority. So if pasta does not support for it I have no problems with
> >> that, however maybe one day we like to reconsider.
> >>
> >> [1]
> >> https://github.com/containers/podman/issues/10205#issuecomment-1010055023  
> > I would wait for David's feedback on this, but to me it looks like a
> > small-ish thing we can add without much thinking and planning.
> >
> > I'm not sure you can close that issue if we implement it in pasta as
> > long as forwarding is done like it's done now for custom networks, but
> > the issue would look less serious I guess.
> >
> > I don't know about the Podman side of it, but probably that would look
> > trivial to you (-t any:80 maybe? or -t :80 ?).  
> 
> I guess something like -t 0:80 could also work. That would allow me to 
> store unassigned ports as 0 and the the convert to pasta cli code would 
> not need any special handling for this case.
> 
> Overall it will be more complicated in Podman which is why I never 
> bothered to take this on. The issue is that commands like podman inspect 
> or podman port need to know the actual ports that were assigned.

Ah, right, I forgot about that.

> So it 
> would need some form of interface from pasta that prints out which host 
> port it uses for each namespace port. Then podman must need to gain 
> support to store two different set of ports, dynamic and static 
> (currently) so we can keep track of the ports pasta returned to us.

That doesn't really sound trivial, I was underestimating this.

> All stuff we can implement but until someone pushes hard for it I don't 
> think it will ever make it. I think there are better network things we 
> can spend our time on.

...so we should rather defer this I guess. Probably it will make sense
to revisit this together with the new forwarding configuration interface
once we have it implemented.

-- 
Stefano