On Fri, Mar 08, 2024 at 05:38:32PM +0000, Tristan wrote:
> Hi Willy,
>
> On 08/03/2024 17:05, Willy Tarreau wrote:
> > We could just have "abns2" and declare that it's the second version of the
> > abns format and know that this one is interoperable with a number of other
> > components.
>
> Having abns@ and abns2@ strictly for that one difference seems like a lot to
> me, to be honest, but I get the argument that they are otherwise
> fundamentally incompatible... Hmm...
Believe me, it's not much (in theory, we can always detect a long tail
and later decide to change our minds). I wouldn't recommend you a painful
option.
> > It would even be easier to deal with for those who document
> > such components because their integration docs would just indicate that
> > the address has to be placed after "abns2@" in haproxy, instead of saying
> > "and don't forget to adjust this global setting in your config, unless you
> > already use abns anywhere else".
>
> Considering there's barely any documentation on the internet as a whole
> about abstract unix sockets, I doubt we should worry too much about that
> bit.
> Also the target audience for those is unlikely to be too deterred.
Maybe. One of the reasons might also be the failure from some to make
it interoperate with their tools, and they give up.
> > Indeed, right now it seems that your patch
> > does nothing for the receivers, bind() still uses sizeof(), and the
> > sock_unix_addrcmp() function continues to compare the full name, making
> > such sockets non-transferable across reloads. So it's unclear to me how
> > this can work in your test :-/
>
> Maybe I'm missing some place/case, but at least for trivial binds it
> certainly works; which explains why the test passes :-)
Yes but based on the code I don't see how this is possible. bind() uses
sizeof() there, not get_addr_len().
> // without tightsocklen
> $ netstat -l | grep @haproxy
> unix 2 [ ACC ] STREAM LISTENING 934838
> @haproxy-f1@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>
> // with tightsocklen
> $ netstat -l | grep @haproxy
> unix 2 [ ACC ] STREAM LISTENING 956878 @haproxy-f1
OK thanks for the capture. I just fail to see what code this passes
through and that worries me a bit more.
> > Thus I think it would be preferable to go that route rather than having
> > to deal with all exceptions everywhere based on a global-only option.
> > We can help on this, of course.
>
> Well I do want it so I'm fine with doing it. But I wonder if it's not a bit
> of a:
> - abns@ eventually not used by anyone as everything keeps on moving away
> from padding
> - abns2@ eventually the default, which is quite jarring I suppose
We don't care. It's like the proxy protocol, one is legacy and still quite
deployed, but all improvements come on the new version. If there's no cost
in keeping legacy stuff for those who already use it, let's keep it running
and don't worry about it. The only configs which will need to be adapted
are those which cannot interoperate with external tools anyway, so it's
not as if it would require more changes.
> If it's easier that way, I'd prefer making it a proper bind/server option,
> which allows for easier transition, and can just change default value down
> the line if necessary?
The problem with default values is that a single behavior cannot be deduced
from reading a single config statement. That's quite painful for lots of
people (including those who copy config blocks from stackoverflow), and
for API tools. And it works half of the time because internally both modes
work but they can't interoperate with other tools. Here we're really
indicating a new address format. There's nothing wrong with that and we
do have the tools to handle that.
I think that the plan should be:
- add AF_CUST_ABNS2 to protocol-t.h before AF_CUST_MAX
- add to str2sa_range() the case for "abns2" just after "abns" with
address family "AF_CUST_ABNS2", and duplicate the test for
ss.ss_family == AF_UNIX later for AF_CUST_ABNS2 removing all the
stuff related to !abstract, or instead, just extend the "if" condition
to the new family and add that case there as well (might be even easier).
- duplicate "proto_fam_unix" to "proto_fam_abns2" in sock_unix.c, referencing
"sock_abns2_addrcmp" for the address comparison
- duplicate sock_unix_addrcmp() to sock_abns2_addrcmp() and implement only
the abns case, basically:
if (a->ss_family != b->ss_family)
return -1;
if (a->ss_family != AF_CUST_ABNS2)
return -1;
if (au->sun_path[0])
return -1;
return memcmp(au->sun_path, bu->sun_path, sizeof(au->sun_path));
- in sock_unix_bind_receiver(), add a case for this address family in
the bind() size argument, replacing sizeof(addr) with the string
length when running AF_CUST_ABNS2.
- for get_addr_len() I need to think :-/ I actually don't know how
that one works with socketpairs already, so there might be a trick
I'm overlooking.
I need to leave now, but I continue to think that if there is any internal
shortcoming, we should not try to work around it at the expense of trickier
long-term maintenance, instead we should address it. And don't worry, I'm
not going to assign you the task of dealing with limited stuff. We may end
up with the workaround in the worst case if we don't find better, but that
would mean declaring a failure and having to break that stuff sometime in
the future, which is always a pain.
Willy
