Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> On 20 Apr 2018, at 09:52, Luca Toscano <toscano.l...@gmail.com> wrote: > 2018-04-20 16:27 GMT+02:00 Jim Riggs <jim...@riggs.me>: > > On 20 Apr 2018, at 08:53, Jim Jagielski <j...@jagunet.com> wrote: > > > > Sorry for coming in late, but what is the exact issue we are trying to > > solve again? My understanding was that if someone wanted something like > > > > ErrorLog "syslog-httpd.log" > > > > that the current implementation would, incorrectly, send the log data to > > syslogd. Is that right? > > Luca is working PR 62102 which has to do with "syslog:...:...", but that > unveiled an inconsistency between core.c and log.c. Before his proposed patch > in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing > strncasecmp(). We have been discussing standardizing both on strn?cmp(), but > that would potentially lead to "breaking" configs that use "SYSLOG" rather > than "syslog". > > > I don't believe that they two things are separate (core/log.c), since the > former checks for ErrorLog's parameter sanity and the latter sets it, so it > would be weird in my opinion if the two logic were different. This is why I > liked your consistency proposal Jim, and applied to my patch. In our docs we > clearly specify to use "syslog" in lowercase, so as far as I can see it using > "SYSLOG" would not be something that people should use.. Agreed, and I believe we should go forward with this. I just wanted to at least bring it up, since regressions and breaking existing configs has been a touchy subject lately. :-)
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> On 20 Apr 2018, at 08:53, Jim Jagielskiwrote: > > Sorry for coming in late, but what is the exact issue we are trying to solve > again? My understanding was that if someone wanted something like > > ErrorLog "syslog-httpd.log" > > that the current implementation would, incorrectly, send the log data to > syslogd. Is that right? Luca is working PR 62102 which has to do with "syslog:...:...", but that unveiled an inconsistency between core.c and log.c. Before his proposed patch in STATUS, core.c is doing a strcmp() for "syslog" whereas log.c is doing strncasecmp(). We have been discussing standardizing both on strn?cmp(), but that would potentially lead to "breaking" configs that use "SYSLOG" rather than "syslog".
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
> On 20 Apr 2018, at 01:42, Luca Toscano <toscano.l...@gmail.com> wrote: > 2018-04-19 17:49 GMT+02:00 Jim Riggs <j...@riggs.me>: > Luca - > > Here's the same thing standardizing on strn?cmp(). Not that you couldn't have > done it yourself, but since I had it up, maybe this will save you 30 seconds. > ;-) > > > Thanks a lot! I added your last suggestions to r1829626 and also a CHANGES > entry. I tested it on my local environment and it seems working fine, let me > know if everything looks good or if anything is missing. I'm fine with it, *BUT* in light of our recent discussions about backports/fixes causing regressions in existing configs, this could cause another. I agree with what we have done in this patch, but we could potentially break existing configs, no? For example: ErrorLog "SYSLOG" This will send to syslog before the patch but will send to file /SYSLOG after. Ugh. So, should we not touch server/log.c???
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Luca - Here's the same thing standardizing on strn?cmp(). Not that you couldn't have done it yourself, but since I had it up, maybe this will save you 30 seconds. ;-) Index: server/core.c === --- server/core.c (revision 1829439) +++ server/core.c (working copy) @@ -4867,7 +4867,8 @@ static int check_errorlog_dir(apr_pool_t *p, server_rec *s) { if (!s->error_fname || s->error_fname[0] == '|' -|| strcmp(s->error_fname, "syslog") == 0) { +|| strcmp(s->error_fname, "syslog") == 0 +|| strncmp(s->error_fname, "syslog:", 7) == 0) { return APR_SUCCESS; } else { @@ -5281,7 +5282,9 @@ apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); tmp = ap_server_root_relative(p, sconf->ap_document_root); apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) +if (s->error_fname[0] != '|' +&& strcmp(s->error_fname, "syslog") != 0 +&& strncmp(s->error_fname, "syslog:", 7) != 0) tmp = ap_server_root_relative(p, s->error_fname); else tmp = s->error_fname; @@ -5456,4 +5459,3 @@ core_cmds,/* command apr_table_t */ register_hooks/* register hooks */ }; - Index: server/log.c === --- server/log.c(revision 1829439) +++ server/log.c(working copy) @@ -396,7 +396,8 @@ } #ifdef HAVE_SYSLOG -else if (!strncasecmp(s->error_fname, "syslog", 6)) { +else if (strcmp(s->error_fname, "syslog") == 0 + || strncmp(s->error_fname, "syslog:", 7) == 0) { if ((fname = strchr(s->error_fname, ':'))) { /* s->error_fname could be [level]:[tag] (see #60525) */ const char *tag; > On 18 Apr 2018, at 13:32, Luca Toscano <toscano.l...@gmail.com> wrote: > > Thanks a lot Jim! I like your code change and the extra checks, but I'd > prefer to use strncmp if possible, also in log.c. > Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in > CHANGES too, your name should be on it :). > > Luca > > 2018-04-18 18:36 GMT+02:00 Jim Riggs <j...@riggs.me>: > Fair enough. I'm fine standardizing either way. strn?cmp() is probably more > "correct". As it stands, though, the check in core is not actually checking > everything that log.c will handle. > > >> On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote: >> >> On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote: >>> I didn't think of this before, but there is one edge case this would miss: >>> if someone (for whatever reason) wants a relative ErrorLog *file* named >>> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog >>> "syslog.log"'. It appears that this is already broken in server/log.c, >>> though. Also, log.c is using str(n)casecmp. The following would make things >>> consistent and handle this weird edge case. Thoughts? >>> >>> Index: server/core.c >>> === >>> --- server/core.c (revision 1829431) >>> +++ server/core.c (working copy) >>> @@ -4867,7 +4867,8 @@ >>> static int check_errorlog_dir(apr_pool_t *p, server_rec *s) >>> { >>>if (!s->error_fname || s->error_fname[0] == '|' >>> -|| strcmp(s->error_fname, "syslog") == 0) { >>> +|| strcasecmp(s->error_fname, "syslog") == 0 >>> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) { >>>return APR_SUCCESS; >>>} >>>else { >>> @@ -5281,7 +5282,9 @@ >>>apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); >>>tmp = ap_server_root_relative(p, sconf->ap_document_root); >>>apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); >>> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) >>> +if (s->error_fname[0] != '|' >>> +&& strcasecmp(s->error_fname, "syslog") != 0 >>> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0) >>>tmp = ap_server_root_relative(p, s->error_fname); >>>else >>>tmp = s-
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Luca - Here's the same thing standardizing on strn?cmp(). Not that you couldn't have done it yourself, but since I had it up, maybe this will save you 30 seconds. ;-) Index: server/core.c === --- server/core.c (revision 1829439) +++ server/core.c (working copy) @@ -4867,7 +4867,8 @@ static int check_errorlog_dir(apr_pool_t *p, server_rec *s) { if (!s->error_fname || s->error_fname[0] == '|' -|| strcmp(s->error_fname, "syslog") == 0) { +|| strcmp(s->error_fname, "syslog") == 0 +|| strncmp(s->error_fname, "syslog:", 7) == 0) { return APR_SUCCESS; } else { @@ -5281,7 +5282,9 @@ apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); tmp = ap_server_root_relative(p, sconf->ap_document_root); apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) +if (s->error_fname[0] != '|' +&& strcmp(s->error_fname, "syslog") != 0 +&& strncmp(s->error_fname, "syslog:", 7) != 0) tmp = ap_server_root_relative(p, s->error_fname); else tmp = s->error_fname; @@ -5456,4 +5459,3 @@ core_cmds,/* command apr_table_t */ register_hooks/* register hooks */ }; - Index: server/log.c === --- server/log.c(revision 1829439) +++ server/log.c(working copy) @@ -396,7 +396,8 @@ } #ifdef HAVE_SYSLOG -else if (!strncasecmp(s->error_fname, "syslog", 6)) { +else if (strcmp(s->error_fname, "syslog") == 0 + || strncmp(s->error_fname, "syslog:", 7) == 0) { if ((fname = strchr(s->error_fname, ':'))) { /* s->error_fname could be [level]:[tag] (see #60525) */ const char *tag; > On 18 Apr 2018, at 13:32, Luca Toscano <toscano.l...@gmail.com> wrote: > > Thanks a lot Jim! I like your code change and the extra checks, but I'd > prefer to use strncmp if possible, also in log.c. > Feel free to amend the patch, or I'll do it tomorrow (I forgot the entry in > CHANGES too, your name should be on it :). > > Luca > > 2018-04-18 18:36 GMT+02:00 Jim Riggs <j...@riggs.me>: > Fair enough. I'm fine standardizing either way. strn?cmp() is probably more > "correct". As it stands, though, the check in core is not actually checking > everything that log.c will handle. > > > > On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote: > > > > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote: > >> I didn't think of this before, but there is one edge case this would miss: > >> if someone (for whatever reason) wants a relative ErrorLog *file* named > >> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog > >> "syslog.log"'. It appears that this is already broken in server/log.c, > >> though. Also, log.c is using str(n)casecmp. The following would make > >> things consistent and handle this weird edge case. Thoughts? > >> > >> Index: server/core.c > >> === > >> --- server/core.c (revision 1829431) > >> +++ server/core.c (working copy) > >> @@ -4867,7 +4867,8 @@ > >> static int check_errorlog_dir(apr_pool_t *p, server_rec *s) > >> { > >> if (!s->error_fname || s->error_fname[0] == '|' > >> -|| strcmp(s->error_fname, "syslog") == 0) { > >> +|| strcasecmp(s->error_fname, "syslog") == 0 > >> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) { > >> return APR_SUCCESS; > >> } > >> else { > >> @@ -5281,7 +5282,9 @@ > >> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); > >> tmp = ap_server_root_relative(p, sconf->ap_document_root); > >> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); > >> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) > >> +if (s->error_fname[0] != '|' > >> +&& strcasecmp(s->error_fname, "syslog") != 0 > >> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0) > >> tmp = ap_serve
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
Fair enough. I'm fine standardizing either way. strn?cmp() is probably more "correct". As it stands, though, the check in core is not actually checking everything that log.c will handle. > On 18 Apr 2018, at 10:15, William A Rowe Jr <wr...@rowe-clan.net> wrote: > > On Wed, Apr 18, 2018 at 7:17 AM, Jim Riggs <jim...@riggs.me> wrote: >> I didn't think of this before, but there is one edge case this would miss: >> if someone (for whatever reason) wants a relative ErrorLog *file* named >> `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog >> "syslog.log"'. It appears that this is already broken in server/log.c, >> though. Also, log.c is using str(n)casecmp. The following would make things >> consistent and handle this weird edge case. Thoughts? >> >> Index: server/core.c >> === >> --- server/core.c (revision 1829431) >> +++ server/core.c (working copy) >> @@ -4867,7 +4867,8 @@ >> static int check_errorlog_dir(apr_pool_t *p, server_rec *s) >> { >> if (!s->error_fname || s->error_fname[0] == '|' >> -|| strcmp(s->error_fname, "syslog") == 0) { >> +|| strcasecmp(s->error_fname, "syslog") == 0 >> +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) { >> return APR_SUCCESS; >> } >> else { >> @@ -5281,7 +5282,9 @@ >> apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); >> tmp = ap_server_root_relative(p, sconf->ap_document_root); >> apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); >> -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) >> +if (s->error_fname[0] != '|' >> +&& strcasecmp(s->error_fname, "syslog") != 0 >> +&& strncasecmp(s->error_fname, "syslog:", 7) != 0) >> tmp = ap_server_root_relative(p, s->error_fname); >> else >> tmp = s->error_fname; >> @@ -5456,4 +5459,3 @@ >> core_cmds,/* command apr_table_t */ >> register_hooks/* register hooks */ >> }; >> - >> Index: server/log.c >> === >> --- server/log.c(revision 1829431) >> +++ server/log.c(working copy) >> @@ -396,7 +396,8 @@ >> } >> >> #ifdef HAVE_SYSLOG >> -else if (!strncasecmp(s->error_fname, "syslog", 6)) { >> +else if (strcasecmp(s->error_fname, "syslog") == 0 >> + || strncasecmp(s->error_fname, "syslog:", 7) == 0) { >> if ((fname = strchr(s->error_fname, ':'))) { >> /* s->error_fname could be [level]:[tag] (see #60525) */ >> const char *tag; > > Shouldn't we normalize the use of strcmp instead of strcasecmp? > In any case it must be entirely normalized to one or the other. > > Linux is a case-sensitive OS in the first place, and if configured > with SYSLOG: today it is broken when you hit core, which ignores > that code path. Since the behavior has always been syslog: I'm > not seeing a benefit to case insensitivity.
Re: svn commit: r1829430 - /httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch
I didn't think of this before, but there is one edge case this would miss: if someone (for whatever reason) wants a relative ErrorLog *file* named `syslog*', for example `ErrorLog "syslog-httpd.log"' or `ErrorLog "syslog.log"'. It appears that this is already broken in server/log.c, though. Also, log.c is using str(n)casecmp. The following would make things consistent and handle this weird edge case. Thoughts? Index: server/core.c === --- server/core.c (revision 1829431) +++ server/core.c (working copy) @@ -4867,7 +4867,8 @@ static int check_errorlog_dir(apr_pool_t *p, server_rec *s) { if (!s->error_fname || s->error_fname[0] == '|' -|| strcmp(s->error_fname, "syslog") == 0) { +|| strcasecmp(s->error_fname, "syslog") == 0 +|| strncasecmp(s->error_fname, "syslog:", 7) == 0) { return APR_SUCCESS; } else { @@ -5281,7 +5282,9 @@ apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); tmp = ap_server_root_relative(p, sconf->ap_document_root); apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); -if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) +if (s->error_fname[0] != '|' +&& strcasecmp(s->error_fname, "syslog") != 0 +&& strncasecmp(s->error_fname, "syslog:", 7) != 0) tmp = ap_server_root_relative(p, s->error_fname); else tmp = s->error_fname; @@ -5456,4 +5459,3 @@ core_cmds,/* command apr_table_t */ register_hooks/* register hooks */ }; - Index: server/log.c === --- server/log.c(revision 1829431) +++ server/log.c(working copy) @@ -396,7 +396,8 @@ } #ifdef HAVE_SYSLOG -else if (!strncasecmp(s->error_fname, "syslog", 6)) { +else if (strcasecmp(s->error_fname, "syslog") == 0 + || strncasecmp(s->error_fname, "syslog:", 7) == 0) { if ((fname = strchr(s->error_fname, ':'))) { /* s->error_fname could be [level]:[tag] (see #60525) */ const char *tag; > On 18 Apr 2018, at 04:57, elu...@apache.org wrote: > > Author: elukey > Date: Wed Apr 18 09:57:08 2018 > New Revision: 1829430 > > URL: http://svn.apache.org/viewvc?rev=1829430=rev > Log: > core-check_errorlog_dir_syslog.patch: add suggestions from STATUS > > Credis to jhriggs and jailletc36 > > > Modified: >httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch > > Modified: httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch > URL: > http://svn.apache.org/viewvc/httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch?rev=1829430=1829429=1829430=diff > == > --- httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch (original) > +++ httpd/httpd/patches/2.4.x/core-check_errorlog_dir_syslog.patch Wed Apr 18 > 09:57:08 2018 > @@ -1,14 +1,23 @@ > Index: server/core.c > === > server/core.c (revision 1829090) > +--- server/core.c (revision 1829429) > +++ server/core.c (working copy) > @@ -4867,7 +4867,7 @@ > static int check_errorlog_dir(apr_pool_t *p, server_rec *s) > { > if (!s->error_fname || s->error_fname[0] == '|' > -|| strcmp(s->error_fname, "syslog") == 0) { > -+|| !strncmp(s->error_fname, "syslog", 6)) { > ++|| strncmp(s->error_fname, "syslog", 6) == 0) { > return APR_SUCCESS; > } > else { > +@@ -5281,7 +5281,7 @@ > + apr_file_printf(out, "ServerRoot: \"%s\"\n", ap_server_root); > + tmp = ap_server_root_relative(p, sconf->ap_document_root); > + apr_file_printf(out, "Main DocumentRoot: \"%s\"\n", tmp); > +-if (s->error_fname[0] != '|' && strcmp(s->error_fname, "syslog") != 0) > ++if (s->error_fname[0] != '|' && strncmp(s->error_fname, "syslog", 6) != > 0) > + tmp = ap_server_root_relative(p, s->error_fname); > + else > + tmp = s->error_fname; >
Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]
On 11 Apr 2018, at 07:11, jhri...@apache.org wrote: > > Author: jhriggs > Date: Wed Apr 11 12:11:05 2018 > New Revision: 1828890 > > URL: http://svn.apache.org/viewvc?rev=1828890=rev > Log: > mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot > spare members are > used as drop-in replacements for unusable workers in the same load balancer > set. This differs > from hot standbys which are only used when all workers in a set are unusable. > PR 61140. Speaking of balancer member types, hot spares (new), and hot standbys (existing), is there really a need for hot standbys? When I first started using mod_proxy_balancer many years ago, I actually thought/assumed that "hot standbys" had the behavior that the new spares now have (i.e. drop-in replacements for unavailable workers) until I studied the documentation more carefully. Standbys are really superfluous, since the same behavior can be achieved by sets. For example, the behavior of the following will be exactly the same: ProxySet lbmethod=bytraffic BalancerMember "http://server1/; lbset=0 BalancerMember "http://server2/; lbset=0 BalancerMember "http://server3/; lbset=0 status=+H BalancerMember "http://server4/; lbset=1 BalancerMember "http://server5/; lbset=1 BalancerMember "http://server6/; lbset=1 status=+H ProxySet lbmethod=bytraffic BalancerMember "http://server1/; lbset=0 BalancerMember "http://server2/; lbset=0 BalancerMember "http://server3/; lbset=1 BalancerMember "http://server4/; lbset=2 BalancerMember "http://server5/; lbset=2 BalancerMember "http://server6/; lbset=3 So, is there really any reason to keep hot standbys? Do we consider the concept of the +H flag to be easier to understand or use than LB sets? If so, do we just need better docs? Am I missing any nuances of the H flag? I would love to see standbys go away, personally. - Jim
Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/
> On 11 Apr 2018, at 08:28, Yann Ylavicwrote: > > On Wed, Apr 11, 2018 at 2:11 PM, wrote: >> Author: jhriggs >> Date: Wed Apr 11 12:11:05 2018 >> New Revision: 1828890 >> >> URL: http://svn.apache.org/viewvc?rev=1828890=rev >> Log: >> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). >> Hot spare members are >> used as drop-in replacements for unusable workers in the same load balancer >> set. This differs >> from hot standbys which are only used when all workers in a set are >> unusable. PR 61140. > > Nice ap_proxy_balancer_get_best_worker() simplification Jim. > > Maybe it could have been a separate commit than the spare members > addition though, not mixing refactoring and features. > Staging helps review IHMO. Thanks for the feedback, Yann! I never really viewed ap_proxy_balancer_get_best_worker() as a refactor separate from the hot spare change that required it, though I suppose it definitely could have stood alone. It did remove a lot of duplicate code. Basically, that commit is just an existing patch that has been out there floating around since ACNA last year that I just updated to the current codebase. In the dev@ thread at the time, we never discussed splitting them either. Regardless, I'm glad to have the hot spare functionality that several of us have always thought was missing, and I'm glad I didn't appear to b0rk the entire repo in the process. Thanks for taking it easy on the new guy. ;-) - Jim
Re: A little nit
> On 3 Aug 2017, at 08:30, Jim Jagielskiwrote: > > It's just GUI magic... Basically, it will internally take '1.1' and > convert it to 11, 1.0 to 10, etc... If that's the case, I would recommend going 2 decimal places (1.1 = 100, 1.25 = 125, etc.) to allow using percentages. I could see people wanting to be able to favor a backend by 25%, for example, by using 1.25. Any more than 2 decimal places could be rounded to 2.
Re: A little nit
> On 2 Aug 2017, at 12:33, Jim Jagielskiwrote: > > I'll be adding some code to allow for lbfactors to be > single decimal numbers (like 1.1, 2.5, etc...)... People > have asked "How do I change it so that machine B is like 10% > preferred" and I mention that "Well, you could make one a > 10 and the other an 11" but that confuses people. :/ Is this just some GUI/config magic with the existing int (e.g. shifting decimal places), or are you thinking of changing lbfactor to a FP type in the struct? It would be really nice if lbfactor (and lbstatus?) were floating point. I have had a couple of instances where that would have helped me do something, but I realize that would not an insignificant change to the code or its compatibility.
Re: 2.4.27
> On 11 Jul 2017, at 11:46, James Clooswrote: > >> "JJ" == Jim Jagielski writes: > > JJ> *) mod_http2: disable and give warning when mpm_prefork is encountered. > The server will > JJ> continue to work, but HTTP/2 will no longer be negotiated. [Stefan > Eissing] > > Why break h2 w/ prefork. > > AIUI, one still needs to use prefork with the perl and php modules. > > And it works fine here with 2.4.25. > > That change makes 2.4.27 worthless. See previous discussion in the "2.4.27" thread, specifically: https://lists.apache.org/thread.html/bae472cadaeeb761b88bb4569cc0b7d87bc2dcb2fbcbf472d895f32e@%3Cdev.httpd.apache.org%3E
Re: svn commit: r20021 - /dev/httpd/
I don't know that it really matters, but this guy is in there twice (in each CHANGES doc), once with the PR # and once without: > + *) mod_proxy: Allow the per-request environment variable "no-proxy" to > + be used as an alternative to ProxyPass /path !. This is primarily > + to set exceptions for ProxyPass specified in context. > +Use SetEnvIf, not SetEnv. [Eric Covener] > + *) mod_proxy: Allow the per-request environment variable "no-proxy" to > + be used as an alternative to ProxyPass /path !. This is primarily > + to set exceptions for ProxyPass specified in context. > + Use SetEnvIf, not SetEnv. PR 60458. [Eric Covener] > +
Re: Ideas from ApacheCon
> On 2 Jun 2017, at 07:20, Jim Jagielski <j...@jagunet.com> wrote: > >> On Jun 1, 2017, at 1:29 PM, Jim Riggs <apache-li...@riggs.me> wrote: >> >> Regardless, even worst case, we are looking at what, iterating 6 pointers >> instead of 3 or 10 instead of 5? We probably have some lower hanging fruit >> across the request lifecycle code to increase performance than saving some >> arithmetic on a handful of structs, no? ;-) >> > > Oh I agree... it just seems that we add overhead during the > normal and nominal case (everybody is up and available) and > that the overhead is worse the more workers we have. I am > personally OK with abnormal situations being slower > > And yeah, I also have never like the duplication that's inherent > in all the lbmethods, and so I really like it being simplified. > But I could never figure out a way to remove that without making > it more complex or slower. Last idea I had was to create some > sort of looping cpp MACRO to encapsulate it at some level :/ Yeah. Take a look at the callback version I submitted last night. It has reduced the code in the lbmethods to almost nothing. The find_best_*() functions just call ap_proxy_balancer_get_best_worker() with a small, quick callback function and then do a little housekeeping. Nothing else. BUT, this implementation makes it extremely flexible if someone want to create a complex lbmethod. It can do whatever special/custom things it wants to in its find_best_() function and/or in the is_best_() callback...or maybe it wants to implement its own iteration for finding usable workers and doesn't want to use ap_proxy_balancer_get_best_worker()'s logic at all. Just because the 3 existing lbmethods use that flow doesn't mean everything has to, but we removed a lot of duplicate code with a shared function for them (and future lbmethods?) to use. I think it's a big win for extensibility and simplification of the lbmethod interface.
Implement hot spares in mod_proxy_balancer [was Re: Ideas from ApacheCon]
> On 1 Jun 2017, at 17:15, Yann Ylavic <ylavic@gmail.com> wrote: > > On Thu, Jun 1, 2017 at 10:48 PM, Jim Riggs <apache-li...@riggs.me> wrote: >>> On 1 Jun 2017, at 15:25, Yann Ylavic <ylavic@gmail.com> wrote: >>> >>> On Thu, Jun 1, 2017 at 10:22 PM, Yann Ylavic <ylavic@gmail.com> >>> wrote: >>>> On Thu, Jun 1, 2017 at 7:29 PM, Jim Riggs <apache-li...@riggs.me> >>>> wrote: >>>>>> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> >>>>>> wrote: 2. I understand the logic behind creating the arrays, >>>>>> but doesn't this increase the overhead. We go thru the full >>>>>> list of workers one time, and then go thru the list of avail >>>>>> works and spares right after that. Assume that all workers >>>>>> are available; doesn't it mean we go thru that last 2x? >>>> [] >>>>> >>>>> The only way I can think of to avoid this without going back >>>>> to duplicating code across lbmethods, which I would argue is >>>>> worse, is to maybe have the lbmethod provide a callback >>>>> function and context pointer to >>>>> ap_proxy_balancer_usable_workers() that gets called during the >>>>> loop iteration to pick the best member in flight. >>>> >>>> Couldn't a simple 'best' for ap_proxy_balancer_usable_workers() >>>> make it return a single entry? >>> >>> ... a simple 'best' *flag* (as argument) ... >> >> I'm not sure I follow what this flag would be. The lbmethod would >> somehow have to tell ap_proxy_balancer_usable_workers() how to pick >> the best worker (e.g. by comparing the number of bytes sent or the >> number of requests processed). I'm not sure how that information >> could be passed as a flag unless we baked the behavior of byrequests, >> bybusyness, and bytraffic into ap_proxy_balancer_usable_workers(). >> But then how would we allow for plugging in additional lbmethods? > > Oh right, nevermind, I thought per lbmethod logic was already there. > After a better look into it, a callback (and its baton) with that > logic looks straightforward, though. Indeed. I put a new patch that uses callbacks on BZ61140 (and visually available at https://github.com/jhriggs/httpd/pull/1/files). So, the lbmethods call ap_proxy_balancer_get_best_worker() with a callback and baton so that the best worker is chosen in flight during iteration of the workers, addressing Jim's overhead concern. Changes: - use a callback and baton to the lbmethod in ap_proxy_balancer_get_best_worker() (previously ap_proxy_balancer_usable_workers()) so that we can find the best worker in flight while iterating rather than returning an array of usable workers that has to then in turn be iterated - consider a draining worker unusable and suitable for replacement by a spare > Regarding: > worker = _ARRAY_IDX(balancer->workers, i, proxy_worker *); > I don't see why worker needs to be a 'proxy_worker**' ('*worker' is > never updated IIUC). > > Looks like: > proxy_worker *worker = APR_ARRAY_IDX(balancer->workers, i, proxy_worker *); > would be fine and allow using 'worker->xxx' instead of > '(*worker)->xxx' all over the place... Much of what I did was based on what already existed. The existing code was iterating through proxy_worker**, so mine did too. You are correct, though, that there is no real reason for it. The latest patch switches to just using * rather than **. Thanks for putting eyes on this! I appreciate it.
Re: Ideas from ApacheCon
> On 1 Jun 2017, at 15:25, Yann Ylavic <ylavic@gmail.com> wrote: > > On Thu, Jun 1, 2017 at 10:22 PM, Yann Ylavic <ylavic@gmail.com> wrote: >> On Thu, Jun 1, 2017 at 7:29 PM, Jim Riggs <apache-li...@riggs.me> wrote: >>>> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> wrote: >>>> 2. I understand the logic behind creating the arrays, but doesn't >>>>this increase the overhead. We go thru the full list of workers >>>>one time, and then go thru the list of avail works and spares >>>>right after that. Assume that all workers are available; doesn't >>>>it mean we go thru that last 2x? >> [] >>> >>> The only way I can think of to avoid this without going back to >>> duplicating code across lbmethods, which I would argue is worse, is >>> to maybe have the lbmethod provide a callback function and context >>> pointer to ap_proxy_balancer_usable_workers() that gets called during >>> the loop iteration to pick the best member in flight. >> >> Couldn't a simple 'best' for ap_proxy_balancer_usable_workers() make >> it return a single entry? > > ... a simple 'best' *flag* (as argument) ... I'm not sure I follow what this flag would be. The lbmethod would somehow have to tell ap_proxy_balancer_usable_workers() how to pick the best worker (e.g. by comparing the number of bytes sent or the number of requests processed). I'm not sure how that information could be passed as a flag unless we baked the behavior of byrequests, bybusyness, and bytraffic into ap_proxy_balancer_usable_workers(). But then how would we allow for plugging in additional lbmethods? >> >> By the way, retrieving ap_proxy_retry_worker() via an OPTIONAL_FN >> looks unnecessary since it's implemented in the exact same >> "proxy_util.c" file. LOL! That's a copy-paste remnant from when I had that function in a different file originally. Patch updated. Thanks for catching it!
Re: Ideas from ApacheCon
> On 1 Jun 2017, at 07:55, Jim Jagielski <j...@jagunet.com> wrote: > > I really like where this is going... I just have a few questions: > > 1. The style, esp with array usage is different; eg APR_ARRAY_PUSH > and APR_ARRAY_IDX... any particular reason why? Well, we don't seem to be entirely consistent with using elts and apr_array*() directly vs. the helper #defines across the codebase. I was doing everything with direct pointer access and arithmetic until I started looking at the heartbeat lbmethod that was using some of the helpers. IMNSHO, I think they are a little cleaner and easier use/read. I got lost in dereferencing hell with all of the `&((*...) **)->(*)'! :-) The helpers are there, and I think they are convenient and worth using, but I'm not digging through all of this code as regularly as some of the others on here. If the standard/consensus is the other way, it's easy to change. > 2. I understand the logic behind creating the arrays, but doesn't > this increase the overhead. We go thru the full list of workers > one time, and then go thru the list of avail works and spares > right after that. Assume that all workers are available; doesn't > it mean we go thru that last 2x? If everything is available, the old implementation only goes through the list once and picks a worker. This new implementation goes through the list once and returns a *subset* that the lbmethod then has to loop through to pick a worker. Worst case (e.g. X workers all available in lbset 0), yes, we would now do 2X. In a more complex scenario (e.g. X workers all available in each lbsets 0, 1, and 2), we do 3X (1st loop) + X (2nd loop) = 4X, not 2 * 3X = 6X. So, yes, there is slightly more overhead...but, the second loop (in the lbmethod) has at most one lbset's worth of workers and really isn't doing anything except an arithmetic comparison to pick the best from them. If a number of workers are unavailable, however, it's different. The old implementation loops the whole list twice per lbset if it doesn't find a worker, once for workers and once for standbys, whereas this new implementation still loops the whole list only once per lbset. In the example above, 2 * 3X = 6X for old, 3X + X = 4X for new. The only way I can think of to avoid this without going back to duplicating code across lbmethods, which I would argue is worse, is to maybe have the lbmethod provide a callback function and context pointer to ap_proxy_balancer_usable_workers() that gets called during the loop iteration to pick the best member in flight. I am open to that...it just seemed to add unnecessary complexity. Regardless, even worst case, we are looking at what, iterating 6 pointers instead of 3 or 10 instead of 5? We probably have some lower hanging fruit across the request lifecycle code to increase performance than saving some arithmetic on a handful of structs, no? ;-) >> On May 31, 2017, at 1:44 PM, Jim Riggs <apache-li...@riggs.me> wrote: >> >>> On 23 May 2017, at 09:16, Jim Riggs <apache-li...@riggs.me> wrote: >>> >>>> On 18 May 2017, at 13:22, Rainer Jung <rainer.j...@kippdata.de> wrote: >>>> >>>> Am 18.05.2017 um 19:46 schrieb Jim Jagielski: >>>>> Based on feedback from various sessions: >>>>> >>>>> o A new-kind of "hot standby" in mod_proxy which kicks >>>>> in whenever a worker moves out of the pool (ie, doesn't >>>>> wait until all workers are out)... ala a redundant >>>>> hard drive. >>>> >>>> Maybe "spare worker" (and we could have more than one such spares). >>> >>> Exactly. I am already working on some code for this, though it to seems to >>> necessarily be a bit convoluted in the current codebase. >>> >>> The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch >>> effort to return something. I.e. *all* workers are unavailable, so then we >>> use the hot standby. (This problem can actually be solved a different way >>> by setting a high value for lbset.) >>> >>> In my mind, though, what is proposed here is actually how I actually expect >>> a "hot standby" to work. Think of it more like a "hot spare" in disk RAID >>> terms. That is, if *any* worker is unavailable, the hot spare will be used >>> (or at least added to the list of potential workers...still to be >>> determined by the lbmethod implementation). >>> >>> Example: >>> >>> >>> BalancerMember "http://192.168.1.50:80; >>> BalancerMember "http://192.168.1.51:80; >>> BalancerMember "http://192.168
Re: Ideas from ApacheCon
> On 23 May 2017, at 09:16, Jim Riggs <apache-li...@riggs.me> wrote: > >> On 18 May 2017, at 13:22, Rainer Jung <rainer.j...@kippdata.de> wrote: >> >> Am 18.05.2017 um 19:46 schrieb Jim Jagielski: >>> Based on feedback from various sessions: >>> >>> o A new-kind of "hot standby" in mod_proxy which kicks >>> in whenever a worker moves out of the pool (ie, doesn't >>> wait until all workers are out)... ala a redundant >>> hard drive. >> >> Maybe "spare worker" (and we could have more than one such spares). > > Exactly. I am already working on some code for this, though it to seems to > necessarily be a bit convoluted in the current codebase. > > The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch > effort to return something. I.e. *all* workers are unavailable, so then we > use the hot standby. (This problem can actually be solved a different way by > setting a high value for lbset.) > > In my mind, though, what is proposed here is actually how I actually expect a > "hot standby" to work. Think of it more like a "hot spare" in disk RAID > terms. That is, if *any* worker is unavailable, the hot spare will be used > (or at least added to the list of potential workers...still to be determined > by the lbmethod implementation). > > Example: > > > BalancerMember "http://192.168.1.50:80; > BalancerMember "http://192.168.1.51:80; > BalancerMember "http://192.168.1.52:80; > BalancerMember "http://192.168.1.53:80; status=+H > > > In this case, .53 will only get used if .50, .51, and .52 are *all* > unavailable. > > > BalancerMember "http://192.168.1.50:80; > BalancerMember "http://192.168.1.51:80; > BalancerMember "http://192.168.1.52:80; > BalancerMember "http://192.168.1.53:80; status=+R # new "hot spare" status > BalancerMember "http://192.168.1.54:80; status=+R # new "hot spare" status > > > In this case, if .50 becomes unavailable, .53 (or .54 depending on > implementation) will be treated as an available worker for the lbmethod to > potentially choose. If 2 or more of .50, .51, and .52 become unavailable, > both .53 and .54 would be available to be chosen. > > So, instead of having a single fallback option when *all* workers are dead, > we will have a way of trying to ensure that a specific number of workers (3 > in the example above) are always available...just like a hot spare drive > plugs into the RAID array when one of the members dies. In our case, though, > once the main worker recovers, the hot spare will go back to being a hot > spare (except for matching routes). > > Comments welcome. As promised, balancer hot spare support: https://bz.apache.org/bugzilla/show_bug.cgi?id=61140
Broken OCSP Stapling
This was mentioned in today's Bulletproof TLS newsletter (https://www.feistyduck.com/bulletproof-tls-newsletter/issue_28_lets_encrypt_downtime.html): https://blog.hboeck.de/archives/886-The-Problem-with-OCSP-Stapling-and-Must-Staple-and-why-Certificate-Revocation-is-still-broken.html It discusses httpd's (and nginx's) broken OCSP stapling implementations. This is outside of my wheelhouse, but wanted to raise awareness for someone familiar with that code who may be interested in taking a look. The post references bz57121 from 2014(!).
Re: Ideas from ApacheCon
> On 18 May 2017, at 12:46, Jim Jagielskiwrote: > > Based on feedback from various sessions: > > o A new-kind of "hot standby" in mod_proxy which kicks >in whenever a worker moves out of the pool (ie, doesn't >wait until all workers are out)... ala a redundant >hard drive. > > o Look into AAA and mod_cache; eg: "bolt in at the end" > > o HTTP/2 no longer experimental! > > o balancer-manager more scriptable (move to REST for realz?) > > o When restarting w/ persistent balancer data, warn if >config files mtime is newer. > > o Warn if the trailing '/'s don't match in ProxyPass/Reverse >directives (eg: ProxyPass /foo http://www.example.com/foo/ ) > > All I can recall at present... o Investigate mod_cache performance when bypassing cache (e.g. `Cache-Control: no-cache'). In benchmark testing for my talk, (1) no mod_cache vs. (2) cached responses vs. (3) cache enabled but requests specify `Cache-Control: no-cache' showed that (3) was significantly slower than (1). I would expect that no-cache would short-circuit/bypass mod_cache and have results similar to (1), but there is *something* going on in there. I have not investigated the code...just noted the results.
Re: Ideas from ApacheCon
> On 18 May 2017, at 13:22, Rainer Jungwrote: > > Am 18.05.2017 um 19:46 schrieb Jim Jagielski: >> Based on feedback from various sessions: >> >> o A new-kind of "hot standby" in mod_proxy which kicks >> in whenever a worker moves out of the pool (ie, doesn't >> wait until all workers are out)... ala a redundant >> hard drive. > > Maybe "spare worker" (and we could have more than one such spares). Exactly. I am already working on some code for this, though it to seems to necessarily be a bit convoluted in the current codebase. The way we treat a "hot standby" in mod_proxy_balancer is as a last-ditch effort to return something. I.e. *all* workers are unavailable, so then we use the hot standby. (This problem can actually be solved a different way by setting a high value for lbset.) In my mind, though, what is proposed here is actually how I actually expect a "hot standby" to work. Think of it more like a "hot spare" in disk RAID terms. That is, if *any* worker is unavailable, the hot spare will be used (or at least added to the list of potential workers...still to be determined by the lbmethod implementation). Example: BalancerMember "http://192.168.1.50:80; BalancerMember "http://192.168.1.51:80; BalancerMember "http://192.168.1.52:80; BalancerMember "http://192.168.1.53:80; status=+H In this case, .53 will only get used if .50, .51, and .52 are *all* unavailable. BalancerMember "http://192.168.1.50:80; BalancerMember "http://192.168.1.51:80; BalancerMember "http://192.168.1.52:80; BalancerMember "http://192.168.1.53:80; status=+R # new "hot spare" status BalancerMember "http://192.168.1.54:80; status=+R # new "hot spare" status In this case, if .50 becomes unavailable, .53 (or .54 depending on implementation) will be treated as an available worker for the lbmethod to potentially choose. If 2 or more of .50, .51, and .52 become unavailable, both .53 and .54 would be available to be chosen. So, instead of having a single fallback option when *all* workers are dead, we will have a way of trying to ensure that a specific number of workers (3 in the example above) are always available...just like a hot spare drive plugs into the RAID array when one of the members dies. In our case, though, once the main worker recovers, the hot spare will go back to being a hot spare (except for matching routes). Comments welcome.
Re: Ideas from ApacheCon
> On 22 May 2017, at 06:45, Jim Jagielski <j...@jagunet.com> wrote: > > I'll let Jim Riggs answer that...it came up during his mod_cache > talk. >> On May 18, 2017, at 2:25 PM, Eric Covener <cove...@gmail.com> wrote: >> >> On Thu, May 18, 2017 at 2:22 PM, Rainer Jung <rainer.j...@kippdata.de> wrote: >>>> o Look into AAA and mod_cache; eg: "bolt in at the end" >> >> Does that differ from "CacheQuickHandler OFF"? This one is a bit quirky and requires some investigation to see the edge cases. There appear to be some situations where Require directives with `CacheQuickHandler off' do not always take effect. Thus, items that have been cached will be served from the cache even if access restrictions would otherwise disallow them. We will just need to do some more testing to see when -- and if -- this is actually occurring. I stumbled across it while doing testing/prep for my talk, but did not investigate it thoroughly. (It is entirely possible that I just messed something up in my config and didn't catch it...or forgot a graceful...or something...) In my talk, I just told people to "be careful" with access control and CQF off.
ACNA Miami: Who? When?
So, who all will be in Miami? From what I've seen on Sched and messages here: Yes : jimjag, rich, jfc, ruggeri, me No : rowe, covener There are several other ACNA regulars who have been quiet around here lately. Anyone else coming in? I'll be there Sunday evening through Friday morning. I would update https://wiki.apache.org/httpd/Face2Face like usual, but it got locked down for spam. I'm up for a meal with any of you anytime. Also, contrary to the seeming consensus on the hackathon thread a couple weeks ago (that I missed), I have had a lot of luck pounding things out during random hacking sessions or down times at the conference, most notably that mod_cache-thundering-herd-lock-file issue that a group of us really hammered on a couple years back. (It was absolutely killing me at work!) That said, I'm always up for hacking on stuff with people, especially when it's something nagging like that.
Re: 2.2 and 2.4 and 2.6/3.0
On 28 May 2015, at 14:30, Reindl Harald h.rei...@thelounge.net wrote: Am 28.05.2015 um 21:22 schrieb Rich Bowen: On 05/27/2015 05:38 PM, olli hauer wrote: - for long time there was no working mod_php module for 2.4, and changing to php-fpm was not for everyone a solution. In my experience, the only reason that php-fpm wasn't a solution for everyone is that it was poorly documented. We could still stand to do better there, but php-fpm is, in fact, a solution for everyone no, because it does not support php_admin_flag and php_admin_value inside of Directory and so would require re-design environments with many hundrets of vhosts running for many years that way with automatic deployment of such rules depending on the target application Having to expend effort (e.g. re-design/update config and deployment) to switch/update/upgrade to a new paradigm does not, IMO, mean that it's not a solution for everyone. Anyone can take the time to implement and automate the switch. Once that effort has been spent, you should have nearly the same (maybe better) setup, with hopefully better security and resource utilization in many cases. All of those php_admin_* directives end up in a php-fpm conf file that can easily be automatically updated and deployed. I'm certainly not saying that this work is trivial with many years of history, and it may not be for everyone, but it is certainly possible for anyone. With fpm and mod_proxy_fcgi, I don't see myself ever using mod_php again, but that's just me.
Re: *Match, RewriteRule POLA violation?
On 1 May 2015, at 10:52, André Malo n...@perlig.de wrote: * Niklas Edmundsson wrote: On Thu, 30 Apr 2015, Yann Ylavic wrote: On Thu, Apr 30, 2015 at 2:57 PM, Jim Riggs apache-li...@riggs.me wrote: Thanks, Yann. I remember looking at this code before. The question remains, though: Is it currently wrong? Does it need to be fixed, or was this distinction made intentionally? Is there a specific use case that requires the regex-matching directives to not get slash-normalized URIs? I would like it to be fixed, non leading /+ is equivalent to /, this would break very few (if any) cases IMHO, and may even unbreak more ones . +1 I would expect Location and LocationMatch using the same uri for comparison. Hmm, that assumption is wrong by definition. Location always matches a prefix (a part of a parsed/unparsed url), while LocationMatch always matches the complete URL. We need to be careful on that phrasing. LocationMatch *can* match the complete URL if it is anchored at both ends. By default, though, it will match anywhere in the URL if it is unanchored, and it can (with the '/+' we are discussing here) behave just like Location and match a prefix if it is only anchored at the beginning. I would actually go so far as the current state might warrant a CVE for being a hidden security risk that might cause inadvertent information exposure. It *is* documented right here, btw: http://httpd.apache.org/docs/2.4/mod/core.html#location (found it, eventually...) Yay! I knew I had read it somewhere. Good find! However, that documentation is actually not correct. This is not true: For example, LocationMatch \^/abc\ would match the request URL /abc but not the request URL //abc. Based on all of my tests, the leading slash *is* collapsed in the *Match and RewriteRule directives. Subsequent slashes after the first are not. So, it is documented, but is there a compelling use case for this? When would someone actually need to match against the multiple slashes (unless it's some really strange hack someone has implemented)? The only thing I can think of is that maybe you want to force a redirect to remove multiple slashes, but couldn't a directive in mod_alias(?) maybe handle that if deemed necessary? Also, yes it is documented, but when I brought it up at a hackathon table full of long-time committers at ApacheCon, no one knew about it, and I had to make an example. This says to me says that maybe it's unexpected behavior and a potential security risk as both Niklas and Ruggeri have suggested. That's why I brought it up in the first place. And so the questions still remain: Right or wrong? Change it or leave it? Expected or unexpected? Security risk or user/configuration error? The world may never know... ;-) I may go ahead and write up a patch this weekend to change them all (*Match and RewriteRule) and then we can all debate it over on bugz too!
Re: Proposal/RFC: informed load balancing
On Wednesday, April 29, 2015, Jim Riggs apache-li...@riggs.me mailto:apache-li...@riggs.me wrote: Warn out from writing all of this and hopeful that someone other than me actually cares, I wish you all well today/tonight! *Worn* out, even! Boy, I was tired!
Re: *Match, RewriteRule POLA violation?
On 28 Apr 2015, at 17:55, Yann Ylavic ylavic@gmail.com wrote: It seems that while Location is compared to ap_no2slash(r-uri), LocationMatch is matched against r-uri directly. That's probably the issue. A possible fix (untested) could be: Index: server/request.c === --- server/request.c(revision 1674695) +++ server/request.c(working copy) @@ -1446,7 +1446,7 @@ pmatch = apr_palloc(rxpool, nmatch*sizeof(ap_regmatch_t)); } -if (ap_regexec(entry_core-r, r-uri, nmatch, pmatch, 0)) { +if (ap_regexec(entry_core-r, entry_uri, nmatch, pmatch, 0)) { continue; } @@ -1456,7 +1456,7 @@ apr_table_setn(r-subprocess_env, ((const char **)entry_core-refs-elts)[i], apr_pstrndup(r-pool, - r-uri + pmatch[i].rm_so, + entry_uri + pmatch[i].rm_so, pmatch[i].rm_eo - pmatch[i].rm_so)); } } Thanks, Yann. I remember looking at this code before. The question remains, though: Is it currently wrong? Does it need to be fixed, or was this distinction made intentionally? Is there a specific use case that requires the regex-matching directives to not get slash-normalized URIs? On Mon, Apr 27, 2015 at 10:52 PM, Jim Riggs apache-li...@riggs.me wrote: This came up at ApacheCon a couple of weeks ago. I just took this knowledge for granted, as I have always accounted for it, but both Rich and Trawick were surprised. As I thought about it some more, it seems this may be a POLA violation. Thoughts? If we agree it should be fixed, I can make the bugz and make a patch. Consider: Location /slash/foo ... /Location vs. LocationMatch ^/slash/foo ... /LocationMatch These do not behave the same if multiple slashes are used. The leading slashes are always coalesced, so ^/... is fine; however, any intermediate slashes are not. So, in order for the LocationMatch directive above to behave the same as the Location, it has to be specified as ^/slash/+foo. Like I said, I have always accounted for this in my regexps, but it doesn't seem right. Should the URL be normalized before being passed to regex-matching directives, or is there a specific reason that is not done? +---+--+--+--+ | Path | Non-Regex |*Match, |*Match, | | | Directive: | RewriteRule: | RewriteRule: | | | /slash/foo | ^/slash/foo | ^/slash/+foo | +---+--+--+--+ | /slash/foo| Match| Match| Match| +---+--+--+--+ | slash/foo | Match| Match| Match| +---+--+--+--+ | /slash///foo | Match| XXX | Match| +---+--+--+--+ | slash///foo// | Match| XXX | Match| +---+--+--+--+
Proposal/RFC: informed load balancing
[ Long message and proposal follows. Bear with me. There are a lot of words, but that is because we need a lot of help/input! ;-) ] So, this has come up in the past several times, and we discussed it again this year at ApacheCon: How do we get the load balancer to make smarter, more informed decisions about where to send traffic? The different LB methods provide some different attempts at balancing traffic, but ultimately none of them is smart about its decision. Other than a member being in error state, the balancer makes its decision solely based on configuration (LB set, factor, etc.) and its own knowledge of the member (e.g. requests, bytes). What we have often discussed is a way to get some type of health/load/capacity information from the backend to make informed balancing decisions. One method is to use health checks (a la haproxy, AWS ELBs, etc.) that request one or more URLs and the response code/time indicates whether or not the service is up and available, allowing more proactive decisions. While this is better than our current state of reactively marking members in error state based on failed requests, it still provides a limited view of the health/state of the backend. We have also discussed implementing a way for backends to communicate a magical load number to the front end to take into account as it balances traffic. This would give a much better view into the backend's state, but requires some way to come up with this calculation that each backend system/server/service/app must provide. This then has to be implemented in all the various backends (e.g. httpd, tomcat, php-fpm, unicorn, mongrel, etc., etc.), probably a hard sell to all of those projects. And, the front end would have limited control over what that number means or how to use it. During JimJag's balancer talk at ApacheCon this year, he brought up this issue of better, more informed decision making again. I put some thought into it that night and came up with some ideas. Jim, Covener, Trawick, Ruggeri, and I then spent some time over the next couple of days talking it through and fleshing out some of the details. Based on all of that, below is what I am proposing. I have some initial code that I am working on to implement the different pieces of this, and I will put them up in bugz or somewhere when they're a little less rudimentary. -- Our hope is to create a general standard that can be used by various projects, products, proxies, servers, etc., to have a more consistent way for a load balancer to request and receive useful internal state information from its backend nodes. This information can then be used by the *frontend* software/admin (this is the main change from what we have discussed before) to calculate a load factor appropriate for each backend node. This communication uses a new, standard HTTP header, X-Backend-Info, that takes this form in RFC2616 BNF: backend-info = version = numeric-entry [ *LWS , *LWS #( numeric-entry | string-entry ) ] numeric-entry = numeric-field = ( float | float ) ; that is, numbers may optionally be enclosed in ; quotation marks float = 1*DIGIT [ . 1*DIGIT ] numeric-field = workers-max ; maximum number of workers the backend supports | workers-used ; current number of used/busy workers | workers-allocated ; current number of allocated/ready workers | workers-free ; current number of workers available for use ; (generally the difference between workers-max and ; workers-used, though some implementations may have ; a different notion) | uptime ; number of seconds the backend has been running | requests ; number of requests the backend has processed | memory-max ; total amount of memory available in bytes | memory-used ; amount of used memory in bytes | memory-allocated ; amount of allocated/committed memory in bytes | memory-free ; amount of memory available for use (generally ; the difference between memory-max and memory-used, ; though some implementations may have a different ; notion) | load-current ; the (subjective) current load for the backend | load-5 ; the (subjective) 5-minute load for the backend | load-15 ; the (subjective) 15-minute load for the backend string-entry = string-field = ( token |
Re: Listen on UDS
On 28 Apr 2015, at 10:20, Graham Leggett minf...@sharp.fm wrote: On 28 Apr 2015, at 5:17 PM, Jim Jagielski j...@jagunet.com wrote: Anyone looked into having httpd be able to Listen on a UDS, as well as scenarios where we may want that even? I have always wanted it - one thing it allows us to do is reverse proxy to versions of httpd (or other daemon software) running as another user, with a proper hope of securing it. +1. Reverse proxy is the prime example I can think of for it.
*Match, RewriteRule POLA violation?
This came up at ApacheCon a couple of weeks ago. I just took this knowledge for granted, as I have always accounted for it, but both Rich and Trawick were surprised. As I thought about it some more, it seems this may be a POLA violation. Thoughts? If we agree it should be fixed, I can make the bugz and make a patch. Consider: Location /slash/foo ... /Location vs. LocationMatch ^/slash/foo ... /LocationMatch These do not behave the same if multiple slashes are used. The leading slashes are always coalesced, so ^/... is fine; however, any intermediate slashes are not. So, in order for the LocationMatch directive above to behave the same as the Location, it has to be specified as ^/slash/+foo. Like I said, I have always accounted for this in my regexps, but it doesn't seem right. Should the URL be normalized before being passed to regex-matching directives, or is there a specific reason that is not done? +---+--+--+--+ | Path | Non-Regex |*Match, |*Match, | | | Directive: | RewriteRule: | RewriteRule: | | | /slash/foo | ^/slash/foo | ^/slash/+foo | +---+--+--+--+ | /slash/foo| Match| Match| Match| +---+--+--+--+ | slash/foo | Match| Match| Match| +---+--+--+--+ | /slash///foo | Match| XXX | Match| +---+--+--+--+ | slash///foo// | Match| XXX | Match| +---+--+--+--+
ApacheCon Arrival
I just updated https://wiki.apache.org/httpd/Face2Face for Austin. If you are getting into town on Sunday and are interested in grabbing some dinner, let me know or update the page. Some of us got together last year on ApacheCon Eve and had a nice time. Also update the page if there are other httpd-related meetings/hangouts/hacking sessions you would like to get going. See you all -- OK, several of you at least -- next week!
Re: mod_proxy_fcgi issues
On Thu, Dec 4, 2014 at 9:58 AM, Eric Covener cove...@gmail.com wrote: On Tue, Dec 2, 2014 at 4:14 PM, Jim Riggs apache-li...@riggs.me wrote: P.S. mod_proxy_balancer - mod_proxy_fcgi - php-fpm is really fun and interesting too! ;-) mod_proxy_fcgi seems to need a bit of work from what I have been seeing in bugzilla and IRC. I hope to spend a little time on the code and doc, but not being an actual user of it I don't know how far I will really get before being distracted. This all may certainly be true, but I just for clarity's sake (since it was my quote that started this new mod_proxy_fcgi thread), my mod_proxy_balancer - mod_proxy_fcgi - php-fpm issue is NOT an httpd issue...at least that is not how I have treated it. It is actually a code fix I have had to make in PHP to get it to work. See: https://bugs.php.net/bug.php?id=54152 https://bugs.php.net/bug.php?id=62172 It would be great if JimJag could maybe address this if he still has PHP access. Jim?
Re: ApacheCon Austin, httpd track
On 11/30/2014 11:08 AM, Jeff Trawick wrote: * deploying Python web apps under uWSGI behind mod_proxy_fcgi/scgi (some material here: http://emptyhammock.com/projects/info/pyweb/index.html) On 1 Dec 2014, at 19:15, Daniel Ruggeri drugg...@primary.net wrote: Similarly, I'm always up for giving my proxy talk if it's welcome (after the first day since I can't make it until Tues). If we think proxy is a big topic, we ought to arrange for a general overview like my proxy talk followed by more specific deep dives such as what Jeff mentions here and a session on new sexiness like WebSockets. Picking up on what Jeff and Daniel are saying, I think some focus on the powerful things mod_proxy_* can do would be really useful. One particular thought that has been in the back of my head for some time is a Begone libphp5.so! talk. For better or worse, PHP is still around and will be for some time, but it is time to get it out of the web server and treat it like the application/backend it is for both security and resource-usage reasons. mod_proxy_fcgi + php-fpm is a really elegant, simple solution to make this happen, but I have found a lot of devs and admins who just aren't even aware of this configuration possibility. (I have explained it to several people at ApacheCon NAs over the past couple of years.) I've actually been using a backported mod_proxy_fcgi in 2.2 for just this purpose for a few years in production. That's certainly a talk I would be willing to give if there is interest. P.S. mod_proxy_balancer - mod_proxy_fcgi - php-fpm is really fun and interesting too! ;-)
Re: Time for httpd 2.2.28??
If so, I can RM. PLEASE! We need those thundering herd fixes in mod_cache! :-)
Re: Time for httpd 2.2.28??
On 16 Jul 2014, at 08:19, Yann Ylavic ylavic@gmail.com wrote: On Wed, Jul 16, 2014 at 3:15 PM, Jim Riggs apache-li...@riggs.me wrote: If so, I can RM. PLEASE! We need those thundering herd fixes in mod_cache! :-) This has already been backported by http://svn.apache.org/r1608302. I know. I mean we need the fixes to be officially released. I've been patching mod_cache for over 2 years now, I believe.
[patch] regexp rewrite map
[Posting separately to both dev and users to see if anyone on either side sees value in getting this committed.] About a year ago, I had an idea for a new type of RewriteMap that would fill an important need for a few particular use cases that we have [1]. While we were at ApacheCon in Denver, I spent some time talking with JimJag, Rich, and Covener as well as updating the code from a crude proof of concept to something real. I would appreciate feedback from anyone, especially on whether or not this is something worth pursuing getting committed. It is a simple concept: the map is just a list of regexp patterns and replacements. These could be done as individual RewriteRules, obviously, but this rewrite map would reduce clutter in the config file, be more readable, and could even be externally generated/maintained without any httpd admin involvement. For example, an application or batch job could generate a map file with dozens or hundreds of entries that httpd would pick up without a restart/graceful, and the config might only contain a single RewriteRule: [map file] /foo(bar)? /baz$1 /(apple|banana|orange) /fruit/$1 /post/(\d+)(/.*)? /article/$1$2 ... [config file] RewriteMap regexptest regexp:path/to/re.map RewriteCond ${regexptest:$1} ^(.+)$ RewriteRule ^(.*)$ %1 [R] ... Possible use cases that I can think of: 1. Redirect list (e.g. legacy site to new site) without pages of RewriteRules/Redirects 2. Simplify 100s or 1000s of rewrite rules into 1 + the map as above 3. White list of URL patterns to proxy through to backend servers (can be application generated; my particular use case) 4. Maps could be application generated, maintained in a spreadsheet or DB, or created with scripts/greps/etc. Just like text and hash maps, results are cached. I did some tests with up to 100K entries in the map, and it was still extremely responsive and worked flawlessly. The only thing this doesn't have is flags (e.g. NC), but that can be handled in the pattern itself via (?i). If interested, I would love it if some folks would try the attached patch and let me know what you think. - Jim [1] http://httpd.markmail.org/thread/3dheejtgwmdpxxt5 regexp_map.patch Description: Binary data
Re: mod_cache thundering herd bug
On 21 Apr 2014, at 06:38, Graham Leggett minf...@sharp.fm wrote: On 19 Apr 2014, at 10:26 PM, Eric Covener cove...@gmail.com wrote: Graham -- related subject brought up either in Denver or in the bug. It seems that when we serve a stale file while the cache is locked, the age headers are small instead of large. I got totally lost trying to track down the issue, maybe it makes sense to you? It's almost as if they time of the revalidation is somehow updated early and the delta in the stale cache hits is based off of that. All thundering herd does is after letting the first conditional request through, it serves stale data (RFC willing) until that conditional request comes back or a specific maximum time is reached, whichever comes first. The most valuable piece of information in this process is the reason variable, which describes the reason why something wasn't eligible for caching. In httpd v2.4 the X-Cache-Detail header will give this to you, in httpd v2.2 you'll need to log at DEBUG level to get this: ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, cache: %s not cached. Reason: %s, r-unparsed_uri, reason); The questions to answer are: - Is there stale content to serve? No stale content, no thundering herd protection. - If stale content is being deleted, identify why that is. This is likely to be unrelated to thundering herd, but rather in other parts of mod_cache. Covener - Are you talking about my comments in #16 on the ticket? (https://issues.apache.org/bugzilla/show_bug.cgi?id=50317#c16) If so, do either you or Graham have thoughts on the Age header getting returned with stale content? In my testing, when stale content is getting returned, no Age header is set which appears to be a violation of HTTP 1.1.
Re: Configuration error handling after httpd restart
On 14 Apr 2014, at 10:38, Eric Covener cove...@gmail.com wrote: On Mon, Apr 14, 2014 at 11:15 AM, Mike Rumph mike.ru...@oracle.com wrote: If there is an unknown directive in the config file, simply ignore it with a warning. You can't do that. What if it was Reqiure? I agree with Eric. I would not want unknown directives to be ignored. It might be a typo of a really important directive like Eric describes. Or, what if a module I really, really need is accidentally disabled and we just ignore all of its directives? Not good. In this particular case, duplicating a Listen directive doesn't seem like it should bomb out the server. Listen 80 ... Listen 80 It's superfluous, but not really a critical error. So, my patch just ignores subsequent duplicate Listens.
Re: Configuration error handling after httpd restart
On 27 Mar 2014, at 14:16, Mike Rumph mike.ru...@oracle.com wrote: Hello all, I have been doing some testing on the results of httpd restart with configuration errors. This gave me some interesting results. For these tests I build httpd trunk with APR trunk on Linux using the following configure: $ ./configure --prefix=/home/mrumph/apache25 --with-included-apr --with-mpm=worker --enable-mpms-shared='worker' Modify the default httpd.conf to include the following line: Listen localhost:8080 Start the httpd server and verify the process information. $ bin/apachectl -k start $ ps -ef | grep -i httpd Now restart httpd from this starting point with the following configuration error cases: Case 1: An unknown directive. Add the following line to the httpd.conf file. Xyzzy bin/apachectl -k restart - Returns with exit code 1 and following error message in stderr: AH00526: Syntax error on line 198 of /home/mrumph/apache25/conf/httpd.conf: Invalid command 'Xyzzy', perhaps misspelled or defined by a module not included in the server configuration httpd: abnormal exit 1 $ ps -ef | grep -i httpd - The httpd server was not stopped and all of the previous processes remain. - And the logs/httpd.pid file remains intact. $ tail logs/error_log - No error message logged. Case 2: A duplicate Listen directive. Duplicate the Listen directive in the httpd.conf file. Listen localhost:8080 Listen localhost:8080 $ bin/apachectl -k restart - Returns with exit code 0 and no error message in stderr. - So the httpd server appears to be working at this point. - (But appearances are deceiving.) $ ps -ef | grep -i httpd - All of the httpd processes have stopped. - But the logs/httpd.pid file remains intact. $ tail logs/error_log [Thu Mar 27 11:26:22.836887 2014] [mpm_worker:notice] [pid 2677:tid 47577479346656] AH00298: SIGHUP received. Attempting to restart (98)Address already in use: AH00072: make_sock: could not bind to address 127.0.0.1:8080 [Thu Mar 27 11:26:22.844003 2014] [mpm_worker:alert] [pid 2677:tid 47577479346656] no listening sockets available, shutting down [Thu Mar 27 11:26:22.844031 2014] [core:emerg] [pid 2677:tid 47577479346656] AH00019: Unable to open logs, exiting This was httpd trunk, but similar results are seen with httpd 2.2.22, 2.2-HEAD and 2.4.6. Before working on this as a bug, I am trying to understand what should be the correct behavior. I think case 1 is working correctly. But case 2 doesn't seem quite right. First of all, it doesn't seem correct to have an httpd.pid file when all of the httpd processes have vanished. Secondly, it would be nice to see an error code from the apachectl -k restart. (But this is probably due to a different processing phase in the validation for both of the cases.) It is also a little strange to see a message Unable to open logs in the log. Does anyone have some opinions what the correct behavior should be for these cases. If there are some actual bugs here, I have some time available to work on them. Check out the attached patch to ignore duplicate Listen directives. duplicate_listen.patch Description: Binary data
Re: mod_cache thundering herd bug
On 9 Apr 2014, at 14:46, Eric Covener cove...@gmail.com wrote: r1023398 for 2.2: http://people.apache.org/~covener/patches/httpd-2.2.x-thunder.diff The remove_url() prevents other threads from serving a stale cached file during refresh of a slow response, but it's unnecessary to have a separate path because the refresh has to deal with 200s already. When the remove_url was added, there as no thundering herd lock / no ability to serve stale content while one guy was reloading. covener, mrumph, and I looked at this today at ApacheCon. I updated the bug with some comments and attached this patch. https://issues.apache.org/bugzilla/show_bug.cgi?id=50317
mod_cache thundering herd bug
https://issues.apache.org/bugzilla/show_bug.cgi?id=50317 While we are at ApacheCon, I would love to address this nasty bug with someone familiar with 2.2's mod_cache. Our sites were brought down a few times last year before we finally tracked it down to being this particular bug. I am using a crude backport of the 2.3 patch (r1023398) in 2.2. It works, but I don't know if it is correct. Can someone look at this one with me? We really need to get this fixed in 2.2, because there is NO thundering herd protection at all as things stand right now. - Jim
Re: FreeBSD make
On 20 Dec 2013, at 07:04, Jim Jagielski j...@jagunet.com wrote: OK... this is weird, FreeBSD 9.2 make doesn't like http://svn.apache.org/viewvc?view=revisionrevision=1327907 gmake works fine. :/ (can't grab what the exact error is right now, but something like needs an operator or something like that... it doesn't like the 'ifdef' statement... or the 'else') BSD's make needs dot-prefixed .ifdef, .else, and .endif. I don't believe there is a good cross-make compatible way to do this. See http://stackoverflow.com/questions/9096018/make-gmake-compatible-if-else-statment.
New RewriteMap Help/Suggestions
I am in the process of preparing a patch to add a new RewriteMap type and could use some input from all of you on the best implementation. What I am creating is basically a clone of the txt map type, except that each line is a regexp followed by a replacement (with potential back-references). You can just do those as RewriteRules...no need for a map, you say. True, except that I am looking at an automated, application-created list, and I don't really want the application to be writing out configuration files or .htaccess files. I would be much more comfortable with the app writing out a map file that has limited functionality and scope. Plus, it would be easy for the app to just create 'regexp replacement' lines at build time. So, I have created a crude, working proof-of-concept of this. It basically copies all of the functionality of the txt maps, including the cache, but in the lookup_map_regexpfile() function, it compiles the regexp for each line, attempts a match, and returns the backref-substituted replacement. (This pair gets cached.) This works beautifully as is, but it is horribly inefficient to have to compile the REs every time we come in with a new key/URL. So, I was thinking of precompiling all of them and see three options: 1. Precompile and store all of the REs at config load time. 2. Compile and store all of the REs the first time we hit lookup_map_regexpfile() or when the map file is updated. 3. Compile and store each RE as we read through the map file in lookup_map_regexpfile() until a match is found and bail (full list will be built over time). #1 is nice, because all of the work is done up front and will be fast from then on. The problem, though, is that I would like this map to reload/refresh if the map file gets changed like the other types do. #2 and #3 solve this. With #2 I worry about performance of compiling everything if the map file gets updated and we get a thundering herd. With #3 there is some coordination to manage with respect to which lines have been compiled and which ones haven't. Does anyone have thoughts as to: 1. When/how should the map REs be compiled/precompiled? One of the options above or something else? 2. Where should the compiled REs be stored: in an existing pool or a new one? Thanks for any input. - Jim
Re: [PATCH] mod_log_forensic security considerations
On Jun 8, 2012, at 11:51 AM, Graham Leggett wrote: On 08 Jun 2012, at 5:45 PM, Joe Schaefer wrote: Well not quite, we'd still have had a problem with storing and archiving those logs even if we hadn't made them available to committers, because they violate our password retention policies. Can you clarify if possible what purpose you were trying to solve by enabling the forensic logs? Forensic logging is to answer the question what is going wrong, and shouldn't be enabled under normal operational circumstances unless there is something genuinely going wrong, at which point you record what you need and then switch it off again. A forensic log that has had a whole lot of filters applied to it is counterproductive, because the forensic log no longer tells you exactly what is going on, and when you're troubleshooting you need to know precisely that. In my situation, we have them enabled so that when an issue arises, we have one more tool at our disposal to identify a root cause. When I get an alert that there is something wrong with one of our sites, it is usually too late to enable forensic logging at that point. Something has already happened. We need to mitigate and get everything back up to normal. The question is usually not what IS going wrong?, but rather what WENT wrong?, because it is often a short-lived event. Having the forensic logs available has proven extremely helpful in this scenario. Might the full, unfiltered forensic data be valuable? Yes, but I don't believe the security risk is worth it in my situation. The rare case where an Authorization header might be truly useful for debugging or RCA is vastly overshadowed by the usefulness of the rest of the request information stored in the forensic log. The key to the forensic log, obviously, is that we have some information about an incoming request before it is completed. We can't get this information from any of the standard or custom logs, and we don't have any control over the format. Perhaps, just like we have LogFormat and now ErrorLogFormat, we should have ForensicLogFormat? If we did, then everyone could have what they want/need, whether full or partial forensic data. - Jim
Re: [PATCH] mod_log_forensic security considerations
On Jun 7, 2012, at 3:11 PM, Stefan Fritsch wrote: I share Williams concern that this makes mod_forensic potentially less useful. Maybe making the forensic log mode 600 by default would be a better idea? I have to agree with Jeff. I would rather have a more difficult or even impossible time debugging a crash than have a security hole that relies solely on file permissions. Maybe it should be a toggle in mod_forensic for debugging purposes (defaulting to hiding Authorization). The problem with just changing the file permissions is that sensitive data is still stored in the files. Even if the files are owned by root, anyone with root access would have access to others' usernames and passwords. I don't want to have that access to others' credentials, nor do I want them to have access to mine. I applied Jeff's patch as soon as it came across, wiped out all of our archived forensic logs, and had all of our affected users reset their passwords. Thanks, Jeff! - Jim
Re: svn commit: r1242351 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy.xml modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_fcgi.c server/util_script.c
On Feb 9, 2012, at 10:22 AM, Ruediger Pluem wrote: j...@apache.org wrote: Author: jim Date: Thu Feb 9 15:07:22 2012 New Revision: 1242351 URL: http://svn.apache.org/viewvc?rev=1242351view=rev Log: Handle cases, esp when using mod_proxy_fcgi, when we do not want SCRIPT_FILENAME to include the query string. Modified: httpd/httpd/trunk/docs/manual/mod/mod_proxy.xml httpd/httpd/trunk/modules/proxy/mod_proxy.c httpd/httpd/trunk/modules/proxy/mod_proxy.h httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c httpd/httpd/trunk/server/util_script.c Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1242351r1=1242350r2=1242351view=diff == --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original) +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Thu Feb 9 15:07:22 2012 @@ -188,7 +188,7 @@ static apr_status_t send_data(proxy_conn while (to_write) { apr_size_t n = 0; rv = apr_socket_sendv(s, vec + offset, nvec - offset, n); -if (rv != APR_SUCCESS) { +if ((rv != APR_SUCCESS) !APR_STATUS_IS_EAGAIN(rv)) { break; } if (n 0) { How is this related to the log message and the query string issue? It appears this commit has to do with the message I sent the other day (http://mail-archives.apache.org/mod_mbox/httpd-dev/201202.mbox/%3c8ea4e5e4-f97e-410b-aad4-257ecb4f9...@riggs.me%3E). The question is, was that EAGAIN fix supposed to be included in this commit or not? Is that even the right fix? That was my brute-force attempt to address the issue I was having, but I don't know if it is correct. Do we need to check in the loop for a timeout or something in case we keep receiving EAGAIN? Or would the timeout get picked up and bail out elsewhere?
mod_proxy_fcgi and EAGAIN (hacking)
Here is what I am testing: I am using (currently one) mod_proxy_fcgi member in a balancer to php-fpm. I have already run into some issues with fcgi:// as a balancer member as described in http://mail-archives.apache.org/mod_mbox/httpd-dev/201109.mbox/%3CB0DADBC2-5154-4C37-93B5-D38B834BE571%40riggs.me%3E. So, I have applied a small patch to httpd and php to get around these issues. Everything has been working perfectly, but we noticed that some uploads fail with a 503. (We could upload a 181KB file but not 182KB.) I spent considerable time debugging and tracing the issue. I finally tracked this down to send_data() in mod_proxy_fcgi. While looping over the calls to apr_socket_sendv(), it would make 23 successful calls of 8200 bytes followed by a partial send and then receive EAGAIN. Because this is not APR_SUCCESS, it breaks the loop in the next line and returns a 503. Since it received EAGAIN, I just brute-forced it to not break the loop on EAGAIN, but what is the correct fix? Is there something wrong in my setup? Is it a bug? Should send_data() be handling EAGAIN and continue the loop up to a timeout? Your thoughts are appreciated. - Jim My brute-force hack: --- mod_proxy_fcgi.c.orig 2012-02-03 13:23:09.132232659 -0600 +++ mod_proxy_fcgi.c2012-02-03 13:25:19.794906516 -0600 @@ -188,7 +188,7 @@ while (to_write) { apr_size_t n = 0; rv = apr_socket_sendv(s, vec + offset, nvec - offset, n); -if (rv != APR_SUCCESS) { +if ((rv != APR_SUCCESS) !APR_STATUS_IS_EAGAIN(rv)) { break; } if (n 0) {
Re: balancer-manager and XML
On Nov 30, 2011, at 9:16 AM, Jim Jagielski wrote: The XML interface for the balancer manager has, admittedly, lagged behind... Anyone have cycles and/or talent to bring it up to snuff? If not, I'll try to muddle thru it ;) As I mentioned to Jim at ApacheCon, I have a 2.2 patch for this that I have been using in production for a couple of years so that we can monitor our balancers. I am in the process of updating this patch for 2.4 right now. I plan to submit the updated patch for discussion/input today. I am just doing some final testing and debugging.
Re: balancer-manager and XML
On Nov 30, 2011, at 10:26 AM, Jim Riggs wrote: On Nov 30, 2011, at 9:16 AM, Jim Jagielski wrote: The XML interface for the balancer manager has, admittedly, lagged behind... Anyone have cycles and/or talent to bring it up to snuff? If not, I'll try to muddle thru it ;) As I mentioned to Jim at ApacheCon, I have a 2.2 patch for this that I have been using in production for a couple of years so that we can monitor our balancers. I am in the process of updating this patch for 2.4 right now. I plan to submit the updated patch for discussion/input today. I am just doing some final testing and debugging. https://issues.apache.org/bugzilla/show_bug.cgi?id=52264
Re: RFE: Control of HTTP cache control headers within mod_rewrite rules
On Oct 27, 2011, at 3:35 PM, Stefan Fritsch wrote: On Tuesday 25 October 2011, Noah Robin wrote: I ran some tests on this and the following modified version will work: Header always set Cache-Control max-age=%{CACHE_LIFETIME}e env=CACHE_LIFETIME RewriteRule /example http://www.example.com/ [E=CACHE_LIFETIME:604800] ..however, this still leaves an open question in my mind: How to solve for the more general case where I want Apache to set the cache control header on any 301 it sends, even if the 301 was generated within the application rather than in Apache's configuration. I don't see a way to set an environment variable based on a response attribute (e.g. r-status). Am I missing something or would something need to be written to handle this case? 2.3/2.4 supports this: Header set Cache-Control max-age=604800 %{REQUEST_STATUS} == 301 or even %{REQUEST_STATUS} -in { 301 , 302 }. This is a good candidate for an example in the docs. I can't think of a way to do this in 2.2.x, though. In 2.2 you can do this with mod_setenvifplus http://modsetenvifplus.sourceforge.net/. I do things like this all of the time.
mod_proxy_fcgi + mod_proxy_balancer vs. php-fpm and query strings
I am having a couple of problems WRT using mod_proxy_fcgi inside a balancer proxied to php-fpm. There are lots of variables in this scenario, but I think I have narrowed the issues down. The setup looks like this: httpd - balancer - fcgi balancer members - php-fpm Issue 1: PHP-FPM does not handle the proxy:balancer prefix in SCRIPT_FILENAME. It does handle proxy:fcgi as a special case (see https://bugs.php.net/bug.php?id=54152 fix by jim). So, it seems we need to also add a proxy:balancer exception there unless a balanced mod_proxy_fcgi member should actually be using proxy:fcgi instead. What are people's thoughts on the prefix that should be sent by httpd in this case? To address this for now, I have modified PHP (fpm_main.c alongside jim's existing changes). Issue 2: Once I got Issue 1 addressed, everything started working except in the case of a query string. I spent considerable time tracing and trying to figure out where the issue is occurring, but I am hoping one of you who is much more familiar with the code than I will be able to say, Oh, look right here. The problem is that the query string is getting appended to SCRIPT_FILENAME if proxied through a balancer. FPM does not like this. It does not seem to happen in the case of proxying directly to fcgi://..., but once I change this to balancer://..., the query string gets added to SCRIPT_FILENAME. I believe this happened with both ProxyPass* and mod_rewrite [P]. In mod_rewrite, this should get handled in splitout_queryargs(), but somehow it is getting added back (probably in proxy_balancer_canon() which adds the query string back to r-filename?). For right now, I have done a brute-force fix for this by adding the code below to the beginning of send_environment() in mod_proxy_fcgi.c, before the calls to ap_add_common_vars() and ap_add_cgi_vars(). I am guessing that this isn't the ultimate fix for this issue, so I am interested in others' thoughts. +/* Remove query string from r-filename (r-args is already set and passed via QUERY_STRING) */ +q = ap_strchr_c(r-filename, '?'); +if (q != NULL) { +*q = '\0'; +}
Re: mod_proxy_ajp: ignoring flush before headers (again)
This does appear to work. Our automated tests are running right now. Shall I submit a but with this patch attached? On Aug 3, 2011, at 7:43 AM, Plüm, Rüdiger, VF-Group wrote: Can you please try if the following patch fixes your issue? Index: mod_proxy_ajp.c === --- mod_proxy_ajp.c (revision 1150558) +++ mod_proxy_ajp.c (working copy) @@ -506,16 +506,18 @@ if (bb_len != -1) conn-worker-s-read += bb_len; } -if (ap_pass_brigade(r-output_filters, -output_brigade) != APR_SUCCESS) { -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, - proxy: error processing body.%s, - r-connection-aborted ? - Client aborted connection. : ); -output_failed = 1; +if (headers_sent) { +if (ap_pass_brigade(r-output_filters, +output_brigade) != APR_SUCCESS) { +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, + proxy: error processing body.%s, + r-connection-aborted ? + Client aborted connection. : ); +output_failed = 1; +} +data_sent = 1; +apr_brigade_cleanup(output_brigade); } -data_sent = 1; -apr_brigade_cleanup(output_brigade); } } else { Currently the code sends an empty brigade in your case which also triggers the sending of headers by httpd. Regards Rüdiger -Original Message- From: Jim Riggs Sent: Dienstag, 2. August 2011 18:03 To: dev@httpd.apache.org Subject: mod_proxy_ajp: ignoring flush before headers (again) For some (old 2007) context, see: http://markmail.org/message/btwcnbl2i7ftwj4n https://community.jivesoftware.com/message/201787 I am proxying an app via AJP to Tomcat 6/7. In certain circumstances, it appears that the app (or possibly Tomcat) is erroneously sending a flush before the headers have been sent. In r57, Jim added an exception to handle this situation with the intention of ignoring the flush. I'm not sure it's working quite right, though, as the brigade is still getting passed through the filter chain. So, ap_headers_output_filter() is getting called too soon, I think. (I am no expert in the httpd code, so I'm not sure this is really the problem.) Can any of you who ARE experts in the code tell me what you think of the issue and how we can fix it? I'm thinking that if we are ignoring a flush at mod_proxy_ajp.c:448 (in 2.2.x), we should not be calling ap_pass_brigade() at line 472, but I don't know if there are any ramifications of that. The symptom is that when this issue happens, the user gets prompted to save a file (Content-Type returned by httpd is 'text/plain' even though Tomcat is returning 'text/html;charset=utf-8'). Below is some debug output showing correct and incorrect behavior: Correct: [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): proxy: APR_BUCKET_IS_EOS [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read (max 8186 at 4) [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of data [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: status = 200 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: Number of headers is = 5 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[0] [Pragma] = [No-cache] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[2] [Expires] = [Wed, 31 Dec 1969 18:00:00 CST] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[4] [Content-Type] = [text/html;charset=utf-8] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(609): ajp_unmarshal_response: ap_set_content_type done [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 03 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697
Re: mod_proxy_ajp: ignoring flush before headers (again)
https://issues.apache.org/bugzilla/show_bug.cgi?id=51608 Thank you for your help and quick fix, Rüdiger!! On Aug 3, 2011, at 10:09 AM, Plüm, Rüdiger, VF-Group wrote: Thanks for testing. Committed to trunk as r1153531. A bug report mentioning the trunk revision could be handy as a backport tracker for 2.2.x. Regards Rüdiger -Original Message- From: Jim Riggs Sent: Mittwoch, 3. August 2011 16:48 To: dev@httpd.apache.org Subject: Re: mod_proxy_ajp: ignoring flush before headers (again) This does appear to work. Our automated tests are running right now. Shall I submit a but with this patch attached? On Aug 3, 2011, at 7:43 AM, Plüm, Rüdiger, VF-Group wrote: Can you please try if the following patch fixes your issue? Index: mod_proxy_ajp.c === --- mod_proxy_ajp.c (revision 1150558) +++ mod_proxy_ajp.c (working copy) @@ -506,16 +506,18 @@ if (bb_len != -1) conn-worker-s-read += bb_len; } -if (ap_pass_brigade(r-output_filters, - output_brigade) != APR_SUCCESS) { -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, - proxy: error processing body.%s, - r-connection-aborted ? - Client aborted connection. : ); -output_failed = 1; +if (headers_sent) { +if (ap_pass_brigade(r-output_filters, + output_brigade) != APR_SUCCESS) { +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, + proxy: error processing body.%s, + r-connection-aborted ? + Client aborted connection. : ); +output_failed = 1; +} +data_sent = 1; +apr_brigade_cleanup(output_brigade); } -data_sent = 1; -apr_brigade_cleanup(output_brigade); } } else { Currently the code sends an empty brigade in your case which also triggers the sending of headers by httpd. Regards Rüdiger -Original Message- From: Jim Riggs Sent: Dienstag, 2. August 2011 18:03 To: dev@httpd.apache.org Subject: mod_proxy_ajp: ignoring flush before headers (again) For some (old 2007) context, see: http://markmail.org/message/btwcnbl2i7ftwj4n https://community.jivesoftware.com/message/201787 I am proxying an app via AJP to Tomcat 6/7. In certain circumstances, it appears that the app (or possibly Tomcat) is erroneously sending a flush before the headers have been sent. In r57, Jim added an exception to handle this situation with the intention of ignoring the flush. I'm not sure it's working quite right, though, as the brigade is still getting passed through the filter chain. So, ap_headers_output_filter() is getting called too soon, I think. (I am no expert in the httpd code, so I'm not sure this is really the problem.) Can any of you who ARE experts in the code tell me what you think of the issue and how we can fix it? I'm thinking that if we are ignoring a flush at mod_proxy_ajp.c:448 (in 2.2.x), we should not be calling ap_pass_brigade() at line 472, but I don't know if there are any ramifications of that. The symptom is that when this issue happens, the user gets prompted to save a file (Content-Type returned by httpd is 'text/plain' even though Tomcat is returning 'text/html;charset=utf-8'). Below is some debug output showing correct and incorrect behavior: Correct: [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): proxy: APR_BUCKET_IS_EOS [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read (max 8186 at 4) [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of data [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: status = 200 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: Number of headers is = 5 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[0] [Pragma] = [No-cache] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache
mod_proxy_ajp: ignoring flush before headers (again)
For some (old 2007) context, see: http://markmail.org/message/btwcnbl2i7ftwj4n https://community.jivesoftware.com/message/201787 I am proxying an app via AJP to Tomcat 6/7. In certain circumstances, it appears that the app (or possibly Tomcat) is erroneously sending a flush before the headers have been sent. In r57, Jim added an exception to handle this situation with the intention of ignoring the flush. I'm not sure it's working quite right, though, as the brigade is still getting passed through the filter chain. So, ap_headers_output_filter() is getting called too soon, I think. (I am no expert in the httpd code, so I'm not sure this is really the problem.) Can any of you who ARE experts in the code tell me what you think of the issue and how we can fix it? I'm thinking that if we are ignoring a flush at mod_proxy_ajp.c:448 (in 2.2.x), we should not be calling ap_pass_brigade() at line 472, but I don't know if there are any ramifications of that. The symptom is that when this issue happens, the user gets prompted to save a file (Content-Type returned by httpd is 'text/plain' even though Tomcat is returning 'text/html;charset=utf-8'). Below is some debug output showing correct and incorrect behavior: Correct: [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(266): proxy: APR_BUCKET_IS_EOS [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read (max 8186 at 4) [Tue Aug 02 09:34:50 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of data [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: status = 200 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: Number of headers is = 5 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[0] [Pragma] = [No-cache] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[2] [Expires] = [Wed, 31 Dec 1969 18:00:00 CST] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[4] [Content-Type] = [text/html;charset=utf-8] [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(609): ajp_unmarshal_response: ap_set_content_type done [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 03 [Tue Aug 02 09:34:50 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03 [Tue Aug 02 09:34:50 2011] [debug] mod_headers.c(756): headers: ap_headers_output_filter() Incorrect (notice how ap_headers_output_filter() is called before the headers are received): [Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(266): proxy: APR_BUCKET_IS_EOS [Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(271): proxy: data to read (max 8186 at 4) [Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(286): proxy: got 0 bytes of data [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 03 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03 [Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(452): Ignoring flush message received before headers [Tue Aug 02 09:32:18 2011] [debug] mod_headers.c(756): headers: ap_headers_output_filter() [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 03 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 03 [Tue Aug 02 09:32:18 2011] [debug] mod_proxy_ajp.c(452): Ignoring flush message received before headers [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(687): ajp_read_header: ajp_ilink_received 04 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(697): ajp_parse_type: got 04 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(516): ajp_unmarshal_response: status = 200 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(537): ajp_unmarshal_response: Number of headers is = 5 [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[0] [Pragma] = [No-cache] [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[1] [Cache-Control] = [no-cache] [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[2] [Expires] = [Wed, 31 Dec 1969 18:00:00 CST] [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[3] [Set-Cookie] = [JSESSIONID=39968855F543CA08A440E6136EA6FC28.app1; Path=/tomcat-manager; Secure; HttpOnly] [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(599): ajp_unmarshal_response: Header[4] [Content-Type] = [text/html;charset=utf-8] [Tue Aug 02 09:32:18 2011] [debug] ajp_header.c(609): ajp_unmarshal_response: ap_set_content_type done