Skip to content

Support phasing off SecurityManager usage in favor of Java Agent #3724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

kumargu
Copy link
Contributor

@kumargu kumargu commented Apr 11, 2025

Description

With Java agent as a replacement to security manager, we start enforcing access to UnixDomainSockets. On windows since UnixDomainSockets are used for loopback address windows builds are now starting to fail for skills and flow-framework.

Additionally for this plugin's integ test to work, it will need a new jvm args to be passed. this change also includes the JVM args included in build.gradle.

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.

build.gradle Outdated
dependencies {
agent "org.opensearch:opensearch-agent-bootstrap:${opensearch_version}"
agent "org.opensearch:opensearch-agent:${opensearch_version}"
agent "net.bytebuddy:byte-buddy:1.17.5"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once this PR is green. I can try using agent "net.bytebuddy:byte-buddy:${versions.bytebuddy}"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can send similar PR to all repos or remove after RC1.
Better solution will be implementing opensearch-project/OpenSearch#17900 in RC2.

build.gradle Outdated
Comment on lines 97 to 105
tasks.withType(Test) {
dependsOn prepareAgent
jvmArgs += ["-javaagent:" + project.layout.buildDirectory.file("agent/opensearch-agent-${opensearch_version}.jar").get()]
}

task prepareAgent(type: Copy) {
from(configurations.agent)
into "$buildDir/agent"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be nested within allprojects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i also think so. figuring that out.

@kumargu kumargu force-pushed the support_java_agent branch from c774cc0 to a9523c2 Compare April 11, 2025 16:42
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 16:44 — with GitHub Actions Failure
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 16:44 — with GitHub Actions Error
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 16:44 — with GitHub Actions Failure
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 16:44 — with GitHub Actions Failure
@kumargu kumargu force-pushed the support_java_agent branch from a9523c2 to ecb1d75 Compare April 11, 2025 17:54
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 17:55 — with GitHub Actions Inactive
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 17:55 — with GitHub Actions Error
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 17:55 — with GitHub Actions Inactive
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 17:55 — with GitHub Actions Failure
@prudhvigodithi
Copy link
Member

I have tested with this PR opensearch-project/OpenSearch#17900, just adding apply plugin: 'opensearch.java-agent' inside :opensearch-ml-plugin:yamlRestTest should solve the issue we dont have to add it across the gradle projects.

@kumargu
Copy link
Contributor Author

kumargu commented Apr 11, 2025

I have tested with this PR opensearch-project/OpenSearch#17900, just adding apply plugin: 'opensearch.java-agent' inside :opensearch-ml-plugin:yamlRestTest should solve the issue we dont have to add it across the gradle projects.

that's only when opensearch-project/OpenSearch#17900 is merged?

@prudhvigodithi
Copy link
Member

that's only when opensearch-project/OpenSearch#17900 is merged?

Yes Once merged, its under review hope to get the PR merged soon.
Thanks
@getsaurabh02

@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 19:57 — with GitHub Actions Inactive
@kumargu kumargu force-pushed the support_java_agent branch from ecb1d75 to 297b51b Compare April 11, 2025 20:06
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:07 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:07 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:07 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:07 — with GitHub Actions Inactive
@kumargu kumargu force-pushed the support_java_agent branch 2 times, most recently from 5016cbe to 12dff37 Compare April 11, 2025 20:10
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 20:12 — with GitHub Actions Failure
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 20:12 — with GitHub Actions Failure
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 20:12 — with GitHub Actions Failure
@kumargu kumargu had a problem deploying to ml-commons-cicd-env-require-approval April 11, 2025 20:12 — with GitHub Actions Failure
@kumargu kumargu force-pushed the support_java_agent branch from 12dff37 to bcc0f72 Compare April 11, 2025 20:14
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:16 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:16 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:16 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 20:16 — with GitHub Actions Inactive
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.11%. Comparing base (82da998) to head (bcc0f72).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3724      +/-   ##
============================================
- Coverage     80.32%   79.11%   -1.22%     
- Complexity     7092     7115      +23     
============================================
  Files           621      623       +2     
  Lines         30937    31578     +641     
  Branches       3494     3571      +77     
============================================
+ Hits          24851    24983     +132     
- Misses         4574     5054     +480     
- Partials       1512     1541      +29     
Flag Coverage Δ
ml-commons 79.11% <ø> (-1.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 21:21 — with GitHub Actions Inactive
@kumargu kumargu temporarily deployed to ml-commons-cicd-env-require-approval April 11, 2025 21:21 — with GitHub Actions Inactive
@@ -83,7 +83,16 @@ allprojects {
}

project.getExtensions().getExtraProperties().set("versions", VersionProperties.getVersions());
}

configurations {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, if we are already doing this in the root gradle file, do we still need to do the same for all the modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. one way to not do it across other modules is to include these on sub-projects, but that hasn't given me much of a success.

As-of-now, it seems like duplicating it across modules works. However this change in temporary to keep us unblocked. We may just define it at one place after this PR opensearch-project/OpenSearch#17900 is being merged and tested once.

@peterzhuamazon
Copy link
Member

@peterzhuamazon
Copy link
Member

Closing as #3727 is merged now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants