Create equivalents of JSM's AccessController in the java agent

[cwperks]

Description

The classes in this PR were on a former iteration of #17894

This PR creates replacements for JSM's AccessController which is marked for removal from the JDK. While JSM was replaced with the java agent in 3.0.0, the logic to extract the ProtectionDomains from the call stack relies on the AccessController to limit the frames when examining the stack. The java agent needs to retain this code marker to know when to stop walking the stack and this PR creates OpenSearch equivalents to the AccessController which is a simple wrapper around a runnable block of code.

Related Issues

Resolves #18339

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[cwperks]

I don't know the right module for this code. The server has a dependency on this lib, but its marked as compileOnly. How are the other classes in this module (like AgentPolicy) available at runtime?

[andrross]

I believe jars passed to the JVM via -javaagent are available on the classpath, so the compileOnly dependency is making the assumption this will be provided at runtime via a -javaagent.

[github-actions]

✅ Gradle check result for 366406f: SUCCESS

[codecov]

Codecov Report

Attention: Patch coverage is 28.57143% with 20 lines in your changes missing coverage. Please review.

Project coverage is 72.67%. Comparing base (fe4a98d) to head (5b04b59).

Files with missing lines	Patch %	Lines
...va/org/opensearch/ingest/geoip/GeoIpProcessor.java	44.44%	10 Missing ⚠️
...ensearch/javaagent/bootstrap/AccessController.java	0.00%	6 Missing ⚠️
...ent/StackCallerProtectionDomainChainExtractor.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18346      +/-   ##
============================================
+ Coverage     72.60%   72.67%   +0.07%     
- Complexity    67682    67748      +66     
============================================
  Files          5497     5498       +1     
  Lines        311819   311825       +6     
  Branches      45265    45265              
============================================
+ Hits         226409   226633     +224     
+ Misses        66941    66771     -170     
+ Partials      18469    18421      -48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

❕ Gradle check result for 53be672: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[andrross]

I think we should improve on this interface. Because these two method only differ in the exceptions that are thrown, the compiler can't disambiguate which overload is being called if you attempt to pass a lambda. The solution is to either make an anonymous class or explicitly add a cast, both of which are unnecessarily verbose. I suggest something like the following:

public static <T> T doPrivileged(PrivilegedAction<T> action) {
    return action.run();
}

public static void doPrivileged(Runnable action) {
    action.run();
}

public static <T> T doPrivilegedChecked(PrivilegedExceptionAction<T> action) throws Exception {
    return action.run();
}

public static void doPrivilegedChecked(PrivilegedExceptionRunnable action) throws Exception {
    action.run();
}

public interface PrivilegedExceptionRunnable {
    void run() throws Exception;
}

The basic idea is to use a different method name for the checked variants, and also provide variants that do not return anything. This will allow for code like:

String foo = AccessController.doPrivilegedChecked(() -> Files.readString(Paths.get("/tmp/foo")));

and

AccessController.doPrivilegedChecked(() -> Files.delete(Paths.get("/tmp/foo")));

[cwperks]

The difference is that the arg in the unchecked doPrivileged is PrivilegedAction vs the arg in the checked doPrivileged is PrivilegedExceptionAction which is how java knows which version of the method you are using.

I like your suggestion though and think this could be an opportunity to remove PrivilegedExceptionAction

[andrross]

But the compiler can't figure it out implicitly if you pass a lambda so you're forced to define a type with a cast. Basically I'd love to be able to change this code:

OpenSearch/plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/ServerComponents.java

Lines 190 to 193 in abe2333

	AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
	boundAddresses.add(bindAddress(address, port));
	return null;
	});

to:

AccessController.doPrivileged(() -> boundAddresses.add(bindAddress(address, port)));

Note the runnable variant is needed here as well to avoid the return null

[cwperks]

wdyt about this interface?

public static void doPrivileged(Runnable action) {
    action.run();
}

public static <T> T doPrivileged(Callable<T> action) throws Exception {
    return action.call();
}

[andrross]

I think we'd want all four variants: the void and return type for both checked and unchecked exceptions:

public static <T> T doPrivileged(Supplier<T> action) {
    return action.get();
}

public static void doPrivileged(Runnable action) {
    action.run();
}

public static <T> T doPrivilegedChecked(Callable<T> action) throws Exception {
    return action.call();
}

public static <T extends Exception> void doPrivilegedChecked(CheckedRunnable<T> action) throws T {
    action.run();
}

public interface CheckedRunnable<E extends Exception> {
    void run() throws E;
}

To avoid ambiguity with lambdas you still have to do the slightly clunky thing of giving a different name to the checked variants, but I think that's preferable compared to requiring a cast.

[cwperks]

Added the checked variants to this interface. I think this is a much cleaner and modern interface now.

[github-actions]

❌ Gradle check result for 00c22c7: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for d79bdc1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[cwperks]

Testing the replacement in the ingest-geoip module after switching :libs:agent-sm:bootstrap from compileOnly to compileOnlyApi in the server.

[github-actions]

❌ Gradle check result for 9cfa314: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 9cfa314: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 9cfa314: SUCCESS

[andrross]

Let's expand on this by including some text about when and why this is needed.

[cwperks]

Expanded on this javadoc with example usages and more explanation. It doesn't quite get into how it relates to the policy files, but perhaps we can link to opensearch-project/project-website#3784 once that's published?

[github-actions]

❌ Gradle check result for 995c66c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 5b04b59: SUCCESS