public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
@ 2024-11-14  8:47 Enrique Llorente
  2024-11-14 17:13 ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: Enrique Llorente @ 2024-11-14  8:47 UTC (permalink / raw)
  To: passt-dev; +Cc: Enrique Llorente

Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
(client fqdn), for some virt deployments like kubevirt is expected to
have the VirtualMachine name as the guest hostname.

This change add the -H --hostname to configure the DHCPv4 and DHCPv6
options to will send hostname to clients.

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
---
 conf.c           | 13 ++++++++++---
 dhcp.c           |  8 +++++++-
 dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
 passt.h          |  2 ++
 test/lib/setup   | 10 +++++-----
 test/passt.mbuto |  1 +
 test/passt/dhcp  | 11 ++++++++++-
 7 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/conf.c b/conf.c
index 14411b4..ddb585c 100644
--- a/conf.c
+++ b/conf.c
@@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
 		"  --freebind		Bind to any address for forwarding\n"
 		"  --no-map-gw		Don't map gateway address to host\n"
 		"  -4, --ipv4-only	Enable IPv4 operation only\n"
-		"  -6, --ipv6-only	Enable IPv6 operation only\n");
+		"  -6, --ipv6-only	Enable IPv6 operation only\n"
+		"  -H, --hostname NAME Hostname to configure client with\n");
 
 	if (strstr(name, "pasta"))
 		goto pasta_opts;
@@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"map-guest-addr", required_argument,	NULL,		22 },
 		{"host-lo-to-ns-lo", no_argument, 	NULL,		23 },
 		{"dns-host",	required_argument,	NULL,		24 },
+		{"hostname",	required_argument,	NULL,		'H' },
 		{ 0 },
 	};
 	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
@@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	if (c->mode == MODE_PASTA) {
 		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
 		fwd_default = FWD_AUTO;
-		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
+		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";
 	} else {
-		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
+		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
 	}
 
 	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
@@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			c->one_off = true;
 			break;
+		case 'H':
+			ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);
+			if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
+				die("Invalid hostname: %s", optarg);
+			break;
 		case 't':
 		case 'u':
 		case 'T':
diff --git a/dhcp.c b/dhcp.c
index a06f143..ec7e78a 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
  */
 int dhcp(const struct ctx *c, const struct pool *p)
 {
-	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
+	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
 	char macstr[ETH_ADDRSTRLEN];
 	const struct ethhdr *eh;
 	const struct iphdr *iph;
@@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opts[6].slen += sizeof(uint32_t);
 	}
 
+	hostname_len = strlen(c->hostname.n);
+	if ( hostname_len > 0 ) {
+		opts[12].slen = hostname_len;
+		memcpy(opts[12].s, &c->hostname.n, hostname_len);
+	}
+
 	if (!c->no_dhcp_dns_search)
 		opt_set_dns_search(c, sizeof(m->o));
 
diff --git a/dhcpv6.c b/dhcpv6.c
index 14a5c7e..190dc0e 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -48,6 +48,7 @@ struct opt_hdr {
 # define  STATUS_NOTONLINK	htons_constant(4)
 # define OPT_DNS_SERVERS	htons_constant(23)
 # define OPT_DNS_SEARCH		htons_constant(24)
+# define OPT_CLIENT_FQDN    	htons_constant(39)
 #define   STR_NOTONLINK		"Prefix not appropriate for link."
 
 	uint16_t l;
@@ -163,6 +164,18 @@ struct opt_dns_search {
 	char list[MAXDNSRCH * NS_MAXDNAME];
 } __attribute__((packed));
 
+/**
+ * struct opt_client_fqdn - Client FQDN option (RFC 4704)
+ * @hdr:		Option header
+ * @flags:		Flags as stated at RFC 4704
+ * @hostname:		Client fqdn
+ */
+struct opt_client_fqdn{
+	struct opt_hdr hdr;
+	uint8_t flags;
+	uint8_t hostname[NS_MAXDNAME];
+} __attribute__((packed));
+
 /**
  * struct msg_hdr - DHCPv6 client/server message header
  * @type:		DHCP message type
@@ -191,6 +204,7 @@ struct msg_hdr {
  * @ia_na:		Non-temporary Address option
  * @ia_addr:		Address for IA_NA
  * @client_id:		Client Identifier, variable length
+ * @client_fqdn:	Client FQDN, variable length
  * @dns_servers:	DNS Recursive Name Server, here just for storage size
  * @dns_search:		Domain Search List, here just for storage size
  */
@@ -203,10 +217,10 @@ static struct resp_t {
 	struct opt_client_id client_id;
 	struct opt_dns_servers dns_servers;
 	struct opt_dns_search dns_search;
+	struct opt_client_fqdn client_fqdn;
 } __attribute__((__packed__)) resp = {
 	{ 0 },
 	SERVER_ID,
-
 	{ { OPT_IA_NA,		OPT_SIZE_CONV(sizeof(struct opt_ia_na) +
 					      sizeof(struct opt_ia_addr) -
 					      sizeof(struct opt_hdr)) },
@@ -228,6 +242,10 @@ static struct resp_t {
 	{ { OPT_DNS_SEARCH,	0, },
 	  { 0 },
 	},
+
+	{ { OPT_CLIENT_FQDN, 0, },
+	  (uint8_t)~0U, { 0 },
+	},
 };
 
 static const struct opt_status_code sc_not_on_link = {
@@ -416,6 +434,29 @@ search:
 	return offset;
 }
 
+/**
+ * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
+ * @c:		Execution context
+ * @buf:	Response message buffer where options will be appended
+ * @offset:	Offset in message buffer for new options
+ *
+ * Return: updated length of response message buffer.
+ */
+static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
+{
+    uint16_t hostname_len = strlen(c->hostname.n);
+    if (hostname_len > 0)  {
+	struct opt_client_fqdn *o = (struct opt_client_fqdn*)(buf + offset);
+	o->hdr.t = OPT_CLIENT_FQDN;
+	o->hdr.l = htons(hostname_len+2);
+	o->flags = 0x00;
+	*o->hostname = hostname_len;
+	memcpy(o->hostname+1, c->hostname.n, hostname_len);
+	offset += sizeof(struct opt_hdr) + hostname_len+2;
+    }
+    return offset;
+}
+
 /**
  * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
  * @c:		Execution context
@@ -549,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
 	n = offsetof(struct resp_t, client_id) +
 	    sizeof(struct opt_hdr) + ntohs(client_id->l);
 	n = dhcpv6_dns_fill(c, (char *)&resp, n);
+	n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n);
 
 	resp.hdr.xid = mh->xid;
 
diff --git a/passt.h b/passt.h
index 72c7f72..d9864e6 100644
--- a/passt.h
+++ b/passt.h
@@ -205,6 +205,7 @@ struct ip6_ctx {
  * @ifi4:		Index of template interface for IPv4, 0 if IPv4 disabled
  * @ip:			IPv4 configuration
  * @dns_search:		DNS search list
+ * @hostname:		Client hostname
  * @ifi6:		Index of template interface for IPv6, 0 if IPv6 disabled
  * @ip6:		IPv6 configuration
  * @pasta_ifn:		Name of namespace interface for pasta
@@ -262,6 +263,7 @@ struct ctx {
 	struct ip4_ctx ip4;
 
 	struct fqdn dns_search[MAXDNSRCH];
+	struct fqdn hostname;
 
 	unsigned int ifi6;
 	struct ip6_ctx ip6;
diff --git a/test/lib/setup b/test/lib/setup
index 5338393..9c7aac9 100755
--- a/test/lib/setup
+++ b/test/lib/setup
@@ -49,7 +49,7 @@ setup_passt() {
 
 	context_run passt "make clean"
 	context_run passt "make valgrind"
-	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
+	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H passt1 -P ${STATESETUP}/passt.pid"
 
 	# pidfile isn't created until passt is listening
 	wait_for [ -f "${STATESETUP}/passt.pid" ]
@@ -146,11 +146,11 @@ setup_passt_in_ns() {
 	if [ ${VALGRIND} -eq 1 ]; then
 		context_run passt "make clean"
 		context_run passt "make valgrind"
-		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
+		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
 	else
 		context_run passt "make clean"
 		context_run passt "make"
-		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
+		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
 	fi
 	wait_for [ -f "${STATESETUP}/passt.pid" ]
 
@@ -215,7 +215,7 @@ setup_two_guests() {
 	[ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
 	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
 
-	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
+	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -H passt1 -t 10001 -u 10001"
 	wait_for [ -f "${STATESETUP}/passt_1.pid" ]
 
 	__opts=
@@ -223,7 +223,7 @@ setup_two_guests() {
 	[ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
 	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
 
-	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
+	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname passt2 -t 10004 -u 10004"
 	wait_for [ -f "${STATESETUP}/passt_2.pid" ]
 
 	GUEST_1_CID=94557
diff --git a/test/passt.mbuto b/test/passt.mbuto
index 138d365..bcd9041 100755
--- a/test/passt.mbuto
+++ b/test/passt.mbuto
@@ -55,6 +55,7 @@ set >> \$LOG
 [ -n "\${new_dhcp6_name_servers}" ]  && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
 [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
 [ -n "\${new_host_name}" ]           && hostname "\${new_host_name}"
+[ -n "\${new_fqdn_hostname}" ]       && hostname "\${new_fqdn_hostname}"
 exit 0
 EOF
 	chmod 755 /sbin/dhclient-script
diff --git a/test/passt/dhcp b/test/passt/dhcp
index 9925ab9..36535f2 100644
--- a/test/passt/dhcp
+++ b/test/passt/dhcp
@@ -11,7 +11,7 @@
 # Copyright (c) 2021 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio@redhat.com>
 
-gtools	ip jq dhclient sed tr
+gtools	ip jq dhclient sed tr hostname
 htools	ip jq sed tr head
 
 test	Interface name
@@ -47,7 +47,12 @@ gout	SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
 hout	HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 check	[ "__SEARCH__" = "__HOST_SEARCH__" ]
 
+test	DHCP: Hostname
+gout	HOSTNAME hostname
+check	[ "__HOSTNAME__" = "passt1" ]
+
 test	DHCPv6: address
+guest	hostname none
 guest	/sbin/dhclient -6 __IFNAME__
 # Wait for DAD to complete
 guest	while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
@@ -70,3 +75,7 @@ test	DHCPv6: search list
 gout	SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 hout	HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 check	[ "__SEARCH6__" = "__HOST_SEARCH6__" ]
+
+test	DHCPv6: Hostname
+gout	HOSTNAME hostname
+check	[ "__HOSTNAME__" = "passt1" ]
-- 
@@ -11,7 +11,7 @@
 # Copyright (c) 2021 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio@redhat.com>
 
-gtools	ip jq dhclient sed tr
+gtools	ip jq dhclient sed tr hostname
 htools	ip jq sed tr head
 
 test	Interface name
@@ -47,7 +47,12 @@ gout	SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
 hout	HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 check	[ "__SEARCH__" = "__HOST_SEARCH__" ]
 
+test	DHCP: Hostname
+gout	HOSTNAME hostname
+check	[ "__HOSTNAME__" = "passt1" ]
+
 test	DHCPv6: address
+guest	hostname none
 guest	/sbin/dhclient -6 __IFNAME__
 # Wait for DAD to complete
 guest	while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
@@ -70,3 +75,7 @@ test	DHCPv6: search list
 gout	SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 hout	HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
 check	[ "__SEARCH6__" = "__HOST_SEARCH6__" ]
+
+test	DHCPv6: Hostname
+gout	HOSTNAME hostname
+check	[ "__HOSTNAME__" = "passt1" ]
-- 
2.47.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-14  8:47 [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
@ 2024-11-14 17:13 ` Stefano Brivio
  2024-11-15  9:25   ` Enrique Llorente Pastora
  0 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2024-11-14 17:13 UTC (permalink / raw)
  To: Enrique Llorente; +Cc: passt-dev

On Thu, 14 Nov 2024 09:47:27 +0100
Enrique Llorente <ellorent@redhat.com> wrote:

> Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> (client fqdn), for some virt deployments like kubevirt is expected to
> have the VirtualMachine name as the guest hostname.
> 
> This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> options to will send hostname to clients.
> 
> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> ---
>  conf.c           | 13 ++++++++++---
>  dhcp.c           |  8 +++++++-
>  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  passt.h          |  2 ++
>  test/lib/setup   | 10 +++++-----
>  test/passt.mbuto |  1 +
>  test/passt/dhcp  | 11 ++++++++++-
>  7 files changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 14411b4..ddb585c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
>  		"  --freebind		Bind to any address for forwarding\n"
>  		"  --no-map-gw		Don't map gateway address to host\n"
>  		"  -4, --ipv4-only	Enable IPv4 operation only\n"
> -		"  -6, --ipv6-only	Enable IPv6 operation only\n");
> +		"  -6, --ipv6-only	Enable IPv6 operation only\n"
> +		"  -H, --hostname NAME Hostname to configure client with\n");

Use one tab instead of one space between "NAME" and "Hostname" so that
the output (and the code itself) is aligned.

>  
>  	if (strstr(name, "pasta"))
>  		goto pasta_opts;
> @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"map-guest-addr", required_argument,	NULL,		22 },
>  		{"host-lo-to-ns-lo", no_argument, 	NULL,		23 },
>  		{"dns-host",	required_argument,	NULL,		24 },
> +		{"hostname",	required_argument,	NULL,		'H' },

Maybe this could go a bit more up, just after the "dns" and "search"
options. Two reasons: we have all the options without a short version
at the end, and this logically belongs together with other name-related
stuff we send to the guest.

>  		{ 0 },
>  	};
>  	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  	if (c->mode == MODE_PASTA) {
>  		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
>  		fwd_default = FWD_AUTO;
> -		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> +		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";

Same here: maybe after S: it makes more sense.

>  	} else {
> -		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> +		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
>  	}
>  
>  	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			c->one_off = true;
>  			break;
> +		case 'H':
> +			ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);

We (generally) wrap lines at 80 columns where doable:

			ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
				       "%s", optarg);

> +			if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> +				die("Invalid hostname: %s", optarg);
> +			break;
>  		case 't':
>  		case 'u':
>  		case 'T':
> diff --git a/dhcp.c b/dhcp.c
> index a06f143..ec7e78a 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
>   */
>  int dhcp(const struct ctx *c, const struct pool *p)
>  {
> -	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> +	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
>  	char macstr[ETH_ADDRSTRLEN];
>  	const struct ethhdr *eh;
>  	const struct iphdr *iph;
> @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  		opts[6].slen += sizeof(uint32_t);
>  	}
>  
> +	hostname_len = strlen(c->hostname.n);
> +	if ( hostname_len > 0 ) {

No space around parentheses in expressions:

	if (hostname_len > 0) {

> +		opts[12].slen = hostname_len;
> +		memcpy(opts[12].s, &c->hostname.n, hostname_len);
> +	}
> +
>  	if (!c->no_dhcp_dns_search)
>  		opt_set_dns_search(c, sizeof(m->o));
>  
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 14a5c7e..190dc0e 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -48,6 +48,7 @@ struct opt_hdr {
>  # define  STATUS_NOTONLINK	htons_constant(4)
>  # define OPT_DNS_SERVERS	htons_constant(23)
>  # define OPT_DNS_SEARCH		htons_constant(24)
> +# define OPT_CLIENT_FQDN    	htons_constant(39)

One tab instead of spaces.

>  #define   STR_NOTONLINK		"Prefix not appropriate for link."
>  
>  	uint16_t l;
> @@ -163,6 +164,18 @@ struct opt_dns_search {
>  	char list[MAXDNSRCH * NS_MAXDNAME];
>  } __attribute__((packed));
>  
> +/**
> + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> + * @hdr:		Option header
> + * @flags:		Flags as stated at RFC 4704

s/stated at/described by/

Maybe mention that we don't really use those ("always zero for us").

> + * @hostname:		Client fqdn

A hostname is not necessarily a fully-qualified domain name. The RFC
calls this "domain name", so we could at least call this 'domain' in
the struct to make it clear we're referring to that.

As to what it should contain: RFC 4702 defines option 81 (FQDN) for
DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
just call this whole thing "FQDN" and switch to option 81 for DHCP
instead? Would this work for KubeVirt?

Or introduce two options: -H / --hostname (option 12, DHCP only), and
--fqdn (setting both DHCP option 81 and DHCPv6 option 39)?

> + */
> +struct opt_client_fqdn{
> +	struct opt_hdr hdr;
> +	uint8_t flags;
> +	uint8_t hostname[NS_MAXDNAME];

NS_MAXDNAME is typically 1025, I don't remember why it's defined
that way, but we shouldn't accept domain names that are longer than 255
bytes (or maybe even 253 bytes, RFC 1035), because otherwise they will
be different when sent over DHCP (where we have a 255 bytes limit).

Right now we only send domain names as part of the DNS search option,
and there we also use NS_MAXDNAME as limit, but for DHCP, we apply
message compression (RFC 1035 4.1.4). In that case, we might be able to
send domain names that are longer than 255 bytes (because of the
compression).

For simplicity, perhaps we should just stick to 253 bytes for any kind
of domain name. <arpa/nameser.h> doesn't really help us:

#define NS_MAXDNAME     1025    /*%< maximum domain name */

#define NS_MAXCDNAME    255     /*%< maximum compressed domain name */

...so maybe we could define our own MAXDNAME as 253 and be done with it.

> +} __attribute__((packed));
> +
>  /**
>   * struct msg_hdr - DHCPv6 client/server message header
>   * @type:		DHCP message type
> @@ -191,6 +204,7 @@ struct msg_hdr {
>   * @ia_na:		Non-temporary Address option
>   * @ia_addr:		Address for IA_NA
>   * @client_id:		Client Identifier, variable length
> + * @client_fqdn:	Client FQDN, variable length
>   * @dns_servers:	DNS Recursive Name Server, here just for storage size
>   * @dns_search:		Domain Search List, here just for storage size
>   */
> @@ -203,10 +217,10 @@ static struct resp_t {
>  	struct opt_client_id client_id;
>  	struct opt_dns_servers dns_servers;
>  	struct opt_dns_search dns_search;
> +	struct opt_client_fqdn client_fqdn;

This should also appear in the description of struct resp_t.

>  } __attribute__((__packed__)) resp = {
>  	{ 0 },
>  	SERVER_ID,
> -

Unrelated change.

>  	{ { OPT_IA_NA,		OPT_SIZE_CONV(sizeof(struct opt_ia_na) +
>  					      sizeof(struct opt_ia_addr) -
>  					      sizeof(struct opt_hdr)) },
> @@ -228,6 +242,10 @@ static struct resp_t {
>  	{ { OPT_DNS_SEARCH,	0, },
>  	  { 0 },
>  	},
> +
> +	{ { OPT_CLIENT_FQDN, 0, },
> +	  (uint8_t)~0U, { 0 },

No need to initialise fields here as this member is here just to
calculate the storage size for 'resp_t': 'flags' can be all zeroes.

> +	},
>  };
>  
>  static const struct opt_status_code sc_not_on_link = {
> @@ -416,6 +434,29 @@ search:
>  	return offset;
>  }
>  
> +/**
> + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> + * @c:		Execution context
> + * @buf:	Response message buffer where options will be appended
> + * @offset:	Offset in message buffer for new options
> + *
> + * Return: updated length of response message buffer.
> + */
> +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)

This whole function mixes up spaces and tabs.

> +{
> +    uint16_t hostname_len = strlen(c->hostname.n);
> +    if (hostname_len > 0)  {
> +	struct opt_client_fqdn *o = (struct opt_client_fqdn*)(buf + offset);

Space before * (struct opt_client_fqdn *).

> +	o->hdr.t = OPT_CLIENT_FQDN;
> +	o->hdr.l = htons(hostname_len+2);

Spaces around operands (hostname_len + 2).

> +	o->flags = 0x00;
> +	*o->hostname = hostname_len;
> +	memcpy(o->hostname+1, c->hostname.n, hostname_len);

Same here (o->hostname + 1).

> +	offset += sizeof(struct opt_hdr) + hostname_len+2;

And here.

> +    }
> +    return offset;
> +}
> +
>  /**
>   * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
>   * @c:		Execution context
> @@ -549,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  	n = offsetof(struct resp_t, client_id) +
>  	    sizeof(struct opt_hdr) + ntohs(client_id->l);
>  	n = dhcpv6_dns_fill(c, (char *)&resp, n);
> +	n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n);
>  
>  	resp.hdr.xid = mh->xid;
>  
> diff --git a/passt.h b/passt.h
> index 72c7f72..d9864e6 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -205,6 +205,7 @@ struct ip6_ctx {
>   * @ifi4:		Index of template interface for IPv4, 0 if IPv4 disabled
>   * @ip:			IPv4 configuration
>   * @dns_search:		DNS search list
> + * @hostname:		Client hostname
>   * @ifi6:		Index of template interface for IPv6, 0 if IPv6 disabled
>   * @ip6:		IPv6 configuration
>   * @pasta_ifn:		Name of namespace interface for pasta
> @@ -262,6 +263,7 @@ struct ctx {
>  	struct ip4_ctx ip4;
>  
>  	struct fqdn dns_search[MAXDNSRCH];
> +	struct fqdn hostname;

Same here: FQDN and hostname aren't necessarily the same thing. We
should pick one, or add two separate options. Or even just --fqdn if
it's what KubeVirt needs.

>  
>  	unsigned int ifi6;
>  	struct ip6_ctx ip6;
> diff --git a/test/lib/setup b/test/lib/setup
> index 5338393..9c7aac9 100755
> --- a/test/lib/setup
> +++ b/test/lib/setup
> @@ -49,7 +49,7 @@ setup_passt() {
>  
>  	context_run passt "make clean"
>  	context_run passt "make valgrind"
> -	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> +	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H passt1 -P ${STATESETUP}/passt.pid"
>  
>  	# pidfile isn't created until passt is listening
>  	wait_for [ -f "${STATESETUP}/passt.pid" ]
> @@ -146,11 +146,11 @@ setup_passt_in_ns() {
>  	if [ ${VALGRIND} -eq 1 ]; then
>  		context_run passt "make clean"
>  		context_run passt "make valgrind"
> -		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> +		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
>  	else
>  		context_run passt "make clean"
>  		context_run passt "make"
> -		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> +		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
>  	fi
>  	wait_for [ -f "${STATESETUP}/passt.pid" ]
>  
> @@ -215,7 +215,7 @@ setup_two_guests() {
>  	[ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
>  	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
>  
> -	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> +	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -H passt1 -t 10001 -u 10001"
>  	wait_for [ -f "${STATESETUP}/passt_1.pid" ]
>  
>  	__opts=
> @@ -223,7 +223,7 @@ setup_two_guests() {
>  	[ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
>  	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
>  
> -	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> +	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname passt2 -t 10004 -u 10004"
>  	wait_for [ -f "${STATESETUP}/passt_2.pid" ]
>  
>  	GUEST_1_CID=94557
> diff --git a/test/passt.mbuto b/test/passt.mbuto
> index 138d365..bcd9041 100755
> --- a/test/passt.mbuto
> +++ b/test/passt.mbuto
> @@ -55,6 +55,7 @@ set >> \$LOG
>  [ -n "\${new_dhcp6_name_servers}" ]  && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
>  [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
>  [ -n "\${new_host_name}" ]           && hostname "\${new_host_name}"
> +[ -n "\${new_fqdn_hostname}" ]       && hostname "\${new_fqdn_hostname}"
>  exit 0
>  EOF
>  	chmod 755 /sbin/dhclient-script
> diff --git a/test/passt/dhcp b/test/passt/dhcp
> index 9925ab9..36535f2 100644
> --- a/test/passt/dhcp
> +++ b/test/passt/dhcp
> @@ -11,7 +11,7 @@
>  # Copyright (c) 2021 Red Hat GmbH
>  # Author: Stefano Brivio <sbrivio@redhat.com>
>  
> -gtools	ip jq dhclient sed tr
> +gtools	ip jq dhclient sed tr hostname
>  htools	ip jq sed tr head
>  
>  test	Interface name
> @@ -47,7 +47,12 @@ gout	SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
>  hout	HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  check	[ "__SEARCH__" = "__HOST_SEARCH__" ]
>  
> +test	DHCP: Hostname
> +gout	HOSTNAME hostname
> +check	[ "__HOSTNAME__" = "passt1" ]
> +
>  test	DHCPv6: address
> +guest	hostname none
>  guest	/sbin/dhclient -6 __IFNAME__
>  # Wait for DAD to complete
>  guest	while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> @@ -70,3 +75,7 @@ test	DHCPv6: search list
>  gout	SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  hout	HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  check	[ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> +
> +test	DHCPv6: Hostname
> +gout	HOSTNAME hostname
> +check	[ "__HOSTNAME__" = "passt1" ]

The rest looks good to me.

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-14 17:13 ` Stefano Brivio
@ 2024-11-15  9:25   ` Enrique Llorente Pastora
  2024-11-15 11:28     ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: Enrique Llorente Pastora @ 2024-11-15  9:25 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

On Thu, Nov 14, 2024 at 6:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 14 Nov 2024 09:47:27 +0100
> Enrique Llorente <ellorent@redhat.com> wrote:
>
> > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > (client fqdn), for some virt deployments like kubevirt is expected to
> > have the VirtualMachine name as the guest hostname.
> >
> > This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> > options to will send hostname to clients.
> >
> > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > ---
> >  conf.c           | 13 ++++++++++---
> >  dhcp.c           |  8 +++++++-
> >  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >  passt.h          |  2 ++
> >  test/lib/setup   | 10 +++++-----
> >  test/passt.mbuto |  1 +
> >  test/passt/dhcp  | 11 ++++++++++-
> >  7 files changed, 78 insertions(+), 11 deletions(-)
> >
> > diff --git a/conf.c b/conf.c
> > index 14411b4..ddb585c 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
> >               "  --freebind           Bind to any address for forwarding\n"
> >               "  --no-map-gw          Don't map gateway address to host\n"
> >               "  -4, --ipv4-only      Enable IPv4 operation only\n"
> > -             "  -6, --ipv6-only      Enable IPv6 operation only\n");
> > +             "  -6, --ipv6-only      Enable IPv6 operation only\n"
> > +             "  -H, --hostname NAME Hostname to configure client with\n");
>
> Use one tab instead of one space between "NAME" and "Hostname" so that
> the output (and the code itself) is aligned.
>
> >
> >       if (strstr(name, "pasta"))
> >               goto pasta_opts;
> > @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >               {"map-guest-addr", required_argument,   NULL,           22 },
> >               {"host-lo-to-ns-lo", no_argument,       NULL,           23 },
> >               {"dns-host",    required_argument,      NULL,           24 },
> > +             {"hostname",    required_argument,      NULL,           'H' },
>
> Maybe this could go a bit more up, just after the "dns" and "search"
> options. Two reasons: we have all the options without a short version
> at the end, and this logically belongs together with other name-related
> stuff we send to the guest.
>
> >               { 0 },
> >       };
> >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
> >       if (c->mode == MODE_PASTA) {
> >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> >               fwd_default = FWD_AUTO;
> > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";
>
> Same here: maybe after S: it makes more sense.
>
> >       } else {
> > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
> >       }
> >
> >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
> >
> >                       c->one_off = true;
> >                       break;
> > +             case 'H':
> > +                     ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);
>
> We (generally) wrap lines at 80 columns where doable:
>
>                         ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
>                                        "%s", optarg);
>
> > +                     if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> > +                             die("Invalid hostname: %s", optarg);
> > +                     break;
> >               case 't':
> >               case 'u':
> >               case 'T':
> > diff --git a/dhcp.c b/dhcp.c
> > index a06f143..ec7e78a 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> >   */
> >  int dhcp(const struct ctx *c, const struct pool *p)
> >  {
> > -     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > +     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
> >       char macstr[ETH_ADDRSTRLEN];
> >       const struct ethhdr *eh;
> >       const struct iphdr *iph;
> > @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >               opts[6].slen += sizeof(uint32_t);
> >       }
> >
> > +     hostname_len = strlen(c->hostname.n);
> > +     if ( hostname_len > 0 ) {
>
> No space around parentheses in expressions:
>
>         if (hostname_len > 0) {
>
> > +             opts[12].slen = hostname_len;
> > +             memcpy(opts[12].s, &c->hostname.n, hostname_len);
> > +     }
> > +
> >       if (!c->no_dhcp_dns_search)
> >               opt_set_dns_search(c, sizeof(m->o));
> >
> > diff --git a/dhcpv6.c b/dhcpv6.c
> > index 14a5c7e..190dc0e 100644
> > --- a/dhcpv6.c
> > +++ b/dhcpv6.c
> > @@ -48,6 +48,7 @@ struct opt_hdr {
> >  # define  STATUS_NOTONLINK   htons_constant(4)
> >  # define OPT_DNS_SERVERS     htons_constant(23)
> >  # define OPT_DNS_SEARCH              htons_constant(24)
> > +# define OPT_CLIENT_FQDN     htons_constant(39)
>
> One tab instead of spaces.
>
> >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> >
> >       uint16_t l;
> > @@ -163,6 +164,18 @@ struct opt_dns_search {
> >       char list[MAXDNSRCH * NS_MAXDNAME];
> >  } __attribute__((packed));
> >
> > +/**
> > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > + * @hdr:             Option header
> > + * @flags:           Flags as stated at RFC 4704
>
> s/stated at/described by/
>
> Maybe mention that we don't really use those ("always zero for us").
>
> > + * @hostname:                Client fqdn
>
> A hostname is not necessarily a fully-qualified domain name. The RFC
> calls this "domain name", so we could at least call this 'domain' in
> the struct to make it clear we're referring to that.
>
> As to what it should contain: RFC 4702 defines option 81 (FQDN) for
> DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
> just call this whole thing "FQDN" and switch to option 81 for DHCP
> instead? Would this work for KubeVirt?
>
> Or introduce two options: -H / --hostname (option 12, DHCP only), and
> --fqdn (setting both DHCP option 81 and DHCPv6 option 39)?

Let me experiment with DHCPv4 option 81 with kubevirt machines, if it's enough
we use 81 instead of 12 and call it --fqdn.

If not we go with --fqdn and --hostname, but in this case it's not
clear to me what kubevirt/libvirt
has to pass though, also is this going to be expose at the domain xml
or libvirt will directly pass always
the vm name as one of them ? (for dual stack or single stack ipv4
hostname is enough, problem is single stack ipv6)

>
> > + */
> > +struct opt_client_fqdn{
> > +     struct opt_hdr hdr;
> > +     uint8_t flags;
> > +     uint8_t hostname[NS_MAXDNAME];
>
> NS_MAXDNAME is typically 1025, I don't remember why it's defined
> that way, but we shouldn't accept domain names that are longer than 255
> bytes (or maybe even 253 bytes, RFC 1035), because otherwise they will
> be different when sent over DHCP (where we have a 255 bytes limit).
>
> Right now we only send domain names as part of the DNS search option,
> and there we also use NS_MAXDNAME as limit, but for DHCP, we apply
> message compression (RFC 1035 4.1.4). In that case, we might be able to
> send domain names that are longer than 255 bytes (because of the
> compression).
>
> For simplicity, perhaps we should just stick to 253 bytes for any kind
> of domain name. <arpa/nameser.h> doesn't really help us:
>
> #define NS_MAXDNAME     1025    /*%< maximum domain name */
>
> #define NS_MAXCDNAME    255     /*%< maximum compressed domain name */
>
> ...so maybe we could define our own MAXDNAME as 253 and be done with it.

Make sense, did use the limit found around.

>
> > +} __attribute__((packed));
> > +
> >  /**
> >   * struct msg_hdr - DHCPv6 client/server message header
> >   * @type:            DHCP message type
> > @@ -191,6 +204,7 @@ struct msg_hdr {
> >   * @ia_na:           Non-temporary Address option
> >   * @ia_addr:         Address for IA_NA
> >   * @client_id:               Client Identifier, variable length
> > + * @client_fqdn:     Client FQDN, variable length
> >   * @dns_servers:     DNS Recursive Name Server, here just for storage size
> >   * @dns_search:              Domain Search List, here just for storage size
> >   */
> > @@ -203,10 +217,10 @@ static struct resp_t {
> >       struct opt_client_id client_id;
> >       struct opt_dns_servers dns_servers;
> >       struct opt_dns_search dns_search;
> > +     struct opt_client_fqdn client_fqdn;
>
> This should also appear in the description of struct resp_t.
>
> >  } __attribute__((__packed__)) resp = {
> >       { 0 },
> >       SERVER_ID,
> > -
>
> Unrelated change.
>
> >       { { OPT_IA_NA,          OPT_SIZE_CONV(sizeof(struct opt_ia_na) +
> >                                             sizeof(struct opt_ia_addr) -
> >                                             sizeof(struct opt_hdr)) },
> > @@ -228,6 +242,10 @@ static struct resp_t {
> >       { { OPT_DNS_SEARCH,     0, },
> >         { 0 },
> >       },
> > +
> > +     { { OPT_CLIENT_FQDN, 0, },
> > +       (uint8_t)~0U, { 0 },
>
> No need to initialise fields here as this member is here just to
> calculate the storage size for 'resp_t': 'flags' can be all zeroes.
>
> > +     },
> >  };
> >
> >  static const struct opt_status_code sc_not_on_link = {
> > @@ -416,6 +434,29 @@ search:
> >       return offset;
> >  }
> >
> > +/**
> > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> > + * @c:               Execution context
> > + * @buf:     Response message buffer where options will be appended
> > + * @offset:  Offset in message buffer for new options
> > + *
> > + * Return: updated length of response message buffer.
> > + */
> > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
>
> This whole function mixes up spaces and tabs.
>
> > +{
> > +    uint16_t hostname_len = strlen(c->hostname.n);
> > +    if (hostname_len > 0)  {
> > +     struct opt_client_fqdn *o = (struct opt_client_fqdn*)(buf + offset);
>
> Space before * (struct opt_client_fqdn *).
>
> > +     o->hdr.t = OPT_CLIENT_FQDN;
> > +     o->hdr.l = htons(hostname_len+2);
>
> Spaces around operands (hostname_len + 2).
>
> > +     o->flags = 0x00;
> > +     *o->hostname = hostname_len;
> > +     memcpy(o->hostname+1, c->hostname.n, hostname_len);
>
> Same here (o->hostname + 1).
>
> > +     offset += sizeof(struct opt_hdr) + hostname_len+2;
>
> And here.
>
> > +    }
> > +    return offset;
> > +}
> > +
> >  /**
> >   * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> >   * @c:               Execution context
> > @@ -549,6 +590,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
> >       n = offsetof(struct resp_t, client_id) +
> >           sizeof(struct opt_hdr) + ntohs(client_id->l);
> >       n = dhcpv6_dns_fill(c, (char *)&resp, n);
> > +     n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n);
> >
> >       resp.hdr.xid = mh->xid;
> >
> > diff --git a/passt.h b/passt.h
> > index 72c7f72..d9864e6 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -205,6 +205,7 @@ struct ip6_ctx {
> >   * @ifi4:            Index of template interface for IPv4, 0 if IPv4 disabled
> >   * @ip:                      IPv4 configuration
> >   * @dns_search:              DNS search list
> > + * @hostname:                Client hostname
> >   * @ifi6:            Index of template interface for IPv6, 0 if IPv6 disabled
> >   * @ip6:             IPv6 configuration
> >   * @pasta_ifn:               Name of namespace interface for pasta
> > @@ -262,6 +263,7 @@ struct ctx {
> >       struct ip4_ctx ip4;
> >
> >       struct fqdn dns_search[MAXDNSRCH];
> > +     struct fqdn hostname;
>
> Same here: FQDN and hostname aren't necessarily the same thing. We
> should pick one, or add two separate options. Or even just --fqdn if
> it's what KubeVirt needs.
>
> >
> >       unsigned int ifi6;
> >       struct ip6_ctx ip6;
> > diff --git a/test/lib/setup b/test/lib/setup
> > index 5338393..9c7aac9 100755
> > --- a/test/lib/setup
> > +++ b/test/lib/setup
> > @@ -49,7 +49,7 @@ setup_passt() {
> >
> >       context_run passt "make clean"
> >       context_run passt "make valgrind"
> > -     context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> > +     context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H passt1 -P ${STATESETUP}/passt.pid"
> >
> >       # pidfile isn't created until passt is listening
> >       wait_for [ -f "${STATESETUP}/passt.pid" ]
> > @@ -146,11 +146,11 @@ setup_passt_in_ns() {
> >       if [ ${VALGRIND} -eq 1 ]; then
> >               context_run passt "make clean"
> >               context_run passt "make valgrind"
> > -             context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > +             context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> >       else
> >               context_run passt "make clean"
> >               context_run passt "make"
> > -             context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> > +             context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H passt1 -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> >       fi
> >       wait_for [ -f "${STATESETUP}/passt.pid" ]
> >
> > @@ -215,7 +215,7 @@ setup_two_guests() {
> >       [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
> >       [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> >
> > -     context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> > +     context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -H passt1 -t 10001 -u 10001"
> >       wait_for [ -f "${STATESETUP}/passt_1.pid" ]
> >
> >       __opts=
> > @@ -223,7 +223,7 @@ setup_two_guests() {
> >       [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
> >       [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
> >
> > -     context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> > +     context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname passt2 -t 10004 -u 10004"
> >       wait_for [ -f "${STATESETUP}/passt_2.pid" ]
> >
> >       GUEST_1_CID=94557
> > diff --git a/test/passt.mbuto b/test/passt.mbuto
> > index 138d365..bcd9041 100755
> > --- a/test/passt.mbuto
> > +++ b/test/passt.mbuto
> > @@ -55,6 +55,7 @@ set >> \$LOG
> >  [ -n "\${new_dhcp6_name_servers}" ]  && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
> >  [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
> >  [ -n "\${new_host_name}" ]           && hostname "\${new_host_name}"
> > +[ -n "\${new_fqdn_hostname}" ]       && hostname "\${new_fqdn_hostname}"
> >  exit 0
> >  EOF
> >       chmod 755 /sbin/dhclient-script
> > diff --git a/test/passt/dhcp b/test/passt/dhcp
> > index 9925ab9..36535f2 100644
> > --- a/test/passt/dhcp
> > +++ b/test/passt/dhcp
> > @@ -11,7 +11,7 @@
> >  # Copyright (c) 2021 Red Hat GmbH
> >  # Author: Stefano Brivio <sbrivio@redhat.com>
> >
> > -gtools       ip jq dhclient sed tr
> > +gtools       ip jq dhclient sed tr hostname
> >  htools       ip jq sed tr head
> >
> >  test Interface name
> > @@ -47,7 +47,12 @@ gout       SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
> >  hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> >  check        [ "__SEARCH__" = "__HOST_SEARCH__" ]
> >
> > +test DHCP: Hostname
> > +gout HOSTNAME hostname
> > +check        [ "__HOSTNAME__" = "passt1" ]
> > +
> >  test DHCPv6: address
> > +guest        hostname none
> >  guest        /sbin/dhclient -6 __IFNAME__
> >  # Wait for DAD to complete
> >  guest        while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> > @@ -70,3 +75,7 @@ test        DHCPv6: search list
> >  gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> >  hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
> >  check        [ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> > +
> > +test DHCPv6: Hostname
> > +gout HOSTNAME hostname
> > +check        [ "__HOSTNAME__" = "passt1" ]
>
> The rest looks good to me.
>
> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-15  9:25   ` Enrique Llorente Pastora
@ 2024-11-15 11:28     ` Stefano Brivio
  2024-11-18 12:20       ` Enrique Llorente Pastora
  0 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2024-11-15 11:28 UTC (permalink / raw)
  To: Enrique Llorente Pastora; +Cc: passt-dev

On Fri, 15 Nov 2024 10:25:33 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Thu, Nov 14, 2024 at 6:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Thu, 14 Nov 2024 09:47:27 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > have the VirtualMachine name as the guest hostname.
> > >
> > > This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> > > options to will send hostname to clients.
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > >  conf.c           | 13 ++++++++++---
> > >  dhcp.c           |  8 +++++++-
> > >  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >  passt.h          |  2 ++
> > >  test/lib/setup   | 10 +++++-----
> > >  test/passt.mbuto |  1 +
> > >  test/passt/dhcp  | 11 ++++++++++-
> > >  7 files changed, 78 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/conf.c b/conf.c
> > > index 14411b4..ddb585c 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
> > >               "  --freebind           Bind to any address for forwarding\n"
> > >               "  --no-map-gw          Don't map gateway address to host\n"
> > >               "  -4, --ipv4-only      Enable IPv4 operation only\n"
> > > -             "  -6, --ipv6-only      Enable IPv6 operation only\n");
> > > +             "  -6, --ipv6-only      Enable IPv6 operation only\n"
> > > +             "  -H, --hostname NAME Hostname to configure client with\n");  
> >
> > Use one tab instead of one space between "NAME" and "Hostname" so that
> > the output (and the code itself) is aligned.
> >  
> > >
> > >       if (strstr(name, "pasta"))
> > >               goto pasta_opts;
> > > @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               {"map-guest-addr", required_argument,   NULL,           22 },
> > >               {"host-lo-to-ns-lo", no_argument,       NULL,           23 },
> > >               {"dns-host",    required_argument,      NULL,           24 },
> > > +             {"hostname",    required_argument,      NULL,           'H' },  
> >
> > Maybe this could go a bit more up, just after the "dns" and "search"
> > options. Two reasons: we have all the options without a short version
> > at the end, and this logically belongs together with other name-related
> > stuff we send to the guest.
> >  
> > >               { 0 },
> > >       };
> > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >       if (c->mode == MODE_PASTA) {
> > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > >               fwd_default = FWD_AUTO;
> > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";  
> >
> > Same here: maybe after S: it makes more sense.
> >  
> > >       } else {
> > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
> > >       }
> > >
> > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > >                       c->one_off = true;
> > >                       break;
> > > +             case 'H':
> > > +                     ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);  
> >
> > We (generally) wrap lines at 80 columns where doable:
> >
> >                         ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
> >                                        "%s", optarg);
> >  
> > > +                     if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> > > +                             die("Invalid hostname: %s", optarg);
> > > +                     break;
> > >               case 't':
> > >               case 'u':
> > >               case 'T':
> > > diff --git a/dhcp.c b/dhcp.c
> > > index a06f143..ec7e78a 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > >   */
> > >  int dhcp(const struct ctx *c, const struct pool *p)
> > >  {
> > > -     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > > +     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
> > >       char macstr[ETH_ADDRSTRLEN];
> > >       const struct ethhdr *eh;
> > >       const struct iphdr *iph;
> > > @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > >               opts[6].slen += sizeof(uint32_t);
> > >       }
> > >
> > > +     hostname_len = strlen(c->hostname.n);
> > > +     if ( hostname_len > 0 ) {  
> >
> > No space around parentheses in expressions:
> >
> >         if (hostname_len > 0) {
> >  
> > > +             opts[12].slen = hostname_len;
> > > +             memcpy(opts[12].s, &c->hostname.n, hostname_len);
> > > +     }
> > > +
> > >       if (!c->no_dhcp_dns_search)
> > >               opt_set_dns_search(c, sizeof(m->o));
> > >
> > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > index 14a5c7e..190dc0e 100644
> > > --- a/dhcpv6.c
> > > +++ b/dhcpv6.c
> > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > >  # define  STATUS_NOTONLINK   htons_constant(4)
> > >  # define OPT_DNS_SERVERS     htons_constant(23)
> > >  # define OPT_DNS_SEARCH              htons_constant(24)
> > > +# define OPT_CLIENT_FQDN     htons_constant(39)  
> >
> > One tab instead of spaces.
> >  
> > >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> > >
> > >       uint16_t l;
> > > @@ -163,6 +164,18 @@ struct opt_dns_search {
> > >       char list[MAXDNSRCH * NS_MAXDNAME];
> > >  } __attribute__((packed));
> > >
> > > +/**
> > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > + * @hdr:             Option header
> > > + * @flags:           Flags as stated at RFC 4704  
> >
> > s/stated at/described by/
> >
> > Maybe mention that we don't really use those ("always zero for us").
> >  
> > > + * @hostname:                Client fqdn  
> >
> > A hostname is not necessarily a fully-qualified domain name. The RFC
> > calls this "domain name", so we could at least call this 'domain' in
> > the struct to make it clear we're referring to that.
> >
> > As to what it should contain: RFC 4702 defines option 81 (FQDN) for
> > DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
> > just call this whole thing "FQDN" and switch to option 81 for DHCP
> > instead? Would this work for KubeVirt?
> >
> > Or introduce two options: -H / --hostname (option 12, DHCP only), and
> > --fqdn (setting both DHCP option 81 and DHCPv6 option 39)?  
> 
> Let me experiment with DHCPv4 option 81 with kubevirt machines, if it's enough
> we use 81 instead of 12 and call it --fqdn.
> 
> If not we go with --fqdn and --hostname, but in this case it's not
> clear to me what kubevirt/libvirt
> has to pass though, also is this going to be expose at the domain xml
> or libvirt will directly pass always
> the vm name as one of them ? (for dual stack or single stack ipv4
> hostname is enough, problem is single stack ipv6)

Note that they are (of course) different things, and neither RFC 4702
nor RFC 4703 make any mention whatsoever of hostname and option 12.

That is, you could legitimately pass x as hostname and y.tld as FQDN
(which gives 'y' as hostname... but RFCs don't specify which one wins).

I think for libvirt we should propose to add one or two separate
attributes ("hostname" / "fqdn"), and not imply them from the name of
the machine or "domain" (the guest "domain", nothing to do with network
domain), because that's not necessarily what users want.

About option 81 itself: it's not just a matter of changing the number,
unfortunately. You would need to set the 'E' flag. We have an
implementation of name compression according to RFC 1035, 4.1.4 in
opt_set_dns_search(), but RFC 4702 says we shouldn't use it.

No other flags set, as those are for clients.

But there's another aspect, RFC 4702 section 2:

   Only one Client FQDN option MAY appear in a message, though it may be
   instantiated in a message as multiple options [9].  DHCP clients and
   servers supporting this option MUST implement DHCP option
   concatenation [9].  In the terminology of [9], the Client FQDN option
   is a concatenation-requiring option.

...and we don't support concatenation as described by RFC 3396. The
messy part of it would be option overload (RFC 2131), which we don't do
at all. But RFC 3396 doesn't make it mandatory.

The question is whether we would practically need it though: can we,
with option 81, reasonably end up with UDP packets that are too large,
so we need to resort to option overload?

I mean, it's nothing crazy to add, it's just a bit of pointer trickery.

As a first step, *if* option 81 helps, I would just stick to what's
mandatory (including option concatenation in the "options" section,
that is, n parts of options where options < n have 255 as length).

Maybe, even before that, we could simply accept FQDNs up to 255 (253,
even) bytes.

As a second step we could look into option overload.

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-15 11:28     ` Stefano Brivio
@ 2024-11-18 12:20       ` Enrique Llorente Pastora
  2024-11-19  0:01         ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: Enrique Llorente Pastora @ 2024-11-18 12:20 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

On Fri, Nov 15, 2024 at 12:29 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Fri, 15 Nov 2024 10:25:33 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Thu, Nov 14, 2024 at 6:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > On Thu, 14 Nov 2024 09:47:27 +0100
> > > Enrique Llorente <ellorent@redhat.com> wrote:
> > >
> > > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > > have the VirtualMachine name as the guest hostname.
> > > >
> > > > This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> > > > options to will send hostname to clients.
> > > >
> > > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > > ---
> > > >  conf.c           | 13 ++++++++++---
> > > >  dhcp.c           |  8 +++++++-
> > > >  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > >  passt.h          |  2 ++
> > > >  test/lib/setup   | 10 +++++-----
> > > >  test/passt.mbuto |  1 +
> > > >  test/passt/dhcp  | 11 ++++++++++-
> > > >  7 files changed, 78 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/conf.c b/conf.c
> > > > index 14411b4..ddb585c 100644
> > > > --- a/conf.c
> > > > +++ b/conf.c
> > > > @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
> > > >               "  --freebind           Bind to any address for forwarding\n"
> > > >               "  --no-map-gw          Don't map gateway address to host\n"
> > > >               "  -4, --ipv4-only      Enable IPv4 operation only\n"
> > > > -             "  -6, --ipv6-only      Enable IPv6 operation only\n");
> > > > +             "  -6, --ipv6-only      Enable IPv6 operation only\n"
> > > > +             "  -H, --hostname NAME Hostname to configure client with\n");
> > >
> > > Use one tab instead of one space between "NAME" and "Hostname" so that
> > > the output (and the code itself) is aligned.
> > >
> > > >
> > > >       if (strstr(name, "pasta"))
> > > >               goto pasta_opts;
> > > > @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > >               {"map-guest-addr", required_argument,   NULL,           22 },
> > > >               {"host-lo-to-ns-lo", no_argument,       NULL,           23 },
> > > >               {"dns-host",    required_argument,      NULL,           24 },
> > > > +             {"hostname",    required_argument,      NULL,           'H' },
> > >
> > > Maybe this could go a bit more up, just after the "dns" and "search"
> > > options. Two reasons: we have all the options without a short version
> > > at the end, and this logically belongs together with other name-related
> > > stuff we send to the guest.
> > >
> > > >               { 0 },
> > > >       };
> > > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > > >       if (c->mode == MODE_PASTA) {
> > > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > > >               fwd_default = FWD_AUTO;
> > > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";
> > >
> > > Same here: maybe after S: it makes more sense.
> > >
> > > >       } else {
> > > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
> > > >       }
> > > >
> > > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > > >
> > > >                       c->one_off = true;
> > > >                       break;
> > > > +             case 'H':
> > > > +                     ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);
> > >
> > > We (generally) wrap lines at 80 columns where doable:
> > >
> > >                         ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
> > >                                        "%s", optarg);
> > >
> > > > +                     if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> > > > +                             die("Invalid hostname: %s", optarg);
> > > > +                     break;
> > > >               case 't':
> > > >               case 'u':
> > > >               case 'T':
> > > > diff --git a/dhcp.c b/dhcp.c
> > > > index a06f143..ec7e78a 100644
> > > > --- a/dhcp.c
> > > > +++ b/dhcp.c
> > > > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > > >   */
> > > >  int dhcp(const struct ctx *c, const struct pool *p)
> > > >  {
> > > > -     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > > > +     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
> > > >       char macstr[ETH_ADDRSTRLEN];
> > > >       const struct ethhdr *eh;
> > > >       const struct iphdr *iph;
> > > > @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > >               opts[6].slen += sizeof(uint32_t);
> > > >       }
> > > >
> > > > +     hostname_len = strlen(c->hostname.n);
> > > > +     if ( hostname_len > 0 ) {
> > >
> > > No space around parentheses in expressions:
> > >
> > >         if (hostname_len > 0) {
> > >
> > > > +             opts[12].slen = hostname_len;
> > > > +             memcpy(opts[12].s, &c->hostname.n, hostname_len);
> > > > +     }
> > > > +
> > > >       if (!c->no_dhcp_dns_search)
> > > >               opt_set_dns_search(c, sizeof(m->o));
> > > >
> > > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > > index 14a5c7e..190dc0e 100644
> > > > --- a/dhcpv6.c
> > > > +++ b/dhcpv6.c
> > > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > > >  # define  STATUS_NOTONLINK   htons_constant(4)
> > > >  # define OPT_DNS_SERVERS     htons_constant(23)
> > > >  # define OPT_DNS_SEARCH              htons_constant(24)
> > > > +# define OPT_CLIENT_FQDN     htons_constant(39)
> > >
> > > One tab instead of spaces.
> > >
> > > >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> > > >
> > > >       uint16_t l;
> > > > @@ -163,6 +164,18 @@ struct opt_dns_search {
> > > >       char list[MAXDNSRCH * NS_MAXDNAME];
> > > >  } __attribute__((packed));
> > > >
> > > > +/**
> > > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > > + * @hdr:             Option header
> > > > + * @flags:           Flags as stated at RFC 4704
> > >
> > > s/stated at/described by/
> > >
> > > Maybe mention that we don't really use those ("always zero for us").
> > >
> > > > + * @hostname:                Client fqdn
> > >
> > > A hostname is not necessarily a fully-qualified domain name. The RFC
> > > calls this "domain name", so we could at least call this 'domain' in
> > > the struct to make it clear we're referring to that.
> > >
> > > As to what it should contain: RFC 4702 defines option 81 (FQDN) for
> > > DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
> > > just call this whole thing "FQDN" and switch to option 81 for DHCP
> > > instead? Would this work for KubeVirt?
> > >
> > > Or introduce two options: -H / --hostname (option 12, DHCP only), and
> > > --fqdn (setting both DHCP option 81 and DHCPv6 option 39)?
> >
> > Let me experiment with DHCPv4 option 81 with kubevirt machines, if it's enough
> > we use 81 instead of 12 and call it --fqdn.
> >
> > If not we go with --fqdn and --hostname, but in this case it's not
> > clear to me what kubevirt/libvirt
> > has to pass though, also is this going to be expose at the domain xml
> > or libvirt will directly pass always
> > the vm name as one of them ? (for dual stack or single stack ipv4
> > hostname is enough, problem is single stack ipv6)
>
> Note that they are (of course) different things, and neither RFC 4702
> nor RFC 4703 make any mention whatsoever of hostname and option 12.
>
> That is, you could legitimately pass x as hostname and y.tld as FQDN
> (which gives 'y' as hostname... but RFCs don't specify which one wins).
>
> I think for libvirt we should propose to add one or two separate
> attributes ("hostname" / "fqdn"), and not imply them from the name of
> the machine or "domain" (the guest "domain", nothing to do with network
> domain), because that's not necessarily what users want.
>
> About option 81 itself: it's not just a matter of changing the number,
> unfortunately. You would need to set the 'E' flag. We have an
> implementation of name compression according to RFC 1035, 4.1.4 in
> opt_set_dns_search(), but RFC 4702 says we shouldn't use it.
>
> No other flags set, as those are for clients.
>

From some experiments I have do with networkmanager at a fedora 41,
the DHCPv4 client fqdn is only used
for clients to state the fqdn but never for client to configure it's
own hostname

But the DHCPv6 client does update the client's hostname with single
stack ipv6, so I think we should do the following:
- Have only one argument -H/--hostname
- Delegate the config to libvirt
- For IPv4 Options(12) with it
- For IPv6 Option(39) with flags S and O <- so we can totally ignore the client
- Document this and if someone is not happy, don't use that option.

Also for DHCPv4 we have also the option (15) for domain names, so
maybe if we detect that -H has a domain name we also add option(15).

> But there's another aspect, RFC 4702 section 2:
>
>    Only one Client FQDN option MAY appear in a message, though it may be
>    instantiated in a message as multiple options [9].  DHCP clients and
>    servers supporting this option MUST implement DHCP option
>    concatenation [9].  In the terminology of [9], the Client FQDN option
>    is a concatenation-requiring option.
>
> ...and we don't support concatenation as described by RFC 3396. The
> messy part of it would be option overload (RFC 2131), which we don't do
> at all. But RFC 3396 doesn't make it mandatory.
>
> The question is whether we would practically need it though: can we,
> with option 81, reasonably end up with UDP packets that are too large,
> so we need to resort to option overload?
>
> I mean, it's nothing crazy to add, it's just a bit of pointer trickery.
>
> As a first step, *if* option 81 helps, I would just stick to what's
> mandatory (including option concatenation in the "options" section,
> that is, n parts of options where options < n have 255 as length).
>
> Maybe, even before that, we could simply accept FQDNs up to 255 (253,
> even) bytes.
>
> As a second step we could look into option overload.
>
> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-18 12:20       ` Enrique Llorente Pastora
@ 2024-11-19  0:01         ` Stefano Brivio
  2024-11-19 13:48           ` Enrique Llorente Pastora
  0 siblings, 1 reply; 8+ messages in thread
From: Stefano Brivio @ 2024-11-19  0:01 UTC (permalink / raw)
  To: Enrique Llorente Pastora; +Cc: passt-dev

Sorry for the delay.

On Mon, 18 Nov 2024 13:20:03 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Fri, Nov 15, 2024 at 12:29 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 15 Nov 2024 10:25:33 +0100
> > Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> >  
> > > On Thu, Nov 14, 2024 at 6:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:  
> > > >
> > > > On Thu, 14 Nov 2024 09:47:27 +0100
> > > > Enrique Llorente <ellorent@redhat.com> wrote:
> > > >  
> > > > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > > > have the VirtualMachine name as the guest hostname.
> > > > >
> > > > > This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> > > > > options to will send hostname to clients.
> > > > >
> > > > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > > > ---
> > > > >  conf.c           | 13 ++++++++++---
> > > > >  dhcp.c           |  8 +++++++-
> > > > >  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > > >  passt.h          |  2 ++
> > > > >  test/lib/setup   | 10 +++++-----
> > > > >  test/passt.mbuto |  1 +
> > > > >  test/passt/dhcp  | 11 ++++++++++-
> > > > >  7 files changed, 78 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/conf.c b/conf.c
> > > > > index 14411b4..ddb585c 100644
> > > > > --- a/conf.c
> > > > > +++ b/conf.c
> > > > > @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
> > > > >               "  --freebind           Bind to any address for forwarding\n"
> > > > >               "  --no-map-gw          Don't map gateway address to host\n"
> > > > >               "  -4, --ipv4-only      Enable IPv4 operation only\n"
> > > > > -             "  -6, --ipv6-only      Enable IPv6 operation only\n");
> > > > > +             "  -6, --ipv6-only      Enable IPv6 operation only\n"
> > > > > +             "  -H, --hostname NAME Hostname to configure client with\n");  
> > > >
> > > > Use one tab instead of one space between "NAME" and "Hostname" so that
> > > > the output (and the code itself) is aligned.
> > > >  
> > > > >
> > > > >       if (strstr(name, "pasta"))
> > > > >               goto pasta_opts;
> > > > > @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > >               {"map-guest-addr", required_argument,   NULL,           22 },
> > > > >               {"host-lo-to-ns-lo", no_argument,       NULL,           23 },
> > > > >               {"dns-host",    required_argument,      NULL,           24 },
> > > > > +             {"hostname",    required_argument,      NULL,           'H' },  
> > > >
> > > > Maybe this could go a bit more up, just after the "dns" and "search"
> > > > options. Two reasons: we have all the options without a short version
> > > > at the end, and this logically belongs together with other name-related
> > > > stuff we send to the guest.
> > > >  
> > > > >               { 0 },
> > > > >       };
> > > > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > > > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > >       if (c->mode == MODE_PASTA) {
> > > > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > > > >               fwd_default = FWD_AUTO;
> > > > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";  
> > > >
> > > > Same here: maybe after S: it makes more sense.
> > > >  
> > > > >       } else {
> > > > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
> > > > >       }
> > > > >
> > > > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > > > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > >
> > > > >                       c->one_off = true;
> > > > >                       break;
> > > > > +             case 'H':
> > > > > +                     ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);  
> > > >
> > > > We (generally) wrap lines at 80 columns where doable:
> > > >
> > > >                         ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
> > > >                                        "%s", optarg);
> > > >  
> > > > > +                     if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> > > > > +                             die("Invalid hostname: %s", optarg);
> > > > > +                     break;
> > > > >               case 't':
> > > > >               case 'u':
> > > > >               case 'T':
> > > > > diff --git a/dhcp.c b/dhcp.c
> > > > > index a06f143..ec7e78a 100644
> > > > > --- a/dhcp.c
> > > > > +++ b/dhcp.c
> > > > > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > > > >   */
> > > > >  int dhcp(const struct ctx *c, const struct pool *p)
> > > > >  {
> > > > > -     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > > > > +     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
> > > > >       char macstr[ETH_ADDRSTRLEN];
> > > > >       const struct ethhdr *eh;
> > > > >       const struct iphdr *iph;
> > > > > @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > > >               opts[6].slen += sizeof(uint32_t);
> > > > >       }
> > > > >
> > > > > +     hostname_len = strlen(c->hostname.n);
> > > > > +     if ( hostname_len > 0 ) {  
> > > >
> > > > No space around parentheses in expressions:
> > > >
> > > >         if (hostname_len > 0) {
> > > >  
> > > > > +             opts[12].slen = hostname_len;
> > > > > +             memcpy(opts[12].s, &c->hostname.n, hostname_len);
> > > > > +     }
> > > > > +
> > > > >       if (!c->no_dhcp_dns_search)
> > > > >               opt_set_dns_search(c, sizeof(m->o));
> > > > >
> > > > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > > > index 14a5c7e..190dc0e 100644
> > > > > --- a/dhcpv6.c
> > > > > +++ b/dhcpv6.c
> > > > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > > > >  # define  STATUS_NOTONLINK   htons_constant(4)
> > > > >  # define OPT_DNS_SERVERS     htons_constant(23)
> > > > >  # define OPT_DNS_SEARCH              htons_constant(24)
> > > > > +# define OPT_CLIENT_FQDN     htons_constant(39)  
> > > >
> > > > One tab instead of spaces.
> > > >  
> > > > >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> > > > >
> > > > >       uint16_t l;
> > > > > @@ -163,6 +164,18 @@ struct opt_dns_search {
> > > > >       char list[MAXDNSRCH * NS_MAXDNAME];
> > > > >  } __attribute__((packed));
> > > > >
> > > > > +/**
> > > > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > > > + * @hdr:             Option header
> > > > > + * @flags:           Flags as stated at RFC 4704  
> > > >
> > > > s/stated at/described by/
> > > >
> > > > Maybe mention that we don't really use those ("always zero for us").
> > > >  
> > > > > + * @hostname:                Client fqdn  
> > > >
> > > > A hostname is not necessarily a fully-qualified domain name. The RFC
> > > > calls this "domain name", so we could at least call this 'domain' in
> > > > the struct to make it clear we're referring to that.
> > > >
> > > > As to what it should contain: RFC 4702 defines option 81 (FQDN) for
> > > > DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
> > > > just call this whole thing "FQDN" and switch to option 81 for DHCP
> > > > instead? Would this work for KubeVirt?
> > > >
> > > > Or introduce two options: -H / --hostname (option 12, DHCP only), and
> > > > --fqdn (setting both DHCP option 81 and DHCPv6 option 39)?  
> > >
> > > Let me experiment with DHCPv4 option 81 with kubevirt machines, if it's enough
> > > we use 81 instead of 12 and call it --fqdn.
> > >
> > > If not we go with --fqdn and --hostname, but in this case it's not
> > > clear to me what kubevirt/libvirt
> > > has to pass though, also is this going to be expose at the domain xml
> > > or libvirt will directly pass always
> > > the vm name as one of them ? (for dual stack or single stack ipv4
> > > hostname is enough, problem is single stack ipv6)  
> >
> > Note that they are (of course) different things, and neither RFC 4702
> > nor RFC 4703 make any mention whatsoever of hostname and option 12.
> >
> > That is, you could legitimately pass x as hostname and y.tld as FQDN
> > (which gives 'y' as hostname... but RFCs don't specify which one wins).
> >
> > I think for libvirt we should propose to add one or two separate
> > attributes ("hostname" / "fqdn"), and not imply them from the name of
> > the machine or "domain" (the guest "domain", nothing to do with network
> > domain), because that's not necessarily what users want.
> >
> > About option 81 itself: it's not just a matter of changing the number,
> > unfortunately. You would need to set the 'E' flag. We have an
> > implementation of name compression according to RFC 1035, 4.1.4 in
> > opt_set_dns_search(), but RFC 4702 says we shouldn't use it.
> >
> > No other flags set, as those are for clients.
> 
> From some experiments I have do with networkmanager at a fedora 41,
> the DHCPv4 client fqdn is only used
> for clients to state the fqdn but never for client to configure it's
> own hostname
> 
> But the DHCPv6 client does update the client's hostname with single
> stack ipv6

Ouch. Of course we have to consider this for compatibility reasons, but
it's obviously inconsistent: RFC 4704 is without a doubt equivalent
to RFC 4702 for IPv6.

Neither says that a client must update its hostname, but if DHCPv6
option 39 causes NetworkManager to, it doesn't make a lot of sense to
me that DHCP option 81 doesn't.

I think support for option 39 (in the sense of setting the hostname,
not in general) was simply added later, by commit 1f74ea52f581
("policy: get the DHCPv6 hostname from the FQDN option"), and that
might simply be the reason.

Well, regardless of whether it makes sense to propose to change this
for NetworkManager, we have to deal with existing versions anyway.

> so I think we should do the following:
> - Have only one argument -H/--hostname

...taking a hostname or a FQDN?

> - Delegate the config to libvirt
> - For IPv4 Options(12) with it

What would we send in this option if we're given a FQDN? We shouldn't
(no MUST NOT, though) send a FQDN in it.

> - For IPv6 Option(39) with flags S and O <- so we can totally ignore the client

...and what would we send in this option if we're given a hostname?
Here, we MUST send a FQDN, not an unqualified hostname.

By the way, if the client sets 'S' to 0, I think that we should (even
though it's not a MUST) set 'O' to 0, because we're not overriding it.

It's not needed but it's less likely to cause issues with a buggy
DHCPv6 client.

> - Document this and if someone is not happy, don't use that option.
> 
> Also for DHCPv4 we have also the option (15) for domain names, so
> maybe if we detect that -H has a domain name we also add option(15).

I'm not sure if we should try to be clever about it. In general I agree
it's a good idea, but: 1. we can't fix the inconsistencies I presented
above (I think) and 2. we're actually losing generality because there
are configurations that a user can't cover this way, such as:

- setting DHCP option 81 itself

- setting hostname only, without FQDN, or FQDN without hostname

- setting both FQDN and hostname to different (non-conflicting) values

- setting a different domain for the guests (it's not possible at the
  moment but it could become a bit complicated to add this option in
  the future), say, the host is called "x" and we would like to call
  the guest "x.guests".

The RFCs are doing us a big favour by not coupling options together and
letting us do pretty much what we want.

Do you think it would be problematic to do this instead:

- -H / --hostname sets DHCP option 12

- --fqdn sets DHCP option 81 and DHCPv6 option 39

- ask libvirt to take zero, one or two different attributes between
  "fqdn" (setting --fqdn) and "hostname" (setting --hostname)

?

In the use case that KubeVirt needs the most (if I understood
correctly), the domain XML would include something like:

  <fqdn>abcd.example.com</fqdn><hostname>abcd</hostname>

which is not ideal, but we have a configuration front-end for it, and,
if NetworkManager ever changes to handling --fqdn as it handles
--hostname, we could skip <fqdn/> at that point.

Side note: with this item from the feature list on the website, at
https://passt.top/#services:

  ⌚ fine-grained configurability of DHCP, NDP, DHCPv6 options

...I was actually thinking of a mechanism where users could set
arbitrary options, some of which with type validation, in a similar
way as what dhcpcd (as client) supports:

$ /sbin/dhcpcd -V
[...]
DHCPv4 options:
[...]
001 subnet_mask                         	 ipaddress request
121 classless_static_routes             	 string rfc3442
002 time_offset                         	 int32
003 routers                             	 array ipaddress request
004 time_servers                        	 array ipaddress
[...]
DHCPv6 options:
00001 client_id                         	 binhex
00002 server_id                         	 binhex
00003 ia_na                             	 embed index norequest
[...]

but without support for fancy attributes such as "embed" or "index".

That is, supporting arbitrary "ia_na" options would be way too
complicated, but simple strings (such as the ones we have in option 12)
or numbers would be okay. Say: '--dhcp-opt 12 abcd'.

On the other hand, this doesn't solve the problem for FQDN options
because they have flags which need some logic in the server itself,
so... yes, this is just a side note, and I'm not saying we should
implement this now (or ever).

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-19  0:01         ` Stefano Brivio
@ 2024-11-19 13:48           ` Enrique Llorente Pastora
  2024-11-19 18:31             ` Stefano Brivio
  0 siblings, 1 reply; 8+ messages in thread
From: Enrique Llorente Pastora @ 2024-11-19 13:48 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

On Tue, Nov 19, 2024 at 1:01 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> Sorry for the delay.
>
> On Mon, 18 Nov 2024 13:20:03 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Fri, Nov 15, 2024 at 12:29 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > On Fri, 15 Nov 2024 10:25:33 +0100
> > > Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> > >
> > > > On Thu, Nov 14, 2024 at 6:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> > > > >
> > > > > On Thu, 14 Nov 2024 09:47:27 +0100
> > > > > Enrique Llorente <ellorent@redhat.com> wrote:
> > > > >
> > > > > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > > > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > > > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > > > > have the VirtualMachine name as the guest hostname.
> > > > > >
> > > > > > This change add the -H --hostname to configure the DHCPv4 and DHCPv6
> > > > > > options to will send hostname to clients.
> > > > > >
> > > > > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > > > > ---
> > > > > >  conf.c           | 13 ++++++++++---
> > > > > >  dhcp.c           |  8 +++++++-
> > > > > >  dhcpv6.c         | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > > > > >  passt.h          |  2 ++
> > > > > >  test/lib/setup   | 10 +++++-----
> > > > > >  test/passt.mbuto |  1 +
> > > > > >  test/passt/dhcp  | 11 ++++++++++-
> > > > > >  7 files changed, 78 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/conf.c b/conf.c
> > > > > > index 14411b4..ddb585c 100644
> > > > > > --- a/conf.c
> > > > > > +++ b/conf.c
> > > > > > @@ -847,7 +847,8 @@ static void usage(const char *name, FILE *f, int status)
> > > > > >               "  --freebind           Bind to any address for forwarding\n"
> > > > > >               "  --no-map-gw          Don't map gateway address to host\n"
> > > > > >               "  -4, --ipv4-only      Enable IPv4 operation only\n"
> > > > > > -             "  -6, --ipv6-only      Enable IPv6 operation only\n");
> > > > > > +             "  -6, --ipv6-only      Enable IPv6 operation only\n"
> > > > > > +             "  -H, --hostname NAME Hostname to configure client with\n");
> > > > >
> > > > > Use one tab instead of one space between "NAME" and "Hostname" so that
> > > > > the output (and the code itself) is aligned.
> > > > >
> > > > > >
> > > > > >       if (strstr(name, "pasta"))
> > > > > >               goto pasta_opts;
> > > > > > @@ -1303,6 +1304,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > > >               {"map-guest-addr", required_argument,   NULL,           22 },
> > > > > >               {"host-lo-to-ns-lo", no_argument,       NULL,           23 },
> > > > > >               {"dns-host",    required_argument,      NULL,           24 },
> > > > > > +             {"hostname",    required_argument,      NULL,           'H' },
> > > > >
> > > > > Maybe this could go a bit more up, just after the "dns" and "search"
> > > > > options. Two reasons: we have all the options without a short version
> > > > > at the end, and this logically belongs together with other name-related
> > > > > stuff we send to the guest.
> > > > >
> > > > > >               { 0 },
> > > > > >       };
> > > > > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > > > > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > > >       if (c->mode == MODE_PASTA) {
> > > > > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > > > > >               fwd_default = FWD_AUTO;
> > > > > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > > > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:H:";
> > > > >
> > > > > Same here: maybe after S: it makes more sense.
> > > > >
> > > > > >       } else {
> > > > > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > > > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:H:";
> > > > > >       }
> > > > > >
> > > > > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > > > > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > > > > >
> > > > > >                       c->one_off = true;
> > > > > >                       break;
> > > > > > +             case 'H':
> > > > > > +                     ret = snprintf(c->hostname.n, sizeof(c->hostname.n), "%s", optarg);
> > > > >
> > > > > We (generally) wrap lines at 80 columns where doable:
> > > > >
> > > > >                         ret = snprintf(c->hostname.n, sizeof(c->hostname.n),
> > > > >                                        "%s", optarg);
> > > > >
> > > > > > +                     if (ret <= 0 || ret >= (int)sizeof(c->hostname.n))
> > > > > > +                             die("Invalid hostname: %s", optarg);
> > > > > > +                     break;
> > > > > >               case 't':
> > > > > >               case 'u':
> > > > > >               case 'T':
> > > > > > diff --git a/dhcp.c b/dhcp.c
> > > > > > index a06f143..ec7e78a 100644
> > > > > > --- a/dhcp.c
> > > > > > +++ b/dhcp.c
> > > > > > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> > > > > >   */
> > > > > >  int dhcp(const struct ctx *c, const struct pool *p)
> > > > > >  {
> > > > > > -     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > > > > > +     size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len;
> > > > > >       char macstr[ETH_ADDRSTRLEN];
> > > > > >       const struct ethhdr *eh;
> > > > > >       const struct iphdr *iph;
> > > > > > @@ -375,6 +375,12 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > > > >               opts[6].slen += sizeof(uint32_t);
> > > > > >       }
> > > > > >
> > > > > > +     hostname_len = strlen(c->hostname.n);
> > > > > > +     if ( hostname_len > 0 ) {
> > > > >
> > > > > No space around parentheses in expressions:
> > > > >
> > > > >         if (hostname_len > 0) {
> > > > >
> > > > > > +             opts[12].slen = hostname_len;
> > > > > > +             memcpy(opts[12].s, &c->hostname.n, hostname_len);
> > > > > > +     }
> > > > > > +
> > > > > >       if (!c->no_dhcp_dns_search)
> > > > > >               opt_set_dns_search(c, sizeof(m->o));
> > > > > >
> > > > > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > > > > index 14a5c7e..190dc0e 100644
> > > > > > --- a/dhcpv6.c
> > > > > > +++ b/dhcpv6.c
> > > > > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > > > > >  # define  STATUS_NOTONLINK   htons_constant(4)
> > > > > >  # define OPT_DNS_SERVERS     htons_constant(23)
> > > > > >  # define OPT_DNS_SEARCH              htons_constant(24)
> > > > > > +# define OPT_CLIENT_FQDN     htons_constant(39)
> > > > >
> > > > > One tab instead of spaces.
> > > > >
> > > > > >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> > > > > >
> > > > > >       uint16_t l;
> > > > > > @@ -163,6 +164,18 @@ struct opt_dns_search {
> > > > > >       char list[MAXDNSRCH * NS_MAXDNAME];
> > > > > >  } __attribute__((packed));
> > > > > >
> > > > > > +/**
> > > > > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > > > > + * @hdr:             Option header
> > > > > > + * @flags:           Flags as stated at RFC 4704
> > > > >
> > > > > s/stated at/described by/
> > > > >
> > > > > Maybe mention that we don't really use those ("always zero for us").
> > > > >
> > > > > > + * @hostname:                Client fqdn
> > > > >
> > > > > A hostname is not necessarily a fully-qualified domain name. The RFC
> > > > > calls this "domain name", so we could at least call this 'domain' in
> > > > > the struct to make it clear we're referring to that.
> > > > >
> > > > > As to what it should contain: RFC 4702 defines option 81 (FQDN) for
> > > > > DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Should we
> > > > > just call this whole thing "FQDN" and switch to option 81 for DHCP
> > > > > instead? Would this work for KubeVirt?
> > > > >
> > > > > Or introduce two options: -H / --hostname (option 12, DHCP only), and
> > > > > --fqdn (setting both DHCP option 81 and DHCPv6 option 39)?
> > > >
> > > > Let me experiment with DHCPv4 option 81 with kubevirt machines, if it's enough
> > > > we use 81 instead of 12 and call it --fqdn.
> > > >
> > > > If not we go with --fqdn and --hostname, but in this case it's not
> > > > clear to me what kubevirt/libvirt
> > > > has to pass though, also is this going to be expose at the domain xml
> > > > or libvirt will directly pass always
> > > > the vm name as one of them ? (for dual stack or single stack ipv4
> > > > hostname is enough, problem is single stack ipv6)
> > >
> > > Note that they are (of course) different things, and neither RFC 4702
> > > nor RFC 4703 make any mention whatsoever of hostname and option 12.
> > >
> > > That is, you could legitimately pass x as hostname and y.tld as FQDN
> > > (which gives 'y' as hostname... but RFCs don't specify which one wins).
> > >
> > > I think for libvirt we should propose to add one or two separate
> > > attributes ("hostname" / "fqdn"), and not imply them from the name of
> > > the machine or "domain" (the guest "domain", nothing to do with network
> > > domain), because that's not necessarily what users want.
> > >
> > > About option 81 itself: it's not just a matter of changing the number,
> > > unfortunately. You would need to set the 'E' flag. We have an
> > > implementation of name compression according to RFC 1035, 4.1.4 in
> > > opt_set_dns_search(), but RFC 4702 says we shouldn't use it.
> > >
> > > No other flags set, as those are for clients.
> >
> > From some experiments I have do with networkmanager at a fedora 41,
> > the DHCPv4 client fqdn is only used
> > for clients to state the fqdn but never for client to configure it's
> > own hostname
> >
> > But the DHCPv6 client does update the client's hostname with single
> > stack ipv6
>
> Ouch. Of course we have to consider this for compatibility reasons, but
> it's obviously inconsistent: RFC 4704 is without a doubt equivalent
> to RFC 4702 for IPv6.

Well from what I have read around IPv6 option 39 can be considered as
IPv4 option 12 + option 15
Where you use option 12 to send the hostname part and option 15 the
rest of the fqdn.

>
> Neither says that a client must update its hostname, but if DHCPv6
> option 39 causes NetworkManager to, it doesn't make a lot of sense to
> me that DHCP option 81 doesn't.
>
> I think support for option 39 (in the sense of setting the hostname,
> not in general) was simply added later, by commit 1f74ea52f581
> ("policy: get the DHCPv6 hostname from the FQDN option"), and that
> might simply be the reason.
>
> Well, regardless of whether it makes sense to propose to change this
> for NetworkManager, we have to deal with existing versions anyway.
>

Well looks like option DHCPv4 81 was never intended for that since you
have the option 12 and 15 already there
in the case of DHCPv6 you just have option 39.

But I will ask NM maintainers about why NetworkManager do not update
client's fqdn with DHCPv4 81.

> > so I think we should do the following:
> > - Have only one argument -H/--hostname
>
> ...taking a hostname or a FQDN?

Always FQDN, if users want just the hostname they can pass "passt1.local"
is better to have just --fqdn option instead of -H/--hostname

>
> > - Delegate the config to libvirt
> > - For IPv4 Options(12) with it
>
> What would we send in this option if we're given a FQDN? We shouldn't
> (no MUST NOT, though) send a FQDN in it.

So some examples and what may mean at DHCPv4 (option 12 and option 15)
and DHCPv6 (option 39)

--fqdn=host1.passt.net
- v4,opt12 = host1
- v4,opt15 = passt.net
- v6.opt39 = host1.passt.net

--fqdn=host1.local
- v4,opt12 = host1
- v6.opt39 = host1[.local] // not sure


--fqdn=host1
[error]

>
> > - For IPv6 Option(39) with flags S and O <- so we can totally ignore the client
>
> ...and what would we send in this option if we're given a hostname?
> Here, we MUST send a FQDN, not an unqualified hostname.

At least NetworkManager does not care if it's fqdn or not and changes
the VM's hostname.

>
> By the way, if the client sets 'S' to 0, I think that we should (even
> though it's not a MUST) set 'O' to 0, because we're not overriding it.
>
> It's not needed but it's less likely to cause issues with a buggy
> DHCPv6 client.

Right, make sense, let's follow RFC's.


>
> > - Document this and if someone is not happy, don't use that option.
> >
> > Also for DHCPv4 we have also the option (15) for domain names, so
> > maybe if we detect that -H has a domain name we also add option(15).
>
> I'm not sure if we should try to be clever about it. In general I agree
> it's a good idea, but: 1. we can't fix the inconsistencies I presented
> above (I think) and 2. we're actually losing generality because there
> are configurations that a user can't cover this way, such as:
>
> - setting DHCP option 81 itself
>
> - setting hostname only, without FQDN, or FQDN without hostname
>
> - setting both FQDN and hostname to different (non-conflicting) values
>
> - setting a different domain for the guests (it's not possible at the
>   moment but it could become a bit complicated to add this option in
>   the future), say, the host is called "x" and we would like to call
>   the guest "x.guests".
>
> The RFCs are doing us a big favour by not coupling options together and
> letting us do pretty much what we want.
>
> Do you think it would be problematic to do this instead:
>
> - -H / --hostname sets DHCP option 12
>
> - --fqdn sets DHCP option 81 and DHCPv6 option 39

The alternative I was thinking (In case 81 is not really doing what we need)
is just having --fqdn and for DHCPv4 use option 12 and 15
and for DHPv6 option 39 and force users to always pass a fqdn (it can
be hostname.local)

>
> - ask libvirt to take zero, one or two different attributes between
>   "fqdn" (setting --fqdn) and "hostname" (setting --hostname)
>
> ?

If we go with just --fqdn then this would be better.

>
> In the use case that KubeVirt needs the most (if I understood
> correctly), the domain XML would include something like:
>
>   <fqdn>abcd.example.com</fqdn><hostname>abcd</hostname>
>
> which is not ideal, but we have a configuration front-end for it, and,
> if NetworkManager ever changes to handling --fqdn as it handles
> --hostname, we could skip <fqdn/> at that point.
>

Make sense.

> Side note: with this item from the feature list on the website, at
> https://passt.top/#services:
>
>   ⌚ fine-grained configurability of DHCP, NDP, DHCPv6 options
>
> ...I was actually thinking of a mechanism where users could set
> arbitrary options, some of which with type validation, in a similar
> way as what dhcpcd (as client) supports:
>
> $ /sbin/dhcpcd -V
> [...]
> DHCPv4 options:
> [...]
> 001 subnet_mask                                  ipaddress request
> 121 classless_static_routes                      string rfc3442
> 002 time_offset                                  int32
> 003 routers                                      array ipaddress request
> 004 time_servers                                 array ipaddress
> [...]
> DHCPv6 options:
> 00001 client_id                                  binhex
> 00002 server_id                                  binhex
> 00003 ia_na                                      embed index norequest
> [...]
>
> but without support for fancy attributes such as "embed" or "index".
>
> That is, supporting arbitrary "ia_na" options would be way too
> complicated, but simple strings (such as the ones we have in option 12)
> or numbers would be okay. Say: '--dhcp-opt 12 abcd'.
>
> On the other hand, this doesn't solve the problem for FQDN options
> because they have flags which need some logic in the server itself,
> so... yes, this is just a side note, and I'm not saying we should
> implement this now (or ever).

Well we didn't have many customers asking for super specific DHCP
stuff, not sure if
it's worth it, like users that do really need to configure their own
network just use layer2 with their own
DHCP server and network infra.

>
> --
> Stefano
>


--
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops
  2024-11-19 13:48           ` Enrique Llorente Pastora
@ 2024-11-19 18:31             ` Stefano Brivio
  0 siblings, 0 replies; 8+ messages in thread
From: Stefano Brivio @ 2024-11-19 18:31 UTC (permalink / raw)
  To: Enrique Llorente Pastora; +Cc: passt-dev

On Tue, 19 Nov 2024 14:48:53 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Tue, Nov 19, 2024 at 1:01 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > Sorry for the delay.
> >
> > On Mon, 18 Nov 2024 13:20:03 +0100
> > Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> >  
> > > [...]
> > >
> > > From some experiments I have do with networkmanager at a fedora 41,
> > > the DHCPv4 client fqdn is only used
> > > for clients to state the fqdn but never for client to configure it's
> > > own hostname
> > >
> > > But the DHCPv6 client does update the client's hostname with single
> > > stack ipv6  
> >
> > Ouch. Of course we have to consider this for compatibility reasons, but
> > it's obviously inconsistent: RFC 4704 is without a doubt equivalent
> > to RFC 4702 for IPv6.  
> 
> Well from what I have read around IPv6 option 39 can be considered as
> IPv4 option 12 + option 15
> Where you use option 12 to send the hostname part and option 15 the
> rest of the fqdn.

Logically, yes, but a client has to prefer DHCP option 81 if sent by
the server, see below.

> > Neither says that a client must update its hostname, but if DHCPv6
> > option 39 causes NetworkManager to, it doesn't make a lot of sense to
> > me that DHCP option 81 doesn't.
> >
> > I think support for option 39 (in the sense of setting the hostname,
> > not in general) was simply added later, by commit 1f74ea52f581
> > ("policy: get the DHCPv6 hostname from the FQDN option"), and that
> > might simply be the reason.
> >
> > Well, regardless of whether it makes sense to propose to change this
> > for NetworkManager, we have to deal with existing versions anyway.
> 
> Well looks like option DHCPv4 81 was never intended for that since you
> have the option 12 and 15 already there

That's not true, see RFC 4702 3.1:

3.1.  Interaction with Other Options

   Other DHCP options MAY carry data that is related to the Domain Name
   field of the Client FQDN option.  The Host Name option [12], for
   example, contains an ASCII string representation of the client's host
   name.  In general, a client does not need to send redundant data, and
   therefore clients that send the Client FQDN option in their messages
   MUST NOT also send the Host Name option.  Clients that receive both
   the Host Name option and the Client FQDN option from a server SHOULD
   prefer Client FQDN option data.  Section 4 instructs servers to
   ignore the Host Name option in client messages that include the
   Client FQDN option.

and later, in 3.5:

   The server MAY be configured to use the name supplied in the client's
   Client FQDN option, or it MAY be configured to modify the supplied
   name or to substitute a different name.  The server SHOULD send its
   notion of the complete FQDN for the client in the Domain Name field.

So, yes, NetworkManager doesn't take the hostname from it, but that's
just one possible client for passt. I'd stick to normative references.

> in the case of DHCPv6 you just have option 39.
> 
> But I will ask NM maintainers about why NetworkManager do not update
> client's fqdn with DHCPv4 81.

Thanks. Regardless of that, even if NetworkManager decides to just take
option 81, we'll have to support existing versions as well, so it
doesn't change things much, I guess.

> > > so I think we should do the following:
> > > - Have only one argument -H/--hostname  
> >
> > ...taking a hostname or a FQDN?  
> 
> Always FQDN, if users want just the hostname they can pass "passt1.local"
> is better to have just --fqdn option instead of -H/--hostname

Okay, so far so good, except that here:

> > > - Delegate the config to libvirt
> > > - For IPv4 Options(12) with it  
> >
> > What would we send in this option if we're given a FQDN? We shouldn't
> > (no MUST NOT, though) send a FQDN in it.  
> 
> So some examples and what may mean at DHCPv4 (option 12 and option 15)
> and DHCPv6 (option 39)
> 
> --fqdn=host1.passt.net
> - v4,opt12 = host1
> - v4,opt15 = passt.net
> - v6.opt39 = host1.passt.net

...this is unexpected. If I didn't know what NetworkManager does with
it, I would expect --fqdn to set DHCP option 81. It's in its name:
"Fully Qualified Domain Name (FQDN) Option".

> --fqdn=host1.local
> - v4,opt12 = host1
> - v6.opt39 = host1[.local] // not sure
> 
> --fqdn=host1
> [error]
> 
> >  
> > > - For IPv6 Option(39) with flags S and O <- so we can totally ignore the client  
> >
> > ...and what would we send in this option if we're given a hostname?
> > Here, we MUST send a FQDN, not an unqualified hostname.  
> 
> At least NetworkManager does not care if it's fqdn or not and changes
> the VM's hostname.

Fine, but again, we have other clients that might be more or less picky.

> > By the way, if the client sets 'S' to 0, I think that we should (even
> > though it's not a MUST) set 'O' to 0, because we're not overriding it.
> >
> > It's not needed but it's less likely to cause issues with a buggy
> > DHCPv6 client.  
> 
> Right, make sense, let's follow RFC's.
> 
> > > - Document this and if someone is not happy, don't use that option.
> > >
> > > Also for DHCPv4 we have also the option (15) for domain names, so
> > > maybe if we detect that -H has a domain name we also add option(15).  
> >
> > I'm not sure if we should try to be clever about it. In general I agree
> > it's a good idea, but: 1. we can't fix the inconsistencies I presented
> > above (I think) and 2. we're actually losing generality because there
> > are configurations that a user can't cover this way, such as:
> >
> > - setting DHCP option 81 itself
> >
> > - setting hostname only, without FQDN, or FQDN without hostname
> >
> > - setting both FQDN and hostname to different (non-conflicting) values
> >
> > - setting a different domain for the guests (it's not possible at the
> >   moment but it could become a bit complicated to add this option in
> >   the future), say, the host is called "x" and we would like to call
> >   the guest "x.guests".
> >
> > The RFCs are doing us a big favour by not coupling options together and
> > letting us do pretty much what we want.
> >
> > Do you think it would be problematic to do this instead:
> >
> > - -H / --hostname sets DHCP option 12
> >
> > - --fqdn sets DHCP option 81 and DHCPv6 option 39  
> 
> The alternative I was thinking (In case 81 is not really doing what we need)
> is just having --fqdn and for DHCPv4 use option 12 and 15
> and for DHPv6 option 39 and force users to always pass a fqdn (it can
> be hostname.local)
> 
> > - ask libvirt to take zero, one or two different attributes between
> >   "fqdn" (setting --fqdn) and "hostname" (setting --hostname)
> >
> > ?  
> 
> If we go with just --fqdn then this would be better.

...why is it better? I mean, I understand that it would make libvirt's
XML shorter, but I don't expect the matching libvirt implementation to
be significantly more complicated.

That would also risk being at the expense of any other user that's not
libvirt/KubeVirt plus NetworkManager, by the way.

> > In the use case that KubeVirt needs the most (if I understood
> > correctly), the domain XML would include something like:
> >
> >   <fqdn>abcd.example.com</fqdn><hostname>abcd</hostname>
> >
> > which is not ideal, but we have a configuration front-end for it, and,
> > if NetworkManager ever changes to handling --fqdn as it handles
> > --hostname, we could skip <fqdn/> at that point.
> 
> Make sense.
> 
> > Side note: with this item from the feature list on the website, at
> > https://passt.top/#services:
> >
> >   ⌚ fine-grained configurability of DHCP, NDP, DHCPv6 options
> >
> > ...I was actually thinking of a mechanism where users could set
> > arbitrary options, some of which with type validation, in a similar
> > way as what dhcpcd (as client) supports:
> >
> > $ /sbin/dhcpcd -V
> > [...]
> > DHCPv4 options:
> > [...]
> > 001 subnet_mask                                  ipaddress request
> > 121 classless_static_routes                      string rfc3442
> > 002 time_offset                                  int32
> > 003 routers                                      array ipaddress request
> > 004 time_servers                                 array ipaddress
> > [...]
> > DHCPv6 options:
> > 00001 client_id                                  binhex
> > 00002 server_id                                  binhex
> > 00003 ia_na                                      embed index norequest
> > [...]
> >
> > but without support for fancy attributes such as "embed" or "index".
> >
> > That is, supporting arbitrary "ia_na" options would be way too
> > complicated, but simple strings (such as the ones we have in option 12)
> > or numbers would be okay. Say: '--dhcp-opt 12 abcd'.
> >
> > On the other hand, this doesn't solve the problem for FQDN options
> > because they have flags which need some logic in the server itself,
> > so... yes, this is just a side note, and I'm not saying we should
> > implement this now (or ever).  
> 
> Well we didn't have many customers asking for super specific DHCP

Yes, I understand that's good enough for KubeVirt, but we have other
users.

In any case, this was asked a couple of times but I can't remember
really fundamental use cases around it that couldn't be solved
otherwise, and we have a long list of features we never seem to be
getting to.

This was just a side note because it's one possible direction/future
implementation, but again, I'm not saying it will come soon (or ever).

> stuff, not sure if
> it's worth it, like users that do really need to configure their own
> network just use layer2 with their own
> DHCP server and network infra.

It depends if privileges and other features matter to you. I've also
seen users experimenting with passt on Android, and there you can't
just use a DHCP server. Or muvm. Or:

  https://github.com/siderolabs/talos/issues/9725

-- 
Stefano


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-11-19 18:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-14  8:47 [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
2024-11-14 17:13 ` Stefano Brivio
2024-11-15  9:25   ` Enrique Llorente Pastora
2024-11-15 11:28     ` Stefano Brivio
2024-11-18 12:20       ` Enrique Llorente Pastora
2024-11-19  0:01         ` Stefano Brivio
2024-11-19 13:48           ` Enrique Llorente Pastora
2024-11-19 18:31             ` Stefano Brivio

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).