ENH: opensearch source secrets refreshment suppport #3437

chenqi0805 · 2023-10-05T03:37:31Z

Description

This PR integrates secrets refreshment with OpenSearch Source clients.

Issues Resolved

Resolves #3436

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.

Signed-off-by: George Chen <[email protected]>

…e-secrets-refreshment-suppport Signed-off-by: George Chen <[email protected]>

Signed-off-by: George Chen <[email protected]>

…ppport Signed-off-by: George Chen <[email protected]>

dlvenable · 2023-10-06T01:59:08Z

...rg/opensearch/dataprepper/plugins/source/opensearch/worker/client/ElasticsearchAccessor.java


    private static final Logger LOG = LoggerFactory.getLogger(ElasticsearchAccessor.class);

    static final String PIT_RESOURCE_LIMIT_ERROR_TYPE = "rejected_execution_exception";
    static final String INDEX_NOT_FOUND_EXCEPTION = "index_not_found_exception";

-    private final ElasticsearchClient elasticsearchClient;
+    private final ElasticsearchClientRefresher elasticsearchClientRefresher;


This bit of code doesn't need to know or care about the refresh aspect. It just cares that it get this dynamically from a get() method. You can decouple this class from the refresh code by providing a Supplier<ElasticsearchClient> here.

Then, make ElasticsearchClientRefresher implement Supplier<ElasticsearchClient>.

@dlvenable Thanks for the comment. The tricky part for your strategy is OpenSearchIndexPartitionCreationSupplier where two client needs to use different models to make cat/_indices API call. I am looking into workaround but let me know any suggestions.

You can use the generic OpenSearchClientRefresher in OpenSearchIndexPartitionCreationSupplier. That does not prevent you from using a Supplier<> in this code. See my other recent comment for how to make use of the generics there.

This class (ElasticsearchAccessor) can use the Supplier<> to decouple this class from future changes.

There might be ways to further decouple OpenSearchIndexPartitionCreationSupplier, but we don't need to think through those right now.

dlvenable · 2023-10-06T02:00:20Z

...org/opensearch/dataprepper/plugins/source/opensearch/worker/client/ClusterClientFactory.java

+import org.opensearch.dataprepper.plugins.source.opensearch.OpenSearchSourceConfiguration;
+
+public interface ClusterClientFactory<Client> {
+    PluginComponentRefresher<Client, OpenSearchSourceConfiguration> getClientRefresher();


In line with my comment elsewhere, change this to:

Supplier<Client> getClientSupplier();

This probably would not work for OpenSearchIndexPartitionCreationSupplier. Changed into

ClientRefresher<Client> getClientRefresher();

You can make ClientRefresher implement Supplier<C>. Then you can use a simpler interface in the majority of the code.

Unfortunately there are some tight coupling in using APIs, e.g. OpenSearchIndexPartitionCreationSupplier needs the SearchAccessor to return more than Supplier. I improved it to be propagating PluginComponentRefresher<Client, OpenSearchConfiguration>

...ain/java/org/opensearch/dataprepper/plugins/source/opensearch/OpenSearchClientRefresher.java

dlvenable · 2023-10-06T02:04:42Z

...ain/java/org/opensearch/dataprepper/plugins/source/opensearch/OpenSearchClientRefresher.java

+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+public class OpenSearchClientRefresher


Use something like the following to bring together some of my comments.

public class <C> OpenSearchClientRefresher implements Supplier<C>, PluginComponentRefresher(C, OpenSearchSourceConfiguration);

dlvenable · 2023-10-06T02:06:00Z

...a/org/opensearch/dataprepper/plugins/source/opensearch/worker/client/OpenSearchAccessor.java


    private static final Logger LOG = LoggerFactory.getLogger(OpenSearchAccessor.class);

    static final String PIT_RESOURCE_LIMIT_ERROR_TYPE = "rejected_execution_exception";
    static final String INDEX_NOT_FOUND_EXCEPTION = "index_not_found_exception";
    static final String SCROLL_RESOURCE_LIMIT_EXCEPTION_MESSAGE = "Trying to create too many scroll contexts";

-    private final OpenSearchClient openSearchClient;
+    private final OpenSearchClientRefresher openSearchClientRefresher;


Similar to my comment above, make this: Supplier<OpenSearchClient> to decouple getting the client from the fact that it is refreshed.

...java/org/opensearch/dataprepper/plugins/source/opensearch/OpenSearchClientRefresherTest.java

Signed-off-by: George Chen <[email protected]>

graytaylor0 · 2023-10-06T15:36:52Z

...urce/src/main/java/org/opensearch/dataprepper/plugins/source/opensearch/ClientRefresher.java

+        readWriteLock.readLock().lock();
+        try {
+            return currentClient;
+        } finally {
+            readWriteLock.readLock().unlock();
+        }


Are these locks needed just in case we had multiple worker threads going at once?

Yes. when secrets are configured there are at least two threads accessing this: pipeline worker thread and secret polling thread

graytaylor0 · 2023-10-06T16:04:04Z

...rg/opensearch/dataprepper/plugins/source/opensearch/worker/client/ElasticsearchAccessor.java

@@ -149,7 +153,8 @@ public CreateScrollResponse createScroll(final CreateScrollRequest createScrollR
        SearchResponse<ObjectNode> searchResponse;

        try {
-            searchResponse = elasticsearchClient.search(SearchRequest.of(request -> request
+            searchResponse = elasticsearchClientRefresher.get()


Since this change is somewhat intrusive to the code, and we don't currently have integration tests for this source, just curious what testing you did to verify no regression?

Yes. I run a smoke test with pipeline:

pipeline_configurations: aws: secrets: secret1: secret_id: my-es-secret region: us-east-1 sts_role_arn: "arn:aws:iam::xxxx:role/ecs-test-role" secret2: secret_id: my-self-managed-os region: us-east-1 sts_role_arn: "arn:aws:iam::xxxx:role/ecs-test-role" opensearch-migration-pipeline: source: opensearch: hosts: [ "https://localhost:9200" ] username: "${{aws_secrets:secret2:username}}" password: "${{aws_secrets:secret2:password}}" processor: - date: from_time_received: true sink: - opensearch: hosts: [ "https://search-test-domain-r3dk3xkywa3vqqdzdqrddq3kyq.us-east-1.es.amazonaws.com" ] username: "${{aws_secrets:secret1:username}}" password: "${{aws_secrets:secret1:password}}" index: "test-migration"

Signed-off-by: George Chen <[email protected]>

kkondaka · 2023-10-07T02:37:01Z

...rce/src/main/java/org/opensearch/dataprepper/plugins/source/opensearch/OpenSearchSource.java

@@ -59,7 +63,8 @@ private void startProcess(final OpenSearchSourceConfiguration openSearchSourceCo

        final OpenSearchClientFactory openSearchClientFactory = OpenSearchClientFactory.create(awsCredentialsSupplier);
        final OpenSearchSourcePluginMetrics openSearchSourcePluginMetrics = OpenSearchSourcePluginMetrics.create(pluginMetrics);
-        final SearchAccessorStrategy searchAccessorStrategy = SearchAccessorStrategy.create(openSearchSourceConfiguration, openSearchClientFactory);
+        final SearchAccessorStrategy searchAccessorStrategy = SearchAccessorStrategy.create(
+                openSearchSourceConfiguration, openSearchClientFactory, pluginConfigObservable);


It looks like the code always assume that the pluginConfigObservable is always non-null, is that right? Is OpenSearchSource without key refresh not allowed anymore?

It is indeed always non-null. If there is no secret usage, the PluginConfigPublisher notifier would never be invoked even though the PluginConfigObservable will still be instantiated and loaded with PluginConfigObserver, i.e. PluginConfigObservable::update will never be invoked. At the client level, this means there is only read access to client although there is indeed a bit overhead from acquiring the read lock.

dlvenable

We can still improve this code, perhaps after the 2.5 release

ENH: opensearch source secrets refreshment suppport (#3437) Signed-off-by: George Chen <[email protected]> (cherry picked from commit 0d2e491)

ENH: opensearch source secrets refreshment suppport (#3437) Signed-off-by: George Chen <[email protected]>

chenqi0805 added 19 commits September 28, 2023 17:46

INIT: secrets refreshment infra

edeb6bf

Signed-off-by: George Chen <[email protected]>

MAINT: add interval and test validity

2661629

Signed-off-by: George Chen <[email protected]>

MAINT: some more refactoring

9ae948b

Signed-off-by: George Chen <[email protected]>

MAINT: delete unused classes

2e33eac

Signed-off-by: George Chen <[email protected]>

TST: AwsSecretsPluginConfigPublisherExtensionProviderTest

6421c50

Signed-off-by: George Chen <[email protected]>

MAINT: inject PluginConfigPublisher into PluginCreator

3256efb

Signed-off-by: George Chen <[email protected]>

MAINT: complete test cases for AwsSecretPluginIT

3ba133f

Signed-off-by: George Chen <[email protected]>

MAINT: test refresh secrets

9439296

Signed-off-by: George Chen <[email protected]>

MAINT: refactoring and documentation

d5fe312

Signed-off-by: George Chen <[email protected]>

STY: import

c8af912

Signed-off-by: George Chen <[email protected]>

MAINT: fix test cases

af0c206

Signed-off-by: George Chen <[email protected]>

MAINT: missing test case

ebdc448

Signed-off-by: George Chen <[email protected]>

ADD: client refresher

94513cf

Signed-off-by: George Chen <[email protected]>

REF: client -> clientRefresher

93b3794

Signed-off-by: George Chen <[email protected]>

MAINT: refactor method and add listener

06a3c27

Signed-off-by: George Chen <[email protected]>

MAINT: address minor comments

1ba290e

Signed-off-by: George Chen <[email protected]>

REF: PluginConfigurationObservableRegister

0bd7a0f

Signed-off-by: George Chen <[email protected]>

Merge branch 'enh/secret-refreshment-infra' into enh/opensearch-sourc…

ca74d46

…e-secrets-refreshment-suppport Signed-off-by: George Chen <[email protected]>

MAINT: pluginConfigObserver

a7d7fd0

Signed-off-by: George Chen <[email protected]>

chenqi0805 added the backport 2.5 label Oct 5, 2023

Merge branch 'main' into enh/opensearch-source-secrets-refreshment-su…

613770f

…ppport Signed-off-by: George Chen <[email protected]>

chenqi0805 marked this pull request as ready for review October 5, 2023 23:09

chenqi0805 requested review from engechas, graytaylor0, dinujoh, kkondaka, asifsmohammed, dlvenable and oeyh as code owners October 5, 2023 23:09

dlvenable requested changes Oct 6, 2023

View reviewed changes

chenqi0805 added 2 commits October 6, 2023 07:18

MNT: createObjectUnderTest

c8fc2fe

Signed-off-by: George Chen <[email protected]>

REF: share common interface

8f90a0e

Signed-off-by: George Chen <[email protected]>

chenqi0805 requested a review from dlvenable October 6, 2023 15:25

graytaylor0 reviewed Oct 6, 2023

View reviewed changes

MAINT: use interface

43972fb

Signed-off-by: George Chen <[email protected]>

chenqi0805 requested a review from graytaylor0 October 7, 2023 00:02

kkondaka reviewed Oct 7, 2023

View reviewed changes

chenqi0805 requested a review from kkondaka October 9, 2023 00:23

kkondaka approved these changes Oct 9, 2023

View reviewed changes

graytaylor0 approved these changes Oct 9, 2023

View reviewed changes

dlvenable approved these changes Oct 9, 2023

View reviewed changes

dlvenable merged commit 0d2e491 into opensearch-project:main Oct 9, 2023
24 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 9, 2023

[Backport 2.5] ENH: opensearch source secrets refreshment suppport #3462

Merged

chenqi0805 deleted the enh/opensearch-source-secrets-refreshment-suppport branch October 9, 2023 15:26

chenqi0805 pushed a commit that referenced this pull request Oct 9, 2023

ENH: opensearch source secrets refreshment suppport (#3437) (#3462)

73d8d19

ENH: opensearch source secrets refreshment suppport (#3437) Signed-off-by: George Chen <[email protected]>

chenqi0805 restored the enh/opensearch-source-secrets-refreshment-suppport branch November 13, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: opensearch source secrets refreshment suppport #3437

ENH: opensearch source secrets refreshment suppport #3437

chenqi0805 commented Oct 5, 2023

dlvenable Oct 6, 2023

chenqi0805 Oct 6, 2023

dlvenable Oct 6, 2023

dlvenable Oct 6, 2023

chenqi0805 Oct 6, 2023

dlvenable Oct 6, 2023

chenqi0805 Oct 7, 2023 •

edited

Loading

dlvenable Oct 6, 2023

dlvenable Oct 6, 2023

graytaylor0 Oct 6, 2023

chenqi0805 Oct 6, 2023

graytaylor0 Oct 6, 2023

chenqi0805 Oct 6, 2023

kkondaka Oct 7, 2023

chenqi0805 Oct 9, 2023 •

edited

Loading

dlvenable left a comment

ENH: opensearch source secrets refreshment suppport #3437

ENH: opensearch source secrets refreshment suppport #3437

Conversation

chenqi0805 commented Oct 5, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

chenqi0805 Oct 7, 2023 •

edited

Loading

chenqi0805 Oct 9, 2023 •

edited

Loading