Open Stack

Hi all,

I've got an input from a fellow worker[1].  Basically, transparently
transforming user data in puppet is opening a big can of worms.

He came up with a rather contrived example, but it's definitively worth
discussing it.

So in the vncproxy example, if the user does this:

   $vncproxy_host => 'ff02::1:ff00:1:80',
   $vncproxy_port => '80'

thinking that vncproxy_host is set to host 'ff02::1:ff00:1' with port
":80" (he forgot to add brackets) then the code will transform that to a
valid uri '[ff02::1:ff00:1:80]:80'

Without the code it would give this 'ff02::1:ff00:1:80:80' which would
fail as it lacks the brackets and is an invalid uri.

There is no way to make the difference between "wrong ipv6 + port" and
"valid ipv6", so mangling user input can lead to unexpected result.

I'm going to put the patches on WIP, as maybe, this may not be a good
idea to have user input transformed at all in puppet as all corner cases
cannot be detected.

The trade-off, of course, is user convenience.

So what do you think, parse and transform user input or not ?

[1] thanks Lukas

Sofer Athlan-Guyot <sathlang at redhat.com> writes:

> Cody Herriges <cody at herriges.org> writes:
>
> Sorry I didn't see you reply before.  Comments below.
>
>> Sofer Athlan-Guyot wrote:
>>> Hi,
>>> 
>>> There are a few places where I would like to be able to check for IPv6
>>> address and add bracket to the parameters.  I think that would be a nice
>>> addition to the puppet-openstacklib/lib/puppet/parser.
>>> 
>>> Here the interface I have in mind with the puppet-nova module:
>>> 
>>> class nova::vncproxy::common (
>>>   $vncproxy_host     = undef,
>>>   $vncproxy_protocol = undef,
>>>   $vncproxy_port     = undef,
>>>   $vncproxy_path     = undef,
>>> ) {
>>> 
>>>   include ::nova::deps
>>> 
>>>   $vncproxy_host_real     = pick(
>>>     ipv6_add_bracket_maybe($vncproxy_host,
>>>                            $::nova::compute::vncproxy_host,
>>>                            $::nova::vncproxy::host,
>>>                            false)
>>> 
>>> 
>>> This would returns an array with the host decorated with "[]" if the
>>> value is an IPv6 address.  Ideally the function could take only one
>>> value and return it or take an array and return an array for seamless
>>> integration in the code.
>>> 
>>> WDYT?
>>> 
>>
>> I see this and it looks like that only only reason this is a problem is
>> because we've broken up all the pieces of data needed to generate a URI
>> so it becomes inappropriate to decorate the vncproxy_host variable's
>> value with "[]" because it lacks the port appended to the end.  What are
>> the ramifications of simply switching to a "$vnc_uri" variable much the
>> same that has happened with identity_uri and auth_uri, e.g.
>> https://review.openstack.org/262799. If one has to simply define the
>> entire URI, they'll be able to properly decorate the IPv6 address.
>
> Yes, that could be something to consider as well.  The difficulties with
> this approach, that I see are that it's not easy on the user (change of
> interface) and must rely on a deprecation period (code to maintain).
>
> Adding a function on the other hand is transparent for the user and it
> may be useful in other part of the code.
>
> As for this solution (adding a function) I had to lower my expectation
> to meet puppet < 4.1 reality.  There is no splat operator, and no way to
> chain functions as I wanted at the beginning.  What I did is a simpler
> function that take only one argument, the stuff to maybe transform.  The
> example above adjusted:
>
>
>   $vncproxy_host_real = ipv6_add_bracket_maybe(pick(
>       $vncproxy_host,
>       $::nova::compute::vncproxy_host,
>       $::nova::vncproxy::host,
>     false))
>
> Please let me know what you think.

-- 
Sofer Athlan-Guyot