[PUP-5839] Potential regressions in relationship validation Created: 2016/02/08  Updated: 2016/12/08  Resolved: 2016/02/10

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

Type: Bug Priority: Major
Reporter: Ken Barber Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-5855 Deprecate inexact resource reference ... Closed
relates to PDB-2402 Terminus tests fail on puppet master Closed
relates to PUP-5659 Puppet master does not fail catalog o... Closed
Template:

 Description   

So our tests in PuppetDB have divulged a number of new validation errors, on what seems to be previously legal Puppet code.

https://jenkins.puppetlabs.com/job/legacy_puppetdb_unit-rspec_puppet-master/label=unit,puppet_branch=master,ruby=ruby-2.0.0-p481/5/console

This seems to have come about after this change was merged into master:

https://github.com/puppetlabs/puppet/commit/676f0b7a2d78b2727c991f44e1c23f6df8e5233e

We found 2 problems that might concern Puppet, and should be treated like potential bugs in the code-base:

a) Resource relationships to resource aliases no longer seems to work:

notify { 'a': alias => 'b'} ... notify { 'c': require => Notify['b'] }

b) In the past a trailing forward slash was handled as the same thing, as if it was missing:

file { '/tmp/': } ... notify { 'a': require => File['/tmp'] } (note the trailing forward slash).

I've written some tests in puppet that confirm this behaviour change:

https://github.com/kbarber/puppet/commit/76ae1022cc754389bfac157c29159741b523b41d

I'm fairly certain problem a) is a valid bug. Problem b) however might be first construed as a better behaviour now we validate/stop this, but it might indicate users in the wild will now have failing manifests/code if we don't handle the backwards compatible behaviour - something controversial for a Y or Z release.



 Comments   
Comment by Julien Pivotto [ 2016/02/08 ]

Please note that the following already create errors now:

a)

notify { 'a': alias => 'b'}
notify { 'c':}
Notify['c'] -> Notify['b']

Returns:
Error: Could not find resource 'Notify[b]' for relationship from 'Notify[c]' on node nitrogen

and b)

file{ '/tmp/': }
notify { 'a':  }
Notify['a'] -> File['/tmp']

Error: Could not find resource 'File[/tmp]' for relationship from 'Notify[a]' on node nitrogen

Comment by Julien Pivotto [ 2016/02/08 ]

Ken Barber I think this bug is that metaparameters relationship were more permissive than explicit relationships?

Comment by Henrik Lindberg [ 2016/02/08 ]

Julien Pivotto Thanks for confirming what I though would be the case and was just about to test. You are right, the reason it fails when using relationship operators is that those validate the same way as the validation I added in PUP-5659.

So, if relationship using alias should work, then it should be fixed in both places.

Julian, you are also correct, earlier there was no check at all when using meta parameters, and the trailing slash is handled by the File type as knows about paths and can equate two paths even if they are not strictly equal in a string sense.

We need to decide what to do about these special cases.

Comment by Julien Pivotto [ 2016/02/09 ]

IMO we need to be backward compatible about the aliases, because it makes sense (maybe some people use them to stub 3rd party classes or resources)

For the file resource, we need to look further because some custom types and providers might behave in the same way.

Maybe we can imagine a --strict_relationship_validator config option to get back to the old behaviour for people relying on the second behaviour? And we could deprecate it directly so we can remove it for the next y release?

Comment by Ken Barber [ 2016/02/09 ]

Thanks for looking into this Julien Pivotto & Henrik Lindberg ... seems you've understood the essence of these problems.

Comment by Henrik Lindberg [ 2016/02/09 ]

To be 100% backwards compatible we basically cannot do any validation at all, and the fix should be reverted and moved to Puppet 5.0.0.
This because we have no way of statically knowing if a resource type RT will equate two different titles and what the logic for that is without actually creating those instances and testing. That may even involve running on the agent as such equality may be based on the physical traits of the resources (calls to the system are used to resolved if they are equal; e.g. we cannot know server side if two files is the same file because they are symlinked).

Ping @kylo - I think (unfortunately) that we have to defer this fix to 5.0.0 unless we accept that there will be potential breakage.

Comment by Henrik Lindberg [ 2016/02/09 ]

Kylo and I decided to revert the change in PUP-5659 and reschedule that work for Puppet 5.0.0 as there are too many things breaking and too little time to fix it for Puppet 4.4.0.

Comment by Ken Barber [ 2016/02/10 ]

Shame really, but I get it Henrik Lindberg, thanks very much for looking into this again. I hope we can move forward on this one day, as it was a good patch in general.

Comment by Henrik Lindberg [ 2016/02/10 ]

Yeah, we may add it back in in 4.x as deprecation warnings when it finds things that does not exist under the new rules. But it was broken in other ways as well when given nested arrays and empty arrays. So, needs more spec work before we can implement.

Comment by Julien Pivotto [ 2016/02/10 ]

As PUP-5659 has been reverted, can we close this ticket and continue the discussion there?

Comment by Henrik Lindberg [ 2016/02/10 ]

Pine Ken Barber - I am closing this as PUP-5659 now includes the concerns in this ticket. Reopen if the revert did not help.

Generated at Tue Sep 29 23:42:52 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.