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

[Ulrich Windl]

Hi!
IMHO handling blanks in variables cannot be done in a shell reliably if you
set variables from variables at various levels. Using some reasonable language
like Perl instead seems to be an option. Maybe when putting each parameter as
tring intp an shell array could preserve the blanks until needed, but handling
arrays in the shell is not much fun most of the time...

Regards,
Ulrich

>>> Jan Pokorný<jpokorny at redhat.com> schrieb am 12.05.2015 um 15:03 in
Nachricht
<20150512130337.GA4886 at redhat.com>:
> Aloha,
> 
> recently I realized one of those rough edges that probably were never
> run into (per quick ML seach) but are lurking to bite anyone that will
> leave the safe, well-tested paths.
> 
> What I have in mind in particular is unability to propagate parameters
> participating directly in underlying resource execution as command's
> arguments[*] (referred to as "argv items" for clarity) when they contain
> (white)spaces (per IFS variable?) due to (intentionally?) preserving them
> as-are in the majority of shell scripts constituting resource agents
> (will leave fence agents for a follow-up now):
> 
> 1. no wrapping them with quotes just for the sake of protecting against
>    word-splitting/preserving a _single_ argv item (eventually handled
>    in unquoted form, see the next point) to cover cases with space
>    contained within
> 
> 2. no unwrapping in case the user-provided parameter already is wrapped
>    with {single,double} quotes so as to denote "this is to be treated
>    as a single argv item" explicitly from user's input (somewhat
>    intuitive workaround attempt if one is familiar with shell and/or
>    quoting rules in some configuration files)
> 
> [*] not exclusively, but these parameters are most exposed to the problem
>     at hand
> 
> 
> To demonstrate the problem in a real-world scenario:
> 
> - 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)
> 
>   # pcs resource debug-start --full mysql-test1 \
>     | grep /mnt/foo/mysql/config
>   >  stderr: ++ 06:47:57: 52: : /mnt/foo/mysql/config final.cnf
>   >  stderr: + 06:47:57: mysql_common_validate:108: \
>   >    '[' '!' -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
>                                   ^^^
>                                  OOPS#1b
>   >  stderr: + 06:47:58: mysql_common_start:209: \
>   >    /usr/bin/mysqld_safe \
>   >    --defaults-file=/mnt/foo/mysql/config final.cnf \
>                                            ^^^
>                                            OOPS#2a
>   >    --pid-file=/var/run/mysql/mysqld.pid \
>   >    --socket=/var/lib/mysql/mysql.sock --datadir=/var/lib/mysql
>   >    --log-error=/var/log/mysqld.log --user=mysql
> 
>   # grep '/mnt/foo/mysql/config' /var/log/mysqld.log 
>   Could not open required defaults file: /mnt/foo/mysql/config
>                                                          ^^^
>                                                         OOPS#2b
> 
>  - well, no good, let's try a workaround sketched in point 2. above:
> 
>   # pcs resource create mysql-test2 ocf:heartbeat:mysql \
>     'config="/mnt/foo/mysql/config final.cnf"'
>     (outer singlequotes so as to preserve inner doublequotes)
> 
>   # pcs resource debug-start --full mysql-test2 \
>     | grep '/mnt/foo/mysql/config'
>  >  stderr: ++ 07:06:44: 52: : '"/mnt/foo/mysql/config' 'final.cnf"'
>  >  stderr: + 07:06:44: mysql_common_validate:108: \
>       '[' '!' -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
>                                   ^^^
>                                  OOPS#1b ???
>  >  stderr: + 07:06:45: mysql_common_start:209: /usr/bin/mysqld_safe \
>       '--defaults-file="/mnt/foo/mysql/config' 'final.cnf"' \
>                                            ^^^
>                                            OOPS#2a
>       --pid-file=/var/run/mysql/mysqld.pid \
>       --socket=/var/lib/mysql/mysql.sock --datadir=/var/lib/mysql \
>       --log-error=/var/log/mysqld.log --user=mysql
> 
>     # grep '/mnt/foo/mysql/config' /var/log/mysqld.log 
>     Could not open required defaults file: /root/"/mnt/foo/mysql/config
>                                                          ^^^
>                                                         OOPS#2b
> 
> 
> Possible solutions:
> 
> A. claim whitespace as invalid character in agents' parameters
>    o seems pretty dull solution
>      - we shouldn't limit the domain that hasn't been limited like that
>        from the beginning
> 
> B. smart quoting logic per 1. on the Pacemaker side to accommodate for
>    shell  scripts deficiencies causing the stated issues
>    (like whitespace as a proper radix of parameter -> add quotes)
>    o non-systemic
>      - no guarantee all shell-based agents are broken in the same way
>      - would almost certainly break non-shell-based agents
> 
> C. fix the shell-based resource agents
> 
>    o C0. use doublequotes whenever suitable (like the tests denoted with
>          OOPS#1 above) in the shell code
> 
>    o C1. (optionally) smart quoting logic per 1. directly in the agents
>      - otherwise, a convention would have to be introduced that user
>        has to quote the parameters on her own when configuring the
>        cluster -- suboptimal as there would be artificial distinction
>        betweeb shell-based RAs and the rest (if any)
> 
>    o C2. instead of invoking the underlying commands:
>            $exec $param1 $param2
>          use:
>            eval $exec $param1 $param2
>          to ensure that the quoting from C1. will be taken into account
>          properly and not passed as part of argv items
>      - together with C1., this would fix OOPS#2
>      - possibly, there could also be OCF helper's level function to unify
>        the approach
> 
> 
> What the community thinks?  I hope this is not too crazy :-)
> Let's get rid of these remaining rough edges.
> 
> 
> Some related bugs, these are against pcs but other management tools may
> be similarly affected (or be using [influential] approaches already):
> - https://bugzilla.redhat.com/show_bug.cgi?id=1219574 
>   [gui] resource optional arguments: quoted strings missing
> - https://bugzilla.redhat.com/show_bug.cgi?id=1220778 
>   [RFC] should pcs escape double-quotes to prevent clashes/irregularity
with
>   string-denoting ones in parameter printouts?
> 
> 
> P.S. OCF list CC'd only for posterity and to remind its audience that the
>      other lists (developers and users at ClusterLabs) have become
>      preferred venues to discuss these things (this in particular
>      is a better fit for the former, admittedly).
> 
> -- 
> Jan