Hi Krisztian,
On Wed, Sep 10, 2014 at 06:02:13PM +0200, KOVACS Krisztian wrote:
> > > +void conn_clear_network_namespace(struct connection* conn)
> > > +{
> > > + if((conn->flags & CO_FL_NAMESPACE_RECV) &&
> > (conn->network_namespace != NULL))
> > > + {
> > > + free(conn->network_namespace);
> > > + conn->network_namespace = NULL;
> > > + conn->flags &= ~CO_FL_NAMESPACE_RECV;
> > > + }
> >
> > It seems to me that this test is unnecessary and that the code could
> > be performed unconditionally. We'd better do it in this case, as history
> > has shown that many bugs arise from conditions that are improperly
> > converted when flags change later.
> >
>
> Well, there's a reason for that if here: there are two possible ways a
> namespace name could be set for a connection. The first is that there's a
> namespace configured for the bind stanza and the connection 'inherits' that
> namespace from the listener it arrived on. In this case, we store a pointer
> to the string in the bind structure. This obviously must not be freed,
> because it is part of the config. The other way is that a namespace TLV is
> received in the proxy protocol. In this case the connection structure
> *owns* the string it has a pointer to: basically this is what the
> CO_FL_NAMESPACE_RECV flag means.
OK, I didn't think about this case. Thanks for explaining.
> > > + char buf[512];
> > > + int fd;
> > > +
> > > + snprintf(buf, sizeof(buf), "/var/run/netns/%s", ns_name);
> >
> > Just out of curiosity, is it the location defined by LSB/FHS/whatever
> > standard
> > or is it on a specific distro ? We're seeing haproxy embedded into various
> > products and non-standard locations rarely follow the ones of "fat"
> > distros.
> >
>
> Well, there's generally very little documentation on this. The
> '/var/run/netns' directory is used by iproute, so the path above came from
> the need to be able to open namespaces created by 'ip netns add'. We'll
> check whether or not it can be changed during compilation.
OK thanks.
> > > + return sock;
> > > +}
> > > diff --git a/src/proto_tcp.c b/src/proto_tcp.c
> > > index 72dc92b..94cb091 100644
> > > --- a/src/proto_tcp.c
> > > +++ b/src/proto_tcp.c
> > > @@ -33,6 +33,7 @@
> > > #include <common/errors.h>
> > > #include <common/mini-clist.h>
> > > #include <common/standard.h>
> > > +#include <common/namespace.h>
> > >
> > > #include <types/global.h>
> > > #include <types/capture.h>
> > > @@ -284,6 +285,7 @@ int tcp_connect_server(struct connection *conn, int
> > data, int delack)
> > > struct server *srv;
> > > struct proxy *be;
> > > struct conn_src *src;
> > > + int namespace_set = conn->flags & CO_FL_NAMESPACE_SET;
> >
> > Isn't the namespace a property of the server instead of the connection,
> > given that you configure it on the server line ? That makes me think that
> > this flag (or at least this test) is not needed and that the server's NS
> > will be enough.
> >
>
> Well, in certain cases it's the property of the server. However, when
> recvproxy was enabled for a connection it is possible that a namespace name
> was received via the proxy protocol, and that can also be used to determine
> which namespace the server is in.
OK, got it, thanks.
> > > diff --git a/include/types/connection.h b/include/types/connection.h
> > > index 89f4f38..a75d45d 100644
> > > --- a/include/types/connection.h
> > > +++ b/include/types/connection.h
> > > @@ -334,6 +334,7 @@ struct proxy_hdr_v2 {
> > > #define PP2_TYPE_SSL 0x20
> > > #define PP2_TYPE_SSL_VERSION 0x21
> > > #define PP2_TYPE_SSL_CN 0x22
> > > +#define PP2_TYPE_NETWORK_NAMESPACE 0x30
> >
> > Could you please avoid to fragment the values here, I fear the day we need
> > a contiguous array and we can't find one anymore. Let's start at 0x23 for
> > now. Note, since we're in development, we can change these numbers later,
> > so let's not consider the PPv2 spec as "official" whenever it comes to what
> > we stuff in haproxy-dev.
> >
>
> Ok. So which ID should we use? I must admit that at some point I lost track
> of the discussion about SSL related IDs... ;)
Until Dave comes with a proposal for all IDs to put into the spec, we
don't really care, so don't bother with this immediately. I above all
wanted to insist on being careful about the fact that IDs could become
scarce if we start to fragment them.
> > Then providing a pointer to such a struct into each listener/server should
> > work reasonably well for all your uses. You can also consider that by
> > design
> > the default one is inserted first into the list.
> >
>
> Ok. Will consider this as part of the namespace/fd cache.
Great.
> > Overall, that's great, I'm seeing this as a really useful feature of 1.6
> > that will gain some traction from users. We always need to have good stuff
> > at the beginning to incite users to test development releases and detect
> > the new bugs we add, this is a great opportunity to please everyone :-)
> >
>
> Thanks a lot for your comments and feedback. We're a bit overwhelmed at the
> moment, but will come back with an updated patchset sooner-or-later.
No problem, take your time and do not hesitate to share your thoughts here
on the list if you have any doubt about anything or if I said stupid things.
Thanks!
Willy
