[HI-530] Hiera silently drops malformed hierarchy sources Created: 2016/08/09  Updated: 2016/11/02  Resolved: 2016/08/31

Status: Closed
Project: Hiera
Component/s: CLI
Affects Version/s: HI 3.2.0
Fix Version/s: HI 3.2.2

Type: Improvement Priority: Normal
Reporter: Rich tea Assignee: Phong Ly
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File backend.rb.patch    
Template:
Story Points: 1
Sprint: Language 2016-08-24, Language 2016-09-07
Release Notes: Not Needed

 Description   

Hiera will silently drop malformed hierarchy sources (Those matching /(^\/|\/\/|\/$)/).
There should be some notification when this happens.

e.g. see attached patch for an example of this.



 Comments   
Comment by Henrik Lindberg [ 2016/08/17 ]

Rich tea A PR against the hiera project at github would be appreciated.
It is unclear to me why hierarchy sources matching the pattern in question are illegal and ignored in the first place. If you happen to know why that is being done it would also help.

Comment by Rich tea [ 2016/08/18 ]

Henrik Lindberg I have created https://github.com/puppetlabs/hiera/pull/360 PR360

As to why, I can see why in the current code the restriction is required the method "datafile_in(datadir, source, extension)" returns the full path and name of the file (if it exists) by adding together a "base directory" (datadir), the "hierarchy source" (source) and extension.
Since the source is the middle part of a full file name / path it can not have those pattens. e.g.
datadir = " /etc/puppetlabs/code/environments/production/hieradata/"
source = "os/${::operatingsystem}"
extension = ".json"
making the full file path "/etc/hieradata/os/RedHat.json"

It was my original understanding that if a "source" started with a "/" that "datadir" would be skipped, but that is not so.

looking at this now i wonder what would happen if source was something like "../../${::operatingsystem}"

I guess the method "datafile_in" could be modified to check if "source" started with a "/" and not join basedir in if so.

Comment by Henrik Lindberg [ 2016/08/18 ]

Rich tea Thanks. I asked about the rules because there is no specification that says why it is doing what it is doing, and there is also no specification of what should happen when using relative paths. For instance, it could be that the skipping is a feature - such that if you have an undefined variable that is used to interpolate a path, and it therefore ends up starting with a slash is actually a feature, and those entries are to be skipped by design.

Ping R.I.Pienaar - any input here?

While logging warnings is fine, making it a hard error may in practice be disruptive.

Comment by R.I.Pienaar [ 2016/08/18 ]

missing sources is normal yes - you might have 100 hosts and only 10 have certname specific files, this should not produce errors/warnings it will be very noisy.

badly formed ones should produce errors, though the feedback cycle available to hiera is limited but yeah I think if there's actually a set documented known good thing it should error when a known not good thing is given to it. I suspect the rules arent defined though so ultimately this might just end up as design feedback for lookup rather than fixes to hiera given the place in hieras life cycle.

Comment by Rich tea [ 2016/08/23 ]

resolved by Pull #360

Comment by Rich tea [ 2016/08/23 ]

Resolved in https://github.com/puppetlabs/hiera/pull/360

Comment by Henrik Lindberg [ 2016/08/23 ]

Rich tea Thanks for the contribution. I did some housekeeping on the ticket. Once something has been merged, the next state is "Ready for CI", which means that there needs to be a complete acceptance test run with the version in question before the ticket progresses to "Ready for Test" - this is manual testing and a review that the test coverage is suitable (in this case it should be quick). Finally, the ticket goes to Resolved, and it is finally closed/done when the release goes out (In this case the next Hiera release).

Comment by Phong Ly [ 2016/08/31 ]

Verified it works with puppet-agent at SHA=a3ddefc5

hiera.yaml file

[root@wopshcnouwefxof ~]# cat /etc/puppetlabs/code/hiera.yaml
---
    :backends:
      - "yaml"
    :logger: "console"
    :hierarchy:
      - "%{fqdn}"
      - "%{environment}"
      - "/bad_hiera_source"
      - "//bad_hiera_source"
      - "bad_hiera_source/"
      - "global"
 
    :yaml:
      :datadir: "/etc/puppetlabs/code/hieradata"
  [root@wopshcnouwefxof ~]#

Seeing warning messages in debug logs

[root@wopshcnouwefxof ~]puppet apply -e "notice(hiera('http_port'))" --debug | grep "Ignoring bad definition"
Debug: hiera(): Ignoring bad definition in :hierarchy: '/bad_hiera_source'
Debug: hiera(): Ignoring bad definition in :hierarchy: '//bad_hiera_source'
Debug: hiera(): Ignoring bad definition in :hierarchy: 'bad_hiera_source/'
[root@wopshcnouwefxof ~]#

Generated at Sat Feb 22 15:08:25 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.