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.

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

Jump to bottom

Removed explicit dependency on log4j #1563

Merged
merged 4 commits into from
Jan 8, 2022
Merged

Removed explicit dependency on log4j #1563

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2994831
Removed explicit dependency on log4j
Jan 6, 2022
63915b5
Removed extra space
Jan 6, 2022
482855f
Replaced log4j with slf4j-api
Jan 6, 2022
0a81c13
Adding log4j-core as test dependency
Jan 7, 2022
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
34 changes: 0 additions & 34 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@
<!-- deps -->
<netty-native.version>2.0.25.Final</netty-native.version>
<bc.version>1.67</bc.version>
<log4j.version>2.17.1</log4j.version>
<guava.version>25.1-jre</guava.version>
<commons.cli.version>1.3.1</commons.cli.version>
<jackson-databind.version>2.11.2</jackson-databind.version>
Expand Down Expand Up @@ -161,10 +160,6 @@
<artifactId>jts</artifactId>
<groupId>com.vividsolutions</groupId>
</exclusion>
<exclusion>
<artifactId>log4j-api</artifactId>
<groupId>org.apache.logging.log4j</groupId>
</exclusion>
<exclusion>
<artifactId>spatial4j</artifactId>
<groupId>org.locationtech.spatial4j</groupId>
Expand Down Expand Up @@ -207,13 +202,6 @@
<scope>provided</scope>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<version>${log4j.version}</version>
<scope>provided</scope>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
Expand All @@ -226,18 +214,6 @@
</exclusions>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<version>${log4j.version}</version>
<exclusions>
<exclusion>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
<groupId>org.ldaptive</groupId>
<artifactId>ldaptive</artifactId>
Expand All @@ -247,10 +223,6 @@
<artifactId>commons-cli</artifactId>
<groupId>commons-cli</groupId>
</exclusion>
<exclusion>
<artifactId>slf4j-api</artifactId>
<groupId>org.slf4j</groupId>
</exclusion>
</exclusions>
</dependency>

Expand Down Expand Up @@ -341,12 +313,6 @@
<groupId>org.apache.kafka</groupId>
<artifactId>kafka-clients</artifactId>
<version>${kafka.version}</version>
<exclusions>
<exclusion>
<artifactId>slf4j-api</artifactId>
<groupId>org.slf4j</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>com.onelogin</groupId>
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/com/amazon/dlic/auth/ldap/srv/LdapServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
import java.util.concurrent.locks.ReentrantLock;

import org.apache.commons.lang3.exception.ExceptionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.apache.logging.log4j.LogManager;
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using Log4j explicitly here? You could still use SLF4J and swap out the underlying logger, even using the slf4j-simple if desired for tests.

Copy link
Author

Choose a reason for hiding this comment

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

The SLF4J was added explicitly in this plugin and is not provided from OpenSearch. Only Log4J is. The purpose of this PR is to clear any explicit logger dependencies in our POM and use whatever is provided in core. That's why I switched to Log4J

Copy link
Member

Choose a reason for hiding this comment

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

Just chiming in from the peanut gallery, but my two cents is that is generally better to declare explicit dependencies on things that you use as opposed to implicitly using transitive dependencies. In this case, I would think the ideal solution is like what @dlvenable mentioned, which is to declare an explicit dependency on a thin API (something like slf4j or log4j-api) and use classes defined in that API while letting core bring in the actual concrete implementation.

Copy link
Member

Choose a reason for hiding this comment

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

So first, I do see that this is a test. It does seem that if you really want to use log4j here, you should probably include log4j as an explicit test dependency in the Maven pom file. But, I don't think any of us want that. :)

I suggest that you add slf4j-api as an explicit dependency for this project in the Maven pom. Then use only that in code. You can get an slf4j implementation from OpenSearch core which will work for your tests when they run.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @dlvenable and @andrross for the help here! It turns out these are not just test dependencies, they are used all over (100+ files). I've taken a stab at replacing our uses with SLF4J... it builds locally, now letting it run the whole CI suite.

Unrelated, this CI job has been failing running out of memory... first time I see this (or at least a few times in a row), and not really sure this PR is the culprit...

import org.apache.logging.log4j.Logger;

import org.opensearch.security.test.helper.file.FileHelper;
import org.opensearch.security.test.helper.network.SocketUtils;
Expand All @@ -48,7 +48,7 @@
import com.unboundid.util.ssl.TrustStoreTrustManager;

final class LdapServer {
private final static Logger LOG = LoggerFactory.getLogger(LdapServer.class);
private final static Logger LOG = LogManager.getLogger(LdapServer.class);

private static final int LOCK_TIMEOUT = 60;
private static final TimeUnit TIME_UNIT = TimeUnit.SECONDS;
Expand Down
9 changes: 0 additions & 9 deletions src/test/java/org/opensearch/security/auditlog/impl/TracingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@
import org.opensearch.common.settings.Settings;
import org.opensearch.common.xcontent.XContentType;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.Logger;
import org.junit.Assert;
import org.junit.Test;

Expand All @@ -54,11 +51,6 @@ protected String getResourceFolder() {

@Test
public void testHTTPTrace() throws Exception {

Logger logger = (Logger) LogManager.getLogger("opendistro_security_action_trace");
final Level level = logger.getLevel();
logger.setLevel(Level.TRACE);

final Settings settings = Settings.builder()
.put(ConfigConstants.SECURITY_AUDIT_TYPE_DEFAULT, "debug")
.put(ConfigConstants.OPENDISTRO_SECURITY_AUDIT_RESOLVE_BULK_REQUESTS, "true")
Expand Down Expand Up @@ -228,7 +220,6 @@ public void testHTTPTrace() throws Exception {
"}";

System.out.println(rh.executePostRequest("a/b/_delete_by_query", dbqBody, encodeBasicHeader("admin", "admin")));
logger.setLevel(level);
Thread.sleep(5000);
}

Expand Down