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

[Console] Update autocomplete route configuration to include proxy headers #149498

Merged

Conversation

mibragimov
Copy link
Contributor

@mibragimov mibragimov commented Jan 25, 2023

Summary

This PR updates the autocomplete route configuration to include missing proxy headers such as x-forwarded-for,
x-forwarded-host, x-forwarded-port, and x-forwarded-proto to ensure that remote headers are forwarded to the Elasticsearch server. And also adds a check to ensure that the host header is set to the hostname if it is not already set. Users reported that the autocomplete indices were not showing up in the autocomplete dropdown in some cases due to the misconfiguration in autocomplete route. This PR should fix that issue.

This PR is only aimed to 8.6.x since in main we recently merged a PR which refactors this raw queries in favour of using the native es client which already takes into consideration proxy header and es ssl configurations.

Testing

To test this PR, verify that the correct headers are being sent to the Elasticsearch server when using the autocomplete route. You can use the console.log statement in the getEntity function to verify that the headers are being sent correctly.

Release note

This PR fixes an issue where the autocomplete indices were not showing up in the autocomplete dropdown in some cases due to the misconfiguration in autocomplete route.

Checklist

Delete any items that are not applicable to this PR.

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
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@mibragimov mibragimov requested a review from sabarasaba January 25, 2023 11:25
@mibragimov mibragimov self-assigned this Jan 25, 2023
@sabarasaba sabarasaba added Feature:Console Dev Tools Console Feature Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.6.2 labels Jan 25, 2023
}

const hasHostHeader = Object.keys(requestHeaders).some((key) => key.toLowerCase() === 'host');
if (!hasHostHeader) {
Copy link
Member

Choose a reason for hiding this comment

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

could we add some comments here on why this replacement needs to be made?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
osquery 108 113 +5
securitySolution 441 447 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 69 75 +6
osquery 109 115 +6
securitySolution 518 524 +6
total +20

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mibragimov

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Changes lgtm, tested locally and all headers seem to be correctly sent!

Screenshot 2023-01-26 at 09 15 26

@mibragimov mibragimov marked this pull request as ready for review January 26, 2023 08:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@mibragimov mibragimov merged commit 923a6ab into elastic:8.6 Jan 26, 2023
@alisonelizabeth
Copy link
Contributor

@mibragimov would you mind updating the PR description when you get a chance to explain why this is only needed for 8.6?

@sabarasaba
Copy link
Member

I've added a small section in the summary, but feel free to add more context if you feel like we need to @mibragimov

@paulmakeraiimi
Copy link

I have Kibana 8.6.2 and dropped a console.log into the getEntity function to check both the headers and response, and both look fine, yet autocomplete is still not working?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.6.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants