[Reporting] Use server basepath when creating reporting jobs #72722
Conversation
poffdeluxe
added
v8.0.0
release_note:skip
|
@elasticmachine merge upstream |
|
@poffdeluxe I guess there's no automated tests impacted by this change. In light of that, would you mind describing what testing you did on the fix? @kobelb I don't know if you only code reviewed or actually did testing of this PR? This is a fix for a blocker bug on 7.9.0 and so while it's important to get a good fix in, it's also a significant context switch for QA team to stop testing on a 7.9.0 BC and spin up a PR build to test so any help in verifying would be appreciated. |
|
Hey @LeeDr, I did some testing myself and reviewed the code. Without this change, if you try to generate a PDF or a PNG report from the non-default Space, the report generation will fail. I verified that this fixed the issue. The gap in automated tests should be addressed, but I didn't think it should block us fixing the blocker. |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
* master: (34 commits) Adds Role Based Access-Control to the Alerting & Action plugins based on Kibana Feature Controls (elastic#67157) [Monitoring] Revert direct shipping code (elastic#72505) Use server basepath when creating reporting jobs (elastic#72722) Adding api test for transaction_groups /breakdown and /avg_duration_by_browser (elastic#72623) [Task Manager] Addresses flaky test introduced by buffered store (elastic#72815) [Observability] filter "hasData" api by processor event (elastic#72810) do not pass title as part of tsvb request (elastic#72619) [Lens] Legend config (elastic#70619) Stabilize closing toast (elastic#72097) stabilize failing test (elastic#72086) Stabilize filter bar test (elastic#72032) Unskip vislib tests (elastic#71452) [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689) fix preAuth/preRouting mocks (elastic#72663) [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788) [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584) [Security Solution] Fixes exception modal not loading content (elastic#72770) [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748) [Detections] Add validation for Threshold value field (elastic#72611) [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730) ...
Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
It appears that as a part of #66331, the move to using the
core.http.basePathfrom the plugin is causing a different or new basePath to be returned when creating a reporting job. Previous to this change, when creating jobs, the basePath for the job does not include the space. Following this change, the basePath began to include the space (if using a non-default space). When queuing reporting jobs, consumer apps already include the non-default space path in the job URLs so the end result was the space path being appended twice causing an error:(Note the repeating
/s/new-space)My proposed fix here is to just have the basePath in the job be set to the server base path (which is what prior versions appear to return anyway),
Perhaps in the future, the consuming apps should not attempt to add the non-default space to the path of requested URLs in the job and just let the reporting plugin handle that. But I'm personally not entirely sure of the implications of that so I'll leave that to Joel and Tim to figure out :)
Fixes #72475
Checklist
Delete any items that are not applicable to this PR.
For maintainers