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

On Tue, Jul 07, 2026 at 12:48:30AM +0200, Stefano Brivio wrote:

On Mon, 6 Jul 2026 13:08:29 +1000 David Gibson 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 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