Clean up sysouts and replace with logger statements where necessary (o…

…pensearch-project#3231) A lot of files (mostly tests) have sysout's which do nothing but degrade the performance. This PR removes all sysouts from tests, replace sysouts with logger in project files. Also removes all e.printStacktrace(); calls. - `tools/` package - no sysouts were removed from here - HTTPSpnegoAuthenticator.java - a portion was not modified since it enabled debugging on console. Signed-off-by: Darshit Chanpura <[email protected]>
DarshitChanpura · Aug 23, 2023 · 683c4f5 · 683c4f5
1 parent 75f529a
commit 683c4f5
Show file tree

Hide file tree

Showing 70 changed files with 691 additions and 1,011 deletions.
diff --git a/build.gradle b/build.gradle
@@ -323,6 +323,7 @@ jacocoTestReport {
 
 checkstyle {
     configFile file("checkstyle/sun_checks.xml")
+    configFile file("checkstyle/println_checks.xml")
 }
 
 opensearchplugin {

diff --git a/checkstyle/println_checks.xml b/checkstyle/println_checks.xml
@@ -0,0 +1,21 @@
+<?xml version="1.0"?>
+<!DOCTYPE module PUBLIC
+        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+        "https://checkstyle.org/dtds/configuration_1_3.dtd">
+
+<module name="Checker">
+    <module name="BeforeExecutionExclusionFileFilter">
+        <property name="fileNamePattern" value="src/main/java/org/opensearch/security/tools/*"/>
+    </module>
+    <module name="BeforeExecutionExclusionFileFilter">
+        <property name="fileNamePattern" value="src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java"/>
+    </module>
+    <module name="TreeWalker">
+        <module name="RegexpSinglelineJava">
+            <property name="format" value="System.out.println"/>
+            <property name="ignoreCase" value="true"/>
+            <property name="message" value="Do not use System.out.println" />
+            <property name="severity" value="error"/>
+        </module>
+    </module>
+</module>
diff --git a/checkstyle/sun_checks.xml b/checkstyle/sun_checks.xml
@@ -222,4 +222,5 @@
     <property name="checkFormat" value="$1"/>
   </module>
 
+  <module name="PrintlnModule"/>
 </module>
diff --git a/src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java
@@ -98,8 +98,7 @@ public Void run() {
                         }
                     } catch (Throwable e) {
                         log.error("Unable to enable krb_debug due to ", e);
-                        System.err.println("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
-                        System.out.println("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
+                        log.debug("Unable to enable krb_debug due to " + ExceptionsHelper.stackTrace(e));
                     }
 
                     System.setProperty(KrbConstants.USE_SUBJECT_CREDS_ONLY_PROP, "false");

diff --git a/src/main/java/org/opensearch/security/auditlog/sink/DebugSink.java b/src/main/java/org/opensearch/security/auditlog/sink/DebugSink.java
@@ -11,11 +11,15 @@
 
 package org.opensearch.security.auditlog.sink;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.security.auditlog.impl.AuditMessage;
 
 public final class DebugSink extends AuditLogSink {
 
+    final Logger log = LogManager.getLogger(DebugSink.class);
+
     public DebugSink(String name, Settings settings, AuditLogSink fallbackSink) {
         super(name, settings, null, fallbackSink);
     }
@@ -27,7 +31,7 @@ public boolean isHandlingBackpressure() {
 
     @Override
     public boolean doStore(final AuditMessage msg) {
-        System.out.println("AUDIT_LOG: " + msg.toPrettyString());
+        log.info("AUDIT_LOG: " + msg.toPrettyString());
         return true;
     }
 

diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
@@ -244,8 +244,6 @@ public void onChange(Map<CType, SecurityDynamicConfiguration<?>> typeToConfig) {
             log.debug("Static roles loaded ({})", staticRoles.getCEntries().size());
 
             if (actionGroups.containsAny(staticActionGroups)) {
-                System.out.println("static: " + actionGroups.getCEntries());
-                System.out.println("Static Action Groups:" + staticActionGroups.getCEntries());
                 throw new StaticResourceException("Cannot override static action groups");
             }
             if (!actionGroups.add(staticActionGroups) && !staticActionGroups.getCEntries().isEmpty()) {

diff --git a/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java b/src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
@@ -158,8 +158,6 @@ public Object run() {
             final String renegoMsg =
                 "Client side initiated TLS renegotiation enabled. This can open a vulnerablity for DoS attacks through client side initiated TLS renegotiation.";
             log.warn(renegoMsg);
-            System.out.println(renegoMsg);
-            System.err.println(renegoMsg);
         } else {
             if (!rejectClientInitiatedRenegotiation) {
 
@@ -225,8 +223,6 @@ public Object run() {
 
         if (!httpSSLEnabled && !transportSSLEnabled) {
             log.error("SSL not activated for http and/or transport.");
-            System.out.println("SSL not activated for http and/or transport.");
-            System.err.println("SSL not activated for http and/or transport.");
         }
 
         if (ExternalSecurityKeyStore.hasExternalSslContext(settings)) {

diff --git a/src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java b/src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java
@@ -175,8 +175,7 @@ public Boolean run() {
                     }
                     principal = principalExtractor == null ? null : principalExtractor.extractPrincipal(x509Certs[0], Type.HTTP);
                 } else if (engine.getNeedClientAuth()) {
-                    final OpenSearchException ex = new OpenSearchException("No client certificates found but such are needed (SG 9).");
-                    throw ex;
+                    throw new OpenSearchException("No client certificates found but such are needed (SG 9).");
                 }
 
             } catch (final SSLPeerUnverifiedException e) {

diff --git a/src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java b/src/main/java/org/opensearch/security/tools/AuditConfigMigrater.java
@@ -132,7 +132,6 @@ public static void main(String[] args) {
                     + " Please remove the deprecated keys from your opensearch.yml or replace with the generated file after reviewing."
             );
         } catch (final Exception e) {
-            e.printStackTrace();
             formatter.printHelp("audit_config_migrater.sh", options, true);
             System.exit(-1);
         }

diff --git a/src/main/java/org/opensearch/security/tools/Migrater.java b/src/main/java/org/opensearch/security/tools/Migrater.java
@@ -190,7 +190,6 @@ private static boolean backupAndWrite(File file, SecurityDynamicConfiguration<?>
         } catch (Exception e) {
             System.out.println("Unable to write " + file.getAbsolutePath() + ". This is unexpected and we will abort migration.");
             System.out.println("    Details: " + e.getMessage());
-            e.printStackTrace();
         }
 
         return false;

diff --git a/src/main/java/org/opensearch/security/tools/SecurityAdmin.java b/src/main/java/org/opensearch/security/tools/SecurityAdmin.java
@@ -1657,7 +1657,6 @@ private static int migrate(RestHighLevelClient tc, String index, File backupDir,
 
         } catch (Exception e) {
             System.out.println("ERR: Unable to migrate config files due to " + e);
-            e.printStackTrace();
             return -1;
         }
 
@@ -1890,7 +1889,6 @@ public InputStream openStream() throws IOException {
 
             return value;
         } catch (Exception e) {
-            e.printStackTrace();
             return "ERR: Unable to handle response due to " + e;
         }
     }

diff --git a/...a/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java b/...a/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java
@@ -37,9 +37,7 @@ public static void tearDown() {
         if (mockIdpServer != null) {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception e) {}
         }
     }
 
@@ -314,7 +312,6 @@ public void testNbfInSkew() throws Exception {
 
         long expiringDate = 20 + System.currentTimeMillis() / 1000;
         long notBeforeDate = 5 + System.currentTimeMillis() / 1000;
-        ;
 
         AuthCredentials creds = jwtAuth.extractCredentials(
             new FakeRestRequest(

diff --git a/src/test/java/com/amazon/dlic/auth/http/jwt/keybyoidc/KeySetRetrieverTest.java b/src/test/java/com/amazon/dlic/auth/http/jwt/keybyoidc/KeySetRetrieverTest.java
@@ -55,9 +55,7 @@ public static void tearDown() {
         if (mockIdpServer != null) {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 

diff --git a/...zon/dlic/auth/http/jwt/keybyoidc/SingleKeyHTTPJwtKeyByOpenIdConnectAuthenticatorTest.java b/...zon/dlic/auth/http/jwt/keybyoidc/SingleKeyHTTPJwtKeyByOpenIdConnectAuthenticatorTest.java
@@ -45,9 +45,7 @@ public void basicTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 
@@ -69,9 +67,7 @@ public void wrongSigTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 
@@ -96,9 +92,7 @@ public void noAlgTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 
@@ -120,9 +114,7 @@ public void mismatchedAlgTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 
@@ -174,9 +166,7 @@ public void keyExchangeTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception e) {}
         }
 
         mockIdpServer = new MockIpdServer(TestJwk.Jwks.RSA_2);
@@ -198,9 +188,7 @@ public void keyExchangeTest() throws Exception {
         } finally {
             try {
                 mockIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 

diff --git a/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java b/src/test/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticatorTest.java
@@ -112,9 +112,7 @@ public void tearDown() {
         if (mockSamlIdpServer != null) {
             try {
                 mockSamlIdpServer.close();
-            } catch (Exception e) {
-                e.printStackTrace();
-            }
+            } catch (Exception ignored) {}
         }
     }
 

diff --git a/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendIntegTest.java b/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendIntegTest.java
@@ -53,7 +53,6 @@ protected String getResourceFolder() {
     public void testIntegLdapAuthenticationSSL() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("jacksonm", "secret")).getStatusCode());
@@ -63,7 +62,6 @@ public void testIntegLdapAuthenticationSSL() throws Exception {
     public void testIntegLdapAuthenticationSSLFail() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("wrong", "wrong")).getStatusCode());
@@ -87,7 +85,6 @@ public void testAttributesWithImpersonation() throws Exception {
                 encodeBasicHeader("spock", "spocksecret")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
         Assert.assertTrue(res.getBody().contains("ldap.dn"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.entryDN"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.subschemaSubentry"));

diff --git a/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendTest.java b/src/test/java/com/amazon/dlic/auth/ldap/LdapBackendTest.java
@@ -1081,7 +1081,6 @@ public void testMultiCn() throws Exception {
         );
         Assert.assertNotNull(user);
         Assert.assertEquals("cn=cabc,ou=people,o=TEST", user.getName());
-        System.out.println(user.getUserEntry().getAttribute("cn"));
     }
 
     @AfterClass

diff --git a/src/test/java/com/amazon/dlic/auth/ldap2/LdapBackendIntegTest2.java b/src/test/java/com/amazon/dlic/auth/ldap2/LdapBackendIntegTest2.java
@@ -53,7 +53,6 @@ protected String getResourceFolder() {
     public void testIntegLdapAuthenticationSSL() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config_ldap2.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("jacksonm", "secret")).getStatusCode());
@@ -63,7 +62,6 @@ public void testIntegLdapAuthenticationSSL() throws Exception {
     public void testIntegLdapAuthenticationSSLFail() throws Exception {
         String securityConfigAsYamlString = FileHelper.loadFile("ldap/config_ldap2.yml");
         securityConfigAsYamlString = securityConfigAsYamlString.replace("${ldapsPort}", String.valueOf(ldapsPort));
-        System.out.println(securityConfigAsYamlString);
         setup(Settings.EMPTY, new DynamicSecurityConfig().setConfigAsYamlString(securityConfigAsYamlString), Settings.EMPTY);
         final RestHelper rh = nonSslRestHelper();
         Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("wrong", "wrong")).getStatusCode());
@@ -87,7 +85,6 @@ public void testAttributesWithImpersonation() throws Exception {
                 encodeBasicHeader("spock", "spocksecret")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
         Assert.assertTrue(res.getBody().contains("ldap.dn"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.entryDN"));
         Assert.assertTrue(res.getBody().contains("attr.ldap.subschemaSubentry"));

diff --git a/src/test/java/org/opensearch/security/AggregationTests.java b/src/test/java/org/opensearch/security/AggregationTests.java
@@ -114,7 +114,6 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("nagilum", "nagilum")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");
@@ -134,7 +133,6 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("nagilum", "nagilum")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");
@@ -154,7 +152,6 @@ public void testBasicAggregations() throws Exception {
                 encodeBasicHeader("worf", "worf")
             )).getStatusCode()
         );
-        System.out.println(res.getBody());
         assertNotContains(res, "*xception*");
         assertNotContains(res, "*erial*");
         assertNotContains(res, "*mpty*");
@@ -168,11 +165,11 @@ public void testBasicAggregations() throws Exception {
 
         Assert.assertEquals(
             HttpStatus.SC_FORBIDDEN,
-            (res = rh.executePostRequest(
+            rh.executePostRequest(
                 "_search?pretty",
                 "{\"size\":0,\"aggs\":{\"myindices\":{\"terms\":{\"field\":\"_index\",\"size\":40}}}}",
                 encodeBasicHeader("worf", "worf")
-            )).getStatusCode()
+            ).getStatusCode()
         );
 
     }