Python3 support #128

Closed
wants to merge 30 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

kwirk commented Feb 24, 2013

I've made some changes that enable fail2ban to work with python3 once 2to3 has been used. Fail2ban still functions with python2.4+ (if you haven't run 2to3 of course 😉)

One potential issue, is that all log files are opened as 'utf-8' encoding, with encoding errors ignored. Not sure how much of an issue this is, as far is I'm aware, all logs are utf-8 or plain ascii.
Did consider logging a warning if failed to decode a log line, but was worried would spam the fail2ban log files if there is a reoccurring issue.
Also looked at just using bytes, but then regexs complain, such that you need both a compiled bytes and str/unicode for regexs.

I've run testscases on python2.[4-7] and python3.[0-3] with success. Also ran actively with python2.7 and python3.3 and all seems to function with no issues.

To use, execute ./fail2ban-2to3. You can then use ./fail2ban-testcases-all-python3 or ./fail2ban-testcases to run tests.

Any testers would be appreciated. 😄

Thanks

kwirk added some commits Feb 23, 2013

@kwirk kwirk Remove spurious space at start of line c926c88
@kwirk kwirk Check for unicode and encode before md5sum in filter 99c92a5
@kwirk kwirk Change sort in datedetector to use key based comparison 2ecf345
@kwirk kwirk time.mktime argument should be tuple ad7119a
@kwirk kwirk Change filter to ignore unicode errors in python3
Also do not try to convert unicode to unicode again for python3 and
python2
2bb3469
@kwirk kwirk Wrap open method to allow python3 to ignore unicode errors 53be2ad
@kwirk kwirk Ensure jail names return in alphabetical order in status
Primarily to support testing as order can switch
5d0d362
@kwirk kwirk python3: Default open encoding for files to UTF-8
Also tidy up unicode check in processLine, which is handled by `2to3`
df25506
@kwirk kwirk Remove spurious space in fail2ban-server 6f4da8f
@kwirk kwirk Fix up for client/server socket for python3 46a2b6e
@kwirk kwirk Change filter testcases python3 open wrapper to utf-8 e28a698
@kwirk kwirk Fix use of python3 bytes in client/server socket for python2.5 0dd3a81
@kwirk kwirk Fix DNSUtils dnsToIp not catching general socket.error exception c8c9ed4
@kwirk kwirk Added helper scripts to carry out 2to3 and testcases on python3 dbc40d6
@kwirk kwirk fail2ban-regex open files as utf-8 for python3 418d845
@kwirk kwirk Remove functools from filter testcases for python2.4 compatibility 31b173f
@kwirk kwirk Undo removal of unicode prefix in server/datetemplate.py 3a3d07e
@kwirk kwirk Minor typo in fail2ban-testcases-all-python3 78d86bc
@kwirk kwirk Remove redundant reassignment of variable 184e0ec
@kwirk kwirk Fix incorrect exit code from fail2ban-2to3 7e1819e
Owner

yarikoptic commented Feb 25, 2013

On Sun, 24 Feb 2013, Steven Hiscocks wrote:

I've made some changes that enable fail2ban to work with python3 once 2to3
has been used. Fail2ban still functions with python2.5+ (if you haven't
run 2to3 of course [1]😉)

yikes -- wow -- thanks! ;) do you think we should target master
with it or future 0.9?

what about 2.4 -- we still 'support' it, tests seems to pass with it in
your branch... didn't inspect your diff much yet

it would be nice to reduce 'if .*version' clauses use at run time (just
bind functions to convenience helpers once upon imports)

One potential issue, is that all log files are opened as 'utf-8' encoding,
with encoding errors ignored. Not sure how much of an issue this is, as
far is I'm aware, all logs are utf-8 or plain ascii.

it might sense to providing encoding as an option for jails (default to
'auto' ie not specifying any, thus going for system configuration). or
am I missing the point?

Did consider logging a warning if failed to decode a log line, but was
worried would spam the fail2ban log files if there is a reoccurring issue.

if it reoccurs -- configuration better be fixed. swallowing them might
be more detrimental

Also looked at just using bytes, but then regexs complain,

yeah -- correct operation should be to decode from utf-8 into unicode
asap and stick with it all the way until dumping out (e.g. including
into log msgs) when it should be encoded again

Yaroslav O. Halchenko
http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org
Postdoctoral Fellow, Department of Psychological and Brain Sciences
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

Contributor

kwirk commented Feb 25, 2013

@yarikoptic I originally meant to do the pull request to 0.9 branch, but did it against master in error. There doesn't seem to be a way to change it. I can close it a create a new pull request against the 0.9 branch if you like?

I added commit 31b173f which means should work with python2.4. I added it not long after creating the pull request. Turns out only the tests themselves failed ;)

I'm torn on the idea of setting the encoding: The idea of setting the encoding option in jails makes sense but this does get complicated quickly, as arguably you need to specify the encoding per log file...

@kwirk kwirk Move handling of unicode decoding to FileContainer readline
Also print warning for unicode decode failure, leaving as str in python2
and ignoring erroneous characters in python3
d23d365
Owner

yarikoptic commented Feb 26, 2013

On Mon, 25 Feb 2013, Steven Hiscocks wrote:

[1]@yarikoptic I originally meant to do the pull request to 0.9 branch,
but did it against master in error. There doesn't seem to be a way to
change it. I can close it a create a new pull request against the 0.9
branch if you like?

or whenever it is ready I could just merge into 0.9 and close this PR
manually... so no worries ;)

I added commit [2]31b173f which means should work with python2.4. I added
it not long after creating the pull request. Turns out only the tests
themselves failed ;)

;-)

I'm torn on the idea of setting the encoding: The idea of setting the
encoding option in jails makes sense but this does get complicated
quickly, as arguably you need to specify the encoding per log file...

in most of the use-cases setting of encoding would not be needed at all
and where needed, per jail would imho be fine since usually it is only a
single file (or a set of very similar ones) to be monitored. In very
rare (imho) cases where needed per file -- initiate two jails
pointing to different files and specifying different encodings (and hope
that we fix #57 by then). In
summary -- let's look for common scenarios first.

Yaroslav O. Halchenko
http://neuro.debian.net http://www.pymvpa.org http://www.fail2ban.org
Postdoctoral Fellow, Department of Psychological and Brain Sciences
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

Contributor

kwirk commented Feb 27, 2013

@yarikoptic I've added the ability to set log file encoding to the jail. Default is the system locale. If there any issues decoding a log line, it prints a warning to the fail2ban log. In python2 it maintains current behaviour of reverting back to str, but with pyhton3 it redoes the decode ignoring any errors (simply drops any parts of the line it cant decode).

Wasn't sure if python2 should suppress the warning message, as it just passes back to string; and also should python2 have the same behaviour of python3, requiring use of Unicode, ignoring any characters with decode errors?

Contributor

kwirk commented Mar 23, 2013

I've been running this on a server with python3.3 (UTF-8 environment) and all appears to working well. If anyone else could have a test that would be great 😄

Contributor

kwirk commented Mar 23, 2013

Hmm... I was hoping that Travis CI would run testcases with the new configuration including python3, but for some reason it hasn't...?

Owner

yarikoptic commented Mar 25, 2013

to me it looks like it did not run ANY tests... what if you enable testing on travis-ci.org for your particular branch where you keep this work? at least then it must test when you would push to your branch on github

Contributor

kwirk commented Mar 27, 2013

I believe it's because there are conflicts in the auto-merge that Travis CI isn't running. I'll look at Travis CI on my fork to see if that works. Equally, give me a shout if you'd like me to update this PR with a commit merging the latest master, and I can resolve all the conflicts to make it easier for you if/when you merge into 0.9.

Will the master branch also be merged into 0.9 at regular intervals?

Contributor

kwirk commented Mar 30, 2013

Travis CI config looks good now:
https://travis-ci.org/kwirk/fail2ban/builds/5928256

Turns out Travis CI doesn't support python 3.{0,1}, so I had to remove them.

Owner

yarikoptic commented Mar 30, 2013

as for 0.9 -- yeah -- I should have merged it more often.... did now (and pushed). If you feel that it is needed, just merge and send a PR ;)

@kwirk kwirk Merge branch 'master' into py3
Conflicts:
	.travis.yml
	server/datetemplate.py
	server/server.py
	testcases/filtertestcase.py
77aa523

Coverage remained the same when pulling 77aa523 on kwirk:py3 into 74e76e0 on fail2ban:master.

View Details

Contributor

kwirk commented Apr 9, 2013

I assumed you needed setuptools to execute 2to3 with setup.py, but found out there is a trick with pythons standard distutils1. Last commit implements this. For python2.5 compatibility and python3.x, I had to change print statements to sys.stdout.write, which works a treat 😄

Arguably fail2ban-2to3 is redundant but it's useful for testing currently, so I've left it for now.

@yarikoptic If you like, I can merge this with 0.9 branch, close this pull request and open a new one against 0.9 branch. I believe there are a fair few conflicts (I'll also wait for the fail2ban module pull request to be merged 0.9 before doing this).

Contributor

kwirk commented Apr 13, 2013

Closing and opening new pull request against 0.9 branch

kwirk closed this Apr 13, 2013

kwirk referenced this pull request Apr 13, 2013

Merged

Python3 support #171

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