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

Elasticsearch: Fix URL creation and allowlist for /_mapping requests #80970

Merged
merged 6 commits into from
Jan 23, 2024

Conversation

svennergr
Copy link
Contributor

What is this feature?

When Elasticsearch datasources were configured without an indexname one of the requested URLs ended in //_mapping. In our cloud infrastructured that was normalized to _mapping, which ended up in a 403 thrown by the datasource backend.

This PR fixes two things:

  1. URL generation in the frontend will now check for trailing and leading slashes, and for empty index names. Thus a request to //_mapping should not happen again.
  2. In the datasource backend we now also allow requests that don't use an index name, thus we will allow requests to _mapping.

@svennergr svennergr added type/bug add to changelog backport v10.3.x Mark PR for automatic backport to v10.3.x labels Jan 22, 2024
@svennergr svennergr requested a review from a team as a code owner January 22, 2024 11:35
Copy link
Contributor

This PR must be merged before a backport PR will be created.

@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.4.x milestone Jan 22, 2024
Copy link
Contributor

This PR must be merged before a backport PR will be created.

pkg/tsdb/elasticsearch/elasticsearch.go Show resolved Hide resolved
@@ -205,7 +205,18 @@ export class ElasticDatasource
indexList = [this.indexPattern.getIndexForToday()];
}

const indexUrlList = indexList.map((index) => index + url);
// make sure `url` does not start with a slash
url = url.replace(/^\//, '');
Copy link
Member

Choose a reason for hiding this comment

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

requestAllIndices is private function used only in return this.requestAllIndices('/_mapping', range). Can't we just remove / from /_mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, we can, but shouldn't we then also hardcode requestAllIndices to not accept an url parameter? Because what if the next person/developer wants to use requestAllIndices at a different place.

I removed it in 3367a8c but would still leave the detection and "normalization" as part of requestAllIndices.

Copy link
Member

Choose a reason for hiding this comment

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

but shouldn't we then also hardcode requestAllIndices to not accept an url parameter?

I think that it make sense then. And it is more clear and understandable.

Copy link
Contributor Author

@svennergr svennergr Jan 22, 2024

Choose a reason for hiding this comment

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

Updated in d464811

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM.

@svennergr svennergr merged commit 3d03383 into main Jan 23, 2024
13 checks passed
@svennergr svennergr deleted the svennergr/fix-elasticsearch-mapping-bug branch January 23, 2024 11:41
grafana-delivery-bot bot pushed a commit that referenced this pull request Jan 23, 2024
#80970)

* Elasticsearch: Fix URL creation for mapping requests

* remove leading slash by default

* add comment for es route

* hardcode `_mapping`

* update doc

(cherry picked from commit 3d03383)
svennergr added a commit that referenced this pull request Jan 23, 2024
…g` requests (#81057)

Elasticsearch: Fix URL creation and allowlist for `/_mapping` requests (#80970)

* Elasticsearch: Fix URL creation for mapping requests

* remove leading slash by default

* add comment for es route

* hardcode `_mapping`

* update doc

(cherry picked from commit 3d03383)

Co-authored-by: Sven Grossmann <sven.grossmann@grafana.com>
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
#80970)

* Elasticsearch: Fix URL creation for mapping requests

* remove leading slash by default

* add comment for es route

* hardcode `_mapping`

* update doc
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
grafana-delivery-bot bot pushed a commit that referenced this pull request May 13, 2024
#80970)

* Elasticsearch: Fix URL creation for mapping requests

* remove leading slash by default

* add comment for es route

* hardcode `_mapping`

* update doc

(cherry picked from commit 3d03383)
@svennergr
Copy link
Contributor Author

@jcalisto Out of curiosity, what's the reason for this backport?

jcalisto pushed a commit that referenced this pull request May 14, 2024
…g` requests (#87711)

Elasticsearch: Fix URL creation and allowlist for `/_mapping` requests (#80970)

* Elasticsearch: Fix URL creation for mapping requests

* remove leading slash by default

* add comment for es route

* hardcode `_mapping`

* update doc

(cherry picked from commit 3d03383)

Co-authored-by: Sven Grossmann <sven.grossmann@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants