New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Python Modules #1845

Open
sword2100 opened this Issue Nov 14, 2016 · 18 comments

Comments

Projects
None yet
7 participants
@sword2100

sword2100 commented Nov 14, 2016

Issue type

  • Defect

Defect description

Using SLES 12 SP1 with github source compiled version 3.0.13

radiusd: FreeRADIUS Version 3.0.13, for host x86_64-unknown-linux-gnu, built on Nov 14 2016 at 10:58:45
FreeRADIUS Version 3.0.13
Copyright (C) 1999-2016 The FreeRADIUS server project and contributors

Trying to use multiple python modules/files.
I have created two python files:

  • /usr/local/etc/raddb/mods-config/python/one.py

  • /usr/local/etc/raddb/mods-config/python/two.py

Changed the needed lines at python module

  • /usr/local/etc/raddb/mods-available/python
python one{
        python_path="/usr/local/etc/raddb/mods-config/python/:/usr/lib64/python2.7/:/usr/lib64/python2.7/site-packages/:/usr/lib64/python2.7/lib-dynload/"
        module = one

        mod_authorize = ${.module}
        func_authorize = authorize
}

python two{
        python_path="/usr/local/etc/raddb/mods-config/python/:/usr/lib64/python2.7/:/usr/lib64/python2.7/site-packages/:/usr/lib64/python2.7/lib-dynload/"
        module = two

        mod_authorize = ${.module}
        func_authorize = authorize
}

And added to the default server

  • /usr/local/etc/raddb/sites-available/default
authorize {
        filter_username
        preprocess
        auth_log
        one
        two
}

Running FreeRadius with radius -XXX and get this message

Debug:   # Instantiating module "one" from file /usr/local/etc/raddb/mods-enabled/python
Info: Python version: 2.7.9 (default, Dec 21 2014, 11:02:59) [GCC]
Debug:   # Instantiating module "two" from file /usr/local/etc/raddb/mods-enabled/python
Error: python_function_load - Module 'two' not found
Error: <type 'exceptions.SystemError'> (null argument to internal routine)
Error: python_function_load - Failed to import python function 'two.authorize'
Error: /usr/local/etc/raddb/mods-enabled/python[27]: Instantiation failed for module "two"

It looks like for the second module the python_path is not working.
I've already tried to export the PYTHONPATH="[as in module]" but same message.

@herwinw

This comment has been minimized.

Show comment
Hide comment
@herwinw

herwinw Nov 15, 2016

Member

The code that sets the python path basicly looks like this:

path = talloc_strdup(NULL, inst->python_path);
PySys_SetPath(path);
talloc_free(path);

There is no reference to the actual interpreter that should be used here, so I guess this is a kind of global setting. I can't really find any information on how to set the path for a specific interpreter (which still would be flawed, because the cext_compat option allows you to reuse the main interpreter)

A possible option would to move the python path out of the instance config to a global setting, but this doesn't really fit well with the config format used in Freeradius.

Is it possible to set the Python path from within a script? In that case I guess the best solution here would be to encourage that, and eventually drop the python_path config option.

Member

herwinw commented Nov 15, 2016

The code that sets the python path basicly looks like this:

path = talloc_strdup(NULL, inst->python_path);
PySys_SetPath(path);
talloc_free(path);

There is no reference to the actual interpreter that should be used here, so I guess this is a kind of global setting. I can't really find any information on how to set the path for a specific interpreter (which still would be flawed, because the cext_compat option allows you to reuse the main interpreter)

A possible option would to move the python path out of the instance config to a global setting, but this doesn't really fit well with the config format used in Freeradius.

Is it possible to set the Python path from within a script? In that case I guess the best solution here would be to encourage that, and eventually drop the python_path config option.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Nov 15, 2016

Member

Yeah I remember it being global. Maybe just complain that it's already set?

Member

arr2036 commented Nov 15, 2016

Yeah I remember it being global. Maybe just complain that it's already set?

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Nov 15, 2016

Member

...and yes Python really doesn't embed well.

Member

arr2036 commented Nov 15, 2016

...and yes Python really doesn't embed well.

@herwinw

This comment has been minimized.

Show comment
Hide comment
@herwinw

herwinw Nov 15, 2016

Member

Yeah I remember it being global. Maybe just complain that it's already set?

That can still cause some unexpected behaviour. Imagine this config

python python_stable {
  module = blah
  ...
}
python python_testing {
  python_path = /somewhere/python/testing
  module = blah
 ....
}

This won't issue any complaints because we only set it once, but suddenly your python_stable call uses the testing libraries.

Member

herwinw commented Nov 15, 2016

Yeah I remember it being global. Maybe just complain that it's already set?

That can still cause some unexpected behaviour. Imagine this config

python python_stable {
  module = blah
  ...
}
python python_testing {
  python_path = /somewhere/python/testing
  module = blah
 ....
}

This won't issue any complaints because we only set it once, but suddenly your python_stable call uses the testing libraries.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Nov 15, 2016

Member

Refuse to start if it's not set to the same value across all instances.

Member

arr2036 commented Nov 15, 2016

Refuse to start if it's not set to the same value across all instances.

@Deisuan

This comment has been minimized.

Show comment
Hide comment
@Deisuan

Deisuan Nov 22, 2016

Contributor

I'll have a look whether or not the python3 module still has the same issue, as soon as I've fixed some other issues.

Contributor

Deisuan commented Nov 22, 2016

I'll have a look whether or not the python3 module still has the same issue, as soon as I've fixed some other issues.

@herwinw

This comment has been minimized.

Show comment
Hide comment
@herwinw

herwinw Nov 22, 2016

Member

Refuse to start if it's not set to the same value across all instances.

I'm afraid that's the only working option left. Then again, I would probably start cursing at my monitor when I would get that as a config error, it would just feel a bit retarded.

Member

herwinw commented Nov 22, 2016

Refuse to start if it's not set to the same value across all instances.

I'm afraid that's the only working option left. Then again, I would probably start cursing at my monitor when I would get that as a config error, it would just feel a bit retarded.

alandekok added a commit that referenced this issue Feb 27, 2017

@alandekok

This comment has been minimized.

Show comment
Hide comment
@alandekok

alandekok Feb 27, 2017

Member

I've pushed documentation to help. For now, I don't think it's worth fixing v3. This may be easier to fix in v4.

Member

alandekok commented Feb 27, 2017

I've pushed documentation to help. For now, I don't think it's worth fixing v3. This may be easier to fix in v4.

@erindru

This comment has been minimized.

Show comment
Hide comment
@erindru

erindru Apr 27, 2017

So right now, theres no way of instantiating multiple python modules in v3?

Thats a bugger - I have a throughput issue where I have used the python module to make an AMQP request to another service authenticate the user. However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

I was hoping to be able to instantiate multiple instances of the python module and use the redundant-load-balance option to load balance between them - will this be possible in v4? I tried it in 3.0.13 but I encountered the same issue as @sword2100

erindru commented Apr 27, 2017

So right now, theres no way of instantiating multiple python modules in v3?

Thats a bugger - I have a throughput issue where I have used the python module to make an AMQP request to another service authenticate the user. However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

I was hoping to be able to instantiate multiple instances of the python module and use the redundant-load-balance option to load balance between them - will this be possible in v4? I tried it in 3.0.13 but I encountered the same issue as @sword2100

@herwinw

This comment has been minimized.

Show comment
Hide comment
@herwinw

herwinw Apr 27, 2017

Member

In reply to @erindru

So right now, theres no way of instantiating multiple python modules in v3?

That's not what this case is about. It's perfectly possible possible to instantiate to have multiple Python modules, just as long as every module uses the same python_path. This looks like a limitation in Python itself, a few workarounds have been discussed here, but none of them really work.

I have a throughput issue where I have used the python module to make an AMQP request to another service

This kind of things might be easier with the rlm_rest module.

However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

Wouldn't just increasing the number of threads fix this (or is the python module not thread safe?)

Member

herwinw commented Apr 27, 2017

In reply to @erindru

So right now, theres no way of instantiating multiple python modules in v3?

That's not what this case is about. It's perfectly possible possible to instantiate to have multiple Python modules, just as long as every module uses the same python_path. This looks like a limitation in Python itself, a few workarounds have been discussed here, but none of them really work.

I have a throughput issue where I have used the python module to make an AMQP request to another service

This kind of things might be easier with the rlm_rest module.

However, its become a bottleneck because it has to block and wait for a reply, which means it quickly becomes overloaded when for example a NAS restarts and ~100 users reauthenticate at the same time

Wouldn't just increasing the number of threads fix this (or is the python module not thread safe?)

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Apr 27, 2017

Member

@erindru You can instantiate multiple python modules but it won't improve performance because you're stuck with a single interpreter.

If you're really suffering from blocking the async rlm_rest module in v4.0.x allows many thousands of concurrent requests.

Member

arr2036 commented Apr 27, 2017

@erindru You can instantiate multiple python modules but it won't improve performance because you're stuck with a single interpreter.

If you're really suffering from blocking the async rlm_rest module in v4.0.x allows many thousands of concurrent requests.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Apr 27, 2017

Member

LDAP would benefit from a global configuration section as well.

@alandekok What do you think about having

global {
	ldap {
		debug_level = 0x000
	}

	python {
		path = "whatever"
	}
}

and an extra global instantiation callback in the modules?

There's just some stuff which doesn't belong in module instance configs and needs to be set globally.

Member

arr2036 commented Apr 27, 2017

LDAP would benefit from a global configuration section as well.

@alandekok What do you think about having

global {
	ldap {
		debug_level = 0x000
	}

	python {
		path = "whatever"
	}
}

and an extra global instantiation callback in the modules?

There's just some stuff which doesn't belong in module instance configs and needs to be set globally.

@alanbuxey

This comment has been minimized.

Show comment
Hide comment
@alanbuxey

alanbuxey Apr 27, 2017

Member
Member

alanbuxey commented Apr 27, 2017

@alandekok

This comment has been minimized.

Show comment
Hide comment
@alandekok

alandekok Apr 27, 2017

Member

Templates already do that to a large extent, tho people don't use them a lot.

Member

alandekok commented Apr 27, 2017

Templates already do that to a large extent, tho people don't use them a lot.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Apr 27, 2017

Member

@alanbuxey no, and you can't today.

The point is there are some configuration items which map to library level settings, so they shouldn't really be set in module instances. They're global to the server.

Member

arr2036 commented Apr 27, 2017

@alanbuxey no, and you can't today.

The point is there are some configuration items which map to library level settings, so they shouldn't really be set in module instances. They're global to the server.

@alanbuxey

This comment has been minimized.

Show comment
Hide comment
@alanbuxey

alanbuxey Apr 27, 2017

Member
Member

alanbuxey commented Apr 27, 2017

@herwinw

This comment has been minimized.

Show comment
Hide comment
@herwinw

herwinw Apr 27, 2017

Member

Excerpt from eap:

tls-config tls-common {
  private_key_password = whatever
  ...
}

ttls {
  tls = tls-common
 ...
}

Excerpt from rest

tls {
  ca_file = ${certdir}/cacert.pem
  ...
}

authorize {
  tls = ${..tls}
  ...
}

These are just the first two things that came to my mind when thinking about having some global setting and a possibility to override them.

Member

herwinw commented Apr 27, 2017

Excerpt from eap:

tls-config tls-common {
  private_key_password = whatever
  ...
}

ttls {
  tls = tls-common
 ...
}

Excerpt from rest

tls {
  ca_file = ${certdir}/cacert.pem
  ...
}

authorize {
  tls = ${..tls}
  ...
}

These are just the first two things that came to my mind when thinking about having some global setting and a possibility to override them.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Apr 27, 2017

Member

Offline discussion.

Decided on a library section which gets passed to the load callback in modules.

Member

arr2036 commented Apr 27, 2017

Offline discussion.

Decided on a library section which gets passed to the load callback in modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment