-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add missing APM entitlements #123462
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
Add missing APM entitlements #123462
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
I checked in QA, it looks like we're in deed missing permissions for the agent to write it's log file. Though, it could also be due to the fact that we're just logging >= WARN. We should deploy a project with INFO logging for the agent to be sure. |
|
I can see metrics, so it seems to be able to connect to APM server |
| - AsyncProfiler.safemode | ||
| - load_native_libraries | ||
| - manage_threads | ||
| - outbound_network |
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.
wondering, why is elastic.apm.agent needed at all? shouldn't this be covered by the default agent policy?
a bit worrying is that we receive metrics from my QA projects despite not allowing outbound_network here or in the default agent policy 😱
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.
Were they apm metrics or just generic container metrics provided by metricbeat on the host?
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.
The issue about missing outbound_network but still working might be due to missing instrumentation to URLConnection & derived that we discovered.
why is elastic.apm.agent needed at all
Hypothesis (that I will try to confirm): in the plugin that jar/module is used kind of a client/an API to talk to the agent/to the server?
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.
But I do wonder if we need these policies in both places, or just here, or just on the agentEntitlements fixed set.
I'll dig a bit more on this point too.
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.
I moved these entitlements to the agentEntitlements policy; considering the dependency is runtimeOnly and the code using these entitlements is in the agent, seems like that is the right place for them.
Tests pass, even though APM tests are really limited - I tried to run ES with-apm-server too and no errors there.
Will also try a Serverless QA deployment before merging.
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.
lgtm
| new LoadNativeLibrariesEntitlement(), | ||
| new FilesEntitlement( | ||
| List.of( | ||
| FileData.ofPath(Path.of("/co/elastic/apm/agent/"), READ), |
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.
Actually these weird paths are ZipPaths! #123744 will fix that and we can remove them.
| new SetHttpsConnectionPropertiesEntitlement(), | ||
| new OutboundNetworkEntitlement(), | ||
| new WriteSystemPropertiesEntitlement(Set.of("AsyncProfiler.safemode")), | ||
| new LoadNativeLibrariesEntitlement(), |
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.
As @rjernst pointed out, we don't need this anymore: the APM Agent is loaded before entitlements are turned on, so native loading happens before that and an entitlement is not needed. I'll remove this.
💔 Backport failed
You can use sqren/backport to manually backport by running |
We currently do not match these 3 permissions on elastic-apm-agent (module elastic.apm.agent)
I've added the first 2; I was surprised we did not caught the first one, it's pretty important to communicate to the APM agent, but then I looked at IT tests and we might have a miss there (we test only to localhost).
It's worth more investigation though, I made a note to do that.
The second one might be already covered by
agentEntitlements, but I added it to the module as well - I think it's safer to err on the safer side :)I'm very reluctant to add the third permission however. Read and write to all files? No. Wdyt?
Relates to ES-10031
Complete picture for reference:
(legend:
xwe have it,-we don't care about it,?missing