[Backport 2.x] Clean up sysouts and replace with logger statements wh…

…ere necessary (opensearch-project#3231) (opensearch-project#3240) Backports opensearch-project#3231. Needed manual backport because of http5 imports and server `shutdown` (2.x) vs `awaitTermination` (main) method calls in test. Signed-off-by: Darshit Chanpura <[email protected]>
cwperks · Aug 25, 2023 · d0544cf · d0544cf
1 parent abeedb1
commit d0544cf
Show file tree

Hide file tree

Showing 71 changed files with 672 additions and 1,013 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
@@ -215,4 +215,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
@@ -253,8 +253,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
@@ -173,8 +173,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
@@ -1649,7 +1649,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;
         }
 
@@ -1866,7 +1865,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) {}
         }
     }
 
@@ -311,7 +309,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
@@ -53,9 +53,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()
         );
 
     }