Skip to content

Fixed Java LDAP tests in JDK 17 #2430

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fixed Java LDAP tests in JDK 17 #2430

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions apm-agent-plugins/apm-java-ldap-plugin/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,15 @@
<scope>test</scope>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>@{argLine} --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED</argLine>
Copy link
Member

@felixbarny felixbarny Jan 28, 2022

Choose a reason for hiding this comment

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

What happens if someone uses Java 17 to run their app and does add this export? Will the agent break their app or throw errors?

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 28, 2022

Choose a reason for hiding this comment

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

As exceptions, that are thrown from plugins, are caught/handled by ByteBuddy/the agent, I assume the plugin won't work, but the rest of the app work.

Copy link
Member

@felixbarny felixbarny Jan 28, 2022

Choose a reason for hiding this comment

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

That's true but these exceptions still affect the application - more overhead due to creation and logging and the logs are going to be spammed, too.
Just disabling the module on Java 17 is probably also not the best option as even Java 11 can be configured to enforce the module boundaries instead of warning about them.
I think we'll probably have to remove the LDAP plugin until we have found a solution for that.

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 28, 2022

Choose a reason for hiding this comment

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

Ok, I will create an external plugin in the mean time.

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 28, 2022

Choose a reason for hiding this comment

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

Are you taking care of the removal or should I?

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 31, 2022

Choose a reason for hiding this comment

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

classLoaderCanLoadClass("com.sun.jndi.ldap.Connection") seems to work.

Copy link
Member

@felixbarny felixbarny Jan 31, 2022

Choose a reason for hiding this comment

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

Seems to work in the sense that it returns false on Java 17 without --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED but true with that flag?

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 31, 2022

Choose a reason for hiding this comment

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

Hm it doesn't work with --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED.

But

public class LdapClientAdvice {

    static {
        try {
            com.sun.jndi.ldap.LdapResult.class.getConstructor().newInstance();
        } catch (Exception e) {
            throw new IllegalStateException(e);
        }
    }
...

works with --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED and "only" logs one exception without --add-exports java.naming/com.sun.jndi.ldap=ALL-UNNAMED

Copy link
Member

@felixbarny felixbarny Jan 31, 2022

Choose a reason for hiding this comment

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

The downside of that is that this eagerly links com.sun.jndi.ldap.LdapResult and initializes a bunch of classes that otherwise would not be initialized at this point. We usually try not to impact the order in which classes are loaded as this can negatively impact the application. For example, if java.util.logging is used somewhere in the prematurely initialized classes, app server can't set up their custom log handlers.

Copy link
Contributor Author

@tobiasstadler tobiasstadler Jan 31, 2022

Choose a reason for hiding this comment

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

yes, this is true.

</configuration>
</plugin>
</plugins>
</build>
</project>