librelist archives

Re: [mongrel2] mongrel2 falls back to default_host only when there is no Host: header in the request.

From:

Dalton Barreto

Date:

2011-04-25 @ 23:23

2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Sat, Apr 23, 2011 at 11:17:24PM -0300, Dalton Barreto wrote:
>> Hello Zed,
>>
>> I saw that ticket ea23f3f8e0 [1] was closed so tried a simple test case.
>>
>> I have as server with default_host = 'localhost'. And the host table
>> has just the localhost entry. I have one route, linked with this
>> default_host.
>
>> mongrel2 still gives the same error message as before:
>> [ERROR] (src/connection.c:104: errno: Resource temporarily
>> unavailable) Request for a host we don't have registered: 127.0.0.1
>
>> Shouldn't mongrel2 redirect this request to default_host too? Or did I
>> miss something about this default_host fall back stuff?
>
> So, I'll have to see your but "default_host" doesn't mean "default
> address of a server" but the actuall Host.name of a Host you've
> configured.  Are you doing it this way:
>
> Server(default_host="localhost", hosts=[Host(name="localhost")])
>
> If that's what you're doing then go ahead and reopen the ticket, and
> I'll do up a test case that matches your config.

That's it. This is the exactly config I'm using for this test:

main = Server(
    uuid="f400bf85-4538-4f7a-8908-67e313d515c2",
    access_log="/logs/access.log",
    error_log="/logs/error.log",
    chroot="./",
    default_host="localhost",
    name="test",
    pid_file="/run/mongrel2.pid",
    port=8888,
    hosts = [
        Host(name="localhost", routes={
            '/': Handler(send_spec='tcp://127.0.0.1:5002',
                         send_ident='ca468090-6f8c-11e0-9f56-001fe149503a',
                         recv_spec='tcp://127.0.0.1:5003')
        })
    ]
)

servers = [main]

And the results:

daltonmatos@maya ~ [7]$ curl http://localhost:8888/
OK
daltonmatos@maya ~ [8]$ curl http://127.0.0.1:8888/
Not Found

daltonmatos@maya ~ [9]$ curl -H "Host:" http://127.0.0.1:8888/
OK
daltonmatos@maya ~ [10]$


Reading the code at server.c:Server_match_backend() it does this:

    Route *found = RouteMap_match_suffix(srv->hosts, target);

if found is not NULL, ok end of search and we have a positive match.
But if found == NULL the code tries to match against
srv->default_host->matching.
Based on this I made a change in the config: Changed the host from
matching="localhost" to matching="127.0.0.1" and did the tests one
more time:

daltonmatos@maya ~ [10]$ curl http://localhost:8888/
Not Found
mongrel2 generates the error log: Request for a host we don't have
registered: localhost

daltonmatos@maya ~ [11]$ curl http://127.0.0.1:8888/
OK

daltonmatos@maya ~ [12]$ curl -H "Host:" http://localhost:8888/
OK
daltonmatos@maya ~ [13]$

Now we have the oposite situation.

Maybe the code could do:

    Route *found = RouteMap_match_suffix(srv->hosts, target); //Search
on all hosts of this server
    if (!found)
      found = RouteMap_match_suffix(srv->default_host, target); //
Search only on default_host

    // Rest of code here...

OR

we could include srv->default_host in srv->hosts, so default_host
would be automatically searched.

Note: I'm still learning about mongrel internals and did not fully
understand all the struct tst_t -> {low, equal, high} stuff, but I
suppose match_suffix(srv->hosts, target) will traverse all hosts
trying to match target against one of them.

Thanks.

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

On Mon, Apr 25, 2011 at 08:23:34PM -0300, Dalton Barreto wrote:
 
> Reading the code at server.c:Server_match_backend() it does this:
> 
>     Route *found = RouteMap_match_suffix(srv->hosts, target);
> 
> if found is not NULL, ok end of search and we have a positive match.
> But if found == NULL the code tries to match against
>srv->default_host->matching.

Ok, just checked and you're right on this, basically for your config to
work you'd have to have the matching="(.*)", but that's probably counter
intuitive.

I'm in the middle of fixing another part of the code, so how about you
do the fix for this.  You should just need to have it return
srv->default_host if it didn't find anything, and if that's NULL then
it'll fail as expected.  No need to redo the search or anything since
srv->default is a Host struct.

-- 
Zed A. Shaw
http://zedshaw.com/

2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Mon, Apr 25, 2011 at 08:23:34PM -0300, Dalton Barreto wrote:
>
>> Reading the code at server.c:Server_match_backend() it does this:
>>
>>     Route *found = RouteMap_match_suffix(srv->hosts, target);
>>
>> if found is not NULL, ok end of search and we have a positive match.
>> But if found == NULL the code tries to match against
>>srv->default_host->matching.
>
> Ok, just checked and you're right on this, basically for your config to
> work you'd have to have the matching="(.*)", but that's probably counter
> intuitive.
>
> I'm in the middle of fixing another part of the code, so how about you
> do the fix for this.  You should just need to have it return
> srv->default_host if it didn't find anything, and if that's NULL then
> it'll fail as expected.  No need to redo the search or anything since
> srv->default is a Host struct.

Alright! I'll study how unit test are done and try to write one for
this case and implement the fix.

Thanks.

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

2011/4/25 Dalton Barreto <daltonmatos@gmail.com>:
> 2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
>> On Mon, Apr 25, 2011 at 08:23:34PM -0300, Dalton Barreto wrote:
>>
>>> Reading the code at server.c:Server_match_backend() it does this:
>>>
>>>     Route *found = RouteMap_match_suffix(srv->hosts, target);
>>>
>>> if found is not NULL, ok end of search and we have a positive match.
>>> But if found == NULL the code tries to match against
>>>srv->default_host->matching.
>>
>> Ok, just checked and you're right on this, basically for your config to
>> work you'd have to have the matching="(.*)", but that's probably counter
>> intuitive.
>>
>> I'm in the middle of fixing another part of the code, so how about you
>> do the fix for this.  You should just need to have it return
>> srv->default_host if it didn't find anything, and if that's NULL then
>> it'll fail as expected.  No need to redo the search or anything since
>> srv->default is a Host struct.
>
> Alright! I'll study how unit test are done and try to write one for
> this case and implement the fix.
>


Realized that the test case already exists, just adjusted it. I'll
wait for your review (at least on first patches) just to avoid
committing any crap to the main repo. =)


Patch follows:


Index: src/server.c
===================================================================
--- src/server.c
+++ src/server.c
@@ -223,18 +223,11 @@
     Route *found = RouteMap_match_suffix(srv->hosts, target);

     if(found) {
         return found->data;
     } else {
-        if(bstring_match(target, srv->default_host->matching)) {
-            return srv->default_host;
-        } else {
-            debug("Failed to match against any target and didn't
match default_host: %s:%s",
-                    bdata(srv->default_hostname),
-                    bdata(srv->deafult_host->matching));
-            return NULL;
-        }
+    	return srv->default_host;
     }

 error: // fallthrough
     return NULL;
 }

Index: tests/server_tests.c
===================================================================
--- tests/server_tests.c
+++ tests/server_tests.c
@@ -43,11 +43,11 @@
     Server_set_default_host(srv, host);

     Host *zedshaw = Server_match_backend(srv, host->name);
     mu_assert(zedshaw == host, "Didn't get the right one back.");

-    mu_assert(Server_match_backend(srv, bfromcstr("NOWAY")) == NULL,
"Should not find this one.");
+    mu_assert(Server_match_backend(srv, bfromcstr("NOWAY")) == host,
"Didn't fall back to default_host");

     Server_destroy(srv);

     return NULL;
 }


If you agree with this fix, I'll commit it right away.

Thanks.

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

On Mon, Apr 25, 2011 at 09:28:21PM -0300, Dalton Barreto wrote:
 
> Realized that the test case already exists, just adjusted it. I'll
> wait for your review (at least on first patches) just to avoid
> committing any crap to the main repo. =)

Perfect, commit it and I'll have a few things I'm changing to fix too.

-- 
Zed A. Shaw
http://zedshaw.com/

2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Mon, Apr 25, 2011 at 09:28:21PM -0300, Dalton Barreto wrote:
>
>> Realized that the test case already exists, just adjusted it. I'll
>> wait for your review (at least on first patches) just to avoid
>> committing any crap to the main repo. =)
>
> Perfect, commit it and I'll have a few things I'm changing to fix too.

Great! I'll do it later tonight.

Thanks.

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

2011/4/26 Dalton Barreto <daltonmatos@gmail.com>:
> 2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
>> On Mon, Apr 25, 2011 at 09:28:21PM -0300, Dalton Barreto wrote:
>>
>>> Realized that the test case already exists, just adjusted it. I'll
>>> wait for your review (at least on first patches) just to avoid
>>> committing any crap to the main repo. =)
>>
>> Perfect, commit it and I'll have a few things I'm changing to fix too.
>
> Great! I'll do it later tonight.

Done.

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

On Tue, Apr 26, 2011 at 07:50:00PM -0300, Dalton Barreto wrote:
> >> Perfect, commit it and I'll have a few things I'm changing to fix too.
> >
> > Great! I'll do it later tonight.
> 
> Done.

FYI, you had a bug in the SQL query such that it did a segfault.  Go
look at the latest commit to see what I had to change and then confirm
that I didn't break anything.

Also, install valgrind and run your stuff under it.  Valgrind found this
right away.

-- 
Zed A. Shaw
http://zedshaw.com/

2011/4/27 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, Apr 26, 2011 at 07:50:00PM -0300, Dalton Barreto wrote:
>> >> Perfect, commit it and I'll have a few things I'm changing to fix too.
>> >
>> > Great! I'll do it later tonight.
>>
>> Done.
>
> FYI, you had a bug in the SQL query such that it did a segfault.  Go
> look at the latest commit to see what I had to change and then confirm
> that I didn't break anything.
>

Hello Zed,

I think there is a little confusion here.
The patch I committed does not touch any query. Was just about
returning srv->defaul_host. [1]

The patch for dir_ttl was another one, from another contributor. [2]

> Also, install valgrind and run your stuff under it.  Valgrind found this
> right away.
>

valgrind is great! I already use it!


[1] http://mongrel2.org/info/65539e2024
[2] http://mongrel2.org/info/61ec414d5a

-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

On Wed, Apr 27, 2011 at 02:13:13PM -0300, Dalton Barreto wrote:
> I think there is a little confusion here.
> The patch I committed does not touch any query. Was just about
> returning srv->defaul_host. [1]
> 
> The patch for dir_ttl was another one, from another contributor. [2]

Oh, ha my bad, getting people confused.  Carry on then! Good job! :-)

-- 
Zed A. Shaw
http://zedshaw.com/

2011/4/27 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Wed, Apr 27, 2011 at 02:13:13PM -0300, Dalton Barreto wrote:
>> I think there is a little confusion here.
>> The patch I committed does not touch any query. Was just about
>> returning srv->defaul_host. [1]
>>
>> The patch for dir_ttl was another one, from another contributor. [2]
>
> Oh, ha my bad, getting people confused.  Carry on then! Good job! :-)
>

Alright, no problem! =)


-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com

On Mon, Apr 25, 2011 at 08:23:34PM -0300, Dalton Barreto wrote:
> > Server(default_host="localhost", hosts=[Host(name="localhost")])
> >
> > If that's what you're doing then go ahead and reopen the ticket, and
> > I'll do up a test case that matches your config.
> 
> That's it. This is the exactly config I'm using for this test:

Alright, go ahead and reopen it, I'll fix it in a few.

>     Route *found = RouteMap_match_suffix(srv->hosts, target); //Search
> on all hosts of this server
>     if (!found)
>       found = RouteMap_match_suffix(srv->default_host, target); //

Nah, it's supposed to be set always in the new code, and then used so it
should be the default_host element of the struct if not found.

> we could include srv->default_host in srv->hosts, so default_host
> would be automatically searched.

And, it should also always be included in the list, so maybe it's not.

> Note: I'm still learning about mongrel internals and did not fully
> understand all the struct tst_t -> {low, equal, high} stuff, but I
> suppose match_suffix(srv->hosts, target) will traverse all hosts
> trying to match target against one of them.

Yep, that's right, but then the srv->default_host is supposed to be a
"fallback" that always gets used.  I'll go look and see why it's not.

-- 
Zed A. Shaw
http://zedshaw.com/

2011/4/25 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Mon, Apr 25, 2011 at 08:23:34PM -0300, Dalton Barreto wrote:
>> > Server(default_host="localhost", hosts=[Host(name="localhost")])
>> >
>> > If that's what you're doing then go ahead and reopen the ticket, and
>> > I'll do up a test case that matches your config.
>>
>> That's it. This is the exactly config I'm using for this test:
>
> Alright, go ahead and reopen it, I'll fix it in a few.
>

Done.


-- 
Dalton Barreto
http://daltonmatos.wordpress.com
http://wsgid.com