Skip to content

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented May 9, 2025

Description

SAP has its guava dependencies set at compileOnly level which means they are not packaged into the SAP assembly and available at runtime.

SAP uses these deps at runtime, however, the way this works is because of the extending relationship between SAP and JS plugin where the deps of the extended plugin are available to the extending plugin at runtime.

In opensearch-project/job-scheduler#773, guava was removed from JS which means these deps are no longer available to SAP at runtime. I'm creating this PR to change the guava deps from compileOnly to implementation to ensure they are included in the assembly and available at runtime.

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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
Copy link
Member Author

cwperks commented May 9, 2025

Build failures may be related to opensearch-project/OpenSearch#17989

CC: @pranu2502

cwperks added 3 commits May 9, 2025 14:15
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented May 9, 2025

@kumargu @pranu2502 @RajatGupta02

This is something that needs to be considered for the AccessController replacement. Resources on the classpath should be exempt from permissions checks.

In java, I believe this is handled in the ClassLoader: https://github.com/openjdk/jdk21u/blob/master/src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java#L500-L505

Ref run with failures: https://github.com/opensearch-project/security-analytics/actions/runs/14935397717/job/41961528538

@cwperks
Copy link
Member Author

cwperks commented May 9, 2025

@kumargu @pranu2502 @RajatGupta02

This is something that needs to be considered for the AccessController replacement. Resources on the classpath should be exempt from permissions checks.

In java, I believe this is handled in the ClassLoader: https://github.com/openjdk/jdk21u/blob/master/src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java#L500-L505

CC: @reta putting this on your radar too.

Signed-off-by: Craig Perkins <[email protected]>
@cwperks
Copy link
Member Author

cwperks commented May 9, 2025

We're going to need a solution to this to support JDK 24 in this repo, because the calls to AccessController.doPrivileged within the JDK have now been removed: openjdk/jdk24u@db7ee3d

@reta
Copy link
Contributor

reta commented May 9, 2025

We're going to need a solution to this to support JDK 24 in this repo, because the calls to AccessController.doPrivileged within the JDK have now been removed: openjdk/jdk24u@db7ee3d

Have you seen the failures related to that? For the limited coverage we have now, it seems like we are fine [1]? In any case, I think we do have a couple of options to consider (java agent should have access to application classpath).

[1] opensearch-project/OpenSearch#18085

@cwperks
Copy link
Member Author

cwperks commented May 9, 2025

Have you seen the failures related to that?

I haven't tried with JDK24 yet, but I can update this repo to run the same tests with the change in this PR.

This was the run where I saw issues reading classpath resources after opensearch-project/OpenSearch#17989 was merged into core: https://github.com/opensearch-project/security-analytics/actions/runs/14935397717/job/41961528538

@AWSHurneyt AWSHurneyt merged commit e61462e into opensearch-project:main May 13, 2025
15 checks passed
toepkerd pushed a commit to toepkerd/security-analytics that referenced this pull request Jun 12, 2025
…ect#1530)

* Switch guava deps from compileOnly to implementation

Signed-off-by: Craig Perkins <[email protected]>

* Use AccessController

Signed-off-by: Craig Perkins <[email protected]>

* Use getResource

Signed-off-by: Craig Perkins <[email protected]>

* Use getResourceAsStream

Signed-off-by: Craig Perkins <[email protected]>

* Try removing lead forward slash

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
toepkerd pushed a commit to toepkerd/security-analytics that referenced this pull request Jun 12, 2025
…ect#1530)

* Switch guava deps from compileOnly to implementation

Signed-off-by: Craig Perkins <[email protected]>

* Use AccessController

Signed-off-by: Craig Perkins <[email protected]>

* Use getResource

Signed-off-by: Craig Perkins <[email protected]>

* Use getResourceAsStream

Signed-off-by: Craig Perkins <[email protected]>

* Try removing lead forward slash

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
toepkerd pushed a commit to toepkerd/security-analytics that referenced this pull request Jun 17, 2025
…ect#1530)

* Switch guava deps from compileOnly to implementation

Signed-off-by: Craig Perkins <[email protected]>

* Use AccessController

Signed-off-by: Craig Perkins <[email protected]>

* Use getResource

Signed-off-by: Craig Perkins <[email protected]>

* Use getResourceAsStream

Signed-off-by: Craig Perkins <[email protected]>

* Try removing lead forward slash

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
toepkerd pushed a commit to toepkerd/security-analytics that referenced this pull request Jun 17, 2025
…ect#1530)

* Switch guava deps from compileOnly to implementation

Signed-off-by: Craig Perkins <[email protected]>

* Use AccessController

Signed-off-by: Craig Perkins <[email protected]>

* Use getResource

Signed-off-by: Craig Perkins <[email protected]>

* Use getResourceAsStream

Signed-off-by: Craig Perkins <[email protected]>

* Try removing lead forward slash

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Dennis Toepker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants