-
Notifications
You must be signed in to change notification settings - Fork 528
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
Copy Java attacher jar to a tmp directory #8803
Conversation
📚 Go benchmark reportDiff with the
report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
/test windows |
/test windows |
Status update:
Following is how the proposed implementation should work, until we discover something new when testing that:
Placing here an offline discussion for reference- the above solution for Linux relies on us properly maintaining directory tree within a root that has a
|
Verified through manual testing on Linux. Eventual tmp directory tree is as follows:
Summary:
One comment remained unresolved, waiting for input. In addition, I am pretty sure there are better ways to exclude tests on a specific OS than what I did and maybe there is a better way to do OS-polymorphism than this |
Co-authored-by: Andrew Wilkins <[email protected]>
I won't be available for some time to merge this. |
@elastic/apm-server - anything else still required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A handful of minor comments, and what looks like a bug with error handling in copying the attacher jar.
Co-authored-by: Andrew Wilkins <[email protected]>
…nto java-attacher-copy-to-temp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. LGTM.
@eyalkoren no worries. Yes, I think a changelog entry is worthwhile. Please also update the PR description to use the PR template, explaining steps to be taken for manual testing. Otherwise I think it's good to merge. |
Done, please just see that the changelog addition is done as expected. |
Co-authored-by: Andrew Wilkins <[email protected]>
Tested with 8.5.0 Fleet agent used https://github.com/elastic/elastic-agent/commits/2b60ccf8da06fca193cb37a1186c2baa5c3b1f28 Works as expected, is there any os-specific variant that requires further testing ? For example is it required to also test on |
@SylvainJuge thanks! To the best of my knowledge there's no need to test on any specific Linux distros. If anyone knows otherwise, it would be @eyalkoren I'll go ahead and mark this tested then. |
FYI @eyalkoren is on PTO, but I remember reproducing the bug before this change was applied, thus if it used to not work at all and now at least works on one of Linux distributions, so we already have some positive progress. |
Motivation/summary
Fixing a bug discovered while testing this end-to-end: when Elastic Agent is ran through
./elastic-agent install
, it creates a file tree that is not readable for all users on Linux by default.Therefore, when running the attacher as a non-root user, it can't access the attacher jar.
The fix is to create a tmp directory with write permissions only to root and copy the attacher jar to it with
0755
permissions to make it accessible for all users for read and execute.Fixe verified on:
Note for reviewer:
Checklist
How to manually test these changes
debug
-Delastic.apm.attach=true
in the command line that runs it0700
mode and be owned by the same user that runs the Java app. It should contain the attacher jar with0600
mode and be owned by the same user. If attachment was successful, it should contain anagent
folder with internal structure that includes some PGP-utility libraries and a directory per agent version as requested within the integration.Related issues
#8718
#8660
#6969
#7832