Page MenuHomePhabricator

Puppet types causing issues around nftables::service
Open, MediumPublic

Description

Currently the nftables::service source/destination parameters are defined like this:

# @param $src_ips
#       If neither $src_ips nor $src_sets are provided, all source addresses will be allowed.
#       Otherwise only traffic coming from the addresses in the parameter and/or $src_sets
# @param $dst_ips: Likewise, but with destination addresses
# @param src_sets see srange docs
# @param dst_sets see srange docs
define nftables::service (
# ...
    Array[Stdlib::IP::Address]    $src_ips    = [],
    Array[Stdlib::IP::Address]    $dst_ips    = [],
    Array[String[1]]              $src_sets   = [],
    Array[String[1]]              $dst_sets   = [],
# ...
) {}

This causes some problems, however. For example firewall::service has srange/drange defaulting to undef which it'll pass through to src_ips/dst_ips, relying (?) on Puppet converting the undef to the nftables resource default value at some point, however this doesn't work reliably with https://gerrit.wikimedia.org/r/c/operations/puppet/+/1211651.

Relying on an empty array to indicate "no filter" seems problematic in other regards as well: there are various examples in puppet.git relying on functions like wmflib::role::hosts to build the source range, and if the class name used as a selector gets outdated for some reason, the current logic will make the firewall open to the whole world which is not the intention of the code.

The obvious solution, both to resolve the empty array meaning ambiguity problem and to fix the type errors in the patch linked above, is to make the {src,dst}_{ips,sets} variables Optional and default to undef, and then change the code to treat an empty array as no rules rather than a rule open to the whole world.

I plan to submit patches doing that, but figured this is complex enough that I should at least document it on a task :-)

Related Objects

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change #1212097 had a related patch set uploaded (by Majavah; author: Majavah):

[operations/puppet@production] nftables::service: Improve src/dst filter handling

https://gerrit.wikimedia.org/r/1212097

jhathaway renamed this task from Typing issues around nftables::service to Puppet types causing issues around nftables::service.Dec 1 2025, 3:31 PM
jhathaway triaged this task as Medium priority.Dec 8 2025, 3:51 PM