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 <[email protected]> writes: > Cody Herriges <[email protected]> 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 __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
