Skip to content

out_opensearch: expand logstash_prefix placeholders in the target_index_affinity search - #204

Open
lbarrosby wants to merge 3 commits into
fluent:mainfrom
lbarrosby:fix-affinity-prefix-placeholder
Open

lbarrosby wants to merge 3 commits into
fluent:mainfrom
lbarrosby:fix-affinity-prefix-placeholder

Conversation

@lbarrosby

Copy link
Copy Markdown
Contributor

With target_index_affinity true, get_affinity_target_indices searches for existing documents in "#{logstash_prefix}#{@logstash_prefix_separator}*". logstash_prefix there is the reader generated by config_param, so it holds the raw configuration value, placeholders included. With logstash_prefix ${tag} the search goes to POST /${tag}-*/_search and never finds anything. The update then goes to the index for the current date, which brings back the duplicate document this option is meant to avoid at index rotation.

process_message already writes to the index built from the prefix expanded for the chunk (expand_placeholders). This change uses extract_placeholders(@logstash_prefix, chunk) for the search too, so both look at the same indices. Configurations without placeholders search the same pattern as before.

Test added to test/plugin/test_out_opensearch.rb:

  • test_writes_to_affinity_target_index_with_logstash_prefix_placeholder: logstash_prefix ${tag} with tag test searches test-* and updates the document in the index that search returns. On main it fails with Unregistered request: POST http://localhost:9200/$%7Btag%7D-*/_search.

The existing affinity tests still pass. bundle exec rake test: 461 tests, 0 failures, 1 error. The error is test_out_opensearch_data_stream.rb test_time_placeholder, which fails the same way on main here: its stub uses the local date and the request the UTC date, and they differ near midnight in my time zone.

(check all that apply)

  • tests added
  • tests passing
  • README updated (if needed)
  • README Table of Contents updated (if needed)
  • History.md and version in gemspec are untouched
  • backward compatible

…ex_affinity search

get_affinity_target_indices built the search index from the
logstash_prefix reader, which returns the raw configuration value, so
with `logstash_prefix ${tag}` it searched `${tag}-*` and found nothing.
The update then went to the index of the current time instead of the
one that already holds the document. Fill in the placeholders from the
chunk, as process_message does for the index it writes to.

Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>

@Watson1978 Watson1978 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix. I checked it with OpenSearch 2.19.1. For an ordinary tag or record value, the search now finds the existing document and the update goes to its index.

One case needs a change before merge. Please see the inline comment.

Comment thread lib/fluent/plugin/out_opensearch.rb Resolved
With the logstash_prefix placeholders expanded in the affinity search,
a tag or chunk key value with "," or "*" widened the search beyond
the prefix, so an update could go to another tenant's index, and a
value that is not a valid index name made the search, and the chunk,
fail. Skip the search when a value used in logstash_prefix has such a
character, or makes the expression start with _, - or +, so the record
goes where it went before. "*" or "," written in logstash_prefix
itself is kept.

Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>

@Watson1978 Watson1978 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update. The three cases from my previous comment now keep the main result. I checked them with OpenSearch 2.19.1.

The previous comment is not fully resolved yet, though: #204 (comment). Some values still widen the search or make it fail. Each case below uses the earlier configuration with only the listed changes. So target_index_key destination is set, and the record has "destination": "logs-safe-2021.04.29". main writes the record to logs-safe-2021.04.29 in all of them.

  1. logstash_prefix ${tenant} and logstash_prefix_separator _, and the record has no tenant or an empty one. The search goes to _* and returns 400 (must not start with '_'), so the chunk fails. logstash_prefix ${tag[1]} with the tag app fails the same way.
  2. The same as 1, but with logstash_prefix_separator "". The search goes to *, and the update goes to the document in logs-blue-2021.04.28.
  3. logstash_prefix logs-${tag}, the tag is literally ${tenant}, and tenant is *,private. Fluentd fills in ${tenant} after it puts in the tag. So the search goes to logs-*,private-* and updates logs-blue-2021.04.28. The check does not look at tenant, because ${tenant} is not in the configured prefix.

Cases 1 and 2 happen because the check looks at the prefix without logstash_prefix_separator, and an empty result passes it. Case 3 happens because the check collects the values, and a value can also come in through another placeholder.

One way to cover all three is to compare the prefix before and after the expansion. Compare @logstash_prefix plus @logstash_prefix_separator with the expanded prefix plus the separator. Leave out the trailing * that the search adds.

  • Skip the search when any character of UNSAFE_INDEX_EXPRESSION_CHARACTERS appears more times after the expansion than before. Count each character separately. A strftime format like %#p drops its #, so a total count can hide an added *.
  • Also skip it when a comma-separated target becomes empty, or starts with _, - or +, while the configured target does not. "".split(",", -1) returns [], so an empty result needs its own check.

I tried this on top of 36ad95e. Your tests pass, and the cases above give the main result.

Could you add tests for these three cases?

Checking the values used by the configured placeholders missed an empty
value, the separator, and a value that comes in through another
placeholder. Compare the configured and the expanded prefix with the
separator instead: skip the affinity search when an unsafe character is
added, or when a target becomes empty or starts with _, - or +.

Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>
@lbarrosby

Copy link
Copy Markdown
Contributor Author

Thanks for the cases and the approach. Pushed 9f19c8b, which compares the prefix before and after the expansion as you suggested:

  • @logstash_prefix plus @logstash_prefix_separator is compared with the expanded prefix plus the separator, without the trailing *.
  • The search is skipped when any character of UNSAFE_INDEX_EXPRESSION_CHARACTERS appears more times after the expansion, counted per character.
  • It is also skipped when a comma-separated target becomes empty, or starts with _, - or +, while the configured one does not. An empty result is treated as one empty target, since "".split(",", -1) is [].
  • The check of the values used by the configured placeholders is gone.

New data-driven test test_skips_affinity_search_when_the_expanded_prefix_is_unsafe, with your configuration and destination logs-safe-2021.04.29:

  1. ${tenant} with separator _, and no tenant, an empty tenant, or ${tag[1]} with the tag app
  2. ${tenant} with separator "" and no tenant
  3. logs-${tag} with the tag ${tenant} and tenant *,private

Each asserts no _search request and a single update to logs-safe-2021.04.29. On 36ad95e they search _*, * and logs-*,private-*. The earlier tests, including logs*-${tenant}, still pass. TZ=UTC bundle exec rake test: 470 tests, 0 failures, 0 errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants