Skip to content
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

[API_PARSER][PROOFPOINT TRAP] Add timeout choice #487

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

DBischoff0
Copy link
Contributor

No description provided.

@DBischoff0 DBischoff0 requested a review from 0xanalog January 28, 2025 10:37
@DBischoff0 DBischoff0 self-assigned this Jan 28, 2025
@DBischoff0 DBischoff0 changed the base branch from master to dev January 28, 2025 10:38
@@ -56,6 +56,7 @@ def __init__(self, data):
self.proofpoint_trap_host = "https://" + self.proofpoint_trap_host

self.proofpoint_trap_apikey = data["proofpoint_trap_apikey"]
self.proofpoint_trap_timeout = int(data.get("proofpoint_trap_timeout", 20))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small detail, here the .get() is confusional, as your Django parameter had a already a default value, in order to check that GUI parameter works as expected, please don't default this value here

@@ -94,7 +95,7 @@ def __execute_query(self, url, query=None, timeout=20):
logger.info(f"[{__parser__}]:execute: API Rate limit exceeded, waiting 10 seconds...",
extra={'frontend': str(self.frontend)})
time.sleep(10)
return self.__execute_query(url, query, timeout)
return self.__execute_query(url, query)
Copy link
Contributor

@0xanalog 0xanalog Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmmh this recursive call makes me anxious, we don't need it at all, could you please do it iteratively using a while for example ?
Please ensure to execute a self.update_lock() whenever you are entering the loop, to avoid multiple simultaneous executions, moreover you needs to set the following condition not self.event_stop.is_set() in your loop to avoid infinite loop blocking service shutdown/updates

@DBischoff0 DBischoff0 force-pushed the add_timeout_choice_for_proofpoint_trap branch from 89a1f18 to b365277 Compare January 29, 2025 09:21
@DBischoff0 DBischoff0 requested a review from frikilax February 4, 2025 15:03
@DBischoff0 DBischoff0 force-pushed the add_timeout_choice_for_proofpoint_trap branch from 406ca9d to 65e54ab Compare February 4, 2025 15:14
@@ -1172,6 +1172,11 @@ class Frontend(models.Model):
help_text = _("ProofPoint TRAP API key"),
default = "",
)
proofpoint_trap_timeout = models.PositiveIntegerField(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this parameter 'generic' for every API collector: this seems like a change that can be implemented for all of them (you don't necessarily need to implement it in very collector yet, though)

@DBischoff0 DBischoff0 force-pushed the add_timeout_choice_for_proofpoint_trap branch from 17f7fb0 to 3b9a04a Compare February 4, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants