bartavelle / language-puppet

A library to work with Puppet manifests, test them and eventually replace everything ruby.
BSD 3-Clause "New" or "Revised" License
51 stars 8 forks source link

Hiera 'alias' function is not implemented #275

Closed PierreR closed 5 years ago

PierreR commented 5 years ago

https://puppet.com/docs/hiera/3.3/variables.html#the-alias-lookup-function

PierreR commented 5 years ago

One person from my team has lost hours because of this ... alias is coming empty, then goes elsewhere and null was crashing on each ... I can understand the frustration. For once the language-puppet tool instead of saving time had the opposite effect ... Arguably abusing the alias function is not a great idea either ...

bartavelle commented 5 years ago

Should interpolation failures throw an exception?

bartavelle commented 5 years ago

Pushed an implementation for alias

PierreR commented 5 years ago

@bartavelle I have this kind of declaration in common.yaml:

elastic_host: 'elastic.%{facts.zone}.srv.cirb.lan'

After pulling the last changes I have got this regression:

Could not lookup variable 
                            facts.zone at ./modules/application/...

I haven't had the time to take a closer look and understand what the regression is about (yet).

bartavelle commented 5 years ago

This means that this used to silently return the empty string ... I added this exception to catch this behavior. Will look this up.

PierreR commented 5 years ago

Yep you are right. I have just checked this and indeed it silently returns the empty string.

I am passing these values using loadTestDB which takes as an argument a file that looks like this:

---
facts:
  kafka0.dev:
    zone: dev
    role: kafka
    hostname: kafka0
    environment: kafka_dev
resources: {}
bartavelle commented 5 years ago

should work better now

PierreR commented 5 years ago

@bartavelle are the latest commits fixing "%{facts.zone}.srv.cirb.lan" or "%{alias('facts.zone')}".

Doesn't seem to fix the first and to be honest I am not even sure the latest form is valid or not in Puppet (I will need to check it out)

bartavelle commented 5 years ago

So, I imported a large part of hiera test suite for interpolation. Turns out dotted stuff support is terrible currently, but it will hopefully be easier to fix now that there is a test suite!

bartavelle commented 5 years ago

I passed most of the test case for interpolation. Lookup failures throw an error, contrary to hiera that returns empty strings. Not sure how important this is.

PierreR commented 5 years ago

Would master support something like "%{facts.zone}.srv.cirb.lan" already ? I have just tested it but it does not seem to succeed. If it is supposed to work, I will go and investigate in more details.

bartavelle commented 5 years ago

I think it should work, but as I touched facts, I might have broken something.

PierreR commented 5 years ago

It seems to work with "%{::facts.zone}.srv.cirb.lan" !

PierreR commented 5 years ago

Relevant information: https://puppet.com/docs/puppet/5.0/hiera_interpolation.html#what-about-classic-factname-facts

Currently in language-puppet, "%{::zone}.srv.cirb.lan" pass whereas "%{zone}.srv.cirb.lan" does not but I don't quite see where we enforce that.

IIRC facts is just a normal hash variable currently so it it not surprising that "%{facts.zone}.srv.cirb.lan" is not working. Somehow in Puppet we need to consider facts, trusted and server_facts as special valid global scope hash for Hiera.

bartavelle commented 5 years ago

So, for me, fact.XXX works, but facts.zone does not, as it is not a fact for me. With the following yaml:

---
toto: '%{facts.id}.test'

Calling hiera('toto') returns bartavelle.test. Try just using %{facts}, and you will get an error listing the facts that are known.

Perhaps it is an issue of loaded facts vs. gathered facts?

PierreR commented 5 years ago

Interesting. It is not really an issue of loaded facts vs gathered facts. Using the exact same hiera definition in a common.yaml file:

node testme {
  include application::test
  # $test = hiera('toto')
  # notify { "test is ${test}": }
}

If I uncomment the code, it works fine but as soon as I include the exact same code into a test.pp file I will get the Could not lookup variable error.

PierreR commented 5 years ago

We hit the same strange dichotomy behavior in #213. I have the feeling that somehow trying this out with inline code in a node declaration is not representative enough.

PierreR commented 5 years ago

In case it helps here are both debug statements:

[nix-shell:~/projects/bric/elk/puppet-stack-elk]$ puppetresources -p . -o testme -v DEBUG
DEBUG: Initialize daemon
INFO: Detect a hiera environment configuration format in ./hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/yum/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/subscription_manager/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/profile/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/limits/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/systemd/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/filebeat/hiera.yaml at version 5
DEBUG: Received query for node testme
DEBUG: Looking up 'toto' with backends YamlBackend "data"
DEBUG: testme: enterScope, scopename=application::test caller_module_name=:: module_name=application

ERROR: (testme) Could not lookup variable 
                facts.id at ./modules/application/manifests/test.pp:3:3

[nix-shell:~/projects/bric/elk/puppet-stack-elk]$ puppetresources -p . -o testme -v DEBUG
DEBUG: Initialize daemon
INFO: Detect a hiera environment configuration format in ./hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/yum/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/subscription_manager/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/profile/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/limits/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/systemd/hiera.yaml at version 5
INFO: Detect a hiera module configuration format in ./modules/filebeat/hiera.yaml at version 5
DEBUG: Received query for node testme
DEBUG: Looking up 'toto' with backends YamlBackend "data"
notify {
  test is unitesting.test: ./manifests/site.pp:23:12 testme [Scope []]
      withpath => false,
      require  => Class[::];
}
stage {
  main: 1:1 testme [Scope []]
      ;
}
PierreR commented 5 years ago

So this is probably related to the scope of the variables. Within a class, the global variables are accessible through :: but facts is a special case and should also be accessible as a global variable.

I am not sure where these rules about variable scope are defined. It might be here.

bartavelle commented 5 years ago

Ok, so this is a scoping issue. I am not sure how it should work. Should we be able to directly name all variables from previous scopes?

PierreR commented 5 years ago

I think global variables should be made accessible with

https://puppet.com/docs/puppet/5.0/hiera_interpolation.html#which-variables-should-you-use

I only need and use the first two for now.

bartavelle commented 5 years ago

So I just pushed code that can access all variables, just like in a puppet file. Perhaps I should prune this to only access the 3 top variables and the local scope?

PierreR commented 5 years ago

That would be more in line with the current situation. I would expect puppet to complain if a global variable is accessed directly without :: but I am not sure about this (I will need to check that but I am currently cooking ;-).

PierreR commented 5 years ago

I have just tested this and the current code in master is correct. Both "%{zone}.srv.cirb.lan" and "%{::zone}.srv.cirb.lan" are indeed accepted in Puppet.

Thanks so much.