serilog-contrib / serilog-sinks-elasticsearch

A Serilog sink that writes events to Elasticsearch
Apache License 2.0
434 stars 197 forks source link

DetectElasticsearchVersion requires monitor role #270

Open michelealda opened 5 years ago

michelealda commented 5 years ago

Does this issue relate to a new feature or an existing bug?

What version of Serilog.Sinks.Elasticsearch is affected? Please list the related NuGet package. 8.0

What is the target framework and operating system? See target frameworks & net standard matrix.

Please describe the current behavior? The DetectElasticsearchVersion property of the latest version will trigger a GET _cat/nodes?h=v to the elasticsearch url to pick up the version of the cluster.

Using basic authentication of the users, the above call is authorized only if the user has the monitor role assigned, this might not be the case for every ELK environment.

Wondering if a simpler call to the elasticsearch root uri will be enough to detect the version as it will return something like

{
"name": "...",
"cluster_name": "...",
"cluster_uuid": "...",
"version": {
   "number": "7.2.0",
   [other stuff]
}
}

If the current behavior is a bug, please provide the steps to reproduce the issue and if possible a minimal demo of the problem To reproduce, create a user in Elasticsearch with the bare minimum rights to write to an existing index

mivano commented 5 years ago

Interesting point. @Mpdreamz implemented the current detection, maybe he has an idea if this is possible?

nenadvicentic commented 1 year ago

This is a good idea. However, it would require additional effort to find a way to mock root GET response coming from InMemoryConnection.

The request itself could be updated to this:

var response = _client.DoRequest<DynamicResponse>(HttpMethod.GET, "/");
if (!response.Success) return null;

var discoveredVersion = response.Dictionary["version"]["number"];

if (!discoveredVersion.HasValue)
    return null;

return new Version(discoveredVersion);

But a lot of unit-tests would get broken.

Currently, InMemoryConnection has hard-coded mocked response to the root GET request, which always returns hard-coded version number. We would need to be able to mock different version numbers for unit-tests to work correctly. At the moment it was easy to do in ConnectionStub , beacuse it was separate _cat/nodes?h=v GET request.

The reason for this hard-coded response is the "pre-flight" request that was introduced in Elasticsearch.NET version 7.16, to check if product at a configured endpoint is indeed "Elasticsearch" and if version of the product matches supported version (6, 7, or 8).

Sadly, this pre-flight request is internal to dll (class RootResponse here) and the product version parsed during this request not exposed to the consumer.