asn, fake expert: default paths, asn: add checks #2606
Conversation
sebix
force-pushed
the
sane-default-path
branch
2 times, most recently
from
bf38b18 to
905a62e
Compare
|
rebased to fix the tests |
| @@ -40,6 +41,8 @@ def init(self): | |||
| self.logger.error("Read 'bots/experts/asn_lookup/README' and " | |||
| "follow the procedure.") | |||
| self.stop() | |||
| if not Path(self.database).is_file: | |||
There was a problem hiding this comment.
An idea - maybe add also a check in the check method that would verify if the file is not a file or is not writable (also check from L121)? Not existing file should not cause a failure in check (it could be like this for a new bot), but could be an info
There was a problem hiding this comment.
The line is actually wrong. is_file is a method, not a member, therefore is_file always evaluates to true.
An idea - maybe add also a check in the check method that would verify if the file is not a file or is not writable (also check from L121)?
I updated the existence check and added a type check
Not existing file should not cause a failure in check (it could be like this for a new bot), but could be an info
I can follow you, but on the other hand, if the database file does not exist at the time of checking, the bot can also not operate with this current state and needs intervention by the admin to get into a working state.
There was a problem hiding this comment.
Ahh, good that you caught it!
I can follow you, but on the other hand, if the database file does not exist at the time of checking, the bot can also not operate with this current state and needs intervention by the admin to get into a working state.
This is also true, so a failure may be expected. I'd anyway keep it more as warning as an error as it could be handled automatically, and is a desired state on the first run - until manual or automated action take place - and the bot configuration itself might be correct.
is_file is a method, not a member, therefore is_file always evaluates to true
| try: | ||
| pyasn.pyasn(parameters['database']) | ||
| except Exception as exc: | ||
| return [["error", "Error reading database: %r." % exc]] | ||
| return [["error", f"Error reading database ({database_path!s}): %r." % exc]] |
There was a problem hiding this comment.
Why mixing f-string and %? Let's pack exc inside the f-string
| return [["error", "File given as parameter 'database' does not exist."]] | ||
| database_path = Path(parameters.get('database', '')) | ||
| if not database_path.exists(): | ||
| return [["error", f"File given as parameter 'database' ({database_path!s}) does not exist."]] |
There was a problem hiding this comment.
Light suggestion: make it as warning
Strong suggestion: "...not exists. You may need to trigger first downloading manually. See: https://docs.intelmq.org/latest/user/bots/#asn-lookup"
Otherwise we create an error message for eventually expected state, and people will think the bot is buggy
| @@ -66,12 +67,15 @@ def process(self): | |||
|
|
|||
| @staticmethod | |||
| def check(parameters): | |||
| if not os.path.exists(parameters.get('database', '')): | |||
| return [["error", "File given as parameter 'database' does not exist."]] | |||
| database_path = Path(parameters.get('database', '')) | |||
There was a problem hiding this comment.
What would you think about adding a one more check - how old the file is? And issue a warning if it's older than a week (I think it should be updated more often, right?)? Something like "The ASN database is older than 7 days and may be not up-to-date any more. Is automated database update working?"
There was a problem hiding this comment.
I recall we had once a problem with broken ASN update cronjob, and it resulted in wrong assigning and wrong addressing some events - so a check for that might help someone :)
asn, fake expert: use state path for database default
asn: Check for database file existence and writability fixes #2566