Skip to content

Conversation

@Yumasi
Copy link
Member

@Yumasi Yumasi commented Feb 28, 2023

What Does This Do

This PR adds a new instrumentation, sslsocket, that captures HTTPS traffic in Java applications that use SSLSocket under the hood, and sends it to system-probe via a custom ioctl. This part of USM's HTTPS monitoring support.

Few things to note:

  • This is meant to be attached to Java process by system-probe (which uses jattach under the hood), which requires a different way to get the javaagent args, hence the addition to agentmain.
  • The instrumentation does not produce traces, but instead does an ioctl using jna. The dependency to jna is the reason UsmMessage and UsmExtractor implementations are in separate packages.
  • Due to it only making an ioctl call as a side-effect, the included instrumentation test focuses on the "instrumentation part" i.e the advices, to make sure it does the expected calls to the implementation that makes the actual ioctl call.

Changes for second review round

  • The instrumentation now targets SSLSocket instead of SSLSocketImpl.

Motivation

Additional Notes

val06 and others added 27 commits February 15, 2023 08:14
- removed SSLSocketImpl and its inner classes from ignored_class_name.trie to allow instrumenting them
    - sending captured data via ioctl with a special request code
- using USMExtractor directly from SSLSocket advices
- updated MAX_HTTP_BUFFER_SIZE to match the relevant value on the Systemprobe side
…ffer

- added prints to ioctl execution
- fixed connection encoding
- fixed message validation bug
Signed-off-by: Guillaume Pagnoux <[email protected]>
Signed-off-by: Guillaume Pagnoux <[email protected]>
- added args parsing when injecting the agent dynamically
Signed-off-by: Guillaume Pagnoux <[email protected]>
…encies in internal-api and agent-tooling modules. Instead, passing the connenction tuple directly from the advice
…tion-profile' into valeri.pliskin/java-tls-support-java-sdk16-and-above
…-java-sdk16-and-above

Remove SSLSocketImpl references to support JDK 16 and above
Signed-off-by: Guillaume Pagnoux <[email protected]>
@Yumasi Yumasi requested a review from a team as a code owner February 28, 2023 14:45
Yumasi added 5 commits March 28, 2023 16:44
Signed-off-by: Guillaume Pagnoux <[email protected]>
Signed-off-by: Guillaume Pagnoux <[email protected]>
Signed-off-by: Guillaume Pagnoux <[email protected]>
Signed-off-by: Guillaume Pagnoux <[email protected]>
Yumasi added 2 commits April 5, 2023 12:41
Signed-off-by: Guillaume Pagnoux <[email protected]>
Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

Needs a bit more cleanup before this can be merged - see comments about using USM specific package for new types and preferring agent-bootstrap over internal-api for bootstrap related instrumentation types

Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

Once the unrelated formatting changes have been cleaned up this should be OK to merge

Yumasi added 2 commits April 5, 2023 14:29
Signed-off-by: Guillaume Pagnoux <[email protected]>
@Yumasi Yumasi dismissed val06’s stale review April 5, 2023 13:09

Valeri is on PTO but we need this merged this week

Signed-off-by: Guillaume Pagnoux <[email protected]>
@Yumasi Yumasi merged commit 8019600 into master Apr 5, 2023
@Yumasi Yumasi deleted the guillaume.pagnoux/usm-intrumentation-profile branch April 5, 2023 16:18
@github-actions github-actions bot added this to the 1.12.0 milestone Apr 5, 2023
@nikita-tkachenko-datadog
Copy link
Contributor

nikita-tkachenko-datadog commented Apr 9, 2023

This breaks logic introduced in #4888

Specifically the part in AgentBootstrap:

parseAsSystemProperties(agentArgs);

parses agent arguments and injects them as system properties.

The same logic was implemented in AgentArgsInjector in the PR linked above (the only difference being that , is used to separate arguments rather than ).

As the code added here expects args to be separated with space, when args are supplied in previous comma-separated format, they are all lumped together and injected under the name of the first arg.

My suggestion would be to revert the parseAsSystemProperties part of the changes done here in favour of the AgentArgsInjector logic, as that has already been released to the public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: universal service monitoring Universal Service Monitoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants