Repository navigation
forgerock: fix handling of query time ranges - #11240
Merged
Merged
Conversation
efd6
added
bugfix
The Forgerock API requires that the time range query satisfy three rules[1]. We were not satisfying the last rule, that the end of the range must not be more than 24 hours after the start of the range. The API does not require that the end of the range must not be in the future, so change the end time template to always render as the start of the range plus 24 hours. [1]https://backstage.forgerock.com/docs/idcloud/latest/tenants/audit-debug-logs.html#get-log-results-for-a-time-period
The current tests do not correctly set the pagination token in some cases, and in all cases the stream mock does not restrict queries that do not contain a token from re-initiating a pagination cycle. Fix both of these and apply event count assertions.
efd6
force-pushed
the
11220-forgerock
branch
from
September 25, 2024 20:43
e1ea1bf to
ddc231c
Compare
Contributor
🚀 Benchmarks reportPackage
|
| Data stream | Previous EPS | New EPS | Diff (%) | Result |
|---|---|---|---|---|
idm_core |
50000 | 29411.76 | -20588.24 (-41.18%) | 💔 |
am_authentication |
20000 | 15625 | -4375 (-21.88%) | 💔 |
idm_config |
25641.03 | 18181.82 | -7459.21 (-29.09%) | 💔 |
To see the full report comment with /test benchmark fullreport
|
💚 Build Succeeded
History
cc @efd6 |
efd6
marked this pull request as ready for review
September 25, 2024 21:18
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
andrewkroh
approved these changes
Sep 26, 2024
andrewkroh
left a comment
andrewkroh
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
It might be worthwhile to take what we know about the API, and codify that into a mock / emulator.
Contributor
|
Package forgerock - 1.18.1 containing this change is available at https://epr.elastic.co/search?package=forgerock |
efd6
added a commit
to efd6/integrations
that referenced
this pull request
Oct 15, 2024
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime.
efd6
added a commit
that referenced
this pull request
Oct 17, 2024
In #11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime. Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosen as a conservative value (the report claimed milliseconds of documents were lost).
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 4, 2025
The Forgerock API requires that the time range query satisfy three rules[1]. We were not satisfying the last rule, that the end of the range must not be more than 24 hours after the start of the range. The API does not require that the end of the range must not be in the future, so change the end time template to always render as the start of the range plus 24 hours. The current tests do not correctly set the pagination token in some cases, and in all cases the stream mock does not restrict queries that do not contain a token from re-initiating a pagination cycle. Fix both of these and apply event count assertions. [1]https://backstage.forgerock.com/docs/idcloud/latest/tenants/audit-debug-logs.html#get-log-results-for-a-time-period
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 4, 2025
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime. Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosen as a conservative value (the report claimed milliseconds of documents were lost).
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 5, 2025
The Forgerock API requires that the time range query satisfy three rules[1]. We were not satisfying the last rule, that the end of the range must not be more than 24 hours after the start of the range. The API does not require that the end of the range must not be in the future, so change the end time template to always render as the start of the range plus 24 hours. The current tests do not correctly set the pagination token in some cases, and in all cases the stream mock does not restrict queries that do not contain a token from re-initiating a pagination cycle. Fix both of these and apply event count assertions. [1]https://backstage.forgerock.com/docs/idcloud/latest/tenants/audit-debug-logs.html#get-log-results-for-a-time-period
harnish-crest-data
pushed a commit
to chavdaharnish/integrations
that referenced
this pull request
Feb 5, 2025
In elastic#11240 we attempted to fix preparation of endTime where it could be more than 24 hours after the startTime parameter, a case that is invalid for the API. To do this, we just always added 24 hours to the startTime on the basis that the API would crop at the present moment, and — notably — under the assumption that the returned document would be used to construct the next startTime. Clearly this is not the case, since we use the actual endTime parameter to construct the next startTime; the API does not provide an HTTPJSON-easy way to get the last timestamp. So set the endTime query parameter to the earliest of now and 24 hours after startTime. Queries with an end time of now may not get all documents up to now. Since we are using the query parameter to define the end of the range of documents that have definitively been retrieved, this means that we will never try to get these missed documents. We could us the maximum timestamp of the collected documents to define this range, but that less easy to do here, so just set the end of the time range to be some short period before now. Ten seconds is chosen as a conservative value (the report claimed milliseconds of documents were lost).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots