[ClusterLabs] [OCF] [RFC at agents] unresolved issue with handling parameters containing whitespaces

[Jan Pokorný]

On 13/05/15 11:44 +0200, Lars Ellenberg wrote:
> On Tue, May 12, 2015 at 07:23:36AM -0600, Alan Robertson wrote:
>> This is not a problem in the OCF API, but in the pcs command.
>> 
>> There is no issue with having spaces in environment variables.
>> 
>> It's the pcs command that's doing you dirty here...

I am afraid I sense some short-circuiting here.  I should have provided
more details to preempt blaming pcs for everything, as it only served
to demonstrate the point (and the bugs at the end served to show somewhat
relevant issues with handling doublequotes, e.g., in case explicit quoting
of white-space contained parameter value becomes recommended practice
~ C1.-second option/"suboptimal").

To compensate it now, clarification follows:

  # pcs resource create mysql-test1 ocf:heartbeat:mysql \
    'config=/mnt/foo/mysql/config final.cnf'

will lead to the following nvpair in CIB:

  <nvpair id="mysql-test1-instance_attributes-config"
          name="config"
          value="/mnt/foo/mysql/config final.cnf"/>

and, likewise,

  # pcs resource create mysql-test2 ocf:heartbeat:mysql \
    'config="/mnt/foo/mysql/config final.cnf"'

to

  <nvpair id="mysql-test2-instance_attributes-config"
          name="config"
	  value=""/mnt/foo/mysql/config final.cnf""/>

So there is _no_ issue on pcs side so far.
Also Pacemaker will pass the environment variables in a correct manner
(yes, it's perfectly valid to have spaces there).

Only later in chain, some assumptions may be broken in such a case:

> In this case it is also the mysql RA (and helper functions)
> that do not properly quote.
> 
> https://github.com/ClusterLabs/resource-agents/blob/master/heartbeat/mysql-common.sh#L108
> ff.
> 
> if [ ! -f $OCF_RESKEY_config ]; then
> 
> Well, sure, that breaks.

Sure, I know :)  But this is not an isolated case at all (see below) and
that's why I wanted to bring this up.

> But Alan is right, of course, this is not a problem with the OCF API.

I hope I haven't implied the contrary -- I only offered a prospect
of possibly solving this issue on the API level by either excluding
spaces from a parameter value domain altogether (A./"dull") or setting
rules how to enquote spec-contained parameter values explictly
(C1.-second option/"suboptimal").  This is the only relevance to OCF.

Oh, there's also a possibility for more granular parameter value domain/
data type restrictions akin to what you can achieve in Relax NG
(mentioned that at the HA Summit in Brno) -- parameters could
determine themselves whether space in string is supported or not
and the validators in management tools could enforce that.
Apparently, OCF standard would have to be amended first, but
I anticipate this would be optional, non-disruptive extension.

> More a problem in the implementation of the individual resource agents.

From a little bit of digging, following RAs seem affected:

- anything (binfile, pidfile, workdir, logfile, ...)
- AoEtarget (pid, binary, ...)
- apache (configfile, ...)
- asterisk (config, ...)
- ClusterMon (pidfile, htmlfile, ...)
- conntrackd (config, ...)
- CTDB (ctdb_config_dir, ...)
- dhcpd (chrooted_path, config, ...)
- dnsupdate (keyfile, ...)
- Dummy (state)
- eDir88 (eDir_config_file, ...)
- ...

Other possible discrepancies:
- clvm (unquoted $CLVMDOPTS in start_process call)
- ...

... this is why I wanted to discuss the options prior to mass-patching
is started (also I have to agree with Ulrich Windl that doing some stuff
properly in shell is excessively hard compared to other options).

> And possibly something the UIs need to review in their internals.
> I know CRM shell handles white space quite well
> (or at least in a way I expect it to).
> I personally don't use PCS that often, so I'm unsure there.
> 
>>> - I want to run ocf:heartbeat:mysql resource in my cluster
>>> 
>>> - I am a rebel going against best practices, hence my configuration
>>>   file for this mysql instance is "/mnt/foo/mysql/config final.cnf"
>>>   (notice the space)
>>> 
>>> - so I go ahead and type:
>>> 
>>>   # pcs resource create mysql-test1 ocf:heartbeat:mysql \
>>>     'config=/mnt/foo/mysql/config final.cnf'
>>>   (quotes present so as to pass the whole config=... argument at once,
>>>   otherwise the same word-splitting problem as mentioned would be
>>>   present)
> 
> 
> In crmsh, you would say
> shell# crm configure primitive mysql-test1 ocf:heartbeat:mysql params 'config="/mnt/foo/mysql/config final.cnf"'
> (from the shell prompt), or
> shell# crm configure
> crm(live)configure# primitive mysql-test1 ocf:heartbeat:mysql params config="/mnt/foo/mysql/config final.cnf"
> from the crmsh prompt.

[Hmm, this is a bit more convenient as just raw values are stated
without special protections against shell parser.]

> From the error output below, PCS handles it just fine,
> and passes the white space just fine.

Yep, a direct pcs equivalent of the provided "crm configure" invocation
already crossed the thread.

> But it still breaks in
> 
>>>   >    '[' '!' -f /mnt/foo/mysql/config final.cnf ']'
>>>                                       ^^^
>>>                                      OOPS#1a
>>>   >  stderr: /usr/lib/ocf/lib/heartbeat/mysql-common.sh: line 108: \
>>>   >    [: /mnt/foo/mysql/config: binary operator expected
>>>                                   ^^^
> 
> etc., because that RA script does not use quotes properly.
> 
> So: fix (all) the agents, or don't do that.

In other words, C., or A. from the originally proposed solutions...

> Or both ;-)

There will always be someone getting bitten needlessly (also
considering that whitespace hadn't been explicitly excluded before)
unless the agents are fixed.


So, does C0. + C1.(first option) + C2. combo seem like a right way
forward?
(as a recap, C0. is meant to cover problematic issues like the ones
mentioned in-line, C1.+C2. is meant to cover cases like that
"/mnt/foo/mysql/config final.cnf" so as to ensure passing as a single
argument without propagating quote-wrapping down the invocations).


Addendum:
Some particular parameters should support (and _properly document_ in
metadata) direct quoting of character groups as it's intuitively
expected (here's where those pcs bugs would be relevant) -- such
quoting should then be used for parsing into particular argv items of
the constructed command invocation:

- anything/cmdline_options
- CrmMon/extra_options
- nsupdate/nsupdate_opts
- ...

On the other hand, some parameters should never be enquoted for a use:
- AudibleAlarm/nodelist
- ...

-- 
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.clusterlabs.org/pipermail/users/attachments/20150515/e8110cd3/attachment-0003.sig>