Skip to content
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

Adding support for scenarios where the RMI registry has SSL enabled #835

Merged
merged 3 commits into from
Apr 21, 2023

Conversation

dmitchnewrelic
Copy link
Contributor

Description:

The JMX Metric Gatherer is not working properly with servers that have SSL protection enabled on the RMI registry. Oracle's Java documentation describes this setting as follows:

The system property com.sun.management.jmxremote.registry.ssl=true was new in JDK 6 and aims at resolving security issues with the RMI registry used in relation with JMX. This property must be used in conjunction with com.sun.management.jmxremote.ssl.need.client.auth=true in order to fully secure the RMI registry.

Changes were made to the JMX Metrics Gatherer to support this scenario, by adding a new helper class called JmxConnectorHelper. This class implements different connection logic when SSL is enabled on the RMI registry.

Changes were also made to JmxConfig, as a new parameter is required to handle this scenario. Specifically, the following parameter was added:

otel.jmx.remote.registry.ssl=true

The readme file was updated to explain when to use this new parameter.

Existing Issue(s):

Issue #834

Testing:

The following scenarios were tested:

  • SSL not enabled, and no credentials required
  • SSL not enabled, but with credentials
  • SSL enabled for JMX, but not on the RMI registry
  • SSL enabled for JMX including the RMI registry

Documentation:

README.md was updated to reflect the new configuration parameters.

Outstanding items:

Nothing at this time.

@dmitchnewrelic dmitchnewrelic requested a review from a team April 20, 2023 16:04
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dmitchnewrelic (4ff1195)

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

This seems good to me. Had one rather small suggestion.

I'd really like to see an integration test that covers this. Would you mind opening an issue to do that work in a follow-up PR?

Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmitchnewrelic
Copy link
Contributor Author

Thanks @breedx-splk. I've made the suggested change and will create a separate issue for the integration test.

@dmitchnewrelic
Copy link
Contributor Author

I've created issue #838 for this.

@jeffalder
Copy link

@trask
Copy link
Member

trask commented Apr 20, 2023

just a heads up, you can ignore the EasyCLA failure, it is having an issue currently (open-telemetry/community#1445)

@breedx-splk per https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/RELEASING.md#release-cadence does this mean the next normal release would be May 12th?

👍

@trask
Copy link
Member

trask commented Apr 21, 2023

/easycla

@trask
Copy link
Member

trask commented Apr 21, 2023

/easycla

@trask trask merged commit eb24f80 into open-telemetry:main Apr 21, 2023
@breedx-splk
Copy link
Contributor

@breedx-splk per https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/RELEASING.md#release-cadence does this mean the next normal release would be May 12th?

I don't think we'd want to promise a date, but yeah, basically monthly and more strongly coordinated with the core and instrumentation releases. I defer to the maintainers on anything more specific.

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.

8 participants