librelist archives

Re: [mongrel2] First Cut of Filters Committed

From:

Dalton Barreto

Date:

2011-05-17 @ 12:54

2011/5/16 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Mon, May 16, 2011 at 09:18:13PM -0300, Dalton Barreto wrote:
>> Great news! I'm looking forward to see this fully integrated in mongrel2.
>>
>> Reading the filter example I saw that the main filter callback
>> (filter_transition) returns a StateEvent, this return value will be
>> used by mongrel2 to continue the State transitions for this current
>> connection. The Filter_run() function returns to Connection_task() the
>> value returned by the filter_transition().
>
> Yep, that's right.
>
>> That's ok, for example if we want to write a filter that just closes
>> all connections for some particular route, just return CLOSE. My
>> question is, what a filter should return to tell mongrel2: "ok, just
>> go on, I already did my job and you may now continue with your normal
>> State transitions".
>
> You can return any state that's valid at that point, which includes the
> state you were given.  So say you want to do a logging filter of
> some kind, then you'd assign it to all state events, and your filter
> would just do its logging, and return any state it's given.
>

Good point! =) Didn't thought about this. =) Returning the save event
that the filter reveiced
is exactly what I wanted.


>> Maybe the filters could return something like "CONTINUE" or "STOP",
>
> They can, return CLOSE.  The idea is that, all of the filters need to
> receive these events so they can clean up, so they'd all get the CLOSE
> and then exit like they should.

Ok, I got the idea.


> The one problem that you're bringing up here though is that a filter
> could receive *any* event and not realize it.  I'll fix that up.

Right. I didn't realize this. =)


>> this way mongrel2 could stop calling filters if one of them tell it to
>> do so. The way the code is now we can have a situation that a filter
>> will not work because it's return value will not be respected, for
>> instance consider 2 filters:
>>
>> A. Close all connections for route "/the/route"
>> B. Adds a HPPS: on header to all Requests
>
> How about, the chain runs until one of them returns a different state?
> So, lets say you do 5 filters on HANDLER.  It'll run all 5, as long as
> they keep returning HANDLER, but if one returns CLOSE then it stops.
>

That sounds great! Agreed.

>> Another point is the filter_init() function. To me is non-intuitive to
>> receive a pointer to return the number of states that this filter is
>> interested in.
>>
>> Mongrel2 can calculate this the same way that the example filter did
>> (using Filter_states_length()).
>
> No, mongrel2 can't because you can only return a raw pointer from a
> function call, so you can't call sizeof on it.  It's gotta be setup in
> the init function.  They also have to return allocated ram, so this
> makes it easy for them to make the ram, and makes it hard to return a
> pointer to something on the stack (which is a no-go).
>

Hmm, Didn't know about this. Makes sense. I will study about this some
more and thanks for the clarification.

>
>> Actually, a bad written filter could
>> easily segfault mongrel2, if it returns StateEvent events[] = {STATE}
>> but puts 42 in the *out_nstates parameter. Making this would probably
>> crash mongrel2 on this lines
>
> There is *no* protection against this.  At all.  You are coding raw C
> code and injecting it in the server so all bets are off.
>
>>  for(i = 0; i < nstates; i++) {
>>         StateEvent state = states[i];
>>
>>  Ok, I understand that nobody sane would do this on purpose, but
>> anyway, it's just an observation. =)
>
> Yeah, and it's also impossible to prevent.

Alright.

>
>> Zed, I just committed a tiny fix to the tools/filters/Makefile. On
>> make clean, "rm" was complaining about non-existant test_filter.so.
>> It's here: http://mongrel2.org/info/07b1ffdd2b
>
> Yep saw that, thanks.  I'll work on this some more tonight, but the next
> big thing to discuss is how the hell you config these.
>

Yeah, for sure. I thought about some very basic points:

 * Be able to load filters individually for each registered server. We
can have 2 filters but we could load filter-1 on server A and filter-2
on server B.
     - This could the a new table "filter" with just two cols:
file_name, server_id.
 * Have a new setting pointing to the folder where the .so files are:
"filter_folder"

What do you think?

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

On Tue, May 17, 2011 at 09:54:54AM -0300, Dalton Barreto wrote:
> 2011/5/16 Zed A. Shaw <zedshaw@zedshaw.com>:
> > How about, the chain runs until one of them returns a different state?
> > So, lets say you do 5 filters on HANDLER.  It'll run all 5, as long as
> > they keep returning HANDLER, but if one returns CLOSE then it stops.
> >
> 
> That sounds great! Agreed.

Alright, this is working.  Go ahead and play with it by using the
tests/filter_tests unit test to load and run a filter you write.

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

2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, May 17, 2011 at 09:54:54AM -0300, Dalton Barreto wrote:
>> 2011/5/16 Zed A. Shaw <zedshaw@zedshaw.com>:
>> > How about, the chain runs until one of them returns a different state?
>> > So, lets say you do 5 filters on HANDLER.  It'll run all 5, as long as
>> > they keep returning HANDLER, but if one returns CLOSE then it stops.
>> >
>>
>> That sounds great! Agreed.
>
> Alright, this is working.  Go ahead and play with it by using the
> tests/filter_tests unit test to load and run a filter you write.
>

Great! I'll try to look at this later tonight. Thanks.

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

2011/5/17 Dalton Barreto <daltonmatos@gmail.com>:
> 2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
>> On Tue, May 17, 2011 at 09:54:54AM -0300, Dalton Barreto wrote:
>>> 2011/5/16 Zed A. Shaw <zedshaw@zedshaw.com>:
>>> > How about, the chain runs until one of them returns a different state?
>>> > So, lets say you do 5 filters on HANDLER.  It'll run all 5, as long as
>>> > they keep returning HANDLER, but if one returns CLOSE then it stops.
>>> >
>>>
>>> That sounds great! Agreed.
>>
>> Alright, this is working.  Go ahead and play with it by using the
>> tests/filter_tests unit test to load and run a filter you write.
>>
>
> Great! I'll try to look at this later tonight. Thanks.
>

Great. I've played with this new Filter_run() logic and it seems
alright! Actually I wrote two more test cases for this new
implementation.

To check that the filters was really called I changed the conn->rport
value, and then at the end of the test I just check the final value of
conn->rport.

The first test is for the case when we have more than one filter for
the same Event. The second one tests the logic for interrupting the
filter chain.

Please take a look at this implementation and tell me what you think.
I don't know if this "change the conn->rport approach" is correct. If
you think that this makes no sense at all (it's possible! =P ) let me
know and I will revert the changes.

The commit is here: http://mongrel2.org/info/50bc49bd94
All tests passed.

Thanks!

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

On Tue, May 17, 2011 at 09:54:54AM -0300, Dalton Barreto wrote:
> Yeah, for sure. I thought about some very basic points:
> 
>  * Be able to load filters individually for each registered server. We
> can have 2 filters but we could load filter-1 on server A and filter-2
> on server B.
>      - This could the a new table "filter" with just two cols:
> file_name, server_id.
>  * Have a new setting pointing to the folder where the .so files are:
> "filter_folder"

Yes, similar to what I was thinking.  There'd be two levels of config:

* init config -- These are set in the config file with filters={}
* server config -- These are set in each server, and override the init
config.

Then filters mentioned in the init config are loaded out of a consistent
directory in /usr/local/lib (or whatever the OS guys like to use for
mods in apache).

Here's an example:

Server(
    ....
    filters = {
        "gzip": {"compression": 9, "algorithm": "deflate"},
        "auth-ldap": {"routes": ["/Login", "/Logout"],
            "server": "tcp://127.0.0.1:5550"}
    }
)

filters = {
    "gzip": {"compression": 5},
    "auth-ldap": {}
}

So when this server runs, it'll load the gzip.so and auth-ldap.so, then
pass the options to the filter_init as a tns_value_t.

The other thing I'm going to do is predict the future and say that
filters get tns_value_t as their config, and that the config is stored
in the sqlite as tnetstrings blobs inside a table.  This lets filters
have any wacky config they want, and also makes it ready for the future
when they'll be getting the config that way anyhow.

What do you think?

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

2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, May 17, 2011 at 09:54:54AM -0300, Dalton Barreto wrote:
>> Yeah, for sure. I thought about some very basic points:
>>
>>  * Be able to load filters individually for each registered server. We
>> can have 2 filters but we could load filter-1 on server A and filter-2
>> on server B.
>>      - This could the a new table "filter" with just two cols:
>> file_name, server_id.
>>  * Have a new setting pointing to the folder where the .so files are:
>> "filter_folder"
>
> Yes, similar to what I was thinking.  There'd be two levels of config:
>
> * init config -- These are set in the config file with filters={}
> * server config -- These are set in each server, and override the init
> config.
>
> Then filters mentioned in the init config are loaded out of a consistent
> directory in /usr/local/lib (or whatever the OS guys like to use for
> mods in apache).
>

So mongrel2 will just call dlopen("filename.so") and let the OS search the
library to load. In fact, this is good as it will be easier to
configure all things.

> Here's an example:
>
> Server(
>    ....
>    filters = {
>        "gzip": {"compression": 9, "algorithm": "deflate"},
>        "auth-ldap": {"routes": ["/Login", "/Logout"],
>            "server": "tcp://127.0.0.1:5550"}
>    }
> )
>
> filters = {
>    "gzip": {"compression": 5},
>    "auth-ldap": {}
> }
>
> So when this server runs, it'll load the gzip.so and auth-ldap.so, then
> pass the options to the filter_init as a tns_value_t.

Right. When loadind filters mongrel2 will merge the options, giving
precedence to
the options of "Servetr ( ... filters ={ ... } )" ? In this case the
gzip would use "compression = 9" and not "=5". Right?

How this will be on the database? Which table will hold the general
options for a filter? I'm considering a "filter" table
to hold the server's specific filters.

>
> The other thing I'm going to do is predict the future and say that
> filters get tns_value_t as their config, and that the config is stored
> in the sqlite as tnetstrings blobs inside a table.  This lets filters
> have any wacky config they want, and also makes it ready for the future
> when they'll be getting the config that way anyhow.
>
> What do you think?

No problem. Doing this will, for sure, make it a lot easier to load
all options for a filter. Much better that having one option
per line.

Just a quick question: The tns_value_t type represents an already
parsed tnetstring? The filter_init() function will receive a
"tns_value_t config" or a "tns_value_t *config" ?
Consider that I didn't read the tnetstring.[h|c] and don't know yet
the internal API to deal with tnetstrings in C. =)


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

On Tue, May 17, 2011 at 02:30:28PM -0300, Dalton Barreto wrote:
> So mongrel2 will just call dlopen("filename.so") and let the OS search the
> library to load. In fact, this is good as it will be easier to
> configure all things.

Yep, it'll have a consistent naming scheme and just load modules like
that.

> Right. When loadind filters mongrel2 will merge the options, giving
> precedence to
> the options of "Servetr ( ... filters ={ ... } )" ? In this case the
> gzip would use "compression = 9" and not "=5". Right?

Yep, Server overrides file settings.

> How this will be on the database? Which table will hold the general
> options for a filter? I'm considering a "filter" table
> to hold the server's specific filters.

Me too, probably:

filters:
   server_id -- id of the server this applies to
   load_path -- path it got loaded from
   name -- the name used in the config
   config -- the tnetstring of the normalized options

> Just a quick question: The tns_value_t type represents an already
> parsed tnetstring? The filter_init() function will receive a
> "tns_value_t config" or a "tns_value_t *config" ?
> Consider that I didn't read the tnetstring.[h|c] and don't know yet
> the internal API to deal with tnetstrings in C. =)

tns_value_t *config, and then I'll have some handy functions to help get
options easily, like:

Filter_config_get_int()
Filter_config_get_string()

And so on, similar to settings.

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

2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, May 17, 2011 at 02:30:28PM -0300, Dalton Barreto wrote:
>> So mongrel2 will just call dlopen("filename.so") and let the OS search the
>> library to load. In fact, this is good as it will be easier to
>> configure all things.
>
> Yep, it'll have a consistent naming scheme and just load modules like
> that.
>
>...
>> How this will be on the database? Which table will hold the general
>> options for a filter? I'm considering a "filter" table
>> to hold the server's specific filters.
>
> Me too, probably:
>
> filters:
>   server_id -- id of the server this applies to
>   load_path -- path it got loaded from
>   name -- the name used in the config
>   config -- the tnetstring of the normalized options
>

Strange.... How do we know the "load_path" if mongrel2 will just call
"dlopen("module.so")" and let the OS
decide from where this .so will be loaded? Doing this we will never
know the load_path at config time, just at runtime, maybe looking at
the void * returned
by dlopen(). In fact, I don't know if it's possible to find out the
"full path" of the module just looking at the void * returned by
dlopen().


>> Just a quick question: The tns_value_t type represents an already
>> parsed tnetstring? The filter_init() function will receive a
>> "tns_value_t config" or a "tns_value_t *config" ?
>> Consider that I didn't read the tnetstring.[h|c] and don't know yet
>> the internal API to deal with tnetstrings in C. =)
>
> tns_value_t *config, and then I'll have some handy functions to help get
> options easily, like:
>
> Filter_config_get_int()
> Filter_config_get_string()
>
> And so on, similar to settings.
>

Good. So you will decide which Filter_config_get_*() function to call
based on the config[i]->type ?


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

On Tue, May 17, 2011 at 04:30:24PM -0300, Dalton Barreto wrote:
> Strange.... How do we know the "load_path" if mongrel2 will just call
> "dlopen("module.so")" and let the OS

It doesn't work that way, you have to specify the path to the file.
It's not like a linker.

> Good. So you will decide which Filter_config_get_*() function to call
> based on the config[i]->type ?

Yep, and abort if it's not right.

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

2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, May 17, 2011 at 04:30:24PM -0300, Dalton Barreto wrote:
>> Strange.... How do we know the "load_path" if mongrel2 will just call
>> "dlopen("module.so")" and let the OS
>
> It doesn't work that way, you have to specify the path to the file.
> It's not like a linker.

Well, that's not what I understood reading the dlopen() man page. It says:

"If  filename  contains  a slash ("/"), then it is
interpreted as a  (relative  or  absolute)  pathname.   Otherwise,  the
dynamic  linker  searches  for the library as follows (see ld.so(8) for
further details):"

And then comes the details about the search order, LD_LIBRARY_PATH,
/etc/ld.so.cache and etc. But I think this does only apply to linux? I
just read the OS X dlopen() man page and the behavior is *very*
different.... Ok, I think we need the load_path, then. =)


>> Good. So you will decide which Filter_config_get_*() function to call
>> based on the config[i]->type ?
>
> Yep, and abort if it's not right.

Right.


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

On Tue, May 17, 2011 at 08:20:57PM -0300, Dalton Barreto wrote:
> 2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> > On Tue, May 17, 2011 at 04:30:24PM -0300, Dalton Barreto wrote:
> >> Strange.... How do we know the "load_path" if mongrel2 will just call
> >> "dlopen("module.so")" and let the OS
> >
> > It doesn't work that way, you have to specify the path to the file.
> > It's not like a linker.
> 
> Well, that's not what I understood reading the dlopen() man page. It says:
> 
> "If  filename  contains  a slash ("/"), then it is
> interpreted as a  (relative  or  absolute)  pathname.   Otherwise,  the
> dynamic  linker  searches  for the library as follows (see ld.so(8) for
> further details):"

Nope, that has to be wrong.  Look at the unit test tests/filter_tests.c
and you'll see I'm right there using the path
"tools/filters/test_filter.so" to load it.

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

2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> On Tue, May 17, 2011 at 08:20:57PM -0300, Dalton Barreto wrote:
>> 2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
>> > On Tue, May 17, 2011 at 04:30:24PM -0300, Dalton Barreto wrote:
>> >> Strange.... How do we know the "load_path" if mongrel2 will just call
>> >> "dlopen("module.so")" and let the OS
>> >
>> > It doesn't work that way, you have to specify the path to the file.
>> > It's not like a linker.
>>
>> Well, that's not what I understood reading the dlopen() man page. It says:
>>
>> "If  filename  contains  a slash ("/"), then it is
>> interpreted as a  (relative  or  absolute)  pathname.   Otherwise,  the
>> dynamic  linker  searches  for the library as follows (see ld.so(8) for
>> further details):"
>
> Nope, that has to be wrong.  Look at the unit test tests/filter_tests.c
> and you'll see I'm right there using the path
> "tools/filters/test_filter.so" to load it.

Yes, and this is the relative path mentioned in the man page. This
loading only works because we call the tests having the project's root
as the working directory, so the relative path "tools/filters" exists.

I made this modifications to filter_tests:

--- filter_tests.c
+++ filter_tests.c
@@ -6,11 +6,11 @@
 FILE *LOG_FILE = NULL;

 char *test_Filter_load()
 {
     Server *srv = NULL;
-    bstring load_path = bfromcstr("tools/filters/test_filter.so");
+    bstring load_path = bfromcstr("test_filter.so");

     int res = Filter_load(srv, load_path);
     mu_assert(res == 0, "Failed to load tools/filters/test_filter.so");
     mu_assert(Filter_activated(), "Filters not activated.");


A then ran the test from the project root:

$ ./tests/filter_tests
----
RUNNING: ./tests/filter_tests
FAILED: Failed to load tools/filters/test_filter.so
Tests run: 1

Now if I run the test this way:

$ LD_LIBRARY_PATH=/home/daltonmatos/projetos/mongrel2/mongrel2/tools/filters/
./tests/filter_tests
----
RUNNING: ./tests/filter_tests
ALL TESTS PASSED
Tests run: 4

With LD_LIBRARY_PATH the test passes with any working directory. But
since the BSD dlopen() does not behave like this, it's better to
always use paths, absolute or relative.

The test passes also if you make no modifications but create a symlink
inside tests/: "ln -s ../tools/ tools" and then call the test from
inside the tests/: ./filter_tests. This is because the realtive path
"tools/filters" again is valid.

Note: Remember to modify minunit.h also, if it complains about the log file.

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

On Wed, May 18, 2011 at 10:25 AM, Zed A. Shaw <zedshaw@zedshaw.com> wrote:

> On Tue, May 17, 2011 at 08:20:57PM -0300, Dalton Barreto wrote:
> > 2011/5/17 Zed A. Shaw <zedshaw@zedshaw.com>:
> > > On Tue, May 17, 2011 at 04:30:24PM -0300, Dalton Barreto wrote:
> > >> Strange.... How do we know the "load_path" if mongrel2 will just call
> > >> "dlopen("module.so")" and let the OS
> > >
> > > It doesn't work that way, you have to specify the path to the file.
> > > It's not like a linker.
> >
> > Well, that's not what I understood reading the dlopen() man page. It
> says:
> >
> > "If  filename  contains  a slash ("/"), then it is
> > interpreted as a  (relative  or  absolute)  pathname.   Otherwise,  the
> > dynamic  linker  searches  for the library as follows (see ld.so(8) for
> > further details):"
>
> Nope, that has to be wrong.  Look at the unit test tests/filter_tests.c
> and you'll see I'm right there using the path
> "tools/filters/test_filter.so" to load it.
>
> --
> Zed A. Shaw
> http://zedshaw.com/
>

The manual page is correct, you can use either method.