network: DHCPv4 client- add support to send arbitary option and data

[ssahani]

Closes #5134

[keszybz]

So... I think this is needlessly complicated.
Instead of defining a separate section, what about the following:
DHCPOption=NNN: XYZXYZ
The syntax would be: a number, a colon, and a c-escaped string.
This would simplify parsing a lot, because it would be possible to have the whole option at once, without the need to define a separate section and whatnot.
This will be also much more concise, so nicer for the user.

If the same option is defined multiple times, that's OK. Just keep them in a list and attach in the order specified by the user. For some cases, the order may matter, and since it's simpler to keep the order, let's just do that.

This will allow the hashing and comparison functions and separate DHCPOption structure to be dropped.

[keszybz]

Oh, and just please say Closes #5134 since it does and this will allow github to know this.

[ssahani]

Oh, and just please say Closes #5134 since it does and this will allow github to know this.

done

So... I think this is needlessly complicated.
Instead of defining a separate section, what about the following:
DHCPOption=NNN: XYZXYZ
The syntax would be: a number, a colon, and a c-escaped string.
This would simplify parsing a lot, because it would be possible to have the whole option at once, without the need to define a separate section and whatnot.
This will be also much more concise, so nicer for the user.

If the same option is defined multiple times, that's OK. Just keep them in a list and attach in the order specified by the user. For some cases, the order may matter, and since it's simpler to keep the order, let's just do that.

This will allow the hashing and comparison functions and separate DHCPOption structure to be dropped.

Do we have such such parsing before ? I thought of that dropped as could not find.

[keszybz]

Do we have such such parsing before?

Pretty much any option which takes multiple values. SystemCallFilter= as good example.

[ssahani]

Do we have such such parsing before?

Pretty much any option which takes multiple values. SystemCallFilter= as good example.
yes then we can do that.

[keszybz]

https://tools.ietf.org/html/rfc2131#section-3.5 says:

Options may appear only once, unless
otherwise specified in the options document. The client concatenates
the values of multiple instances of the same option into a single
parameter list for configuration.

and https://tools.ietf.org/html/rfc2132#section-9 doesn't list any options which would be allowed multiple times. So yeah, it seems that you are right, and only allowing each given option once would be appropriate.

So... you may either use a OrderedHashmap to store the options (option number => sd_dhcp_option), or a array. If you use a hashmap, then the hashing function should only use the option number (trivial_hash_ops should work fine). If you use an array, then when inserting to the array, each new option should either replace the existing one with the same number or be appended at the end.

[ssahani]

Thanks for review @keszybz . Updated.

[keszybz]

Looks better, but there's still confusion as to if options are allowed multiple times. The sort order is also lost.

[flo-wer]

Are you sure that multiple option assignments should be allowed in one line?
I think it'd be simpler to always require just one option setting per line.

Of course you only want to consider the first one. The question is whether to allow one word on the rhs or multiple words.

I would consider one assignment per line because the main use-case is binary data as values. But I'm not sure what the best way to encode this is.

Was not the previous code doing that ? Now I am confused . : separation has a issue. How you send the IP addresses ? May be just consider the first one .

If I understand you correctly, you ask about how to use multiple IP addresses as the value of a single option. I think we do not have to consider this case, as no official options consists of the DHCP client sending IP addresses and if it is a proprietary extension, you would have to encode this as single bytes.

I would rather provide a more convenient way to encode a byte-string than to think of any possible encoding of IPs, strings, integers, etc.

[yuwata]

@keszybz and @ssahani I updated this PR. PTAL. I will try to add a test case.

@ssahani Your original version is saved at https://github.com/yuwata/systemd/commits/pr-13663-backup. If you do not like this. Please revert this and restore your original one.

[ssahani]

@yuwata Thanks for working on it. LGTM.

[yuwata]

Hmm, dnsmasq does not logs the sent data. @ssahani Do you have any idea to test this PR?

[ssahani]

Yes we can test it tcpdump . Please see https://src.fedoraproject.org/fork/ssahani/rpms/lldpad/c/451044af05e03da3ac08b154efbb8d0915e7bb73

We have to create a service

[Unit]
Description=TCPDumpd
After=multi-user.target network.target

[Service]
Type=simple
ExecStart=/usr/sbin/tcpdump -pnnli port 67 or port 68 -e -n  -vvv -w "/tmp/dhcp-tcp-dump.pcap"

[Install]
WantedBy=multi-user.target

def CaptureLLDPPackets(self):            
 def FindLLDPFieldsinTCPDump(self, **kwargs):            
"""Look attributes in lldpad logs."""            
  ontents = subprocess.check_output(['tcpdump', '-v', '-r', LLDPAD_TCP_DUMP_FILE]).rstrip().decode('utf-8')            
          if kwargs is not None:            
            for key in kwargs:            
                     self.assertRegex(contents, kwargs[key])               

[yuwata]

@ssahani Excellent! But that needs relatively huge work. Let's do that in another PR.

[keszybz]

Thanks, this version is much more consistent then the previous ones.

We only allow one setting for a given option number, and use the earliest value. Is this intentional?

[yuwata]

@keszybz Thank you for the review. I've force-pushed a revised version. I hope all your comments are addressed.

We only allow one setting for a given option number, and use the earliest value. Is this intentional?

Yes, based on your investigation #13663 (comment).

[keszybz]

@keszybz Thank you for the review. I've force-pushed a revised version. I hope all your comments are addressed.

Thanks, I'll look again tomorrow.

We only allow one setting for a given option number, and use the earliest value. Is this intentional?

Yes, based on your investigation #13663 (comment).

Yep, one setting is OK. But why take the earliest value? This goes against our usual override rules:

DHCPOption = foo:bar
DHCPOption = bar:foo
DHCPOption = foo:barbar

→ I'd expect "foo:barbar bar:foo" to be the options sent to the server.

[keszybz]

Almost there ;)

[keszybz]

OK, let's mereg.

[ssahani]

Thanks @yuwata and @keszybz .

[egberts]

Not there yet...

https://github.com/egberts/systemd-dhclient

Some of the limitation of systemd's DHCP client is that there is no DHCP-Options.

Hence the need to use ISC DHCP client despite pervasiveness of systemd's own built-in DHCP client.

I also like Dynamic DNS update which is those fancy communication between BIND9 DNS server and DHCP server. systemd-ddns is also nice but it didn't have other things (described in next section). Again, my gateway had too much invested into the usages of ISC BIND9 and ISC DHCP server.

systemd-dhcp-server also cannot do custom database interfaces.

systemd-dhcp-server cannot do dynamic hostname replacement regardless of MAC address.

systemd-dhcp-server cannot do failover and high-redundancy.

[q2dg]

Maybe these issues you say, @egberts, could be written as a Github issue each one, don't you think?

[egberts]

Maybe these issues you say, @egberts, could be written as a Github issue each one, don't you think?

It’s a huge undertaking whereas OID parser is needed. Much easier to fallback on good ol’ trusty ISC DHCP.