Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refresh_facts.rb does not expand aliases #250

Closed
smortex opened this issue Apr 9, 2020 · 7 comments
Closed

refresh_facts.rb does not expand aliases #250

smortex opened this issue Apr 9, 2020 · 7 comments

Comments

@smortex
Copy link
Member

smortex commented Apr 9, 2020

After adding a new module to the control repo, we experienced failures from the code of https://github.com/choria-io/mcorpc-ruby-support :

yaml_facts.rb:35:in `rescue in block in load_facts_from_source' Failed to load yaml facts from /etc/puppetlabs/mcollective/generated-facts.yaml: Psych::BadAlias: Unknown alias: 1

The module provide two facts that contain a reference to the same object. The generation of generated-facts.yaml detects this and produce an alias instead of outputing the same content twice.

But then, mcorpc-ruby-support cannot read this file because SafeYaml does not support alias expansion for safety reasons.

The script that build generated-facts.yaml should probably make sure no aliasing happen.

@ripienaar
Copy link
Member

Nasty. Now that the ruby mco is gone we can probably make this script emit JSON which would make me much happier

@smortex
Copy link
Member Author

smortex commented Apr 9, 2020

From my findings digging this issue:

  • A quick & dirty workaround is to convert the yaml to json and then back to yaml (ugly);
  • There is not safe_dump method in Psych (the YAML module, bundled with Ruby if I recall correctly), so as of today, you cannot generate a YAML file that can be read with Psych.safe_load. I started hacking a safe_dump method, this involves quite a huge amount of code, so it's really not trivial.

So if the same could be done with JSON, maybe it would be the fastest cleanest way to proceed…

@ripienaar
Copy link
Member

I see there is a :canonical option to dump, maybe that helps?

Go choria supports reading json or yaml - and anyway since this is run from cron not that frequently we can do the to json to yaml thing

@smortex
Copy link
Member Author

smortex commented Apr 9, 2020

I tried it but it does not solve the issue, we still have aliases…

non canonical:
---
a: &1
- 1
- 2
- 3
b: *1
canonical:
---
{
  ? "a"
  : &1 [
    ! "1",
    ! "2",
    ! "3",
  ],
  ? "b"
  : *1,
}

I just checked the choria code and feel like saving JSON to a filename that ends with "json" (at least not "yaml") and adjusting the hiera data to adjust the filename should be good. I'll try to take some time for this. Thanks!

@ripienaar
Copy link
Member

Yeah json will work if you want to do that great, else we can do the hack I don’t mind

smortex added a commit to smortex/puppet-mcollective that referenced this issue Apr 10, 2020
If Facter build multiple facts from the same object, the serialization
to YAML will generate aliases to avoid duplication.

Aliases can be abused and can be used to cause DOS, so it is advised to
use YANL.safe_load when loading YAML which does not support aliases by
default.  It is done by choria-io/mcorpc-ruby-support.

As a consequence, when a single object is used by more than one fact,
the generated "generated-facts.yaml" file cannot be load.

This commit convert the facts hash to JSON, parse the JSON back to a
new Hash (where all references to the same object will be lost, so
arrays with the same content or different objects) and then converted to
YAML.
@smortex
Copy link
Member Author

smortex commented Apr 10, 2020

Busy day… switching to JSON is not as easy as I though, and we do have an ARM node with legacy mco server and legacy Puppet 5 in production that would be unhappy with such changes in the short term, so I am going the easy way with the hack solution.

For a long term solution, saving JSON indeed makes sense!

ripienaar added a commit that referenced this issue Apr 10, 2020
@ripienaar
Copy link
Member

sweet, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants