[PUP-7402] handle trailing slashes in references between file resources Created: 2017/03/28 Updated: 2017/06/28 Resolved: 2017/04/24
|Affects Version/s:||PUP 5.0.0|
|Fix Version/s:||PUP 5.0.0|
|Remaining Estimate:||Not Specified|
|Time Spent:||Not Specified|
|Original Estimate:||Not Specified|
|Team:||Puppet Developer Experience|
|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|
changed the legal relationships between file resources with trailing slashes. Specifically, this manifest
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:
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.
|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.
Note that, this problem only affects relationships formed via meta parameters. If the operators
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).
|Comment by Henrik Lindberg [ 2017/03/31 ]|
This seemed like an important bug fix, so I brought this in.
|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 ]|
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.
|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.