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
out_es: ensure integrity of already recorded logs #2026
Conversation
[Edit: Solved below] I need some guidance regarding the following issue: If {
"took":9,
"errors":true,
"items":[
{
"create":{
"_index":"myindex-test",
"_type":"flb_type",
"_id":"dOq6HAB5BvOnZv3fWiu0",
"status":409,
"error":{
"type":"version_conflict_engine_exception",
"reason":"[dOq6HAB5BvOnZv3fWiu0]: version conflict, document already exists (current version [1])",
"index_uuid":"MHbN2sP7T8mdTy55MHTlew",
"shard":"0",
"index":"myindex-test"
}
}
},
{
"create":{
"_index":"myindex-test",
"_type":"flb_type",
"_id":"dOq6HAB5BvOnZv3fWiu1",
"status":409,
"error":{
"type":"version_conflict_engine_exception",
"reason":"[dOq6HAB5BvOnZv3fWiu1]: version conflict, document already exists (current version [1])",
"index_uuid":"MHbN2sP7T8mdTy55MHTlew",
"shard":"0",
"index":"myindex-test"
}
}
}
]
} This in turn will cause the es plugin to issue a retry here. This would cause some retries in a row until fluent-bit gives up. Currently, this does not happen because the "index" op_type is interpreted as "replace the existing document". Do we need to handle this issue, or just let fluent-bit naturally give up retrying? It seems to be a waste of bandwidth, so I would like to handle this. Any suggestions? Should I modify |
I just updated this pull request. Now it changes the Please review and comment on your opinion about this approach. |
Example config:
Valgrind and debug log, testing the version conflict behavior:
Valgrind and debug log, testing ordinary behavior:
|
I will now force push exactly the same diff just to get the CI to run again. It timed out installing clang packages. |
Since ElasticSearch 7.5, the "create_doc" index privilege was introduced, which ensures a role can only add new logs, but never modify or delete previously recorded ones. However, the "index" op_type has the semantic of changing a document if it already exists with the same "_id". Therefore, any requests with the "index" op_type are denied for a role whose only privilege is "create_doc". We solve this by replacing all "index" operations by the "create" operation. However, this has the side effect of producing status 409 errors whenever a previously successful operation is retried and the Generate_ID option is turned on. Therefore, we change the "elasticsearch_error_check" function to ignore this kind of error. Signed-off-by: Paulo Matias <matias@ufscar.br>
Well, now the clang tests passed and the gcc tests timed out installing packages 😆. At least, now we know the change passes all tests 👍 |
review deferred after v1.5 release |
Hi @fujimotos |
@edsiper, @PettitWesley - we need your review |
@thotypous @AlekseyKalinin Sorry for delay. I'm fine with this patch. |
@edsiper, @PettitWesley - kindly asking you for making review |
@edsiper @PettitWesley We need your reviews. Thanx in advance! |
1 similar comment
@edsiper @PettitWesley We need your reviews. Thanx in advance! |
@edsiper @PettitWesley can you please approve this pull-request? Thank you! |
This would fall under @edsiper scope more than mine... I can take a look if needed... However, I think normally Fujimotos approval should be sufficient for a merge.... Either way, Eduardo is the only who manages releases and also merging PRs in general. I am the AWS maintainer for the AWS plugins primarily. |
Hi @edsiper and @PettitWesley, Is there any chance of work this out? Thank you
|
@edsiper With Fujimotos approval, can we merge this? |
@thotypous @marco-claudino Looks like there are conflicts in the PR that need to be fixed. |
I trust @fujimotos review. My only requirement is to rebase this PR on top of GIT master so we can get full CI coverage (recently we moved to Github actions) |
@edsiper @PettitWesley I noticed the discussion in this thread. So I decided to take I can confirm that it works. Using Elasticsearch v7.12.1, Fluent Bit can send records Attached is some screenshot from my testing. The test was done on the master HEAD |
I kicked GitHub CI to check this PR, and it seems all green now too. @edsiper @PettitWesley I'm going to step forward and merge this PR. I'm plannning to do a rebase merge for a clean commit history (instead of https://github.com/fluent/fluent-bit/commits/PR2026-for-merge I'll push this PR to master tonight (around 7:00 in EST) after going back to home. |
Merged via 7f0db9e. |
Hello @fujimotos, do you know when will this commit be released? We ran into the same issue as #2664 and the fix does not look available in the latest FluentBit version (1.7.6). Thanks a lot for your work! |
@paulden This patch is on track of included in the next major release (v1.8.0). Ask Eduardo about the exact release date of Fluent Bit v1.8. |
The
create_doc
index privilege was introduced in ElasticSearch 7.5 to ensure a role can only add new logs, but never modify or delete previously recorded ones.If the role has wider privileges than
create_doc
and the system running fluent-bit is compromised, one cannot ensure the integrity of logs previously stored in ElasticSearch, since the past logs could be modified by the adversary after the breach.However, fluent-bit currently does not support running with the
create_doc
privilege, since it uses theindex
op_type, which has the semantics of changing a document if it already exists with the same_id
. Therefore, any requests with theindex
op_type are denied for a role whose only privilege iscreate_doc
, even if they would create a new document, e.g.:We solve this by replacing all
index
operations by thecreate
operation, which is authorized for roles which only have thecreate_doc
privilege.Please note this change is backwards compatible even with very old versions of ElasticSearch.
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Documentation
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.