[BUG] Performance Analyzer breaks commutativity of NetworkPlugin.getTransportInterceptors

[cwperks]

What is the bug?

When PA is running in Dual mode, any request to index a doc fails with the following exception:

curl -XPUT "https://admin:myStrongPassword123\!@localhost:9200/movies/_doc/3" -k \
  -H "Content-Type: application/json" \
  -d '{"hello":"world"}'
{"_index":"movies","_id":"3","_version":1,"result":"created","_shards":{"total":2,"successful":1,"failed":1,"failures":[{"_index":"movies","_shard":0,"_node":"_QoDimt_S7KPc-7gqkfT5w","reason":{"type":"security_exception","reason":"Internal or shard requests not allowed from a non-server node for transport type RTFPerformanceAnalyzerTransportChannelType"},"status":"INTERNAL_SERVER_ERROR","primary":false}]},"_seq_no":0,"_primary_term":1}

Repro steps:

✗ curl -XPOST https://admin:myStrongPassword123\!@localhost:9200/_plugins/_performanceanalyzer/cluster/config -H 'Content-Type: application/json' -d '{"enabled": true}' -k

{"currentPerformanceAnalyzerClusterState":0,"shardsPerCollection":0,"collectorsSetting":0,"batchMetricsRetentionPeriodMinutes":7}%

✗ curl -XPUT https://admin:myStrongPassword123\!@localhost:9200/_cluster/settings -k \
-H "Content-Type: application/json" \
-d '{"transient": {"cluster.metadata.perf_analyzer.collectors.mode":"2"}}'

{"acknowledged":true,"persistent":{},"transient":{"cluster":{"metadata":{"perf_analyzer":{"collectors":{"mode":"2"}}}}}}%

✗ curl -XPUT https://admin:myStrongPassword123\!@localhost:9200/_cluster/settings -k \
-H "Content-Type: application/json" \
-d '{"persistent": {"cluster.metadata.perf_analyzer.collectors.mode":"1"}}'

{"acknowledged":true,"persistent":{"cluster":{"metadata":{"perf_analyzer":{"collectors":{"mode":"1"}}}}},"transient":{}}%

Once the settings above are configured, try index a doc.

Additional details:

There already is tight coupling between the Security plugin and PA where security has logic to try to peel the inner channel inside the SecurityInterceptor if the channel has been wrapped.

Code refs:

• PA transport channel wrapper that implements getInnerChannel: https://github.com/opensearch-project/performance-analyzer/blob/main/src/main/java/org/opensearch/performanceanalyzer/transport/RTFPerformanceAnalyzerTransportChannel.java#L123-L127
• SecurityRequestHandler that uses reflection to call getInnerChannel: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java#L179-L191

The reason for the failure is that in DUAL mode, PA doubly wraps the transport channel. When Security unwraps the outer layer, it still encounters another wrapped channel and stops unwrapping because it assumes the channel is wrapped at most once.

What's changed?

In OpenSearch 3.0, AD has declared an optional extends relationship on the security plugin. This means that security will be loaded before AD and likely will be loaded before performance analyzer as well. In prior versions, PA was likely loaded before security.

On Node Bootstrap, the NetworkModule of core will iterate through the network plugins to create a composite transport interceptor which is composed of all transport interceptors registered with NetworkPlugin.getTransportInterceptors(). Given that there is no nortion of ordering for transport interceptors the assumption is that these interceptors are commutative meaning that order does not matter. PA breaks this assumption.

To fix the issue, PA needs to revisit the logic for wrapping the channel or introduce a mechanism to the core for ordering the transport interceptors if they are no longer supposed to be commutative.

[nibix]

@cwperks my humble opinion would be:

Unfortunately, the TransportChannel interface does not define a contract what getChannelType() is actually supposed to return:

https://github.com/opensearch-project/OpenSearch/blob/b77770c9394b60ae05761b0e068fef7fffd6ee75/server/src/main/java/org/opensearch/transport/TransportChannel.java#L60

This is unfortunate because this leaves devs guessing what would be suitable return types.

However:

• It seems to me that the security plugin is actually the only piece of code that actually calls getChannelType(). I have checked core and the performance-analyzer project and in both it is only called in tests or just in delegate methods.
• If you look at the implementations in core, all TransportChannel implementations that wrap other channels do delegate getChannelType() to the wrapped TransportChannel:
  • https://github.com/opensearch-project/OpenSearch/blob/b77770c9394b60ae05761b0e068fef7fffd6ee75/server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableTcpTransportChannel.java#L82
  • https://github.com/opensearch-project/OpenSearch/blob/b77770c9394b60ae05761b0e068fef7fffd6ee75/server/src/main/java/org/opensearch/transport/TaskTransportChannel.java#L63

Thus, I would argue that also the transport channel wrappers implemented in the performance analyzer should delegate the getChannelType() call to the wrapped channel. That would solve all unwrapping issues.

Also, we should really get rid of using reflection in the security plugin for this purpose; on such a low level and such hot code, using reflection will also have a signficant performance impact (it's a bit ironic that it kicks in when the performance analyzer gets active :)

[cwperks]

@nibix if there are channel wrappers, then we need a mechanism to ensure that they delegate all methods to the original wrapped channel.

In 2.11, there was a bwc issue when PA was enabled: opensearch-project/security#3771

The reason for that is that the ChannelWrapper in this repo did not delegate getVersion() properly. I raised a PR in December 2023 for a fix, but this repo never merged it: #609

Security ended up fixing that issue directly by moving the call to unwrap the inner channel higher, but the PR to this repo should have had traction from maintainers and was left to languish.

[kumjiten]

if there are channel wrappers, then we need a mechanism to ensure that they delegate all methods to the original wrapped channel.

+999, IMO Wrapping of channels in general is error-prone, e.g., if the TransportChannel interface updates the contract with the default implementation (maintaining bwc), then the wrapped channels have to make changes to make sure other plugins get the right information.

Also, for context, this is where PA originally (i.e., the first release of PA) broke the transport plugins opendistro-for-elasticsearch/deprecated-security-ssl#2, which comes after PA in the execution path, leading to getInnerChannel by reflection being added.

The security plugin assumes that transport channel is always of a particular type. The performance analyzer plugin wraps the transportChannel into a different object and this breaks the transport plugin. This change retrieves the underlying transportChannel via reflection.

Security ended up fixing that issue directly by opensearch-project/security#3769, but the PR to this repo should have had traction from maintainers and was left to languish.

We should also check the security plugin to remove any tight coupling, which was introduced due to improper implementations by other plugins; otherwise, it can still lead to errors.

[nibix]

As TransportChannel is annotated with @opensearch.internal could we possibly do the following code change in core?

public sealed interface TransportChannel permits BaseTcpTransportChannel, DirectResponseChannel, TransportChannelWrapper {
   // ...
}

public abstract class TransportChannelWrapper {
   private final TransportChannel delegate;

   @Override 
   public final String getChannelType() {
      return delegate.getChannelType();
   }

   @Override
   public final <T> Optional<T> get(String name, Class<T> clazz) {
      return delegate.get(name, clazz);
   }

   // Potentially more refinement is also necessary for other methods
}

[kumjiten]

As TransportChannel is annotated with @opensearch.internal could we possibly do the following code change in core?

+1, The purpose of sealed interfaces/classes is to control and limit who can extend and implement them. Since these channel types are internal components of Core OpenSearch, it's appropriate to restrict their extensibility and prevent plugins from creating new channel types.

However, TransportChannelWrapper can still lead to improper implementation issues if plugins start extending it.

We can define custom annotations to define guardrails, something like below, thoughts?

public interface TransportChannel {
    @MustBeImplemented
    String getProfileName();

    @MustBeImplemented
    default String getChannelType() {
        return null;
    }
    // ... other methods
}

@Retention(RetentionPolicy.SOURCE)
@Target(ElementType.METHOD)
public @interface MustBeImplemented {
}

@SupportedAnnotationTypes("org.opensearch.transport.MustBeImplemented")
@SupportedSourceVersion(SourceVersion.RELEASE_21) // Adjust Java version
public class MustBeImplementedProcessor extends AbstractProcessor {
    @Override
    public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
        for (Element element : roundEnv.getElementsAnnotatedWith(MustBeImplemented.class)) {
            if (element.getKind() == ElementKind.METHOD) {
                ExecutableElement method = (ExecutableElement) element;
                TypeElement declaringClass = (TypeElement) method.getEnclosingElement();
                
                // Check if method is implemented in subclasses
                // Issue compilation error if not implemented
            }
        }
        return true;
    }
}

[krisfreedain]

Catch All Triage - 1 2