Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SIEM][Detection Engine][Lists] Adds the ability to change the timeout limits from 10 seconds for loads for imports #73103
[SIEM][Detection Engine][Lists] Adds the ability to change the timeout limits from 10 seconds for loads for imports #73103
Changes from all commits
4d84ca1
f709e49
8e84e6a
f091d7d
0682b66
42dfd27
08aa212
881a100
2862e47
8c930d8
4e929bb
1817203
59a3234
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a test for
408 Request timeout
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 408 Request timeout only happens when it is a PUT/POST with a form/payload:
https://hapi.dev/api/?v=19.2.0#-routeoptionspayloadtimeout
And then you have to simulate a connection that starts but then stalls or does it so slowly that the backend decides to then disconnect you because you're taking too long. I looked around at
supertest
yesterday for a quite a while hoping it had an easy way to explicitly test the condition but it does not look like there is a way for me to be able to start an upload and then stall it.For the other endpoints such as GET, DELETE which do not take a payload when the socket does a disconnect Hapi server will do a hangup on them on a timeout and not give back a 408 Request timeout. It doesn't advertise that is its behavior but that is the behavior as the tests are showing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can set a different switch which is the timeout.server here:
https://hapi.dev/api/?v=19.2.0#-routeoptionstimeoutserver
And then increase the socket timeout to be slightly higher than that and instead of a socket hangup we would get a:
For GET, DELETE endpoints and a 408 for when its payload based with PUT/POST, but it seemed like the socket hangup was more preferred from the example you sent me in the tests. Just let me know if you want me to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay if you merge it as is and create an issue for the platform team to add such a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticket I created for this as requested:
#73557
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we test here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests and the others around it test that if the timeout is set it will work (sanity test). I didn't think adding an incredibly long lived test to ensure they can exceed the 2 minutes would be ok so I instead opted for a simple sanity check.
But really exceeding the full 2 minutes but not exceeding 5 minutes would be a typical positive unit test that it is possible to exceed the default 2 minutes and operates as we would want it. However, during integration tests I thought I probably shouldn't exceed 2 minutes for the test so instead I just opted for a test that shows when it is set on a regular endpoint call it will operate normally and not cause any issues such as unexpected timeouts.