Hello Willy,
On Fri, Apr 6, 2018 at 12:02 PM, Willy Tarreau <[email protected]> wrote:
>
> Hi Aurélien,
>
> I finally found some time to review your patch, really sorry for the
> long delay, but bug fixing passes first when they are sensitive :-/
No problem. Adding features over fixing bugs would be quite disturbing. :)
> > Examples:
> > $ echo -e "multiline;set ssl ocsp-response $(base64 ocsp.der)\n" | socat
> > /tmp/sock1 -
> > $ echo -e "multiline ; show\nstat\n" | socat /tmp/sock1 -
>
> I must confess I feel a bit embarrassed by this. I think it isn't a good
> idea for the long term to mix arguments and payload. And the example you
> put below in the doc scares me a little bit :
>
> > set
> + server TEST/A
> + state
> + ready
> +
>
> It's a bit complicated for me to express why I'm not thrilled, I just
> feel it's not right, without having a strong argument. Among the things
> I'm thinking about is the fact that the command line arguments are
> delimited by spaces, so above, "server" and "TEST/A" are two different
> arguments. Then I don't see how we can properly process a *real* payload
> containing spaces, like a PEM formated certificate, without starting to
> run into dirty hacks.
Well, in that case the dirty hack would be what I did in
ocsp-response: treat everything from args[3] as being the payload,
with one argument = one line.
I admit that's quite hackish; I tried to be the less intrusive as
possible into the CLI parser so I tried to reuse what was already
done.
Maybe that's not the best approach but with all the legacy behind I
REALLY didn't want to break something, even if it lands on a
development branch :).
>
> Also, I'm pretty sure that most, if not all, of the commands consuming a
> payload will know how to process it based on the command line arguments.
> So making the payload appear as an argument (or mixed with them) will
> make it harder to conditionally process them.
Yes, at first I designed a solution that involved parsers cooperation
but there were ambiguous cases, especially when the command takes
optional arguments.
It's totally doable but at that time I thought that would make too big
a change that could cause issues or break use cases I didn't think
about.
>
> Another possibly more problematic point is that if you pass a payload as
> an argument, it will be processed at once, thus will always be limited
> to a buffer size, so we won't be able to use this to update maps or ACLs
> for example.
That is very true and in fact I had to increase MAX_STATS_ARGS to an
arbitrary value in my SSL on-the-fly patch.
> I agree with the point you made about keeping the parser independant of
> the commands so that it doesn't have to know which one takes a payload
> and which one not. But here I fear that we'll end up causing the opposite
> problem by forcing all commands to run into tricks to distinguish their
> args and the payload. For example, let's say we use it to add a map
> between HTTP status codes and reasons :
>
> > add map http_status
> + 200 OK
> + 206 Partial Content
> + 302 Found
> + 303 See Other
> + 304 Not Modified
> ...
>
> If it's cut along words, you can as well enter it this way :
>
> > add map
> + http_status
> + 200 OK
> + 206 Partial Content
> + 302 Found
> + 303 See Other
> + 304 Not Modified
> ...
>
> Or even this way :
>
> > add map
> + http_status 200 OK
> + 206 Partial Content
> + 302 Found
> + 303 See Other
> + 304 Not Modified
> ...
>
> Or this way as well :
>
> > add map
> + http_status 200 OK 206 Partial Content 302 Found 303 See Other 304 Not
> Modified
> ...
>
> You probably see the problem : there's no way to cut the arguments anymore.
Indeed, I didn't think about this use case.
> By having arguments and payload separate, both can continue to use their
> respective, non-colliding syntaxes, so that the command's arguments are
> delimited by spaces and only spaces, and are all present once the command
> is started, and that the payload's syntax solely depends on the command
> and is a byte stream ending on an empty line (ie \n\n). This way commands
> consuming multiple entries at once (like maps, acls, crtlists) can easily
> keep the on-disk file format and consider that one line equals one entry,
> and other commands taking more complex formats (like certificates) can use
> a different assumption, including the option to pass the whole stream to
> an underlying framework (typically openssl).
>
> Given that there are very few commands consuming a payload, I think we
> should not use a sticky mode. The multiline prefix should only apply to
> the next command so that there is no need to do this :
>
> > multiline;add map http_status
> + 200 OK
> + 206 Partial Content
> + 302 Found
> + 303 See Other
> + 304 Not Modified
> +
> > multiline
>
> It could thus be shortened (eg: "ml") or even be a different character,
> though I don't really see which one.
>
> Or probably much better, we could end a command with a character indicating
> that a payload follows, a bit a-la "<< EOF" we're used to see in shell.
> Maybe just using "<<" at the end of a line could be sufficient to indicate
> that a payload follows and that it must be ended by an empty line. Eg:
>
> > add map http_status <<
> + 200 OK
> + 206 Partial Content
> + 302 Found
> + 303 See Other
> + 304 Not Modified
> +
> >
Using a special pattern is a good idea. Let's start with "<<" and
tweak it later if it collides with what can be found in a payload.
>
> The benefit here is that it's still the parser which knows whether or
> not a payload follows, regardless of the command, and can simply pass
> a flag to the command saying "you have a payload to read".
Yes, in fact that's exactly what I did at first but I really thought
it was too ambitious and subject to compatibility-breaking (again due
to my lack of background on the CLI/HAProxy usage).
I realize I wanted to be too generic to be sure I would not break
anything but I ended doing something too light.
>
> While typing this I was also thinking that we could also put this as a
> flag in the CLI keywords registration (ie takes a payload or not) but
> I'm not convinced this would be a good idea because it would mean that
> the user (or script) would have to know whether or not the command
> expects a payload, and could quickly get out of sync on the first mistake.
>
> I think that it is reasonable for a first implementation to have some
> technical limitations, such as supporting only a single-buffer payload
> (ie not implement the streaming mode first). It can be documented that
> no more than "tune.bufsize" bytes may be sent at once in a single
> command, and scripts feeding large data chunks will simply have to cut
> them into pieces (eg: ACLs and maps). This will allow you to make the
> parser wait for the empty line when seeing that a multi-line block is
> being sent, and simply pass the pointer to this block to the command,
> doing everything at once (which is less complex). And this will not
> prevent us from removing this limitation later if/once it really
> becomes annoying.
>
Ok, I'll do that in a way it will be easy to enhance.
> Just a very minor cosmetic comments below :
>
> > diff --git a/include/types/applet.h b/include/types/applet.h
> > index 89c318c1d..9789e202a 100644
> > --- a/include/types/applet.h
> > +++ b/include/types/applet.h
> > @@ -57,8 +57,11 @@ struct appctx {
> > /* 3 unused bytes here */
> > unsigned short state; /* Internal appctx state */
> > unsigned int st0; /* CLI state for stats, session state for
> > peers */
> > - unsigned int st1; /* prompt for stats, session error for
> > peers */
> > + unsigned int st1; /* prompt/multiline for stats, session
> > error for peers */
> > +#define APPCTX_CLI_ST1_PROMPT (1 << 0)
> > +#define APPCTX_CLI_ST1_MULTILINE (1 << 1)
>
> Please don't put #define in the middle of the structs, they're harder to
> find later when reading the code, and it's harder also to make other
> defines depend on them. Better place them at the top of the file with the
> other ones. Simply mention in st1 that it makes use of APPCTX_CLI_ST1_*
> and that's fine.
Sure.
--
Aurélien Nephtali
