Allow doing lookups

[oneiros]

I have been working on #264 for a long time now, but finally have something to show:

This is probably still rough around the edges but works well for the cases I could come up with.

Feel free to play around with this, but please do not merge yet.

While I felt like I was almost finished, I realized an important piece of the puzzle is still missing: This does not currently work with encryption ☹️

I am not sure how to implement this and will have to think about this some more.

[tuxmea]

Note on encryption: encryption only works on strings.

[oneiros]

Note on encryption: encryption only works on strings.

Thanks for that, this was helpful. Also, I realized we only support top-level encrypted values anyway, so there is no need for something more complicated (for now).

Still, I have to admit it has been quite a ride. I had to move a lot stuff around, but I believe it should do the trick now.

Please play around with this and see if it works as expected.

Also, please have a look at the test cases in test/models/hiera_data/lookup_test.rb to see if my understanding of the merge behavior is correct.

[tuxmea]

Working perfectly and as expected. Order of the result is correct.

A. Issue on Hash deep merge:

1. Node data:

# data/nodes/foreman.betadots.training.yaml
hash:
  'key1':
    'key1sub1': 'key1sub1value'
    'key1sub2':
      'key1sub1sub1': 'key1sub1sub1value'
  'key2':
    'key2sub1': 'key2sub1value'

2. Common data:

# data/common.yaml
---
lookup_options:
  'hash':
    merge: 'deep'
hash: 'data' 

3. Puppet code:

$hash = lookup('hash')
$hash.each |$key, $value| {
  notify { $key:
    message => "Key: ${key} - Value: ${value}",
  }
}

4. Puppet result:

Notice: Key: key1 - Value: {key1sub1 => key1sub1value, key1sub2 => {key1sub1sub1 => key1sub1sub1value}}
Notice: /Stage[main]/Main/Notify[key1]/message: defined 'message' as 'Key: key1 - Value: {key1sub1 => key1sub1value, key1sub2 => {key1sub1sub1 => key1sub1sub1value}}'
Notice: Key: key2 - Value: {key2sub1 => key2sub1value}
Notice: /Stage[main]/Main/Notify[key2]/message: defined 'message' as 'Key: key2 - Value: {key2sub1 => key2sub1value}'

5. HDM result:

content missing

B. Issue on data presentation:

• Integer values are shown as string
• single Strings and Boolean values always have three dots (...) underneath

[oneiros]

Two things were going on here. I misinterpreted the requirements and documentation and actually threw an error when a deep merge was attempted on something that was not an array or hash. But then I forgot to actually display that error in the UI.

I now fixed both: If an error is thrown it is being displayed and having scalar values in a deep merge no longer throws an error.

There are two scenarios that still throw an error and I am a bit uncertain now if this is correct:

1. If a unique merge is requested, but one of the values is a hash. The documentation states:

   The lookup fails if any of the values are hashes.

2. If a hash merge is requested, but one of the values is not a hash. The documentation states:

   Every match must be a hash and the lookup fails if any of the values aren’t hashes.

[tuxmea]

This is the result from a real puppet agent run:

1. Array unique:

Data:

# common.yaml
---
lookup_options:
  'array_unique':
    merge: 'unique'
array_unique: 'string'

# app/demo.yaml
---
array_unique:
  - 'foo'
  - 'bar'

# nodes/foreman.betadots.training.yaml 
---
array_unique:
  'key': 'value'

Puppet manifest:

$array_unique = lookup('array_unique')
notify { 'arrayunique':
  message => $array_unique,
}

Result from Puppet agent:

Notice: /Stage[main]/Main/Notify[arrayunique]/message: current_value 'absent', should be {
  'key' => 'value'
} 

Note: if we revert node data with common data we retreive an error from puppet:

# node data:
array_unique: 'string'
# common data:
array_unique:
  'key': 'value'

Puppet result:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: The second element of the merge has wrong type, expects a value of type Scalar or Array, got Struct on node foreman.betadots.training

2. Hash deep

Common Data:

# common.yaml
---
lookup_options:
  'hash_merge':
    merge: 'deep'
hash_merge:
  'common':
    'subkey1': 'subvalue1'

App data:

# app/demo.yaml
---
hash_merge:
  'app':
     subkey_app: 'subvalue_app'

Node data:

nodes/foreman.betadots.training.yaml
---
hash_merge: 'foobar'

Puppet Manifest:

$hash_merge = lookup('hash_merge')

notify { 'hashmerge':
  message => $hash_merge,
}

Puppet result:

Notice: /Stage[main]/Main/Notify[hashmerge]/message: current_value 'absent', should be 'foobar'

THis does not follow the documentation.
We check with @binford2k from Puppet.

[tuxmea]

My opinion:

When unique merge is used:

• all occurrences must be of type Array
• HDM should show a warning (no result) with the data path (file) and the key, explaining that the key is not of type array

When deep merge is used:

• all occurrences must be of type Hash
• HDM should show a warning (no result) with the data path (file) and key, explaining, that the key is not of type Hash

I am interested how @bastelfreak and @rwaffen see this.

[oneiros]

First of all, I did not mean to confuse anyone. I think both our unique and deep strategies should work (mostly) like puppet's now. Both this fact and the somewhat strange behavior is no accident: It is just the way the deep_merge gem, that both puppet and we now use under the hood, works.

My specific concern was with what "the lookup fails" in the puppet docs means exactly.

Also, my second point refered to the hash strategy, not the deep strategy. This is also available for shallow merging of hashes.

As for the two examples above: I have a hard time parsing the puppet output, but if what follows after "current_value 'absent', should be" is the result of the lookup I believe both examples to follow the documented behavior.

As for my opinion of how HDM should be have, I will comment in the discussion.

[oneiros]

As promised I tried to remove the dangling three dots from the output. The good news is that I caught and fixed a bug related to unique merges of scalar values.

The bad news is, I could not reproduce the "three dots" problem. They are indeed YAML syntax and yes, I use #to_yaml to format the result, but I cannot for the life of me find a way to coerce #to_yaml to include the three dots. @tuxmea Do you still have an example that provokes this behavior?

[tuxmea]

Example with 3 dots:

hiera.yaml

# /etc/puppetlabs/code/environments/production/hiera.yaml
---
version: 5
defaults:
  # The default value for "datadir" is "data" under the same directory as the hiera.yaml
  # file (this file)
  # When specifying a datadir, make sure the directory exists.
  # See https://puppet.com/docs/puppet/latest/environments_about.html for further details on environments.
  datadir: "%{facts.external_facts.datadir}"
  lookup_key: custom_function
  #data_hash: yaml_data
hierarchy:
  - name: "Per-node data (yaml version)"
    path: "nodes/%{::trusted.certname}.yaml"
    #lookup_key: custom_function
  - name: "Other YAML hierarchy levels"
    paths:
      - "common.yaml"

Node YAML

# /etc/puppetlabs/code/environments/production/data/nodes/foreman.betadots.training.yaml
---
classes:
  #- 'hdm'
  - 'puppetdb'
  - 'puppetdb::master::config'

hdm::version: 'v1.4.1'
postgresql::globals::manage_dnf_module: true
puppetdb::manage_firewall: false
puppetdb::postgres_version: '12'

hdm.yml

# config/hdm.yml
development:
  read_only: false
  allow_encryption: true
  puppet_db:
    server: "http://localhost:8080"
  config_dir: '/etc/puppetlabs/code'
  custom_lookup_function_mapping:
    custom_function: yaml
  #hiera_config_file: "hiera_hdm.yaml" # if not set, the default value 'hiera.yaml' is used

Lookup result for hdm::version:

v1.4.1
...

[binford2k]

@tuxmea could you file an issue at https://github.com/puppetlabs/puppet-docs/issues detailing the docs problem so someone can investigate it?

[rwaffen]

LGTM

[oneiros]

Still no luck. @tuxmea how does common.yaml look in your example?

[tuxmea]

Still no luck. @tuxmea how does common.yaml look in your example?

My common.yaml file:

---
lookup_options:
  'classes':
    merge: 'unique'
  'hash':
    merge: 'deep'

classes:
  - 'profile'

hash: 'data'
#  'key1common': 'key1common_value'
#  'key1': 'key1value'

profile::user_data:
  'name1':
    'sshkeyurl':
      - 'https://www.google.com'
      - 'http://www.betadots.de'
  'name2':
    'sshkeyurl': 'http://heise.de'

[oneiros]

I am stumped. I cannot reproduce this even with the exact same files.

The only idea I have is that we are using different versions of libyaml (Ruby's yaml parser / emitter psych uses libyaml under the hood). But the version of libyaml in the debian bullseye based docker image should be the exact same as the one on my system, so this theory is at least somewhat shaky 🤷‍♂️

FWIW, the quick fix here would be really simple, but I would really like to understand where this difference in behavior comes from. I believe we observed at least one other difference (display of strings IIRC) and there might be more. Without knowing what causes this, it is hard to anticipate what else could go wrong and which way to fix this properly.

On the other hand, I have already invested a lot of time into this problem, which is "only" a cosmetic issue. So I would be fine with either

a) we merge this now or
b) I implement the quick fix and we merge then.

[bastelfreak]

The only idea I have is that we are using different versions of libyaml (Ruby's yaml parser / emitter psych uses libyaml under the hood)

I've the same idea. At Vox Pupuli we had similar problems with json and yaml parsing where the same ruby code provided different results on different machines. The root cause was that sometimes psych was used. I think there was a workaround to prepend :: to the YAML object to force to use the ruby builtin? But I currently cannot find the code and am too stupid to google it right now :(

[oneiros]

Ruby used to bundle another YAML parser, syck, but that was a long time ago. These days psych is the only one and the YAML constant is just an alias for Psych.

[tuxmea]

@oneiros I prefer to merge this one.
Can you please rebase and solve the merge conflict?