Attribute parser breaks passwords

[tinuzz]

The feature::idomysql class had a parameter '$password'. Its value is used twice:

• To construct a MySQL command for importing the schema. Value is used as-is.
• To configure the IdoMysqlConnection parameters. The value is used here in the 'attrs' parameter of the icinga2::object type and thus parsed with the attribute parser.

This leads to a conflict.

I have a password containing a ')', which is mangled by the parser into something syntactically invalid.

    class{ '::icinga2::feature::idomysql':
        password      => $dbpass,
    }

leads to syntactically broken configuration, and

   class{ '::icinga2::feature::idomysql':
        password      => "-:\"$dbpass\"",
    }

leads to inability to import the schema, because the password is wrong.

Trying to parse password values doesn't seem sensible to me. This is a similar issue to #572.

I think this parser, if you insist on keeping it enabled by default, should at the very least, make 100% sure that it doesn't generate syntactically invalid configuration. One of the major points of creating abstractions in Puppet is to take the burden of messing with application-specific configuration syntax away from the user and this parser is in fact making it more difficult.

Regards, Martijn.

[lbetz]

"... and this parser is in fact making it more difficult."

Yes and No. Have you ever tried the old module without a parser and used an assign rule?

But it's a point, not to parse every attribute.

[tinuzz]

Hi,

No I have just started out using this module to configure Icinga2, so I don't have any insight in how it is an improvement over earlier iterations. Yesterday, I added some service groups using assign rules and that just worked, so it's not all bad.

However, to adhere more to POLA, I would recommend to make this parser opt-in, rather than opt-out. I would think it would be extremely easy to put all of its functionality in a function, and if you want to use it, you'd just wrap your attribute values in a single function call. That would be so much more intuitive and less error-prone than to disable the parser by prefixing the value with "-:".

Of course, the default behaviour is hard to change without breaking existing code, but maybe adding a function to call the parser and providing an option to disable it by default could be helpful. I think I would use that.

Best regards, Martijn.

[lbetz]

The discussion isn't new. For which scope this on/off option should belong? An attribute, an object and to all its attributes or globally.

[lbetz]

Since a year I'd like to redesign the parser but ... no one gives time to me.

[tinuzz]

The discussion isn't new. For which scope this on/off option should belong? An attribute, an object and to all its attributes or globally.

I'm not sure I understand the full scope of this question, but intuitively, I would say "globally". When the parser is turned off by default, the only way to have it do anything, is to call it by function. Does that sound reasonable?

[lbetz]

First of all, it affects all attributes that are also used for preparatory work such as passwords to filling databases.

• feature/idomysql
• feature/idopgsql

It becomes problematic if someone does not use schema_import, but still switches off the parser or uses constants.

[lbetz]

So we break the API and I set this to v3.0.0, the up coming release (2.4.2 moved to 3.0.0)

[lbetz]

... and all password attributes don't will be parsed.

feature/elasticsearch feature/icingadb feature/idomysql feature/idopgsql feature/influxdb object/apiuser