choria-legacy / mcollective-choria

Distribution of plugins for MCollective as found in Puppet 6
Apache License 2.0
56 stars 24 forks source link

Default length for single queried facts is too short #625

Closed guillemlc closed 4 years ago

guillemlc commented 4 years ago

We found the 40 character validation for facts to be too short, as some hashed facts may go easily beyond that.


Cannot validate input fact: Input string is longer than 40 character(s) (MCollective::DDLValidationError)

Also we tried modifying in the DLL:

     display :always

     input :fact,
           :prompt      => "The name of the fact",
           :description => "The fact to retrieve",
           :type        => :string,
           :validation  => '^[\w\-\.]+$',
           :optional    => false,
           :maxlength   => 40

choria-mcorpc-support-2.20.8/lib/mcollective/agent/rpcutil.ddl

Changing the maxlength value there seems to have no effect. If I set anything that would fit in my string, like 50, 60, 70, it would always complain about it being larger than 40.

Where can we properly set that maxlength?

Thank you

ripienaar commented 4 years ago

The DDL is in 2 places, on all nodes and where you run the client, all needs updating.

We do input validation on the client for fast, friendly, feedback and on the servers before invoking the agent to not rely on client validation for safety, hence both need it.

We should probably update this so it solves it for eveyrone

guillemlc commented 4 years ago

Thanks for replying so quickly Mr. Pienaar. I changed it in both client and server and I still hit it. Let me show you and maybe you can tell what i am doing wrong here.

client:

/opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/agent/rpcutil.ddl

changed from 40 to 80 here:

     display :always

     input :fact,
           :prompt      => "The name of the fact",
           :description => "The fact to retrieve",
           :type        => :string,
           :validation  => '^[\w\-\.]+$',
           :optional    => false,
           :maxlength   => 80

     output :fact,
            :description => "The name of the fact being returned",
            :display_as => "Fact"

     output :value,
            :description => "The value of the fact",
            :display_as => "Value"

    summarize do
        aggregate summary(:value)
    end
end

Same file for the servers. But still, when invoking:


The facts application failed to run: Cannot validate input fact: Input string is longer than 40 character(s)

Cannot validate input fact: Input string is longer than 40 character(s) (MCollective::DDLValidationError)
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/ddl/base.rb:152:in `rescue in validate_input_argument'  <----
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/ddl/base.rb:132:in `validate_input_argument'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/ddl/agentddl.rb:241:in `block in validate_rpc_request'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/ddl/agentddl.rb:233:in `each_key'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/ddl/agentddl.rb:233:in `validate_rpc_request'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/rpc/client.rb:179:in `validate_request'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/rpc/client.rb:250:in `method_missing'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/application/facts.rb:37:in `main'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/application.rb:283:in `run'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/lib/mcollective/applications.rb:23:in `run'
        from /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/bin/mco:33:in `<top (required)>'
        from /usr/bin/mco:23:in `load'
        from /usr/bin/mco:23:in `<main>'

As you can see that fact is ebsi_containers.trusted-data-sharing.version which is more than 40 char long.

ripienaar commented 4 years ago

ah no the one you want is probably in /opt/puppetlabs/mcollective/plugins/mcollective/agent

guillemlc commented 4 years ago

Hello.

Yes i can confirm that changing it in:

/opt/puppetlabs/mcollective/plugins/mcollective/agent/rpcutil.ddl makes it work.

Now I have two question:

  1. Why did the error led me to /opt/puppetlabs/puppet/lib/ruby/gems/2.5.0/gems/choria-mcorpc-support-2.20.8/ and not the agent path. I see that following your excellent documentation, I had the right idea but I was making changes in the wrong place.

  2. How can i make that parameter configurable via external data?

2.b If possible to make it configurable with external data, shall you give me a hand and we can validate it and send it as a PR?

Thanks again for teh support and being so fast to reply!

ripienaar commented 4 years ago
  1. the ruby libraries that integrate choria and old mcollective is delivered as a gem, however all the mco/choria plugins are in the libdir which is defined as /opt/puppetlabs/mcollective/....., the go libraries will also look there
  2. no this isnt a configurable via data unfortunately its build time settings, it's something we run in very infrequently but this setting predates structured facts so it needs some modernisation

Best is to update it, unfortunately I just did a release today else you'd have been in time to get a fix in for today :(

Honestly this file where you found it shouldnt even exist, its ancient legacy. The current source of truth for these files are

https://github.com/choria-io/mcollective-choria/tree/master/lib/mcollective/agent the .ddl and .json one

guillemlc commented 4 years ago

Thanks again for comments Mr. Pienaar.

So I understand we could not make it configurable at least the way it is now?

The issue I see here is that we will have to go an edit that file every time we refresh the version. That does not seem an elegant way to deal with this. Would the dll file accept loading configuration somehow? I know I could hack my way for this, but i would rather make it in a way that can go back into your code in form of contribution.

ripienaar commented 4 years ago

No unfortunately there's no support for configuring this in any puppet module at present, these things tend to be decided at dev time and end up being quite static. You'd need to update the 2 files in your puppet module to override this until we ship a fix

ripienaar commented 4 years ago

If you want to send a PR to this repo where I moved the issue to now for the 2 files that'd be great, if its urgent I can do a small release

ripienaar commented 4 years ago

Resolved in https://github.com/choria-io/mcollective-choria/pull/627