Pass container hostname to netavark

[gtjoseph]

Passing the hostname allows netavark to include it in DHCP lease
requests which, in an environment where DDNS is used, can cause
DNS entries to be created automatically.

• The current Hostname() function in container.go was updated to
  check the new container_name_as_hostname option in the
  CONTAINERS table of containers.conf. If set and no hostname
  was configured for the container, it causes the hostname to be
  set to a version of the container's name with the characters not
  valid for a hostname removed. If not set (the default), the original
  behavior of setting the hostname to the short container ID is
  preserved.

• Because the Hostname() function can return the host's hostname
  if the container isn't running in a private UTS namespace, and we'd
  NEVER want to send that in a DHCP request for a container, a new
  function NetworkHostname() was added which functions like Hostname()
  except that it will return an empty string instead of the host's
  hostname if the container is not running in a private UTS namespace.

• networking_common.getNetworkOptions() now uses NetworkHostname()
  to set the ContainerHostname member of the NetworkOptions structure.
  That member was added to the structure in a corresponding commit in
  common/libnetwork/types/network.go.

• Added test to containers_conf_test.go

Signed-off-by: George Joseph g.devel@wxy78.net

NOTE:
Requires containers/common#2254
Required by containers/netavark#1130

[openshift-ci]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[gtjoseph]

The use_container_name_as_hostname addition might be a bit controversial. I have no problem with backing it out if needed.

[baude]

can you add a test?

[gtjoseph]

can you add a test?

Sure, if you can give me some guidance on how to do it. I looked at the existing networking tests but none seemed appropriate to use as a template. From a podman perspective, all this new feature does is set the new ContainerHostname field in common's libnetwork NetworkOptions. The only true way to test this is to start a container with a macvlan network and dhcp ipam driver, then check the contents of the DHCP Discover packet.

[Luap99]

can you add a test?

Sure, if you can give me some guidance on how to do it. I looked at the existing networking tests but none seemed appropriate to use as a template. From a podman perspective, all this new feature does is set the new ContainerHostname field in common's libnetwork NetworkOptions. The only true way to test this is to start a container with a macvlan network and dhcp ipam driver, then check the contents of the DHCP Discover packet.

We don't have any DHCP tests here, that should all be tested in netavark as we already have a working DHCP setup there

What you can and should test IMO is that your config field is working by setting that and then checking the hostname in the container (i.e. without any DHCP going on). That does not cover the code path passing the right thing to netavark but at least it should cover your basic logic here in podman

[gtjoseph]

What you can and should test IMO is that your config field is working by setting that and then checking the hostname in the container (i.e. without any DHCP going on). That does not cover the code path passing the right thing to netavark but at least it should cover your basic logic here in podman

Test created.

[openshift-ci]

[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 lsm5 for approval. For more information see the Kubernetes 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]

Force push to fix gofmt issues and rebase to current main.

[gtjoseph]

Holy Moly...checking for typos in comments. What a concept. :)
If CI is going to check that, shouldn't a local make check it, as well as other gofmt issues? That way we can catch them without needing to push and wait for CI.

--EDIT--
Duh. make validate

[gtjoseph]

I don't think the 4 failing tests are related to my change but if they are, let me know and I'll try and track them down. "testing-farm:fedora-rawhide-x86_64:cockpit-revdeps" at least looks like it has dependency issues when doing "dnf install".

[gtjoseph]

Rebased and, since the containers/common#2254 dependency was merged, the temporary DO NOT MERGE commit has been removed. Still waiting on netavark which is waiting on an upstream package PR.