(maint) Compare Environment#name in compiler

[justinstoller]

During catalog compilation, after loading the requested environment we load the node and server specified environment. Previously, We would then compare the Environment objects to determine if the requested environment matched the server specified environment. However, if the environment cache is flushed between those two environments being loaded they could be different object instances representing the same desired environment.

To resolve this race condition we now check the environment names (Environment#name canoncalizes to symbols to avoid symbol/string mismatch).

[justinstoller]

This is an attempt to resolve support request PE-36986 , a quick review would be appreciated but I want to wait until we hear back from the customer before merging.

[joshcooper]

However, if the environment cache is flushed between those two environments being loaded they could be different object instances representing the same desired environment.

In theory, the Puppet::Node::Environment class overrides ==, eql? and hash

puppet/lib/puppet/node/environment.rb

Lines 538 to 549 in e177633

	def ==(other)
	return true if other.kind_of?(Puppet::Node::Environment) &&
	self.name == other.name &&
	self.full_modulepath == other.full_modulepath &&
	self.manifest == other.manifest
	end
	alias eql? ==
	def hash
	[self.class, name, full_modulepath, manifest].hash
	end

So it should be possible to check if two environment objects have the same value. But the check depends on the manifest and full_modulepath. When the env cache is flushed and the environment is reloaded, does it have a different manifest or modulepath due to versioned code symlinks?

That said, I think comparing environment names is correct, since all we care about is whether the agent's requested environment matches the server-specified environment (based on classification rules).

Tangent 1: It seems strange that we're using kind_of? instead of instance_of? as the former means you can compare remote environment with a non-remote one.

irb(main):016:0> a = Puppet::Node::Environment.create(:production, [])
=> <Puppet::Node::Environment:442580 @name="production" @manifest="no_manifest" @modulepath="" >
irb(main):017:0> b = Puppet::Node::Environment.remote(:production)
=> <Puppet::Node::Environment::Remote:458740 @name="production" @manifest="no_manifest" @modulepath="" >
irb(main):018:0> a == b
=> true

We're probably relying on that in various places, but it seems like a footgun.

Tangent 2: In theory, cached environment names should always be symbols, but it wasn't always the case https://puppet.atlassian.net/browse/PUP-10955.

[justinstoller]

@joshcooper great callout about overriding ==, and when using versioned dirs, yes the manifest/module paths will be different. I assume we should not alter that implementation and in this case explicitly compare names. Do you think so? If so, I'll add some of your findings to the comments.

[joshcooper]

I assume we should not alter that implementation and in this case explicitly compare names

Yeah I'm pretty something is relying on being able to compare real Environment and fake Environment::Remote objects, probably in the configurer. So I'm 👍 to comparing names, since in this case we only care if the agent requested a different environment name than the server's node terminus said to use for compilation.

[justinstoller]

Heard back from @vchepkov and it looks like this is working so far. Even if it does turn out not to be the solution to his problem I think it is still a legit bug that should be fixed and doesn't appear to cause regressions. I've updated the comment and commit message based on Josh's comments. I think this is ready to go now.

[joshcooper]

LGTM, could you add/update a test around here so that different Environment objects (with the same name) are set on the node and request? I think it's most realistic to set a remote environment (created via Puppet::Node::Environment.remote(name)) on the request.

[justinstoller]

Thanks, Josh! I've added a test now. (fwiw, I've also changed the comparison back to checking instances and saw the test fail)

[vchepkov]

FYI, running this patch since October, 26th, issues with empty catalog hasn't reoccured since

[justinstoller]

I think this is waiting on an approval from @puppetlabs/phoenix , let me know if there's something else I can or should do!