-
Notifications
You must be signed in to change notification settings - Fork 30
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
Scanner architecture next steps #515
Comments
For the current asyncScan which is performed while an upload happens, we'll have to extend the current plans. The main problem is that we have to scan (or send to scan) partial data that is being written into a stream. Right now, this is done through a stream wrapper. Conceptually, we'll split the architecture between what the scanners are expected to provide to the outside, and how the scanners are expected to be implemented. The
Note that we might need to define what exceptions can be thrown from each of those methods. We'll likely have additional requirements to be implemented, such as:
The expected usage would be something like (just little touches here and there, but it should be the same as what is currently in place):
Additional metadata might be needed for the async scan. The implementations, however, won't require any change. The main trick is that both the In order to do so, first will need to implement a
(Code shown is based on what we currently have. It might be improved) The important point is that subclasses will only have 2-3 methods to implement, and those will be reused for both the sync and async scans. There might be some adjustments to be done with the overall plan. The |
Outside of the ICAP scanner, I'm not sure if we're officially supporting any scanner other than clamav, otherwise we can adjust the
ClamavScanner
class name to another one.For the
ClamavScanner
there are a couple of options:or
The main point is that the
scan
function will be final and it won't be modified.The abstract methods are expected to be implemented by the subclasses in different ways. For example, the
prepareScan
method might open a pipe to a file in case of theLocalScanner
, but theDaemonScanner
might open a socket instead. Buffering can also be implemented by the subclasses, if needed, in thesendData
method in order to send a bigger chunk of data.The method signature can also be adjusted according to necessities. The abstract class can provide some configuration to each implementation, likely from the same place, instead of letting each implementation fetch the configuration from different places.
For the
ICAPScanner
, the idea is the same:In this case, the implementations are expected to provide whatever the
ICAPClient
needs to perform the request in theprepareRequest
method and return it as anICAPRequestData
. Once theICAPClient
sends the request, the implementations have a chance to process the response.There might be some adjustments to be made (
ICAPClient
might need to handle a file stream somehow) but it should fit the current code.Responsibilities are quite clear in this case: the
ICAPScanner
will send the request, while the implementations will just prepare the data and process the result.Also note that, if we need to provide a generic ICAP scanner, it should have its own implementation at the same level of McAfeeWeb and Fortinet. Trying to implement it in the
ICAPScanner
will be confusing because the responsibilities won't be clear any longer.I think this should cover everything we have at the moment and it should allow some improvements without major code changes. So the question is how we can reach this proposed goal.
I'd propose the following steps:
In any case, I think it's critical to have code documentation explaining what are the expectations and who is responsible of what.
Originally posted by @jvillafanez in #512 (comment)
The text was updated successfully, but these errors were encountered: