Skip to content

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Sep 26, 2025

- What I did

By default, dynamically linked dockerd builds now link against libnftables, instead of exec-ing the nft binary.

So, add build deps on libnftables-dev / nftables-devel for those builds ... apart from on RHEL.

RHEL's nftables-devel package is only available with a subscription from its CRB repo. For now, just set the Go build tag "no_libnftables" for these builds so they don't try to link against libnftables.

This is only needed for moby 29.x ... so, unfortunately, I think we'll need to branch this repo again for 28.x builds.

- Description for the changelog

- Dynamically linked dockerd now depends on package "nftables" and has a build dependency on the nftables dev package, apart from on RHEL.

@thaJeztah
Copy link
Member

This one probably also needs to be opened in the packaging repo;
https://github.com/docker/packaging/tree/main/pkg/docker-engine

And we need to check if this PR made its way there as well;

@thaJeztah
Copy link
Member

Looks like it fails to install on CentOS; does it perhaps require one of the optional, disabled by default repositories to be enabled?

---> Making bundle: dynbinary (in bundles/dynbinary)
Building dynamic bundles/dynbinary-daemon/dockerd (linux/arm64)...
# github.com/moby/moby/v2/daemon/libnetwork/internal/nftables
# [pkg-config --cflags  -- libnftables]
Package libnftables was not found in the pkg-config search path.
Perhaps you should add the directory containing `libnftables.pc'
to the PKG_CONFIG_PATH environment variable
Package 'libnftables', required by 'virtual:world', not found
error: Bad exit status from /var/tmp/rpm-tmp.68S0ik (%build)

@thaJeztah
Copy link
Member

OH! Nevermind; you only updated the RHEL variants, not the CentOS ones; so just updating those probably should fix it?

@robmry
Copy link
Contributor Author

robmry commented Oct 3, 2025

Looks like it fails to install on CentOS; does it perhaps require one of the optional, disabled by default repositories to be enabled?

Looking at it now ... it's the conditional check added to the rpm spec in this PR, I need to find a way to match RHEL and not CentOS.

@thaJeztah
Copy link
Member

oh! yes, I recall I ran into similar ones; they are .... finicky

@robmry
Copy link
Contributor Author

robmry commented Oct 3, 2025

OH! Nevermind; you only updated the RHEL variants, not the CentOS ones; so just updating those probably should fix it?

It builds ok with dynamic linking on CentOS, so the build-tag isn't needed in the CentOS Dockerfiles. It's only RHEL that wants a subscription for the nftables-devel package.

So, just a case of getting the conditionals right in the spec ... looks like %if %{undefined rhel} is false for CentOS (so rhel is defined).

@thaJeztah
Copy link
Member

@robmry these two diffs may help; they added / removed some conditionals also on distro-name IIRC;

By default, dynamically linked dockerd builds now link
against libnftables, instead of exec-ing the nft binary.

So, add build deps on libnftables-dev / nftables-devel
for those builds ... apart from on RHEL.

RHEL's nftables-devel package is only available with a
subscription from its CRB repo. For now, just set the Go
build tag "no_libnftables" for these builds so they don't
try to link against libnftables.

Signed-off-by: Rob Murray <rob.murray@docker.com>
@robmry
Copy link
Contributor Author

robmry commented Oct 3, 2025

I couldn't find a way to tell the difference between RHEL/CentOS in the RPM spec file, so @thaJeztah suggested using a --define the spec file can pick up.

That works fine for the rpmbuild step because the list of --define args is expanded in the Makefile. But, that rpmbuild happens in a Dockerfile that does dnf builddep, so it needs the --define too.

I couldn't work out a way to get the shell expansion right when passing the --define options into the Dockerfile via an env var. So, ended up just adding the --define to each of the RHEL Dockerfiles.

Not very satisfactory, but it works ... better ideas very welcome!

@thaJeztah
Copy link
Member

I couldn't work out a way to get the shell expansion right when passing the --define options into the Dockerfile via an env var. So, ended up just adding the --define to each of the RHEL Dockerfiles.

I think that's OK; maybe even better as it allows comparing the Dockerfiles and see where they differ.

Overall, this repository is on its way out with things having moved to docker/packaging, so seems like a good solution for this repository at least (I haven't looked at docker/packaging to see how it has to be done / can be done there)

Comment on lines 29 to +30
iptables,
nftables,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we should make the change to make iptables a weak dependency (or no dependency at all), and only nftables a required dependency.

Given that it's still experimental, we could make nftables a weak dependency, but maybe that's little gain, as it will become a hard requirement in the near future (I expect).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on Slack - let's just make it a hard dependency now. It'll already be installed in most places anyway, it'll be required when we deprecate iptables support, and it's needed to try out the experimental nftables support.

I'll make the update.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

left some comment, in case we do want to start with "weak" dependency, but probably a bit too pedantic (even for me 😂)

@thaJeztah
Copy link
Member

I'll bring this one in; we discussed on slack that most users would already have nftables installed either way, so adding it as required dependency should likely be a "no-op" and not disruptive.

@thaJeztah thaJeztah merged commit 3b242be into docker:master Oct 3, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants