Adding replication (CCR) plugin interface and classes to common-utils

[aggarwalShivani]

Description

Background:

• This change is required for a new feature being added in ism plugin - unfollow-action #726 - integrating ccr and ism plugins.
• For this feature, ism plugin needs to invoke stop-replication action from ccr. As both ccr and ism need to invoke some common code, the common code can be moved to common-utils, as done previously for other plugins too. For ex. notifications plugin
• As sharing of libraries also leads to a type-cast and class-loading issue - previously seen with notification plugin - the request object needs to be of a higher-level class from opensearch-core like ActionRequest and later be recreated into required type i.e. StopIndexReplicationRequest.

Proposed Solution:

1. Common-utils: Move common code of stop-replication from ccr project to common-utils.
2. CCR: Modify ccr plugin to consume classes from common-utils. Also, create a new TransportAction that transforms the request into required type and invokes TransportStopIndexReplicationAction.
3. ISM: Add new action in ism
   (FYI - This is an alternative approach for this feature. I had also raised a draft PR with a different approach, but we didnt go ahead with it as it required changes in opensearch-core as well.)

Change description:
Code for StopIndexReplicationRequest and StopIndexReplicationAction are moved to common-utils. Added UTs for both the new classes added.
This PR caters to point 1 of the proposed solution.

Issues Resolved

Related Issues
unfollow-action #726

Check List

• New functionality includes testing.
  • All tests pass
• [ ] New functionality has been documented.
  • [ ] New functionality has javadoc added
• Commits are signed per the DCO using --signoff

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.

[bowenlan-amzn]

We probably can use some better name here, I'm thinking

indices:admin/plugins/replication/index/stop_from_ism 😅

[aggarwalShivani]

Yeah I agreee 😅
But I was wondering if stop_from_ism would limit it to being only from ism? and what if some other plugin in future intends to invoke stop-replication using these libs. 🤔
Would something like stop_from_transport / transport_stop / stop_from_plugin work?

[bowenlan-amzn]

yeah, better to be intuitive for our user.

Looking at all the existing ones, I am thinking indices:transport/plugins/replication/index/stop
Hopefully transport is a common word for OpenSearch user.

https://github.com/opensearch-project/cross-cluster-replication/blob/d6e7636d42ef87cd483f7439d8a2cf43bf397727/HANDBOOK.md?plain=1#L108-L121

# Index Level Permissions

indices:admin/close
indices:admin/close[s]
indices:admin/create
indices:admin/mapping/put
indices:admin/open
indices:admin/plugins/replication/index/start
indices:admin/plugins/replication/index/stop
indices:data/read/plugins/replication/file_metadata
indices:data/write/index
indices:data/write/plugins/replication/changes
indices:data/write/replication
indices:monitor/stats

@cwperks to give some suggestion about the naming of the new transport action
This new transport action being added here is only supposed to be called from ISM plugin, and doesn't have any REST API associated with it. ISM user will need this permission to do the stop replication action using ISM.

[cwperks]

@bowenlan-amzn indices:transport is not one of the valid action prefixes tracked in core. You can introduce a new action prefix by opening a PR in core to add to that list.

I see there is indices:internal in the list of prefixes. Is that prefix reserved for actions defined in OpenSearch core, or is it possible for a plugin to use that prefix as well?

[bowenlan-amzn]

Thanks!

indices:internal makes sense here

[aggarwalShivani]

I will try with indices:internal/plugins/replication/index/stop then 🙂
Thank you @cwperks and @bowenlan-amzn for these inputs.

[bowenlan-amzn]

From the existing code in AlertingPluginInterface, I see we are doing <Response: BaseResponse>, does that have problem here? can we also use BaseResponse, instead of AcknowledgedResponse.

[bowenlan-amzn]

not sure about what this test is checking, would you explain a bit

[aggarwalShivani]

Sure. I've written this test on similar lines as done in NotificationsPluginInterfaceTests.kt and AlertingPluginInterfaceTests.kt

• An AcknowledgedResponse response object is created with a value of true (expected response).
• Have mocked the client object such that, when stopReplication() invokes client.execute(), it would trigger the onResponse method of the ActionListener with the successful response object created earlier.
• The test runs stopReplication() and verifies that the onResponse() method of the listener was called exactly once with the provided response object as an argument.
• This is to simulate a successful execution and verifies that the stopReplication method correctly handles the response.

[bowenlan-amzn]

Thanks for the explain

What I feel weird is we mock the client.execute to do listener.onResponse
Then we call stopReplication which just does client.execute, and verify listener.onResponse called once

In the end, it seems just testing stopReplication does client.execute exactly once 🤔
but it's fine, just curious what it actually test...