Hello Willy,
Apologies for the delay.
Thank you for taking the time to review.
> On Jun 2, 2019, at 10:11 PM, Willy Tarreau <w...@1wt.eu> wrote:
>
> Hi Daniel,
>
> On Thu, May 30, 2019 at 12:57:10AM -0400, Daniel Corbett wrote:
>> Hello,
>>
>>
>> When communicating over SPOP the AGENT-HELLO, AGENT-DISCONNECT,
>> and ACK frames must have the FIN flag set.
>>
>> This patch also upgrades the SPOP_VERSION to 2.0 and adds the
>> "ip_score" transaction variable to the python & lua scripts.
>>
>> Thanks to Christopher for giving me the answer on how to solve this issue.
>
> Are all these changes related ? It seems to me these are distinct
> features, or maybe even a bug fix plus an extra feature, in which
> case these should be separate patches.
Something like that :)
They are all changes to make the spoa_server work out of the box as currently
it's not working with 2.0-dev.
I have gone ahead and separated the patches.
>
> I'm having a few quick questions below.
>
>> From e0e7992b3b2af3ac0e898d48d6ce5f7e9416b568 Mon Sep 17 00:00:00 2001
>> From: Daniel Corbett <dcorb...@haproxy.com>
>> Date: Wed, 29 May 2019 17:44:05 -0400
>> Subject: [PATCH] BUG/MEDIUM: contrib/spoa_server: Set FIN flag on agent
>> frames
>>
>> When communicating over SPOP the AGENT-HELLO, AGENT-DISCONNECT,
>> and ACK frames must have the FIN flag set.
>>
>> This patch also upgrades the SPOP_VERSION to 2.0 and adds the
>> "ip_score" transaction variable to the python & lua scripts.
> (...)
>> diff --git a/contrib/spoa_server/ps_lua.lua b/contrib/spoa_server/ps_lua.lua
>> index 26620459..aaf373c6 100644
>> --- a/contrib/spoa_server/ps_lua.lua
>> +++ b/contrib/spoa_server/ps_lua.lua
>> @@ -14,4 +14,5 @@ spoa.register_message("check-client-ip", function(args)
>> spoa.set_var_ipv6("ipv6", spoa.scope.txn, "1::f")
>> spoa.set_var_str("str", spoa.scope.txn, "1::f")
>> spoa.set_var_bin("bin", spoa.scope.txn, "1::f")
>> + spoa.set_var_int32("ip_score", spoa.scope.txn, 77)
>
> Be careful, your editor seems to have mixed spaces and tabs here, and
> a few other times in the same patch.
>
Sorry about that. I believe it was mainly due to python indention errors that
I was dealing with :(
>> diff --git a/contrib/spoa_server/spoa-server.conf
>> b/contrib/spoa_server/spoa-server.conf
>> index 13bd126a..87bbb721 100644
>> --- a/contrib/spoa_server/spoa-server.conf
>> +++ b/contrib/spoa_server/spoa-server.conf
>> @@ -1,5 +1,6 @@
>> global
>> debug
>> + nbthread 1
>
> What is the reason for forcing nbthread to 1 here ? Did you face a
> particular bug revealed by threads ? Given that many people use example
> configs as starting point for theirs, I'd rather avoid doing this, or
> this will last 10 years just like "option httpclose" or even the funny
> "ssl-engine rdrand" which in addition to being wrong was so much copied
> that google now auto-completes it when searching "ssl-engine haproxy"!
>
Yes I have hit a bug where the SPOA results are blank between requests if
threading is enabled. I found the quick fix to be setting nbthread to 1 --
however, I agree with you that we must be careful with these settings as I
realize they can persist for long after their intended need.
Unfortunately, I'm not really sure where to start fixing this issue.
>> diff --git a/contrib/spoa_server/spoa.c b/contrib/spoa_server/spoa.c
>> index a958f222..8cb40c54 100644
>> --- a/contrib/spoa_server/spoa.c
>> +++ b/contrib/spoa_server/spoa.c
>> @@ -679,13 +679,16 @@ error:
>> * the number of written bytes otherwise. */
>> static void prepare_agentack(struct worker *w)
>> {
>> + unsigned int flags = SPOE_FRM_FL_FIN;
>> +
>> w->ack_len = 0;
>>
>> /* Frame type */
>> w->ack[w->ack_len++] = SPOE_FRM_T_AGENT_ACK;
>>
>> - /* No flags for now */
>> - memset(w->ack + w->ack_len, 0, 4); /* No flags */
>> + /* Set flags */
>> + flags = htonl(flags);
>
> In general it's not a good idea to have the same variable hold two
> different endian versions of a same value. You can be certain that
> at one point something will be added above or below and will use the
> wrong one. Better simply do "flags = htonl(SPOE_FRM_FL_FIN)" or even
> better, preset flags to zero at the beginning of the function, and
> then add this flag this way : "flags |= htonl(SPOE_FRM_FL_FIN)".
>
>> + memcpy(w->ack + w->ack_len, (char *)&flags, 4);
>
> You don't need the "(char*)" cast here, "&flags" is correct.
>
> The same applies to the two or three other locations. Thanks!
> Willy
>
Thanks for the info. Truthfully, I copied most of this from
contrib/spoa_example/spoa.c
I've fixed with your recommendations.
Thanks,
-- Daniel
0001-MINOR-contrib-spoa_server-Upgrade-SPOP-to-2.0.patch
Description: Binary data
0002-BUG-MEDIUM-contrib-spoa_server-Set-FIN-flag-on-agent.patch
Description: Binary data
0003-MINOR-contrib-spoa_server-Add-random-IP-score.patch
Description: Binary data
0004-DOC-MINOR-contrib-spoa_server-Fix-typo-in-README.patch
Description: Binary data
