Refactor greenlet handling and fix related bugs #493
Conversation
srenfo
changed the title
|
Let me know if you want me to remove parts of this or split it up in a certain way. |
|
Thanks for your contribution 🙂 Would need some time to review this. I hope that's okay. |
|
If this proves too unwieldy I can split it into multiple PRs that build on each other. |
| from gevent.server import StreamServer | ||
| import gevent | ||
| from conpot.helpers import chr_py3 | ||
| import conpot.core as conpot_core | ||
| from conpot.protocols.kamstrup.meter_protocol import request_parser | ||
| from conpot.protocols.kamstrup.meter_protocol.command_responder import CommandResponder | ||
| from conpot.core.protocol_wrapper import conpot_protocol | ||
| # import logging as logger |
There was a problem hiding this comment.
You can leave these comments here. It's super useful when debugging 🙂
There was a problem hiding this comment.
I really dislike commented out code. Commented out code tends to rot. It's clutter. It tends to break over time because it's not tested. If you notice it you don't know whether to maintain it or not: is it obsolete? Was it left there intentionally? Or unintentionally?
Thus I tend to remove commented out code. With a merge request of this size that was likely a bad idea because the change actually just adds clutter (hah) for the reviewer.
Do you really want to keep such code though?
There was a problem hiding this comment.
Note that in this specific case, there was even a line below (former line 32) that was incompatible or at least redundant with the commented out code.
There was a problem hiding this comment.
@xandfury how about adding this code into a debug function instead?
There was a problem hiding this comment.
There was a problem hiding this comment.
It just so happens that I can illustrate my point. 😎 (This was not intentional but here we are.)
You may have noticed two rogue logging lines on every Conpot startup:
$ conpot -f --template default
WARNING:scapy.loading:Cannot read wireshark manuf database
WARNING:scapy.runtime:No route found for IPv6 destination :: (no default route?)
_
___ ___ ___ ___ ___| |_
| _| . | | . | . | _|
|___|___|_|_| _|___|_|
|_|
Version 0.6.0
MushMush Foundation
2020-05-26 15:20:47,753 --force option specified. Using testing configuration
...
Those are (in part) caused by this line at the top of ipmi_server.py:
logger.basicConfig(stream=sys.stdout, level=logger.DEBUG)which I heavily suspect was left in there as a debugging aid, maybe accidentally. (The second reason for those two lines is that BACpypes 0.17.0 adds a root-level logger in its debugging.py. You need both for those lines to appear.)
The offending line was removed in the PR, though I admit this was an unintentional change when I gobbled up all the debugging code.
Re: "debugging function" - that is the intended purpose of start_protocol.py! This seems to be a common misunderstanding throughout the discussion here. I'll try to address it in a separate comment below.
To move forward with the PR I will need directions though: do you want me to restore this code, yes or no?
| @@ -110,16 +107,3 @@ def start(self, host, port): | |||
|
|
|||
| def stop(self): | |||
| self.server.stop() | |||
|
|
|||
There was a problem hiding this comment.
This doesn't impact the implementation. Some devs use it to debug the protocol locally. Super helpful. I think we can leave it here.
There was a problem hiding this comment.
These bits were refactored into start_protocol.py in 3689964. The
next commit c4dad04 then uses that groundwork to test starting and stopping all protocols individually.
Some other changes in this MR would have made it necessary to rewrite these sections. I tried to do that in only one location that was used by both tests and start_protocol.py.
I can reinstate the separate sections (albeit rewritten) in lieu of start_protocol.py if you want.
There was a problem hiding this comment.
as in keeping the __main__ section and using start_protocol for initiating debugging?
There was a problem hiding this comment.
Just to add here: with __main__ you can just go the file itself and starting the protocol with something like python3 <protcol_name>.py Again it's just useful for debugging the protocol locally.
It is not used anywhere for running the protocols which is done conpot executable. So totally in favour of keeping the __main__ sections.
There was a problem hiding this comment.
start_protocol.py was added for exactly this purpose. It accepts a single command line parameter which identifies the protocol, i. e. start_protocol.py <protocol_name>.
Thus, instead of
python3 kamstrup_server.pyyou do
python3 start_protocol.py kamstrup_meterAdvantages are that the startup code is consistently the same, i.e. the protocols are run the same way the tests and (more or less) bin/conpot run them, and that the code is in one place rather than scattered throughout.
Disadvantages are that the code is not as close to the protocols as before and that if you want to play around with the startup logic of one protocol it affects all of them. (The latter may be an advantage, given that you want consistency and all.)
I can remove start_protocols.py and restore the __main__ sections or keep it as is.
bin/conpot
Outdated
| @@ -16,7 +16,7 @@ | |||
| # Foundation, Inc., | |||
| # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||
|
|
|||
| import gevent.monkey; gevent.monkey.patch_all() | |||
| from gevent import monkey; monkey.patch_all() | |||
There was a problem hiding this comment.
How is this change different from what it was previously? 🙂
There was a problem hiding this comment.
This way all monkey patching lines consistently use the same syntax. Before they differed from file to file.
Really it stems from my IDE preferring from foo import bar over import foo.bar.
It's a non-functional change so I can undo it if you want.
There was a problem hiding this comment.
No strong opinion on this one. If your IDE otherwise complains, keep it :)
There was a problem hiding this comment.
Awesome work, much appreciated! Love the refactoring and centralization of all gevent code.
I think if you find an elegant solution for the debugging/testing questions from @xandfury , this is almost ready to go.
How did you end up working on Conpot?
bin/conpot
Outdated
| @@ -16,7 +16,7 @@ | |||
| # Foundation, Inc., | |||
| # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||
|
|
|||
| import gevent.monkey; gevent.monkey.patch_all() | |||
| from gevent import monkey; monkey.patch_all() | |||
There was a problem hiding this comment.
No strong opinion on this one. If your IDE otherwise complains, keep it :)
| from gevent.server import StreamServer | ||
| import gevent | ||
| from conpot.helpers import chr_py3 | ||
| import conpot.core as conpot_core | ||
| from conpot.protocols.kamstrup.meter_protocol import request_parser | ||
| from conpot.protocols.kamstrup.meter_protocol.command_responder import CommandResponder | ||
| from conpot.core.protocol_wrapper import conpot_protocol | ||
| # import logging as logger |
There was a problem hiding this comment.
@xandfury how about adding this code into a debug function instead?
| @@ -110,16 +107,3 @@ def start(self, host, port): | |||
|
|
|||
| def stop(self): | |||
| self.server.stop() | |||
|
|
|||
There was a problem hiding this comment.
as in keeping the __main__ section and using start_protocol for initiating debugging?
|
Good job @srenfo 🎉 I'll be happy to merge your changes. I can live without logger comments. But I strongly believe that we should keep the |
|
Sorry, I'm rather busy at work right now. It may take another week or so but I'll come back to this. Ping me otherwise. |
|
I appreciate the kind words @glaslos and @xandfury, thank you!
Primarily, it's convenient if you want to quickly spin up a server of an ICS protocol. For example, we're currently doing research work on/with IEC104 and sometimes you just want a random IEC104 server to talk to. 😄 For a previous research project we actually had it deployed as a honeypot, though not much came from that unfortunately. So after using it for some time it's only appropriate to give back here and there. We have contributed a few small PRs before, we just haven't tagged ourselves as a group. |
|
@xandfury any remaining concerns? |
|
Hey @srenfo. The changes look good to me. Can you fix the conflicts so that we can merge? |
|
@srenfo you probably want to run black over your branch before merging. Should be easier than fixing the merge conflicts. |
Are you sure this is a good idea with Alternatively you could revert the broken commits (or actually remove them from |
|
@srenfo if you give me permissions to change the PR I can help with resolving the conflicts |
The conflicts should be resolved. There's a chance Travis will complain since |
|
hm, maybe merge master to get the git action to run? |
|
Trying again... it's rebased on the current What's weird is that I've used this workflow before and it had always triggered the build (AFAICT). I've also (re-)enabled "Allow edits and access to secrets by maintainers", though I believe it was the same before. Do you need me to do anything else? |
|
I've even added an empty commit to try to force the build to trigger, but apparently it's just slow. As of this writing, the CI build is queued: https://travis-ci.org/github/mushorg/conpot/pull_requests You can see that the black check succeeded in my fork (i. e. the source of this PR): https://github.com/srenfo/conpot/runs/1343712255 |
Pull Request Test Coverage Report for Build 1236
💛 - Coveralls |
|
Maybe the new pricing policies are having an effect? https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing |
|
Alright, let's get this merged. Travis seems a bit confused due to the force-push. |
This PR refactors all greenlet handling, especially in test code, and fixes all bugs that were exposed in the process.
I'm sorry that this is such a big PR but it's next to impossible to chop it into independent pieces. It may be best to view this commit by commit or as a side-by-side diff rather than a unified diff.
Highlights:
__main__s in the protocol servers into a single script inbin/start_protocol.py. Thosemains would otherwise have been a major pain to keep up to date with these changes. The script should be able to start all protocol individually. It keeps the previous port choices where there were any, though that may be more confusing than necessary.bin/conpotnow waits for greenlets to shut down.There are new tests for the refactored utility code.
Bug fixes:
conpot_protocoltried to override some class properties but failedstart()method, contrary to all other protocolsTests for these bugs are included. (All those tests would fail on the current
master.)Caveats:
greenlets.py. The assumption that the protocol'sstart()is fairly simple does not hold true for the HTTP server, so it is special cased there. That said, the new tests would/should catch such cases (they did for HTTP). It's hard to be certain.bin/conpotstill started up with the default template. No other functional testing was done. That's because the changes are focussed on test code.