Bug #12199
augeas type should use arrays, not parsed strings, to represent operations.
| Status: | Accepted | Start date: | 01/26/2012 | |
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: | - | % Done: | 0% | |
| Category: | augeas | |||
| Target version: | - | |||
| Affected Puppet version: | 2.6.2 | Branch: | ||
| Keywords: | ||||
Description
A simple augeas { ‘…’: changes => ‘set … xxx’ } can fail to put the complete value into the target while wrongly reporting a successful application.
Example manifest:
define puppet::conf ($ensure) {
augeas {
"puppet.conf_${name}" :
changes => "set /files/etc/puppet/puppet.conf/${name} ${ensure}" ;
"puppet.conf_${name}_sq" :
changes => "set /files/etc/puppet/puppet.conf/${name}_sq '${ensure}'" ;
"puppet.conf_${name}_dq" :
changes => "set /files/etc/puppet/puppet.conf/${name}_dq \"${ensure}\"" ;
}
}
puppet::conf {
"agent/server" :
ensure => 'puppetmaster-dev.edvbus' ;
"agent/pluginsync" :
ensure => 'true' ;
"test/sq" :
ensure => "'" ;
"test/sqsq" :
ensure => "''" ;
"test/sqsqsq" :
ensure => "'''" ;
"test/sqsqsqsq" :
ensure => "''''" ;
"test/dq" :
ensure => '"' ;
"test/dqdq" :
ensure => '""' ;
"test/dqdqdq" :
ensure => '"""' ;
"test/dqdqdqdq" :
ensure => '""""' ;
"test/truncated_dq" :
ensure => '"s"bc"d"ef' ;
"test/truncated_sq" :
ensure => "'s'bc'd'ef" ;
"test/truncated_dq_mix" :
ensure => '"s"bc\'d\'ef' ;
"test/truncated_sq_mix" :
ensure => "'s'bc\"d\"ef" ;
"test/truncated_space" :
ensure => "before after" ;
"test/mix" :
ensure => "a\"b'c\"d'e" ;
}
Result in /etc/puppet/puppet.conf (sorted for convenience):
[test]
dqdqdqdq_sq=""""
dqdqdq_sq="""
dqdq_sq=""
dq_sq="
mix=a"b'c"d'e
mix_dq=a
mix_sq=a"b
sq_dq='
sqsq_dq=''
sqsqsq_dq='''
sqsqsqsq_dq=''''
truncated_dq_mix=s
truncated_dq_mix_sq="s"bc
truncated_dq=s
truncated_dq_sq="s"bc"d"ef
truncated_space=before
truncated_space_dq=before after
truncated_space_sq=before after
truncated_sq_dq='s'bc'd'ef
truncated_sq_mix_dq='s'bc
truncated_sq_mix=s
truncated_sq=s
As you can see, depending on the used quoting style, different parts of the values are silently truncated. Only values that cannot be written at all into the file cause an error.
I’ve tested this with 2.6. on squeeze and 2.7.10 on centos 6.2 with the same results.
History
#1
Updated by David Schmitt over 1 year ago
I wish there’d be a simple method to quote any string into augeas so that the string lands in the target file. Since the augeas provider uses the augeas library directly, there should be no fundamental problems there.
A simple rule would e.g. be that all strings can be quoted with single quotes, string containing space must be quoted with single quotes and all single quotes (except for the actual quoting quotes) have to be escaped with a backslash. This rule can be implemented easily by an augeas_quote function (or the human brain) and be stripped in the augeas provider after splitting and before passing it on to the library.
#2
Updated by Daniel Pittman over 1 year ago
- Category set to augeas
- Status changed from Unreviewed to Needs More Information
This seems like a reasonable request, but I have no idea how this could be represented in a way that won’t be very strange, or cause other, deeper problems.
If you use Augeas, can you comment on what would work from your point of view about this? How should it be expressed in the Puppet DSL?
#3
Updated by Dominic Cleal over 1 year ago
I’ve been making similar changes upstream in Augeas to ensure we have consistent quoting between augtool/aug_srun and the Puppet provider, so would like to make sure that any change we make here can be implemented there too. (Eventually I’d like to use aug_srun from Puppet and remove this second parser.)
Looking through the results, the problem seems to come in whenever an ending quote or space exists, and there are more characters beyond it – is that it?
I’m working on something similar to support spaces with readline in augtool, permitting strings such as "/white space/"foo. I think this would solve the problem here too?
We did discuss an alternative DSL quoting method in #7529, but the underlying problem with truncation still exists in the provider. I’m not sure if an augeas_quote function would help, as you’d have to quote your argument to the function too.
#4
Updated by Dominic Cleal over 1 year ago
Thanks for a huge list of tests, I’ve copied them into my Augeas branch to test the new behaviour – they helped me find another bug. The quoting behaviour I’m aiming at is compatible with Bourne shell quoting, which should already be familiar to users and admins and is easy for them to test against.
Could you please review this list of tests David, and make sure that we agree on the expected outputs?
If it seems sensible to you, we can implement it in the Puppet provider too.
#5
Updated by David Schmitt over 1 year ago
Daniel Pittmann wrote:
If you use Augeas, can you comment on what would work from your point of view about this? How should it be expressed in the Puppet DSL?
My first thought was something along these lines:
Quote function does all the work
$quoted_arg = augeas_quote($arg)
command => “set ${path} ${quoted_arg}”
Improved datastructure
command => [ set, $path, $arg ]
I’d prefer the second form (see execve(2) vs. system(3)), but I’m only starting with augeas, so you might want to get a second opinion.
Dominic Cleal wrote:
I’ve been making similar changes upstream in Augeas to ensure we have consistent quoting between augtool/aug_srun and the Puppet provider, so would like to make sure that any change we make here can be implemented there too. (Eventually I’d like to use aug_srun from Puppet and remove this second parser.)
Removing the parser in the provider can only be an improvement.
Looking through the results, the problem seems to come in whenever an ending quote or space exists, and there are more characters beyond it – is that it?
That’s the obvious problem. The other problem is, that it’s impossible to create a value that contains both kinds of quotes.
I’m working on something similar to support spaces with readline in augtool, permitting strings such as
"/white space/"foo. I think this would solve the problem here too?
Perhaps. Although for the multiple-kinds-of-quotes problem, it’d lead to strings as
"a 'b' "'\c "d"'
to get
a 'b' \c "d"
which makes my eyes bleed.
I’d prefer something like this
"a 'b' \\c \"d\"'
which is still ugly as it needs even more back-slashing in a puppet manifest, but it is much more aligned with what the rest of posix was doing the last 30+ years. Also it would facilitate the augeas_quote function as it is easily created by s/[“\]/\&/ and adding quotes.
We did discuss an alternative DSL quoting method in #7529, but the underlying problem with truncation still exists in the provider. I’m not sure if an augeas_quote function would help, as you’d have to quote your argument to the function too.
I’ve seen that ticket, but that’s only putting a bad band-aid on top of a festering problem.
#6
Updated by Daniel Pittman over 1 year ago
- Subject changed from augeas "set" silently truncates values to augeas type should use arrays, not parsed strings, to represent operations.
- Status changed from Needs More Information to Accepted
David Schmitt wrote:
Daniel Pittmann wrote:
If you use Augeas, can you comment on what would work from your point of view about this? How should it be expressed in the Puppet DSL?
My first thought was something along these lines:
Quote function does all the work
$quoted_arg = augeas_quote($arg)
command => “set ${path} ${quoted_arg}”
Improved datastructure
command => [ set, $path, $arg ]
I’d prefer the second form (see execve(2) vs. system(3)), but I’m only starting with augeas, so you might want to get a second opinion.
I strongly lean to the second form, also, which is much saner: it avoids having any sort of parsing, or escaping, in the system – which is really something that we don’t want, because it adds stupendous amounts of complexity to the system.
Ultimately, if we can’t invoke Augeas directly from the array, the type / provider should be able to take the nicely separated strings, paste them together, quote them, and pass them on – strictly internally, so that the complexity of quoting is entirely hidden from the user in the Puppet DSL.
#7
Updated by Dominic Cleal over 1 year ago
David Schmitt wrote:
Dominic Cleal wrote:
Looking through the results, the problem seems to come in whenever an ending quote or space exists, and there are more characters beyond it – is that it?
That’s the obvious problem. The other problem is, that it’s impossible to create a value that contains both kinds of quotes.
Ah yes, that’s what I’m working on upstream in the same branch.
I’d prefer something like this
"a 'b' \\c \"d\"'which is still ugly as it needs even more back-slashing in a puppet manifest, but it is much more aligned with what the rest of posix was doing the last 30+ years. Also it would facilitate the augeas_quote function as it is easily created by s/[“\]/\&/ and adding quotes.
Yes, both methods will be valid with the changes in Augeas, but we’d need to reapply them to Puppet until the parser is removed. The augeas_quote function could be added then too, which would be nice.
Daniel Pittman wrote:
David Schmitt wrote:
Improved datastructure
command => [ set, $path, $arg ]
I’d prefer the second form (see execve(2) vs. system(3)), but I’m only starting with augeas, so you might want to get a second opinion.
I strongly lean to the second form, also, which is much saner: it avoids having any sort of parsing, or escaping, in the system – which is really something that we don’t want, because it adds stupendous amounts of complexity to the system.
That’s a fair request, it’s a simpler method.
I’d like to see the current method remain supported though, as it enables people to copy + paste commands between augtool and Puppet for testing without needing to convert between styles. This is probably the biggest issue with the provider today, which is why I’ve been working to make augtool compatible with the Puppet provider’s parsing, adding context support etc, removing the gap between the two tools.
My only worry then is compatibility, as the type’s changes parameter takes a single string, or an array of strings for multiple commands. Adding this would make an array of strings ambiguous – either it’s an array of commands, or a single command split by arguments. Perhaps a second parameter would be needed?
Ultimately, if we can’t invoke Augeas directly from the array, the type / provider should be able to take the nicely separated strings, paste them together, quote them, and pass them on – strictly internally, so that the complexity of quoting is entirely hidden from the user in the Puppet DSL.
The commands should be present in ruby-augeas on the Augeas object, so it should just be a case of calling the method by the same name given in arg[0]. The downsides remain the same as today though: you rely on ruby-augeas to be up to date and you need a list of known commands in the Puppet provider too, in order to process path contexts – or we make the provider rely on Augeas 0.10.0 for the context feature.
(My plan for aug_srun to replace the parsing in the provider was to remove these two dependencies, so the parser that already exists for augtool is reused to parse these strings.)
#8
Updated by David Schmitt over 1 year ago
Dominic Cleal wrote:
Daniel Pittman wrote:
David Schmitt wrote:
Improved datastructure
command => [ set, $path, $arg ]
I’d prefer the second form (see execve(2) vs. system(3)), but I’m only starting with augeas, so you might want to get a second opinion.
I strongly lean to the second form, also, which is much saner: it avoids having any sort of parsing, or escaping, in the system – which is really something that we don’t want, because it adds stupendous amounts of complexity to the system.
That’s a fair request, it’s a simpler method.
I’d like to see the current method remain supported though, as it enables people to copy + paste commands between augtool and Puppet for testing without needing to convert between styles. This is probably the biggest issue with the provider today, which is why I’ve been working to make augtool compatible with the Puppet provider’s parsing, adding context support etc, removing the gap between the two tools.
As long as the provider and augeas use the same quoting syntax, that is a laudable goal. Of course, I do not have to maintain that. In this case – independent of the actual syntax problems – it should be noted again, that the truncation when mixing quotes is silent, but should instead probably error out.
My only worry then is compatibility, as the type’s
changesparameter takes a single string, or an array of strings for multiple commands. Adding this would make an array of strings ambiguous – either it’s an array of commands, or a single command split by arguments. Perhaps a second parameter would be needed?
Using nested arrays (one for each command) would be a possible answer to that:
[ [ set, ... ] [ changes, ... ] ]
There is no ambiguity there.
Ultimately, if we can’t invoke Augeas directly from the array, the type / provider should be able to take the nicely separated strings, paste them together, quote them, and pass them on – strictly internally, so that the complexity of quoting is entirely hidden from the user in the Puppet DSL.
The commands should be present in ruby-augeas on the Augeas object, so it should just be a case of calling the method by the same name given in arg[0]. The downsides remain the same as today though: you rely on ruby-augeas to be up to date and you need a list of known commands in the Puppet provider too, in order to process path contexts – or we make the provider rely on Augeas 0.10.0 for the context feature.
When the ruby-augeas library is current, the provider can use ruby’s introspection to validate commands, no? Anyways, I don’t see it as a problem to require a certain (ruby-)augeas for certain provider features.
(My plan for aug_srun to replace the parsing in the provider was to remove these two dependencies, so the parser that already exists for augtool is reused to parse these strings.)
That’s always nice :–)