(PUP-4242) Add check for manifests declaring things in the wrong namespace.

Author: kris-bosland

lib/puppet/pops/issues.rb

@@ -486,6 +486,10 @@ def self.hard_issue(issue_code, *args, &block)
     _("Unacceptable name. The name '%{name}' is unacceptable as the name of %{value}") % { name: name, value: label.a_an(semantic) }
   end
 
+  ILLEGAL_DEFINITION_LOCATION = issue :ILLEGAL_DEFINITION_LOCATION, :name, :file do
+    _("Unacceptable location. The name '%{name}' is unacceptable in file '%{file}'") % { name: name, file: file }
+  end
+
   CAPTURES_REST_NOT_LAST = hard_issue :CAPTURES_REST_NOT_LAST, :param_name do
     _("Parameter $%{param} is not last, and has 'captures rest'") % { param: param_name }
   end

lib/puppet/pops/validation/checker4_0.rb

@@ -35,6 +35,7 @@ def initialize(diagnostics_producer)
 
     @check_visitor = self.class.check_visitor
     @acceptor = diagnostics_producer
+    @file_to_namespace = {}
 
     # Use null migration checker unless given in context
     @migration_checker = (Puppet.lookup(:migration_checker) { Migration::MigrationChecker.new() })
@@ -395,6 +396,8 @@ def check_NamedDefinition(o)
     if o.name !~ Patterns::CLASSREF_DECL
       acceptor.accept(Issues::ILLEGAL_DEFINITION_NAME, o, {:name=>o.name})
     end
+
+    internal_check_file_namespace(o, o.name, o.locator.file)
     internal_check_reserved_type_name(o, o.name)
     internal_check_future_reserved_word(o, o.name)
   end
@@ -530,6 +533,75 @@ def internal_check_future_reserved_word(o, name)
     end
   end
 
+  def internal_check_file_namespace(o, name, file)
+    return if file.nil?
+
+    lc_file = file.downcase
+
+    file_namespace = @file_to_namespace[lc_file]
+    if file_namespace.nil?
+      return if @file_to_namespace.key?(lc_file)
+      file_namespace = @file_to_namespace[lc_file] = namespace_for_file(lc_file)
+      return if file_namespace.nil?
+    end
+
+    if !name.downcase.start_with?(file_namespace)
+      acceptor.accept(Issues::ILLEGAL_DEFINITION_LOCATION, o, {:name => name, :file => file})
+    end
+  end
+
+  NEVER_MATCH = '\b'.freeze
+
+  def namespace_for_file(file)
+    path = Pathname.new(file)
+
+    return nil if path.extname != ".pp"
+
+    return nil if initial_manifest?(path)
+
+    path = path.each_filename.to_a
+
+    # Example definition dir: manifests in this path:
+    # <modules dir>/<module name>/manifests/<module subdir>/<classfile>.pp
+    definition_dir_index = find_module_definition_dir(path)
+
+    # How can we get this result?
+    # If it is not an initial manifest, it must come from a module,
+    # and from the manifests dir there.  This may never get used...
+    return NEVER_MATCH if definition_dir_index.nil? || definition_dir_index == 0
+
+    before_definition_dir = definition_dir_index - 1
+    after_definition_dir = definition_dir_index + 1
+    names = path[after_definition_dir .. -2] # Directories inside module
+    names.unshift(path[before_definition_dir]) # Name of the module itself
+    # Do not include name of module init file at top level of module
+    filename = path[-1]
+    if !(path.length == (after_definition_dir+1) && filename == 'init.pp')
+      names.push(filename[0 .. -4]) # Remove .pp from filename
+    end
+
+    names.join("::").freeze
+  end
+
+  def initial_manifest?(path)
+    manifest_setting = Puppet[:manifest]
+
+    # Does this ever happen outside of tests?
+    if manifest_setting.nil?
+      return path.basename.to_s == 'site.pp'
+    end
+
+    full_path = path.expand_path.to_s
+    full_path == manifest_setting || full_path.start_with?(manifest_setting)
+  end
+
+  # Returns module root directory index in path for files under 'manifests', 'functions' and 'types'
+  def find_module_definition_dir(path)
+    reverse_index = path.reverse_each.find_index do |dir|
+      dir == 'manifests' || dir == 'functions' || dir == 'types'
+    end
+    return reverse_index.nil? ? nil : (path.length - 1) - reverse_index
+  end
 
   RESERVED_PARAMETERS = {
     'name' => true,

lib/puppet/pops/validation/validator_factory_4_0.rb

@@ -27,15 +27,16 @@ def severity_producer
     # Configure each issue that should **not** be an error
     #
     # Validate as per the current runtime configuration
-    p[Issues::RT_NO_STORECONFIGS_EXPORT]    = Puppet[:storeconfigs] ? :ignore : :warning
-    p[Issues::RT_NO_STORECONFIGS]           = Puppet[:storeconfigs] ? :ignore : :warning
+    p[Issues::RT_NO_STORECONFIGS_EXPORT]     = Puppet[:storeconfigs] ? :ignore : :warning
+    p[Issues::RT_NO_STORECONFIGS]            = Puppet[:storeconfigs] ? :ignore : :warning
 
     p[Issues::FUTURE_RESERVED_WORD]          = :deprecation
 
     p[Issues::DUPLICATE_KEY]                 = Puppet[:strict] == :off ? :ignore : Puppet[:strict]
     p[Issues::NAME_WITH_HYPHEN]              = :error
     p[Issues::EMPTY_RESOURCE_SPECIALIZATION] = :ignore
-    p[Issues::CLASS_NOT_VIRTUALIZABLE]      = Puppet[:strict] == :off ? :warning : Puppet[:strict]
+    p[Issues::CLASS_NOT_VIRTUALIZABLE]       = Puppet[:strict] == :off ? :warning : Puppet[:strict]
+    p[Issues::ILLEGAL_DEFINITION_LOCATION]   = Puppet[:strict] == :off ? :ignore : Puppet[:strict]
     p
   end
 end

spec/unit/pops/validator/validator_spec.rb

@@ -34,6 +34,47 @@ def validate(factory)
     expect(validate(parse('function ::aaa() {}'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_NAME)
   end
 
+  it 'should raise error for illegal class locations' do
+    expect(validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('class bbb() {}', 'aaa/manifests/init.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('define aaa::bbb::ccc::eee() {}', 'aaa/manifests/bbb/ddd.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should not raise error for legal definition locations' do
+    expect(validate(parse('function aaa::bbb() {}',      'aaa/manifests/bbb.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('define bbb() {}',      'aaa/manifests/site.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('class aaa() {}',      'aaa/manifests/init.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('class aaa() {}',      'manifests/site.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('function aaa::bbB::ccc() {}', 'aaa/manifests/bBb.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('function aaa::bbb::ccc() {}', 'aaa/manifests/bbb/CCC.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should not raise error for class locations when not parsing a file' do
+    #nil file means eval or some other way to get puppet language source code into the catalog
+    expect(validate(parse('function aaa::ccc() {}', nil))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should not raise error for definitions inside initial --manifest' do
+    Puppet[:manifest] = 'a/manifest/file.pp'
+    expect(validate(parse('class aaa() {}', 'a/manifest/file.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should not raise error for definitions inside initial --manifest' do
+    Puppet[:manifest] = 'a/manifest/dir'
+    expect(validate(parse('class aaa() {}', 'a/manifest/dir/file1.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    expect(validate(parse('class bbb::ccc::ddd() {}', 'a/manifest/dir/and/more/stuff.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should raise error for definitions not inside initial --manifest' do
+    Puppet[:manifest] = 'a/manifest/somewhere/else'
+    expect(validate(parse('class aaa() {}', 'a/manifest/dir/file1.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+  end
+
+  it 'should raise error if the manifest file does not come from a well formed module path' do
+    skip("This test path won't work on windows.") if Puppet::Util::Platform.windows?
+    expect(validate(parse('class aaa() {}', '/manifest/dir/file1.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)    
+  end
+
   it 'should raise error for illegal type names' do
     expect(validate(parse('type ::Aaa = Any'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_NAME)
   end
@@ -56,6 +97,13 @@ def validate(factory)
       expect(acceptor.error_count).to eql(0)
       expect(acceptor).to have_issue(Puppet::Pops::Issues::DUPLICATE_KEY)
     end
+
+    it 'produces a warning for illegal function locations' do
+      acceptor = validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))
+      expect(acceptor.warning_count).to eql(1)
+      expect(acceptor.error_count).to eql(0)
+      expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
   end
 
   context 'with --strict set to warning' do
@@ -80,6 +128,13 @@ def validate(factory)
       expect(acceptor.error_count).to eql(0)
       expect(acceptor).to have_issue(Puppet::Pops::Issues::CLASS_NOT_VIRTUALIZABLE)
     end
+
+    it 'produces a warning for illegal function locations' do
+      acceptor = validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))
+      expect(acceptor.warning_count).to eql(1)
+      expect(acceptor.error_count).to eql(0)
+      expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
   end
 
   context 'with --strict set to error' do
@@ -111,6 +166,13 @@ def validate(factory)
       expect(acceptor.error_count).to eql(1)
       expect(acceptor).to have_issue(Puppet::Pops::Issues::CLASS_NOT_VIRTUALIZABLE)
     end
+
+    it 'produces an error for illegal function locations' do
+      acceptor = validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))
+      expect(acceptor.warning_count).to eql(0)
+      expect(acceptor.error_count).to eql(1)
+      expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
   end
 
   context 'with --strict set to off' do
@@ -121,6 +183,13 @@ def validate(factory)
       expect(acceptor.error_count).to eql(0)
       expect(acceptor).to_not have_issue(Puppet::Pops::Issues::DUPLICATE_KEY)
     end
+
+    it 'does not produce an error or warning for illegal function locations' do
+      acceptor = validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))
+      expect(acceptor.warning_count).to eql(0)
+      expect(acceptor.error_count).to eql(0)
+      expect(acceptor).to_not have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
   end
 
   context 'irrespective of --strict' do
@@ -145,7 +214,7 @@ def validate(factory)
       expect(acceptor).to have_issue(Puppet::Pops::Issues::CLASS_NOT_VIRTUALIZABLE)
     end
 
-    it 'produces a  warning for exported class resource' do
+    it 'produces a warning for exported class resource' do
       acceptor = validate(parse('@@class { test: }'))
       expect(acceptor.warning_count).to eql(1)
       expect(acceptor.error_count).to eql(0)
@@ -662,7 +731,7 @@ def issue(at_top)
     end
   end
 
-  def parse(source)
-    Puppet::Pops::Parser::Parser.new.parse_string(source)
+  def parse(source, path=nil)
+    Puppet::Pops::Parser::Parser.new.parse_string(source, path)
   end
 end