Re: [PATCH v2 1/2] fwd_rule: Parse target addresses for forwarding rules

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5640 bytes --]

On Tue, Jul 07, 2026 at 12:48:30AM +0200, Stefano Brivio wrote:
> On Mon, 6 Jul 2026 13:08:29 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sat, Jul 04, 2026 at 05:27:06PM +0200, Stefano Brivio wrote:
> > > On Thu,  2 Jul 2026 17:32:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > > I wonder: if !addr, shouldn't we replace it, for the moment, with
> > > inany_any4 or inany_any6 depending on !!inany_v4(tgt_addr)?
> > > 
> > > I see that an empty address already works with -4 or -6, and I
> > > understand that, by doing this, we'll have a change in behaviour once
> > > forwarding between IP versions is implemented.
> > > 
> > > But it will take a while before we get there, and, meanwhile, I guess
> > > almost all users will just want to do stuff like -t 8080:192.0.2.1/80,
> > > just to hit:
> > > 
> > >   Forwarding between IP versions (* => 192.0.2.1) not implemented
> > > 
> > > which isn't obvious if you aren't familiar with the implementation. I
> > > can also picture a constant flow of incoming tickets as a result.  
> > 
> > That's a reasonable concern.  I did think about this and came to the
> > tentative conclusion that automatically restricting to one family
> > would be worse, but I'm open to persuasion otherwise.  Reasoning as
> > follows.
> > 
> > First, it is a little awkward to implement: I think we should still
> > prevent an explicit *:192.0.2.1, which means we'd have to distinguish
> > the explicit * and implicit any cases.
> 
> Fair, and I think we should, but that looks relatively simple judging
> from fwd_rule_range_except() and callers.

Ok.

> > Mind you, we probably want
> > something similar when we allow listening addresses for -[TU], so it's
> > not such a big deal.
> > 
> > More importly this would become the _only_ place where the target
> > designation affects where and how we listen - in all other ways,
> > that's fully determined by the first part of the spec plus global
> > flags like -[46].  This gets particularly striking when looking at
> > something like:
> > 	-t 5000,5001:192.0.2.1
> > 
> > With an automatic restriction, we'd end up listening on *:5000, but
> > only on 0.0.0.0:5001, which seems pretty confusing.
> 
> So... I wasn't really aware or the fact that:
> 
>   - t x,y
> 
> would always be equivalent to:
> 
>   -t x -t y
> 
> as it is now. I originally intended , as a separator for ports and
> ports only, but originally we just had ports as port forwarding
> specifications. Then forwarding specifiers grew and I missed that, but
> it wasn't my intention.

Right.  Nonetheless, it's been that way for a good long while.

> I think that, eventually, we should make -t x_0,...,x_n a mere port
> specifier like it already is for ranges, because it's rather intuitive,
> as opposed as, say:
> 
>   -t 5000 -t 5001:192.0.2.1
> 
> where it's clear that we want to map port 5001 to 192.0.2.1 as
> destination address. If we say -t 5000,5001:192.0.2.1, it's rather
> obvious that we do *not*, and yet that's the outcome.

Agreed in principle.  However, to do it differently we'd need to work
out how it interacts with target port specifiers.  I don't think the
meaning of -t 5000,6000:7000 is particularly clear one way or the
other.

> On the other hand, "fixing" this is clearly behind the scope of this
> series, so, unless you have in mind a quick way to implement it, let's
> forget about this, and go ahead with this approach which is consistent
> with the existing situation, I'd say.

I don't see a quick way to implement it, but more than that it's not
really clear to me what our options are for syntax without breaking
backwards compatibility too badly.

> > > The change in behaviour once we implement forwarding between IP
> > > versions, by the way, looks a bit like an extension rather than a real
> > > change that could reasonably cause trouble to anybody.  
> > 
> > I'd argue that's even more the case with the errors as is: things that
> > weren't possible before become possible.
> 
> Indeed. I was just suggesting that even with the other option we would
> add an extension at some point, rather than a bug.
> 
> > > > +			char bstr[INANY_ADDRSTRLEN];
> > > > +
> > > > +			die(
> > > > +"Forwarding between IP versions (%s => %s) not implemented",
> > > > +			    inany_ntop(addr, bstr, sizeof(bstr)),
> > > > +			    inany_ntop(tgt_addr, astr, sizeof(astr)));
> > > > +		}  
> > > 
> > > About both validations: shouldn't they live in fwd_rule_add(), where
> > > we already have this kind of stuff? Or at least be duplicated there?  
> > 
> > Yes, yes they should.  Which also means it needs to move to the second
> > patch (or fwd_add_rule() doesn't see the target address - there's
> > nowhere for it in the structure).
> > 
> > For the next spin, I've moved this validation to fwd_rule_add() in
> > patch 2.  I've also tweaked the messages a bit, which might at least
> > slightly mitigate the concerns above.
> 
> Maybe, given that these error messages are temporary, we could indulge
> into some extra message like:
> 
>   "Did you want to say 0.0.0.0/... instead?"
> 
> same for ::1, along with the rest of the forwarding specifier. Users
> would see that when their containers fail to start and they could fix
> their syntax right away.

Good idea, done.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]