Select require_alias for OS bulk inserts from ISM Policy #3560

KarstenSchnitter · 2023-10-30T10:07:21Z

Description

This change sets the require_alias property on OpenSearch bulk requests when writing to an aliased index. This avoids creation of an index without alias, when a previous existing alias and index was deleted. It increases robustness of DataPrepper's trace index against OS user interactions.

Issues Resolved

Resolves #3342

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

KarstenSchnitter · 2023-10-30T10:10:52Z

@dlvenable I am not sure, if I understood your suggestion in #3342 correctly. This change will set the require_alias bulk request property depending on an existing check within the IndexManager. Will this approach work? If so, I can add tests and Javadoc to the changeset. If not, please let me know, what needs changing.

dlvenable

@KarstenSchnitter , Thank you for putting this PR together. I left a couple comments. Additionally, did you test that this solves #3342? We'd also want some unit tests to ensure the behavior. And ideally an integration test in OpenSearchSinkIT, though that is not as important as the unit tests.

dlvenable · 2023-10-30T13:06:20Z

...nsearch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSink.java

    } else if (isEstimateBulkSizeUsingCompression) {
      LOG.warn("Estimate bulk request size using compression was enabled but request compression is disabled. " +
              "Estimating bulk request size without compression.");
-      bulkRequestSupplier = () -> new JavaClientAccumulatingUncompressedBulkRequest(new BulkRequest.Builder());
+      bulkRequestSupplier = () -> new JavaClientAccumulatingUncompressedBulkRequest(new BulkRequest.Builder().requireAlias(requireAlias));


Are there any situations in which the require_alias would vary from document to document? If so, we'd want this on the individual operations. I'm currently not setting any such situations, but I may be missing one.

According to https://opensearch.org/docs/2.11/api-reference/document-apis/bulk/#url-parameters, require_alias is a URL parameter of the bulk request. As such it cannot be changed for each document but only for the entire request.

If you wanted this setting to be depending on the document, you would have to group documents with different settings into different bulk requests. In any case, you would need a configuration on event level, that carries the alias configuration. That could either be the event or the record metadata. Both can easily be accessed in the OpenSearchSink#doOutput to create different bulk requests. This would be a much larger change, also requiring a decision on how to set the alias configuration for the event.

dlvenable · 2023-10-30T13:10:59Z

...rch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexManager.java

@@ -5,4 +5,5 @@
 public interface IndexManager{
 	void setupIndex() throws IOException;
 	String getIndexName(final String indexAlias) throws IOException;
+	boolean checkISMEnabled() throws IOException;


Now that we are looking to expose this, it may make more sense to create a new method such as isIndexAlias. It could internally hold a boolean and thus avoid unnecessary checks on the cluster. As it is now, by calling this method twice, Data Prepper will unnecessarily make two requests to the cluster to get cluster settings.

Also, I believe this method (checkISMEnabled) is actually checking only the cluster. I believe the actual decision is a combination of two things: 1) The index is configured as an alias, and 2) the cluster supports ISM. e.g currentIndexUsesAlias && clusterHasIsmEnabled.

I made some changes to the logic to meet your comment. I am not sure, whether ISM really needs to be enabled, though. Please have a look at the new version. It also includes unit tests for isIndexAlias().

dlvenable · 2023-11-01T13:44:49Z

Thanks @KarstenSchnitter . It looks good. Would you like to make it "Ready for review?"

This change requires an alias when writing to an aliased index. This avoids creation of an index without alias, when a previous existing alias and index was deleted. It increases robustness of DataPrepper's trace index against OS user interactions. Signed-off-by: Karsten Schnitter <[email protected]>

During OS sink initialization it is determined from OS, whether the configured index actually is an alias. If so, bulk request will require the index to always be an alias. The response is cached to avoid further requests. This also ensures, that the alias configuration is kept in the initially intended state. After all, this change is about to prevent an automatic index creation for a formerly existing alias. Signed-off-by: Karsten Schnitter <[email protected]>

Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter · 2023-11-02T07:43:43Z

I fixed the checkstyle issue with the imports and rebased the branch. I will investigate the integration test failures based on this state.

KarstenSchnitter · 2023-11-02T14:05:42Z

The integration test failures stem from an "Forbidden" response on the "isAlias" API. This is the underlying error message:

method [HEAD], host [https://localhost:9200], URI [/_alias/test-08350924-13ec-4ee6-be4f-ffac9d35e479], status line [HTTP/1.1 403 Forbidden]

I can reproduce this locally with the integration test, but not with curl. For this I use:

$ curl --head "https://localhost:9200/_alias/test-08350924-13ec-4ee6-be4f-ffac9d35e479" -u admin:admin -sk
HTTP/1.1 404 Not Found
content-type: application/json; charset=UTF-8
content-length: 82

This is the correct response for a non-existing alias. Note, that it is not a "FORBIDDEN" response.

@dlvenable, do you have an idea, how to fix this?

KarstenSchnitter · 2023-11-02T15:21:18Z

Note, that the failing test are the ones, that do not use the admin credentials but credentials with random uuids. I think the relevant code is in the OpenSearchSecurityAccessor:

data-prepper/data-prepper-plugins/opensearch/src/integrationTest/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSecurityAccessor.java

Lines 25 to 27 in 7869eb7

    
           void createBulkWritingRole(final String role, final String indexPattern) throws IOException { 
        
               createRole(role, indexPattern, "indices:data/write/index", "indices:data/write/bulk*"); 
        
           }

The configured roles do not allow the HEAD request on the alias. Would it be ok to change that, or do I need to find a way around the required roles?

KarstenSchnitter · 2023-11-02T15:36:46Z

I tried adding indices:admin/aliases/exists and indices:admin/aliases/get to the allowed actions above, but it did not help.

The specific user used in some tests of OpenSerachSinkIT needs get permissions on all aliases to test for their existence. Another bug with determining the alias name is fixed as well. As a final result, the DataPrepper OpenSearch user requires write access to the indices and now additionally read access to the aliases. This can be a change for self-managed indices. Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter · 2023-11-07T13:33:03Z

There are still integration tests failures for OpenDistro versions. This might be connected to the fact, that "require_alias" was only introduced in Elasticsearch 7.10. This might lead to errors in the bulk insert operations in earlier versions. Is there a way to detect the OS/ES version, so that the new behavior can be optional?

dlvenable · 2023-11-07T14:04:09Z

@KarstenSchnitter ,

That is a good find on the code failing prior to ES 7.10. We do need to continue supporting versions from 6.8 and beyond.

We do not currently have a mechanism in Data Prepper to check the version. That is something we probably should add. Would you be able to include it in this PR?

KarstenSchnitter · 2023-11-07T14:07:48Z

For me I could do a openSearchClient.info() and see, if I get an exception. This happens if there is no OpenSearch responding, if I understand correctly. I can then make isIndexAlias() return a Boolean and only add the parameter, if this is not null. This will of cause not be proper check on the OS/ES version. It would also exclude ES 7.10 from the feature.

The `require_alias` parameter for bulk requests was only introduced with ES 7.10. Since DataPrepper needs to be compatible down to 6.8, the parameter should not be used in earlier OD versions. This change will apply the parameter only when OpenSearch is detected as target. Signed-off-by: Karsten Schnitter <[email protected]>

For checking the OS version, the test user needs an additional permission. Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter · 2023-11-07T15:49:55Z

I implemented a detection of OpenSearch, so that the require_alias is set only for OpenSearch targets.
There are two unit test failures, that I cannot reproduce locally. They should be disconnected to the changes in this PR.

KarstenSchnitter · 2023-11-08T12:31:53Z

...main/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/AbstractIndexManager.java

+                // Try to get the OpenSearch version. This fails on older OpenDistro versions, that do not support
+                // `require_alias` as a bulk API parameter. All OpenSearch versions do, as this was introduced in
+                // ES 7.10.
+                openSearchClient.info();


@dlvenable from this object, you can get the OpenSearch version as a String. For this feature, it is enough to detect OpenSearch. For other use-cases, you might want to extend this detection, so that you can ask for a required minimum version. But this requires parsing the version string.

dlvenable

Nice. Thank you for your work on this. And adding the error checking should help for users who also do not have permissions to the info API presently.

KarstenSchnitter requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, asifsmohammed, dlvenable and oeyh as code owners October 30, 2023 10:07

KarstenSchnitter marked this pull request as draft October 30, 2023 10:07

dlvenable requested changes Oct 30, 2023

View reviewed changes

KarstenSchnitter requested a review from dlvenable October 31, 2023 13:32

KarstenSchnitter added 3 commits November 2, 2023 08:23

Fix imports for checkstyle

05fcfe3

Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter force-pushed the 3342 branch from b9e3c79 to 05fcfe3 Compare November 2, 2023 07:41

KarstenSchnitter added 2 commits November 7, 2023 15:41

Add Permission to get Cluster Info

63d6bbf

For checking the OS version, the test user needs an additional permission. Signed-off-by: Karsten Schnitter <[email protected]>

KarstenSchnitter marked this pull request as ready for review November 7, 2023 15:50

dlvenable mentioned this pull request Nov 7, 2023

[BUG] ISM index rollover actions fail because of missing setting for otel-v1-apm-span-* indices #3506

Closed

KarstenSchnitter commented Nov 8, 2023

View reviewed changes

dlvenable approved these changes Nov 21, 2023

View reviewed changes

dlvenable merged commit 915e84d into opensearch-project:main Nov 21, 2023
61 of 63 checks passed

KarstenSchnitter deleted the 3342 branch November 22, 2023 10:21

dlvenable mentioned this pull request Apr 3, 2024

Adds KarstenSchnitter as a maintainer to the Data Prepper project #4389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select require_alias for OS bulk inserts from ISM Policy #3560

Select require_alias for OS bulk inserts from ISM Policy #3560

KarstenSchnitter commented Oct 30, 2023 •

edited

Loading

KarstenSchnitter commented Oct 30, 2023

dlvenable left a comment

dlvenable Oct 30, 2023

KarstenSchnitter Oct 31, 2023

dlvenable Oct 30, 2023

KarstenSchnitter Oct 31, 2023

dlvenable commented Nov 1, 2023

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 2, 2023 •

edited

Loading

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 7, 2023

dlvenable commented Nov 7, 2023

KarstenSchnitter commented Nov 7, 2023

KarstenSchnitter commented Nov 7, 2023

KarstenSchnitter Nov 8, 2023

dlvenable left a comment

Select require_alias for OS bulk inserts from ISM Policy #3560

Select require_alias for OS bulk inserts from ISM Policy #3560

Conversation

KarstenSchnitter commented Oct 30, 2023 • edited Loading

Description

Issues Resolved

Check List

KarstenSchnitter commented Oct 30, 2023

dlvenable left a comment

Choose a reason for hiding this comment

dlvenable Oct 30, 2023

Choose a reason for hiding this comment

KarstenSchnitter Oct 31, 2023

Choose a reason for hiding this comment

dlvenable Oct 30, 2023

Choose a reason for hiding this comment

KarstenSchnitter Oct 31, 2023

Choose a reason for hiding this comment

dlvenable commented Nov 1, 2023

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 2, 2023 • edited Loading

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 2, 2023

KarstenSchnitter commented Nov 7, 2023

dlvenable commented Nov 7, 2023

KarstenSchnitter commented Nov 7, 2023

KarstenSchnitter commented Nov 7, 2023

KarstenSchnitter Nov 8, 2023

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

KarstenSchnitter commented Oct 30, 2023 •

edited

Loading

KarstenSchnitter commented Nov 2, 2023 •

edited

Loading