-
Notifications
You must be signed in to change notification settings - Fork 327
Support for the Java LDAP client #2355
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
Conversation
|
👋 @tobiasstadler Thanks a lot for your contribution! It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it. Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it. |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
@felixbarny Could you please review this PR also? |
|
run elasticsearch-ci/docs |
|
Thank you! |
| import com.sun.jndi.ldap.Connection; | ||
| import com.sun.jndi.ldap.LdapResult; |
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.
Seems like IntelliJ has issues with the imports from the java.naming/com.sun.jndi.ldap module.
Applying the suggestion only works until the maven project is reloaded.
I've tried adding
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs>
<arg>--add-exports</arg>
<arg>java.naming/com.sun.jndi.ldap=ALL-UNNAMED</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>But that yields this error: java: exporting a package from system module java.naming is not allowed with --release
@tobiasstadler Did you manage to get this work in an IDE?
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.
Can you please try adding <maven.compiler.source>1.7</maven.compiler.source> to the properties of the plugin pom?
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 problem probably is that the IntelliJ profile in the parent pom sets maven.compiler.target and maven.compiler.target to 11.
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.
Ah, right. This works. However, only with a Java 11 compiler. With Java 17 there are still a few issues. Under Preferences > Build, Execution, Deployment > Compiler > Java Compiler, I had to uncheck the Use '--release' option for cross-compilation option. But even after doing that, the tests fail with an IllegalAccessError:
java.lang.IllegalAccessError: class co.elastic.apm.agent.java_ldap.LdapClientAdvice (in unnamed module @0x25be445f) cannot access class com.sun.jndi.ldap.Connection (in module java.naming) because module java.naming does not export com.sun.jndi.ldap to unnamed module @0x25be445f
at co.elastic.apm.agent.java_ldap.LdapClientAdvice.onEnter(LdapClientAdvice.java:55)
at java.naming/com.sun.jndi.ldap.LdapClient.authenticate(LdapClient.java:153)
...
This will fail the Java 17 compatibility tests and it's probably also an issue in production with Java 17.
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 just tested it with Java17. It compiles fine but the tests are failing with the error you mentioned.
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.
This reverts commit 84e8f8b.

What does this PR do?
I added support for instrumenting the Java LDAP client
Checklist