[Tripleo] Some historic digging

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Tripleo] Some historic digging

Cédric Jeanneret
Hello,

While trying to add some unit tests for some resources in the
puppet-tripleo repository, I stumbled on a not-so-nice case: the
tripleo::firewall::service_rules definition.

In there, a resource is dynamically taken from hiera, using the
following key structure:
tripleo.<name>.firewall_rules

Although this seems to work properly in the tripleo deploy process, it
just doesn't work at all for a unit test.

After some more digging, it appears the "dot" (".") in YAML is a
reserved char for hashes. In the puppet world, we usually use the double
semi-colon, aka "::" as a separator.

Sooo… I'm wondering what's the history behind that non-puppet-standard
choice of "dot" separator, and what would be required in order to move
to a more standard syntax for that kind of resources.

I've checked the puppet-tripleo repository, and apparently only two
resources are using that kind of syntax:
- tripleo::firewall::service_rules (already widely present in the
tripleo-heat-templates)
- tripleo::haproxy::service_endpoints
(at least, those are the only resources using a "$underscore_name" variable)

Currently, if anyone wants to change something close to those two
resources, it won't be tested against regressions, and it's a really bad
situation. Especially since it might break central services (closing all
firewall ports isn't good, for example, or dropping an haproxy endpoint
by mistake)… Unit tests are needed, especially in such a huge piece of
software ;).

Thank you for your historical knowledge and its sharing :).

Cheers,

C.

--
Cédric Jeanneret
Senior Linux System Administrator
Infrastructure Solutions

Camptocamp SA
PSE-A / EPFL, 1015 Lausanne

Phone: +41 21 619 10 32
Office: +41 21 619 10 02
Email: [hidden email]


__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [hidden email]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

signature.asc (895 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Tripleo] Some historic digging

Steven Hardy
On Thu, Dec 7, 2017 at 7:22 AM, Cédric Jeanneret
<[hidden email]> wrote:

> Hello,
>
> While trying to add some unit tests for some resources in the
> puppet-tripleo repository, I stumbled on a not-so-nice case: the
> tripleo::firewall::service_rules definition.
>
> In there, a resource is dynamically taken from hiera, using the
> following key structure:
> tripleo.<name>.firewall_rules
>
> Although this seems to work properly in the tripleo deploy process, it
> just doesn't work at all for a unit test.
>
> After some more digging, it appears the "dot" (".") in YAML is a
> reserved char for hashes. In the puppet world, we usually use the double
> semi-colon, aka "::" as a separator.
>
> Sooo… I'm wondering what's the history behind that non-puppet-standard
> choice of "dot" separator, and what would be required in order to move
> to a more standard syntax for that kind of resources.

dprince may be the best person to answer as IIRC he implemented this originally.

However I suspect the choice of "." was deliberate to differentiate
from :: which implies the hiera values are consumed by puppet class
interfaces, vs parsed inside the class.

Can you work around the yaml issue by using quotes to force the
"tripleo.foo.firewall" to be a string?

We don't seem to require that for any templates in
tripleo-heat-templates though, is that just because the yaml key gets
cast to a string by hiera?

> I've checked the puppet-tripleo repository, and apparently only two
> resources are using that kind of syntax:
> - tripleo::firewall::service_rules (already widely present in the
> tripleo-heat-templates)
> - tripleo::haproxy::service_endpoints
> (at least, those are the only resources using a "$underscore_name" variable)
>
> Currently, if anyone wants to change something close to those two
> resources, it won't be tested against regressions, and it's a really bad
> situation. Especially since it might break central services (closing all
> firewall ports isn't good, for example, or dropping an haproxy endpoint
> by mistake)… Unit tests are needed, especially in such a huge piece of
> software ;).

Yeah I think we need to know more about the reasons this syntax won't
work for unit tests, we could conceivably change it, but as you say
it's widely used for firewall rules, and could potentially break any
out of tree service templates that exist, so we'd have to follow a
deprecation process to change the interface.

Thanks,

Steve

__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [hidden email]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Reply | Threaded
Open this post in threaded view
|

Re: [Tripleo] Some historic digging

Cédric Jeanneret
On 12/07/2017 05:23 PM, Steven Hardy wrote:

> On Thu, Dec 7, 2017 at 7:22 AM, Cédric Jeanneret
> <[hidden email]> wrote:
>> Hello,
>>
>> While trying to add some unit tests for some resources in the
>> puppet-tripleo repository, I stumbled on a not-so-nice case: the
>> tripleo::firewall::service_rules definition.
>>
>> In there, a resource is dynamically taken from hiera, using the
>> following key structure:
>> tripleo.<name>.firewall_rules
>>
>> Although this seems to work properly in the tripleo deploy process, it
>> just doesn't work at all for a unit test.
>>
>> After some more digging, it appears the "dot" (".") in YAML is a
>> reserved char for hashes. In the puppet world, we usually use the double
>> semi-colon, aka "::" as a separator.
>>
>> Sooo… I'm wondering what's the history behind that non-puppet-standard
>> choice of "dot" separator, and what would be required in order to move
>> to a more standard syntax for that kind of resources.
>
> dprince may be the best person to answer as IIRC he implemented this originally.
>
> However I suspect the choice of "." was deliberate to differentiate
> from :: which implies the hiera values are consumed by puppet class
> interfaces, vs parsed inside the class.
>
Hmm, possible, although.. well, a dedicated prefix might be used in
order to avoid weird things

> Can you work around the yaml issue by using quotes to force the
> "tripleo.foo.firewall" to be a string?

That was the first thing I tried - but nope. YAML seems to want to parse
that and kind of "re-cast" it :(

>
> We don't seem to require that for any templates in
> tripleo-heat-templates though, is that just because the yaml key gets
> cast to a string by hiera?

There might be something in some hiera configuration used in the deploy
process. I'll check the hiera.yaml and try to find something about that
weird string. If we can just report it into the unit tests, it can be
fine. Although... dots... well. ;)

>
>> I've checked the puppet-tripleo repository, and apparently only two
>> resources are using that kind of syntax:
>> - tripleo::firewall::service_rules (already widely present in the
>> tripleo-heat-templates)
>> - tripleo::haproxy::service_endpoints
>> (at least, those are the only resources using a "$underscore_name" variable)
>>
>> Currently, if anyone wants to change something close to those two
>> resources, it won't be tested against regressions, and it's a really bad
>> situation. Especially since it might break central services (closing all
>> firewall ports isn't good, for example, or dropping an haproxy endpoint
>> by mistake)… Unit tests are needed, especially in such a huge piece of
>> software ;).
>
> Yeah I think we need to know more about the reasons this syntax won't
> work for unit tests, we could conceivably change it, but as you say
> it's widely used for firewall rules, and could potentially break any
> out of tree service templates that exist, so we'd have to follow a
> deprecation process to change the interface.
Hmmm ok. I didn't check outside of the puppet-tripleo - it indeed might
be some things using this string format. I've already pushed two reviews
that *adds* support for the "::" notation for the said resources. That
way, we have at least a basis for a smooth move. Of course, they are
only "reviews", so they have to be accepted:
https://review.openstack.org/#/c/526589/
https://review.openstack.org/#/c/526292/

I doubt out-of-tree stuff uses those hiera entries, but of course this
would require some grep and, probably, advice from other contributors ;).

Thank you for your feedback!

>
> Thanks,
>
> Steve
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: [hidden email]?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>


__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [hidden email]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

signature.asc (895 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Tripleo] Some historic digging

Zane Bitter
On 08/12/17 03:51, Cédric Jeanneret wrote:

> On 12/07/2017 05:23 PM, Steven Hardy wrote:
>> On Thu, Dec 7, 2017 at 7:22 AM, Cédric Jeanneret
>> <[hidden email]>  wrote:
>>> After some more digging, it appears the "dot" (".") in YAML is a
>>> reserved char for hashes. In the puppet world, we usually use the double
>>> semi-colon, aka "::" as a separator.
>>
>> Can you work around the yaml issue by using quotes to force the
>> "tripleo.foo.firewall" to be a string?
> That was the first thing I tried - but nope. YAML seems to want to parse
> that and kind of "re-cast" it :(

There is most definitely no such thing in YAML:

 >>> yaml.safe_load('- tripleo.foo.firewallrules:\n    bar: baz')
[{'tripleo.foo.firewallrules': {'bar': 'baz'}}]

The problem is hiera:

https://puppet.com/docs/puppet/5.1/hiera_subkey.html

Discussion:

https://tickets.puppetlabs.com/browse/HI-496

The quoting thing was added comparatively recently (March 2016), so
maybe that's why it isn't working for you:

https://tickets.puppetlabs.com/browse/HI-504

- ZB

__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: [hidden email]?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev