[PUP-7402] handle trailing slashes in references between file resources Created: 2017/03/28  Updated: 2017/06/28  Resolved: 2017/04/24

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: PUP 5.0.0
Fix Version/s: PUP 5.0.0

Type: Bug Priority: Normal
Reporter: Wyatt Alt Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: resolved-issue-added
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-6770 Method #uniqueness_key computes diffe... Accepted
relates to PUP-1796 File resource fails for root(/) direc... Closed
relates to PUP-6033 Recursive file resource in static cat... Closed
relates to PUP-6752 regression: Code that doesn't compile... Closed
Template:
Team: Puppet Developer Experience
Story Points: 3
Sprint: PDE 2017-04-05, PDE 2017-04-19
Release Notes: Bug Fix
Release Notes Summary: Certain combination of references to File resources where title and reference were not the same with respect to use of a trailing '/' could cause a reference to not be resolved and resulting in an error. This is now fixed.
QA Risk Assessment: No Action
QA Risk Assessment Reason: covered by unit tests; existing acceptance

 Description   

This change:
https://github.com/puppetlabs/puppet/commit/3577fc913373f05b6c58fd7389e097ba7271c147/lib/puppet/parser/compiler/catalog_validator/relationship_validator.rb#diff-0

changed the legal relationships between file resources with trailing slashes. Specifically, this manifest

file {"/home/wyatt/foo":
  ensure => directory,
  require => File["/home/wyatt/foo/"],
}

will compile on Puppet < 5, and store a catalog in PuppetDB, while presenting an error to the user about a circular dependency. On Puppet 5, the same catalog will fail compilation because it is considering the path with the trailing slash to be different than the one in the resource title, meaning the referenced resource is considered missing. Consequently, the catalog is not stored in PuppetDB.

This issue was caught by the PuppetDB spec tests around this case here:
https://github.com/puppetlabs/puppetdb/blob/master/puppet/spec/unit/indirector/catalog/puppetdb_spec.rb#L623

After talking it over with Henrik Lindberg, we determined that the tests should probably be ported to Puppet and that Puppet compilation needs to handle the trailing slash in a backwards compatible way.



 Comments   
Comment by Henrik Lindberg [ 2017/03/28 ]

I am not sure how to best fix this. Clearly the knowledge of "trailing slash" is specific to the File resource. In general we want to not having to run any code in the resource type's implementation (which is required for this) as that makes us run into the problems of "environment isolation". The File type is however part of Puppet and therefore cannot change between environments.

The operation that is required is to create an instance of the resource and then ask for its ref, and then use that to lookup the resource in the catalog.
That is however expensive if it is done for every resource. We need to find a better way.

Note that, this problem only affects relationships formed via meta parameters. If the operators > < etc. are used then the difference in trailing slash treatment would already have hit the users since it has been that way for quite some time - the relationship operators have both been validating relationships and flagging a diff on trailing slash as a mismatch/not-found.

I suggested to Wyatt Alt that this ticket was logged so we could add the relevant tests to Puppet for the behaviour that we actually expect. But also to have the discussion here - what that behavior is supposed to be. For quite some time now, there has been a difference between metaparam refs and operator refs. (Metaparam refs were not validated, but operator ones were, and validation did not consider aliases. In 5.0.0 both metaparam and operator refs are validated, and alias are considered, but the catalog cannot lookup a File[/foo] and find a File[/foo/] (and vice versa).

Need to dig into how File does this to see how to best support this. My main concern is other resource types that do the same/similar thing and that we must support generate types to avoid running into environment isolation problems.

If we do nothing users may have to change manifests to ensure that naming and referencing files is done consistently with or without slashes. (Would that be terribly bad? They already had to do that in 4.x for relationships formed with the relationship operators).

Ping Eric Sorenson and Lindsey Smith

Comment by Henrik Lindberg [ 2017/03/31 ]

This seemed like an important bug fix, so I brought this in.
Without this, validation of references would turn into a nuisance for users that have not been pedantic about references to files (always using the same style wrt trailing slash in titles and references).

Comment by Henrik Lindberg [ 2017/03/31 ]

Side note: the logic that deals with resource references and aliases and the data structures used to index them is a complete mess of expensive operations that over and over again construct values for various aspects of "identity", and even need to construct semi initialized resources in order to trigger the logic of getting a "uniqueness_key".

The entire API and implementation needs to be refactored.

Comment by Thomas Hallgren [ 2017/04/05 ]

Merged to master at ae066f9.

Comment by Josh Cooper [ 2017/04/07 ]

Failed CI in https://jenkins-master-prod-1.delivery.puppetlabs.net/view/puppet-agent/view/master/view/puppet-agent/job/platform_puppet-agent_intn-van-sys_suite-daily-puppet-master/45/

Looks like the test is looking for already declared error message, which I would still expect when running puppet apply. Except that we're using noop, so maybe we don't trigger the message any more. Not sure if the code or test needs updating.

[root@i62nkbk8ue3rc9q puppet]# cat /tmp/dir-resource.5L4vvL
    $dir='/tmp/create-dir.EBKZab'
    $same_dir='/tmp/create-dir.EBKZab/'
    file {$dir:
      ensure => directory,
    }
    if !defined(File["${same_dir}"]) {
      file { $same_dir:
        ensure => directory,
      }
    }
[root@i62nkbk8ue3rc9q puppet]# bundle exec puppet apply --noop /tmp/dir-resource.5L4vvL
Warning: Support for ruby version 2.0.0 is deprecated and will be removed in a future release. See https://docs.puppet.com/puppet/latest/system_requirements.html#ruby for a list of supported ruby versions.
   (at /root/puppet/lib/puppet.rb:168:in `<module:Puppet>')
Notice: Compiled catalog for i62nkbk8ue3rc9q.delivery.puppetlabs.net in environment production in 0.05 seconds
Notice: /Stage[main]/Main/File[/tmp/create-dir.EBKZab]/ensure: current_value absent, should be directory (noop)
Notice: Class[Main]: Would have triggered 'refresh' from 1 event
Notice: Stage[main]: Would have triggered 'refresh' from 1 event
Notice: Applied catalog in 0.03 seconds
[root@i62nkbk8ue3rc9q puppet]# git revert -m 1 ae066f9
[master f1e47e5] Revert "Merge pull request #5770 from hlindberg/PUP-7402_trailing-slash-alias-in-file"
...
[root@i62nkbk8ue3rc9q puppet]# bundle exec puppet apply --noop /tmp/dir-resource.5L4vvL
Warning: Support for ruby version 2.0.0 is deprecated and will be removed in a future release. See https://docs.puppet.com/puppet/latest/system_requirements.html#ruby for a list of supported ruby versions.
   (at /root/puppet/lib/puppet.rb:168:in `<module:Puppet>')
Notice: Compiled catalog for i62nkbk8ue3rc9q.delivery.puppetlabs.net in environment production in 0.03 seconds
Error: Cannot alias File[/tmp/create-dir.EBKZab/] to ["/tmp/create-dir.EBKZab"] at /tmp/dir-resource.5L4vvL:7; resource ["File", "/tmp/create-dir.EBKZab"] already declared at /tmp/dir-resource.5L4vvL:3

Comment by Henrik Lindberg [ 2017/04/07 ]

I fixed a similar test in the PR. The reason for the change is that the !defined part will now find the resource where it earlier did not. Therefore the test will never add the second conflicting file resource. I.e the test is based on the assumption that it would not find the resource when the endings were different.

Comment by Eric Sorenson [ 2017/04/20 ]

Is this ticket's status correct? Looks like there's already code merged but it's in ready for engineering...

Comment by Henrik Lindberg [ 2017/04/22 ]

I believe it was brought back into "ready for engineering" as there were test failures in acceptance. Those are now corrected. (I also think it is wrong to bring a ticket back to "ready for engineering" when the merged logic was not reverted - it should have stayed in test. Changing status.

Generated at Tue Jan 28 09:35:15 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.