Add container hostname to DHCP requests and use container id as client id #1130

gtjoseph · 2024-11-25T17:17:46Z

Currently, since a container's hostname isn't sent to netavark, we can't
include it in DHCP requests when using a macvlan network and DHCP IPAM
driver. Therefore, if DNS records need to be added for a container, it needs
to be done by external scripts. Another issue is that since every time a
container starts it gets a new random mac address and since, in the absense
of a client id, the mac address is used by DHCP servers to keep track of
leases, it's likely that the container will get a new ip address on restart.
If the container is providing an exposed service, even just ssh, clients may
have the old address cached and may not be able to reach the container.

This commit accepts the container's hostname on incoming JSON setup commands
and adds it to the DHCP client configuration so an Option 12
(Hostname) parameter can be sent in DHCP messages. This should allow DDNS
updates of DNS resords. This is dependent on corresponding commits in Podman
and common to actually pass the host name of course. This commit also sets
the Option 61 (Client identifier) to the container's ID. Since this stays
constant across container restarts, it's likely the DHCP server will use the
existing lease and return the same IP address.

Added "container_hostname" to network::types::NetworkOptions. The setup
command will set this from the incoming JSON and pass it through ultimately
to dhcp_service::DhcpV4Service via macvlan_dhcp.get_dhcp_lease().
Added "container_id" to proxy.proto so it can also be passed to
dhcp_service::DhcpV4Service and used to set the client id.
Added tests to test-dhcp/002-setup.bats to check that hostname is passed
correctly.

Fixes: #676
Signed-off-by: George Joseph [email protected]

The new functionality requires the following PRs but netavark will build and run successfully with existing functionality without them.
containers/podman#24675
containers/common#2254

This PR DOES however require the following mozim PR to set the client id:
nispor/mozim#39

gtjoseph · 2024-11-25T17:21:03Z

With this PR, netvark will still build and run fine with older containers/common code that doesn't set the container_hostname. It just won't send a hostname in the DHCP request.

mheon · 2024-11-25T18:20:38Z

Sure, LGTM

src/dhcp_proxy/dhcp_service.rs

src/network/types.rs

Luap99

Also we should have a test for it. Currently we only have some basic dhcp tests in test-dhcp/ but we need to cover both case at least (with and without hostname)

The basic setup test sets "host_name": "foobar" so we should verify the hostname was passed. We should be able to read the dnsmasq log assuming it logs the hostname.

But then we also should have a second case with an empty hostname so we know we keep no hostname working as well.

src/network/mod.rs

src/dhcp_proxy/dhcp_service.rs

src/commands/teardown.rs

gtjoseph · 2024-11-26T20:49:23Z

I'm marking this as draft until I can think of a better way to handle client id.

gtjoseph · 2024-11-27T01:06:11Z

HA! I just discovered a bug in mozim. If you don't set use_host_name_as_client_id(), it doesn't send the host name at all. I'll be submitting a PR to them to fix that and add a set_client_id() method.

src/proto/proxy.proto

src/dhcp_proxy/dhcp_service.rs

gtjoseph · 2024-11-27T18:30:30Z

I'm setting this back to draft. I've submitted PR nispor/mozim#39 to mozim to fix the hostname issue and add a set_client_id() method so I'd like to see that get merged first. This way we don't need the workaround for setting the client id.

gtjoseph · 2024-11-29T17:11:23Z

The last force push just updated the commit message. No code changes.

gtjoseph · 2024-11-30T18:16:27Z

@Luap99 When I run make validate locally I get tons of the following errors. Any idea on how to get validate to work?

error: the `-Z unstable-options` flag must also be passed to enable the flag `check-cfg`
error: could not compile `unicode-ident` (lib)
error: the `-Z unstable-options` flag must also be passed to enable the flag `check-cfg`
error: could not compile `autocfg` (lib)

Fedora 41 with...

cargo.x86_64                        1.82.0-1.fc41 updates
cargo-rpm-macros.noarch             26.3-3.fc41   fedora
cargo2rpm.noarch                    0.1.18-1.fc41 fedora
rust.x86_64                         1.82.0-1.fc41 updates
rust-analyzer.x86_64                1.82.0-1.fc41 updates
rust-debugger-common.noarch         1.82.0-1.fc41 updates
rust-doc.x86_64                     1.82.0-1.fc41 updates
rust-gdb.noarch                     1.82.0-1.fc41 updates
rust-lldb.noarch                    1.82.0-1.fc41 updates
rust-rpm-sequoia-debugsource.x86_64 1.6.0-1.fc39  <unknown>
rust-src.noarch                     1.82.0-1.fc41 updates
rust-srpm-macros.noarch             26.3-3.fc41   <unknown>
rust-std-static.x86_64              1.82.0-1.fc41 updates
rust2rpm.noarch                     26.2.0-1.fc41 updates
rust2rpm-helper.x86_64              0.1.6-1.fc41  updates
rustfmt.x86_64                      1.82.0-1.fc41 updates

Luap99 · 2024-12-02T11:14:42Z

@Luap99 When I run make validate locally I get tons of the following errors. Any idea on how to get validate to work?

error: the `-Z unstable-options` flag must also be passed to enable the flag `check-cfg`
error: could not compile `unicode-ident` (lib)
error: the `-Z unstable-options` flag must also be passed to enable the flag `check-cfg`
error: could not compile `autocfg` (lib)

Fedora 41 with...

cargo.x86_64                        1.82.0-1.fc41 updates
cargo-rpm-macros.noarch             26.3-3.fc41   fedora
cargo2rpm.noarch                    0.1.18-1.fc41 fedora
rust.x86_64                         1.82.0-1.fc41 updates
rust-analyzer.x86_64                1.82.0-1.fc41 updates
rust-debugger-common.noarch         1.82.0-1.fc41 updates
rust-doc.x86_64                     1.82.0-1.fc41 updates
rust-gdb.noarch                     1.82.0-1.fc41 updates
rust-lldb.noarch                    1.82.0-1.fc41 updates
rust-rpm-sequoia-debugsource.x86_64 1.6.0-1.fc39  <unknown>
rust-src.noarch                     1.82.0-1.fc41 updates
rust-srpm-macros.noarch             26.3-3.fc41   <unknown>
rust-std-static.x86_64              1.82.0-1.fc41 updates
rust2rpm.noarch                     26.2.0-1.fc41 updates
rust2rpm-helper.x86_64              0.1.6-1.fc41  updates
rustfmt.x86_64                      1.82.0-1.fc41 updates

I have not seen anything like this before. Is this only validate? The actual build works fine?

gtjoseph · 2024-12-02T12:36:00Z

I have not seen anything like this before. Is this only validate? The actual build works fine?

Yeah, every other make target works fine. Ah well, no worries. I'll poke at it a bit.

gtjoseph · 2024-12-02T20:25:34Z

@Luap99 Just FYI... I had to switch to the nightly rust toolchain to get make validate to work. However, the 2024-11-26 nightly found issues in netavark stuff I didn't touch which may indicate that if the CI toolchain is updated it may catch issues it didn't before. Just a heads up.

rustc 1.83.0 (90b35a623 2024-11-26)
cargo 1.83.0 (5ffbef321 2024-10-29)

error: elided lifetime has a name
   --> src/network/bridge.rs:337:40
    |
330 | impl<'a> Bridge<'a> {
    |      -- lifetime `'a` declared here
...
337 |     ) -> NetavarkResult<(SetupNetwork, PortForwardConfig)> {
    |                                        ^^^^^^^^^^^^^^^^^ this elided lifetime gets resolved as `'a`
    |
    = note: `-D elided-named-lifetimes` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(elided_named_lifetimes)]`

error: empty line after doc comment
  --> src/firewall/state.rs:20:1
   |
20 | / ///                 - ports/$netID_$conID -> port config
21 | |
   | |_
22 |   const FIREWALL_DIR: &str = "firewall";
   |   ------------------------ the comment documents this constant
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#empty_line_after_doc_comments
   = note: `-D clippy::empty-line-after-doc-comments` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::empty_line_after_doc_comments)]`
   = help: if the empty line is unintentional remove it

error: the following explicit lifetimes could be elided: 'a
  --> src/firewall/varktables/types.rs:85:6
   |
85 | impl<'a> VarkChain<'a> {
   |      ^^            ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
   = note: `-D clippy::needless-lifetimes` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::needless_lifetimes)]`
help: elide the lifetimes
   |
85 - impl<'a> VarkChain<'a> {
85 + impl VarkChain<'_> {
   |

error: could not compile `netavark` (lib) due to 3 previous errors

Luap99 · 2024-12-03T10:34:00Z

New lint fixes for rust v1.83 are already in #1134

gtjoseph · 2024-12-05T15:35:04Z

@Luap99 Do you guys have a policy on how to address upstream projects (nispor/mozim in this case) that aren't responsive? Think I should go back to my workaround for setting client id? It was...

config.set_host_name(container_id);
config.use_host_name_as_client_id();
config.set_host_name(host_name);

Luap99 · 2024-12-12T18:27:23Z

@Luap99 Do you guys have a policy on how to address upstream projects (nispor/mozim in this case) that aren't responsive? Think I should go back to my workaround for setting client id? It was...
config.set_host_name(container_id);
config.use_host_name_as_client_id();
config.set_host_name(host_name);

We have to see, for now we all will be on PTO over the holidays and we are most likely aiming for our next release end of January/start of February. So until then there is plenty of time for him to react and merge it.

We don't have any rules about it, often the answer is wait and hope. However in this case the maintainer is also working for Red Hat and we have been working with him in the past. I will send him an email and ask him to have a look. Maybe he just missed the notification.

So for now just wait and we can see in January if we need to use the work around.

gtjoseph · 2024-12-13T13:22:03Z

Sounds good, thanks.

Luap99 · 2024-12-14T17:26:20Z

I merged #1145 which includes your fixes

…t id Currently, since a container's hostname isn't sent to netavark, we can't include it in DHCP requests when using a macvlan network and DHCP IPAM driver. Therefore, if DNS records need to be added for a container, it needs to be done by external scripts. Another issue is that since every time a container starts it gets a new random mac address and since, in the absense of a client id, the mac address is used by DHCP servers to keep track of leases, it's likely that the container will get a new ip address on restart. If the container is providing an exposed service, even just ssh, clients may have the old address cached and may not be able to reach the container. This commit accepts the container's hostname on incoming JSON setup commands and adds it to the DHCP client configuration so an Option 12 (Hostname) parameter can be sent in DHCP messages. This should allow DDNS updates of DNS resords. This is dependent on corresponding commits in Podman and common to actually pass the host name of course. This commit also sets the Option 61 (Client identifier) to the container's ID. Since this stays constant across container restarts, it's likely the DHCP server will use the existing lease and return the same IP address. * Added "container_hostname" to network::types::NetworkOptions. The setup command will set this from the incoming JSON and pass it through ultimately to dhcp_service::DhcpV4Service via macvlan_dhcp.get_dhcp_lease(). * Added "container_id" to proxy.proto so it can also be passed to dhcp_service::DhcpV4Service and used to set the client id. * Added tests to test-dhcp/002-setup.bats to check that hostname is passed correctly. Fixes: containers#676 Signed-off-by: George Joseph <[email protected]>

openshift-ci · 2024-12-14T20:27:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gtjoseph
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gtjoseph · 2024-12-14T20:36:05Z

Thanks for the assist @Luap99 !
The containers/common#2254 PR has already merged so I've unDRAFTed this PR as it should be good to go. I guess I'll wait for this PR to merge before unDRAFTing the containers/podman#24675 PR.

gtjoseph force-pushed the main-dhcp-hostname branch from 5094f03 to 17f64da Compare November 25, 2024 17:48

Luap99 reviewed Nov 25, 2024

View reviewed changes

src/dhcp_proxy/dhcp_service.rs Outdated Show resolved Hide resolved

src/network/types.rs Outdated Show resolved Hide resolved

gtjoseph force-pushed the main-dhcp-hostname branch 2 times, most recently from db2be94 to 8eb5569 Compare November 25, 2024 21:53

Luap99 reviewed Nov 26, 2024

View reviewed changes

src/network/mod.rs Outdated Show resolved Hide resolved

src/dhcp_proxy/dhcp_service.rs Outdated Show resolved Hide resolved

src/commands/teardown.rs Outdated Show resolved Hide resolved

SilverBut mentioned this pull request Nov 26, 2024

DHCP Proxy Does Not Report Option 12 (Hostname) #676

Open

gtjoseph force-pushed the main-dhcp-hostname branch from 8eb5569 to 8706001 Compare November 26, 2024 20:45

gtjoseph marked this pull request as draft November 26, 2024 20:49

openshift-ci bot added the do-not-merge/work-in-progress label Nov 26, 2024

gtjoseph force-pushed the main-dhcp-hostname branch from 8706001 to bbc316c Compare November 26, 2024 22:17

gtjoseph marked this pull request as ready for review November 26, 2024 22:20

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 26, 2024

gtjoseph requested a review from Luap99 November 26, 2024 22:21

Luap99 reviewed Nov 27, 2024

View reviewed changes

src/proto/proxy.proto Show resolved Hide resolved

Luap99 reviewed Nov 27, 2024

View reviewed changes

src/dhcp_proxy/dhcp_service.rs Outdated Show resolved Hide resolved

gtjoseph marked this pull request as draft November 27, 2024 18:27

openshift-ci bot added the do-not-merge/work-in-progress label Nov 27, 2024

This was referenced Nov 27, 2024

Fix various issues with hostname and client id nispor/mozim#39

Merged

Pass container hostname to netavark containers/podman#24675

Draft

Allow Podman to send the container's hostname to Netavark containers/common#2254

Merged

gtjoseph force-pushed the main-dhcp-hostname branch from bbc316c to bf8ec12 Compare November 29, 2024 17:07

gtjoseph changed the title ~~Add container hostname to DHCP requests~~ Add container hostname to DHCP requests and use container id as client id Nov 29, 2024

gtjoseph requested a review from Luap99 November 29, 2024 17:11

gtjoseph force-pushed the main-dhcp-hostname branch from bf8ec12 to ef26b55 Compare November 30, 2024 15:46

gtjoseph force-pushed the main-dhcp-hostname branch 2 times, most recently from 4c89534 to d75fd36 Compare November 30, 2024 16:38

gtjoseph force-pushed the main-dhcp-hostname branch from d75fd36 to 2bf9cfd Compare December 14, 2024 20:27

gtjoseph marked this pull request as ready for review December 14, 2024 20:27

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container hostname to DHCP requests and use container id as client id #1130

Add container hostname to DHCP requests and use container id as client id #1130

gtjoseph commented Nov 25, 2024 •

edited

Loading

gtjoseph commented Nov 25, 2024

mheon commented Nov 25, 2024

Luap99 left a comment

gtjoseph commented Nov 26, 2024

gtjoseph commented Nov 27, 2024

gtjoseph commented Nov 27, 2024

gtjoseph commented Nov 29, 2024

gtjoseph commented Nov 30, 2024

Luap99 commented Dec 2, 2024

gtjoseph commented Dec 2, 2024

gtjoseph commented Dec 2, 2024

Luap99 commented Dec 3, 2024

gtjoseph commented Dec 5, 2024

Luap99 commented Dec 12, 2024

gtjoseph commented Dec 13, 2024

Luap99 commented Dec 14, 2024

openshift-ci bot commented Dec 14, 2024

gtjoseph commented Dec 14, 2024

Add container hostname to DHCP requests and use container id as client id #1130

Are you sure you want to change the base?

Add container hostname to DHCP requests and use container id as client id #1130

Conversation

gtjoseph commented Nov 25, 2024 • edited Loading

gtjoseph commented Nov 25, 2024

mheon commented Nov 25, 2024

Luap99 left a comment

Choose a reason for hiding this comment

gtjoseph commented Nov 26, 2024

gtjoseph commented Nov 27, 2024

gtjoseph commented Nov 27, 2024

gtjoseph commented Nov 29, 2024

gtjoseph commented Nov 30, 2024

Luap99 commented Dec 2, 2024

gtjoseph commented Dec 2, 2024

gtjoseph commented Dec 2, 2024

Luap99 commented Dec 3, 2024

gtjoseph commented Dec 5, 2024

Luap99 commented Dec 12, 2024

gtjoseph commented Dec 13, 2024

Luap99 commented Dec 14, 2024

openshift-ci bot commented Dec 14, 2024

gtjoseph commented Dec 14, 2024

gtjoseph commented Nov 25, 2024 •

edited

Loading