kibana
kibana copied to clipboard
elastic
Change all connectors to use the basic auth header instead of the `auth` property of `axios`
Summary
Fixes: https://github.com/elastic/kibana/issues/182391
Framework changes
- Utils to construct basic header from username and password:
fad6bde(#183162),b10d103(#183162) - Automatically convert
authto basic auth header in the sub-actions framework:ee27353(#183162) - Automatically convert
authto basic auth header in axios utils:94753a7(#183162)
Jira
Commit: c366163 (#183162)
All ServiceNow connectors
Commit: 4324d93 (#183162)
IBM Resilient
IBM Resilient already uses the basic auth headers. PR https://github.com/elastic/kibana/pull/180561 added this functionality. The connector was manually tested when reviewing the PR.
In 7d9edab (#183162) I updated the connector to use the new util function.
Webhook
Commit: 1a62c77 (#183162)
Cases webhook
Commit: 104f881 (#183162)
xMatters
Commit: ea7be2b (#183162)
Connectors that do not use the axios auth property
- D3Security
- Microsoft Teams
- OpenAI
- Opsgenie
- PagerDuty
- Sentinel One
- Slack
- Slack API
- Swimlane
- Tines
- Torq
Checklist
Delete any items that are not applicable to this PR.
- [x] Unit or functional tests were updated or added to match the most common scenarios
- [ ] Flaky Test Runner was used on any tests changed
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
| Risk | Probability | Severity | Mitigation/Notes |
|---|---|---|---|
| Connectors not working correctly | Low | High | Unit test and manual testing of all connectors affected |
For maintainers
- [x] This was checked for breaking API changes and was labeled appropriately
LGTM, noticed one test name that I think needs to be changed.
Also not sure whether this PR should include the changes for axios / follow-redirects. I figured that would be done separately, later, but I'm not sure. I'm curious if these versions were needed for the PR, or maybe you were just testing with them.
Thank you @pmuellr for the review. Indeed, the upgrade is not needed in this PR. I put it on this PR to avoid manually testing the connectors again when we upgrade the libraries. Any concerns with doing it in one PR?
Indeed, the upgrade is not needed in this PR. I put it on this PR to avoid manually testing the connectors again when we upgrade the libraries. Any concerns with doing it in one PR?
I'll leave that up to @jeramysoucy ... since Fleet also uses axios, I'm guessing we will want to do some coordination ...
Indeed, the upgrade is not needed in this PR. I put it on this PR to avoid manually testing the connectors again when we upgrade the libraries. Any concerns with doing it in one PR?
I'll leave that up to @jeramysoucy ... since Fleet also uses axios, I'm guessing we will want to do some coordination ...
It looks like the only axios use case in Fleet does not use the auth property.
https://github.com/elastic/kibana/blob/ed2e59fea95b39dcc853f948ce122544515370f0/x-pack/plugins/fleet/server/telemetry/sender.ts#L173
I do, however, see a use case in the CI stats performance metrics APM client. https://github.com/elastic/kibana/blob/ed2e59fea95b39dcc853f948ce122544515370f0/packages/kbn-ci-stats-performance-metrics/src/apm_client.ts#L53 But I don't think this would be subject to a redirect.
@cnasikas I don't necessarily want to hold up merging of this PR. So if you are complete with all of your testing, I can follow up with fleet and ops separately. Otherwise I am happy to ping them here if we can get a quick answer and merge everything at the same time. What do you think?
@cnasikas I don't necessarily want to hold up merging of this PR. So if you are complete with all of your testing, I can follow up with fleet and ops separately. Otherwise I am happy to ping them here if we can get a quick answer and merge everything at the same time. What do you think?
Ty @jeramysoucy! We are not ready with the testing yet. I would say to ping them if you don't mind and merge everything at the same time.
I do, however, see a use case in the CI stats performance metrics APM client. kibana/packages/kbn-ci-stats-performance-metrics/src/apm_client.ts But I don't think this would be subject to a redirect.
I confirmed with ops that we do not need to consider their use case:
"it's safe, it's requesting a kibana internal api and i verified it's not going to redirect it's also a development package forwarding metrics from an es cluster to another es cluster, run once a day, so if there's a remote auth behavior change the impact would be minimal"
It looks like the only axios use case in Fleet does not use the auth property.
Also, confirmed this with Fleet.
"we don't use Axios except for that single case in telemetry. We should be ok in regard to that change."
So, I think we are all set to apply the upgrade in this PR along with your changes.
All connectors were tested successfully. Details: https://github.com/elastic/kibana/issues/182391#issuecomment-2117157879
:green_heart: Build Succeeded
- Buildkite Build
- Commit: c9e8bceb7f719bc5d298a416f9e16337ad4c4e74
- Kibana Serverless Image:
docker.elastic.co/kibana-ci/kibana-serverless:pr-183162-c9e8bceb7f71
Metrics [docs]
Public APIs missing comments
Total count of every public API that lacks a comment. Target amount is 0. Run
node scripts/build_api_docs --plugin [yourplugin] --stats commentsfor more detailed information.
| id | before | after | diff |
|---|---|---|---|
actions |
295 | 297 | +2 |
Async chunks
Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app
| id | before | after | diff |
|---|---|---|---|
canvas |
1.1MB | 1.1MB | +330.0B |
Canvas Sharable Runtime
The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.
| id | before | after | diff |
|---|---|---|---|
module count |
- | 5405 | +5405 |
total size |
- | 8.8MB | +8.8MB |
History
- :green_heart: Build #210648 succeeded 851a16f0c99a7fa31e230a209bf3282a36bb6c22
- :green_heart: Build #209896 succeeded 382fe2634618d5ed2ba5e7aa9e50fbdc1d5a1f36
- :green_heart: Build #209753 succeeded c00248f81bd18c78117552fcd9ddcc84c6cc4c16
- :broken_heart: Build #209343 failed ea7be2bbee89b71c3855769fc480a013e4020732
To update your PR or re-run it, just comment with:
@elasticmachine merge upstream
cc @cnasikas
@cnasikas What is the risk/effort to backport this to 8.14 and 7.17? I assume 8.14 wouldn't be too bad, but 7.17 could be considerably more risky.
I do not see any risk for 8.14 and the effort should be low. For 7.17, the effort should be big because a lot has changed to all of these connectors. We may need to do it all over again. About the risk, I am not sure tbh. Let me give it a try for both and I will let you know.
💚 All backports created successfully
| Status | Branch | Result |
|---|---|---|
| ✅ | 8.14 |  |
Note: Successful backport PRs will be merged automatically after passing CI.
Questions ?
Please refer to the Backport tool documentation
@jeramysoucy PR for 8.14 https://github.com/elastic/kibana/pull/184091. I tried for 7.17 but the conflicts are too many. We would have to do it manually again. Is it something you would like us to do?
PR for 8.14 https://github.com/elastic/kibana/pull/184091. I tried for 7.17 but the conflicts are too many. We would have to do it manually again. Is it something you would like us to do?
Thanks @cnasikas! I think we're ok not backporting to 7.17 if the level of effort is high.
cc @legrego @azasypkin