(PUP-3239) Stop compilation if $environment has bad manifest settings

Before this commit we raised a warning if '$environment' was found
in the 'environmentpath' or 'basemodulepath' settings but continued
catalog compilation.

This commit adds in a validation method which raises an error before
compilation if an unsafe interpolation of '$environment' was found in
the 'environmentpath' or 'basemodulepath' settings thus stopping the
compilation process all together. This commit also consolidates the
'conflicting_manifest_settings?' method into the 'validate' method since
it also needs to halt compilation.

When new pre-compilation validations are added they can be added to the
new 'validate' method.

lib/puppet/node/environment.rb

@@ -258,6 +258,43 @@ def conflicting_manifest_settings?
     !original_manifest.nil? && !original_manifest.empty? && original_manifest != Puppet[:default_manifest]
   end
 
+  # Test to see if $environment is in the environmentpath or basemodulepath.
+  # If $environment is found in either of the settings then return false, otherwise
+  # return true.
+  # @api public
+  def unsafe_manifest_interpolation?
+    return true if /\$environment/ =~ Puppet.settings.value(:environmentpath, name, true)
+    return true if /\$environment/ =~ Puppet.settings.value(:basemodulepath, name, true)
+  end
+
+  # Validates a manifest for this environment. If the manifest cannot be
+  # validated for the environment then an error is raised. The possible errors
+  # that can be raised are
+  # @api public
+  def validate
+    if conflicting_manifest_settings?
+      errmsg = [
+        "The 'disable_per_environment_manifest' setting is true, and this '#{name}'",
+        "has an environment.conf manifest that conflicts with the 'default_manifest' setting.",
+        "Compilation has been halted in order to avoid running a catalog which may be using",
+        "unexpected manifests. For more information, see",
+        "http://docs.puppetlabs.com/puppet/latest/reference/environments.html",
+      ]
+      raise(Puppet::Error, errmsg.join(' '))
+    end
+
+    if unsafe_manifest_interpolation?
+      errmsg = [
+        "While using directory environments '$environment' may not be interpolated in the",
+        "'environmentpath' or 'basemodulepath' settings.",
+        "Compilation has been halted in order to avoid an environments final configuration",
+        "from being invalid after interpolation. For more information, see",
+        "https://docs.puppetlabs.com/puppet/latest/reference/environments.html",
+      ]
+      raise(Puppet::Error, errmsg.join(' '))
+    end
+  end
+
   # Return an environment-specific Puppet setting.
   #
   # @api public

lib/puppet/parser/compiler.rb

@@ -20,16 +20,7 @@ def self.compile(node)
     $env_module_directories = nil
     node.environment.check_for_reparse
 
-    if node.environment.conflicting_manifest_settings?
-      errmsg = [
-        "The 'disable_per_environment_manifest' setting is true, and this '#{node.environment}'",
-        "has an environment.conf manifest that conflicts with the 'default_manifest' setting.",
-        "Compilation has been halted in order to avoid running a catalog which may be using",
-        "unexpected manifests. For more information, see",
-        "http://docs.puppetlabs.com/puppet/latest/reference/environments.html",
-      ]
-      raise(Puppet::Error, errmsg.join(' '))
-    end
+    node.environment.validate
 
     new(node).compile.to_resource
   rescue => detail

spec/unit/parser/compiler_spec.rb

@@ -104,6 +104,13 @@ def resource(type, title)
     expect { Puppet::Parser::Compiler.compile(@node) }.to raise_error(Puppet::Error, /disable_per_environment_manifest.*true.*environment.conf.*manifest.*conflict/)
   end
 
+  it "fails if $environment is in environmentpath or basemodulepath" do
+    conflicted_environment = Puppet::Node::Environment.create(:testing, [], '/some/environment.conf/manifest.pp')
+    conflicted_environment.stubs(:unsafe_manifest_interpolation?).returns(true)
+    @node.environment = conflicted_environment
+    expect { Puppet::Parser::Compiler.compile(@node) }.to raise_error(Puppet::Error, /\$environment.*interpolated.*environmentpath.*basemodulepath/)
+  end
+
   it "should include the resource type collection helper" do
     Puppet::Parser::Compiler.ancestors.should be_include(Puppet::Resource::TypeCollectionHelper)
   end