From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=UmW4+461; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 58FDF5A061A for ; Tue, 19 Nov 2024 14:49:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732024150; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QzrD0Uk5/tisFG9aqSCEGZviGdWPB2Xmdl/N+v5ZoIs=; b=UmW4+461OUB9rvjEf52k712Y6Hb9OYA0w62av/KtT0yPe0uS6GbcLZNPnwphg5EQG6yTDw DQuj/vgXapRQ+cstjJliOLzOOGvrqMPQ9S1UZ+w2E6Oi0QNrKUAbx70+vPR2zANZumcUG4 nL51zgLK95slXk0J3UJO4I0BkL/wu20= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-272-JTAHcj9pMGGFLPgg_QnsgQ-1; Tue, 19 Nov 2024 08:49:06 -0500 X-MC-Unique: JTAHcj9pMGGFLPgg_QnsgQ-1 X-Mimecast-MFC-AGG-ID: JTAHcj9pMGGFLPgg_QnsgQ Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-a99fa9f0c25so67121066b.3 for ; Tue, 19 Nov 2024 05:49:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732024145; x=1732628945; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QzrD0Uk5/tisFG9aqSCEGZviGdWPB2Xmdl/N+v5ZoIs=; b=RANgmog/MyXs9XDuui48LQl7glDZis1iRwFvLDKQf9xc/Zg1MjFvcZ1wDG2p04Dq6a ORJD4PSBuQHKFv8F9jS7XFIVJKeIbgXZb0pB1Y0dYc8bJnXiB6wADh6EpB8/YaZgEMMK Xy+ROPGGM9B3g6/c7xQkdITMh4o3rJP1jBLirXqRj1qaYynajBByXgFsIdxfW0i9c+u6 4jdTCMZy/lvK577of90iUFcjlI3b6a0MH61czLxfQb9WgSxGxB/UGUUTzYILzl7t6EFz 6zdlu47KpdlCF4S9LfnUgW8DZ+CauI3/f/v6ixweiN7fxZHmq5utSNlFAeQKWo0MoL3i b4Ug== X-Gm-Message-State: AOJu0YxS7zkeZ0x13C5wrPEpuYDCjBzGEgEebpgYqpBG6bPPGht7ODJu 0vHTSdSav+6tQommvWDHtbDLx0wAWqBpotkWOrbwu0DacHOS8oguRpJ/RaixJv9hohLUereXs2r nPeQXYDi2LvkFllHyOqKBGqTMWkiFVmI3kseKNY+Sv1T0L8i63SL82gt9vR8QQhWsXoJ3xJ5D7j dUugUl1JibgShmsgMpM55crbYF X-Received: by 2002:a17:907:2cc7:b0:aa4:8282:6601 with SMTP id a640c23a62f3a-aa483525eb6mr1503522066b.44.1732024145325; Tue, 19 Nov 2024 05:49:05 -0800 (PST) X-Google-Smtp-Source: AGHT+IHubp22IM4a8AGXOxtZ1rt9SDupm6jnwp+c2nqJLiZljSmQ7XthfLCQ4ExKfNzZ+lftTRi1+9nzCg0OfjxKVlM= X-Received: by 2002:a17:907:2cc7:b0:aa4:8282:6601 with SMTP id a640c23a62f3a-aa483525eb6mr1503519666b.44.1732024144750; Tue, 19 Nov 2024 05:49:04 -0800 (PST) MIME-Version: 1.0 References: <20241114084727.37263-1-ellorent@redhat.com> <20241114181323.14fd3d36@elisabeth> <20241115122857.7bf2b8e2@elisabeth> <20241119010115.77f7fcba@elisabeth> In-Reply-To: <20241119010115.77f7fcba@elisabeth> From: Enrique Llorente Pastora Date: Tue, 19 Nov 2024 14:48:53 +0100 Message-ID: Subject: Re: [PATCH v2] dhcp, dhcpv6: Add hostname and client fqdn ops To: Stefano Brivio X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 5WCkC4nsvyuo4EXxmmNgm-WcBVE7KLjilZZTvEUJQGA_1732024146 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 55S7AH3APLYMJVNAL7Q23UV7X6H3LAIV X-Message-ID-Hash: 55S7AH3APLYMJVNAL7Q23UV7X6H3LAIV X-MailFrom: ellorent@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, Nov 19, 2024 at 1:01=E2=80=AFAM Stefano Brivio = wrote: > > Sorry for the delay. > > On Mon, 18 Nov 2024 13:20:03 +0100 > Enrique Llorente Pastora wrote: > > > On Fri, Nov 15, 2024 at 12:29=E2=80=AFPM Stefano Brivio wrote: > > > > > > On Fri, 15 Nov 2024 10:25:33 +0100 > > > Enrique Llorente Pastora wrote: > > > > > > > On Thu, Nov 14, 2024 at 6:13=E2=80=AFPM Stefano Brivio wrote: > > > > > > > > > > On Thu, 14 Nov 2024 09:47:27 +0100 > > > > > Enrique Llorente 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 expec= ted to > > > > > > have the VirtualMachine name as the guest hostname. > > > > > > > > > > > > This change add the -H --hostname to configure the DHCPv4 and D= HCPv6 > > > > > > options to will send hostname to clients. > > > > > > > > > > > > Signed-off-by: Enrique Llorente > > > > > > --- > > > > > > 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 f= orwarding\n" > > > > > > " --no-map-gw Don't map gateway address= to host\n" > > > > > > " -4, --ipv4-only Enable IPv4 operation onl= y\n" > > > > > > - " -6, --ipv6-only Enable IPv6 operation onl= y\n"); > > > > > > + " -6, --ipv6-only Enable IPv6 operation onl= y\n" > > > > > > + " -H, --hostname NAME Hostname to configure clie= nt 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 "sear= ch" > > > > > options. Two reasons: we have all the options without a short ver= sion > > > > > at the end, and this logically belongs together with other name-r= elated > > > > > stuff we send to the guest. > > > > > > > > > > > { 0 }, > > > > > > }; > > > > > > const char *logname =3D (c->mode =3D=3D MODE_PASTA) ? "pa= sta" : "passt"; > > > > > > @@ -1325,9 +1327,9 @@ void conf(struct ctx *c, int argc, char *= *argv) > > > > > > if (c->mode =3D=3D MODE_PASTA) { > > > > > > c->no_dhcp_dns =3D c->no_dhcp_dns_search =3D 1; > > > > > > fwd_default =3D FWD_AUTO; > > > > > > - optstring =3D "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:= 46t:u:T:U:"; > > > > > > + optstring =3D "+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 =3D "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:= 461t:u:"; > > > > > > + optstring =3D "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:= 461t:u:H:"; > > > > > > } > > > > > > > > > > > > c->tcp.fwd_in.mode =3D c->tcp.fwd_out.mode =3D FWD_UNSET; > > > > > > @@ -1680,6 +1682,11 @@ void conf(struct ctx *c, int argc, char = **argv) > > > > > > > > > > > > c->one_off =3D true; > > > > > > break; > > > > > > + case 'H': > > > > > > + ret =3D snprintf(c->hostname.n, sizeof(c-= >hostname.n), "%s", optarg); > > > > > > > > > > We (generally) wrap lines at 80 columns where doable: > > > > > > > > > > ret =3D snprintf(c->hostname.n, sizeof(c-= >hostname.n), > > > > > "%s", optarg); > > > > > > > > > > > + if (ret <=3D 0 || ret >=3D (int)sizeof(c-= >hostname.n)) > > > > > > + die("Invalid hostname: %s", optar= g); > > > > > > + 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 =3D 0, opt_len, opt_off =3D 0; > > > > > > + size_t mlen, dlen, offset =3D 0, opt_len, opt_off =3D 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 +=3D sizeof(uint32_t); > > > > > > } > > > > > > > > > > > > + hostname_len =3D strlen(c->hostname.n); > > > > > > + if ( hostname_len > 0 ) { > > > > > > > > > > No space around parentheses in expressions: > > > > > > > > > > if (hostname_len > 0) { > > > > > > > > > > > + opts[12].slen =3D 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 f= or 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) f= or > > > > > DHCP, which is equivalent to DHCPv6's option 39 (RFC 4704). Shoul= d we > > > > > just call this whole thing "FQDN" and switch to option 81 for DHC= P > > > > > 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 x= ml > > > > 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 netwo= rk > > > 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=3Dhost1.passt.net - v4,opt12 =3D host1 - v4,opt15 =3D passt.net - v6.opt39 =3D host1.passt.net --fqdn=3Dhost1.local - v4,opt12 =3D host1 - v6.opt39 =3D host1[.local] // not sure --fqdn=3Dhost1 [error] > > > - For IPv6 Option(39) with flags S and O <- so we can totally ignore th= e 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: > > abcd.example.comabcd > > 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 at that point. > Make sense. > Side note: with this item from the feature list on the website, at > https://passt.top/#services: > > =E2=8C=9A 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