[#394] Provide non-reflective access to JVM Attach API on JDK9+ #464
Conversation
|
Please do not merge yet. Work in progress. |
grgrzybek
force-pushed
the
jdk11-vm-attach-api
branch
2 times, most recently
from
f4c1eaa
to
febc4cd
Compare
agent/jvm/pom.xml
Outdated
| <source>1.6</source> | ||
| <target>1.6</target> | ||
| <source>1.7</source> | ||
| <target>1.7</target> |
There was a problem hiding this comment.
I guess this jump is required, and I'm not against it. But we need to bump the version number accordingly, to indicate that the release is not compatible with Java 1.6. anymore. Also, I'd prefer to have this as a global switch for all components (not only for the JVM agent)
There was a problem hiding this comment.
It is required, when I tried to run the tests using JDK17 (as it no longer can handle target=1.6), but it's not a requirement for any other change. I can skip this change.
There was a problem hiding this comment.
Maybe we can skip it for now, and consider to bump to 1.7 as minimum in a followup PR (and then already on the overall level) ?
There was a problem hiding this comment.
I reverted this change, so it's still 1.6. 1.7 was needed if I wanted to build using JDK17. But I needed it to test using JDK17. After verifying it works, we can leave at 1.6 value ;)
|
|
||
| package org.jolokia.jvmagent.security.asn1; | ||
|
|
||
| public class DERInteger implements DERObject { |
There was a problem hiding this comment.
Are those ASN1 objects copied over from some other open-source library (like bouncycastle) or do you have written those ?
It's probably over the top here, but are there any tests for those classes ?
There was a problem hiding this comment.
The tests are in org/jolokia/jvmagent/security/asn1/DEREncodingTest.java and I confirm - I looked a bit at BC classes (only to check what DER Tag is), but every line of code is written by me. I was consulting only the X.680/X.690 specifications.
There was a problem hiding this comment.
Thanks a lot for all of your efforts, looks good to me in general ! Maybe it would have been better to split it up in two PRs (one for the reflection in the VirtualMachineHandler, one for the CERT handling).
I have two questions though, see the comments inline. Also if the ASN.1 objects are borrowed from somewhere else we should check the license and add the corresponding license headers/files. For the switch to 1.7 I wonder whether this is required, and if so, we should do it at the top-level.
|
First - this PR can be treated as "for review only" and I agree that separate PRs are better. DER classes are written by me, as I didn't want to borrow anything from BC (or internal JDK classes). I also didn't want to introduce any dependency to external, lightweight ASN.1/DER libraries. |
|
I'm just checking the final part - |
….RequiredModelMBean as gently as possible
grgrzybek
force-pushed
the
jdk11-vm-attach-api
branch
from
febc4cd
to
e04e6d1
Compare
ah, very cool. Do you think we should some unit tests for those ? They don't look trivial |
|
I think we can keep it 1 PR, don't bother with fiddling around with multiple PRs. Maybe you can have several commits per feature, but happy also if not. Don't waste efforts here. |
the tests are in |
|
And #466 is a separate PR for cert generation. |
|
Sorry that I overlooked the tests ;( going to continue tomorrow on the PRs ... |
No description provided.