Re: [PATCH ulogd2] nfct: add network namespace support

[Corubba Smith <corubba@gmx.de>]

On 3/12/25 09:37, Florian Westphal wrote:
> Corubba Smith <corubba@gmx.de> wrote:
>> Add a new option which allows opening the netlink socket in a different
>> network namespace. This way you can run ulogd in one (management)
>> network namespace which is able to talk with your export target (e.g.
>> database or IPFIX collector), and import flows from multiple (customer)
>> network namespaces.
>
> Makes sense to me.
>
>> This commit only implements it for NFCT. I wanted to gather some
>> feedback before also implementing it for the other netlink-based
>> plugins.
>
> Does it make sense to have this configured on a per-plugin basis?

I honestly don't see the usecase for it. Enabling namespace support
depends on whether the used libc supports the required syscall, which is
the same for all plugins. But even if namespace support is compiled in
globally, you still need to activate it per plugin instance by setting
the namespace path config option. That option is not mandatory, and if
not set the plugin will behave the same as before, opening the netlink
socket in the current network namespace. So compiling in namespace
support alone does not change the runtime behaviour (or actually use the
syscall), it only does when you also explicitly use the namespace path
option on a plugin instance.

>
>>    Input plugins:
>>      NFLOG plugin:			${enable_nflog}
>>      NFCT plugin:			${enable_nfct}
>
>> +#ifdef NETNS_SUPPORT
>> +	if (strlen(target_netns_path) > 0) {
>> +		errno = 0;
>> +		original_netns_fd = open("/proc/self/ns/net", O_RDONLY | O_CLOEXEC);
>> +		if (original_netns_fd < 0) {
>> +			ulogd_log(ULOGD_FATAL, "error opening original network namespace: %s\n", strerror(errno));
>> +			goto err_ons;
>> +		}
>
> I think that in order to not have copypastry in all relevant plugins
> it would be better to turn code in the NETNS_SUPPORT ifdefs section
> into library helpers.
>
> The helpers would always exist; in case ulogd2 is built without
> support they would raise an error.
>
> That would also keep the ifdef out of plugin code.

Very good point, will do that. Thanks!

--
Corubba