8.7.0 complex regex node definitions don't seem to work

[jstange]

Describe the Bug

We have node definitions of the form node /^thing-(foo|bar)(?:-test)?-(\d+)/, the backreferences in which are known to trigger #9329 in Puppet 7. We thought we'd take a look at Puppet 8 and see if that behavior has changed. However it seems that this pattern doesn't work at all in Puppet 8:

ERROR [qtp437212910-1916] [puppetserver] Puppet Server Error: Cannot find definition Node on node thing-foo-0.example.com

(the pattern works as expected in Puppet 7, though concurrent runs of multiple nodes that match the pattern will often get their backreferences transposed)

This may the same issue as #9483. I've done some code spelunking that may be of use to someone, so be sure to read to the end before closing this as a duplicate (if indeed it is a duplicate).

Expected Behavior

Strings like "thing-foo-0" and "thing-bar-9" should match the node definition pattern /^thing-(foo|bar)(?:-test)?-(\d+)/.

Environment

• Puppet Server 8.7.0, Puppet Agent 8.10.0
• Alma Linux 8.10

Additional Context

I crawled into the codebase to work out what's going on. It seems to me like Resource::Type#match is intended to match the name of a node (e.g. thing-foo-0) against the regex pattern of a node definition. I could be wrong about this assumption, but if I am then I'm not sure what else this method would be for.

But what's being passed into it at least some of the time is, rather than a node's real name, a string representation of the node definition. It's trying to match the node definition pattern to itself, having munged into a string like __node_regexp__thing-foobarlpqu:-testrp-d. For simple patterns this may accidentally work, but anything complex will fail.

Walking the call stack it sure seems like #match is being used in the wrong context here, something to do with initializing/evaluating a resource type rather than the actual node matching operation. We go from:

1. Puppet::Parser::Compiler#evaluate_ast_node, the only place in this chain I see the literal node name, calls resource.ensure_in_catalog. Note that it has already successfully identified the node name as belonging to a resource type by this point (astnode = krt.node(name.to_s.downcase), where name is one of my "thing-foo" guys, worked)

2. Puppet::Resource::Type#ensure_in_catalog calls scope.catalog.resource, passing into it not the name of the node, but the stringified node definition that was already matched in evaluate_ast_node.

3. A few layers of indirection later we're in the class method Puppet::Resource::resource_type invoking environment.known_resource_types.node(title), where title is the regex node definition

4. After a few more layers of administrivia we end up in Puppet::Resource::TypeCollection#node, still being passed a string that is the regex node definition, and which is the thing that ultimately calls Resource::Type#match:

<...>
    @node_list.each do |n|
      next unless n.name_is_regex?
      return n if n.match(name)
    end

name here is of course the string literal __node_regexp__thing-foobarlpqu:-testrp-d, and will never match anything useful.

5. Bubbling back up to Puppet::Parser::Compiler#evaluate_ast_node we make the next call, to resource.evaluate, and it doesn't work because (I assume) we didn't match anything in steps 1-4.

[joshcooper]

@jstange could you try reverting this commit on your puppetserver, reload the server and see if it the problem goes away? Something like:

❯ curl -sLO https://github.com/puppetlabs/puppet/commit/47a8df6a227f5a5725089267cbd8dbe5de50939d.diff  
❯ patch --reverse -p1 -d /opt/puppetlabs/puppet --input ~/47a8df6a227f5a5725089267cbd8dbe5de50939d.diff
checking file lib/puppet/resource/type.rb
checking file spec/integration/parser/node_spec.rb
❯ sudo systemctl reload puppetserver

[jstange]

The paths in that patch don't align with what's shipped in the RPM (which is what I've been meddling with), so I applied the changes to resource/type.rb manually. The test doesn't appear to exist.

Reverting does correct the behavior, insofar as the node pattern matches again. It's passing __node_regexp__thing-foobar:-test-d around now in the internals.

What's interesting is that I no longer see that string representation of a regex passing through Resource::Type#match at all, just the normal calls with the raw hostname. I take that to mean that was only happening because something else in the call chain had tried and failed to identify the resource type (perhaps via an already-resolved node object) based on the normalized regexp string.

I'd put my money on Resource::TypeCollection#node, which tries to to just that:

    node = @nodes[name]
    if node
      return node
    end

This succeeds now, never falling through to the spurious .match call, and it looks like @nodes there is keyed by __node_regexp__thing-foobar:-test-d. So the ultimate fix is probably to align that thing's hash keys with the stuff happening in Resource::Type#name. I have no idea what else might depend on the particular formatting of that string, of course.