Hi David,
On Mon, Apr 21, 2014 at 06:17:58PM -0400, David S wrote:
> As a foundation for extending the proxy-protocol to include additional
> information, I've implemented version 2 of the proxy protocol. As we
> discussed in the "Extending PROXY protocol for SSL" thread, I made one
> change to the protocol.
Great.
> Version and Command are combined into one byte.
> Length is now two bytes.
>
> +struct proxy_hdr_v2 {
> + uint8_t sig[12]; /* hex 0D 0A 0D 0A 00 0D 0A 51 55 49 54 0A */
> + uint8_t cmd; /* protocol version and command */
> + uint8_t fam; /* protocol family and transport */
> + uint16_t len; /* number of following bytes part of the header */
> +};
>
> The server keyword "send-proxy-v2" will cause Version 2 of the protocol to
> be sent.
OK, we need the same for health checks.
> /* returns true is the transport layer is ready */
> static inline int conn_xprt_ready(const struct connection *conn)
> diff --git a/include/types/connection.h b/include/types/connection.h
> index 5341a86..b3b85ab 100644
> --- a/include/types/connection.h
> +++ b/include/types/connection.h
> @@ -245,6 +245,7 @@ struct connection {
> enum obj_type obj_type; /* differentiates connection from applet
> context */
> unsigned char err_code; /* CO_ER_* */
> signed short send_proxy_ofs; /* <0 = offset to (re)send from the end,
> >0 = send all */
> + unsigned int send_proxy_opts; /* PROXY protocol option flags */
Adding fields to struct connection is really not welcome, these ones should
remain as small as possible. I don't think there's anything in these options
that cannot be deduced from the target. So we'd rather check the connection's
target from the make_proxy_line() function instead.
(...)
> diff --git a/include/types/server.h b/include/types/server.h
> index 54ab813..4782607 100644
> --- a/include/types/server.h
> +++ b/include/types/server.h
> @@ -57,6 +57,9 @@
> #define SRV_SEND_PROXY 0x0800 /* this server talks the PROXY protocol
> */
> #define SRV_NON_STICK 0x1000 /* never add connections allocated to
> this server to a stick table */
>
> +/* configured server options for send-proxy */
> +#define SRV_PP_V2 0x0001 /* proxy protocol version 2 */
> +
> /* function which act on servers need to return various errors */
> #define SRV_STATUS_OK 0 /* everything is OK. */
> #define SRV_STATUS_INTERNAL 1 /* other unrecoverable errors. */
> @@ -183,6 +186,7 @@ struct server {
> int line; /* line where the section
> appears */
> struct eb32_node id; /* place in the tree of used
> IDs */
> } conf; /* config information */
> + int pp_opts; /* proxy protocol options */
> };
Here I think that it's not much a matter of space but confusion, either
we find enough place in srv->state to add the options, or we move all
of them together including SRV_SEND_PROXY to the same set of proxy-protocol
options. BTW, if you keep pp_opts, you should move it above, as the bottom
of this structure is large an used only by the config parser, so we want
as much as possible that all hot data stay close together.
> /* Descriptor for a "server" keyword. The ->parse() function returns 0 in
> case of
> diff --git a/src/backend.c b/src/backend.c
> index d878028..ad8812a 100644
> --- a/src/backend.c
> +++ b/src/backend.c
> @@ -1061,6 +1061,8 @@ int connect_server(struct session *s)
> srv_conn->send_proxy_ofs = 0;
> if (objt_server(s->target) && (objt_server(s->target)->state &
> SRV_SEND_PROXY)) {
> srv_conn->send_proxy_ofs = 1; /* must compute size */
> + if ((objt_server(s->target)->pp_opts & SRV_PP_V2))
> + srv_conn->send_proxy_opts |= CO_PP_V2;
If this is made in make_proxy_line(), this can easily go away.
> +int make_proxy_line_v2(char *buf, int buf_len, unsigned int options, struct
> connection *remote)
> +{
> + const char pp2_signature[12] = {0x0D, 0x0A, 0x0D, 0x0A, 0x00, 0x0D,
> 0x0A, 0x51, 0x55, 0x49, 0x54, 0x0A};
> + int ret = 0;
> + struct proxy_hdr_v2 *hdr_p = (struct proxy_hdr_v2 *)buf;
> + union proxy_addr *addr_p = (union proxy_addr *)(buf + PP2_HEADER_LEN);
> + struct sockaddr_storage *src = NULL;
> + struct sockaddr_storage *dst = NULL;
> +
> + if (buf_len < PP2_HEADER_LEN)
> + return 0;
> + memcpy(hdr_p->sig, pp2_signature, PP2_SIGNATURE_LEN);
> +
> + if (remote) {
> + src = &remote->addr.from;
> + dst = &remote->addr.to;
> + }
Be careful above, you have a mix of random-sized indents (as in a few other
structs in other areas). Please double-check your editor's configuration to
use tabs for indents and spaces for alignments.
> + if (src && dst && src->ss_family == dst->ss_family && src->ss_family ==
> AF_INET) {
> + if (buf_len < PP2_HDR_LEN_INET)
> + return 0;
> + hdr_p->cmd = PP2_VERSION | PP2_CMD_PROXY;
> + hdr_p->fam = PP2_FAM_INET | PP2_TRANS_STREAM;
> + hdr_p->len = htons(PP2_ADDR_LEN_INET);
> + addr_p->ipv4_addr.src_addr = ((struct sockaddr_in
> *)src)->sin_addr.s_addr;
> + addr_p->ipv4_addr.dst_addr = ((struct sockaddr_in
> *)dst)->sin_addr.s_addr;
> + addr_p->ipv4_addr.src_port = ((struct sockaddr_in
> *)src)->sin_port;
> + addr_p->ipv4_addr.dst_port = ((struct sockaddr_in
> *)dst)->sin_port;
> + ret = PP2_HDR_LEN_INET;
> + }
> + else if (src && dst && src->ss_family == dst->ss_family &&
> src->ss_family == AF_INET6) {
> + if (buf_len < PP2_HDR_LEN_INET6)
> + return 0;
> + hdr_p->cmd = PP2_VERSION | PP2_CMD_PROXY;
> + hdr_p->fam = PP2_FAM_INET6 | PP2_TRANS_STREAM;
> + hdr_p->len = htons(PP2_ADDR_LEN_INET6);
> + memcpy(addr_p->ipv6_addr.src_addr, &((struct sockaddr_in6
> *)src)->sin6_addr, 16);
> + memcpy(addr_p->ipv6_addr.dst_addr, &((struct sockaddr_in6
> *)dst)->sin6_addr, 16);
> + addr_p->ipv6_addr.src_port = ((struct sockaddr_in6
> *)src)->sin6_port;
> + addr_p->ipv6_addr.dst_port = ((struct sockaddr_in6
> *)dst)->sin6_port;
> + ret = PP2_HDR_LEN_INET6;
> + }
> + else {
> + if (buf_len < PP2_HDR_LEN_UNSPEC)
> + return 0;
> + hdr_p->cmd = PP2_VERSION | PP2_CMD_LOCAL;
> + hdr_p->fam = PP2_FAM_UNSPEC | PP2_TRANS_UNSPEC;
> + hdr_p->len = htons(PP2_ADDR_LEN_UNSPEC);
> + ret = PP2_HDR_LEN_UNSPEC;
> + }
I like how the code becomes just a bunch of memcpy() and assignments with v2 :-)
OK so overall it goes into the right direction, but I think the points
above need to be addressed. We could merge the result before v1.5 which
will significantly stimulate adoption of version 2.
Regards,
Willy
