Merge pull request #39 from newrelic/newrelic/ignore_dot_delimited_en…

…vironment_variables

🌶  Ignore sysprop formatted envvars and log the correct variable name

newrelic-agent/src/main/java/com/newrelic/agent/config/SystemPropertyProvider.java

@@ -24,7 +24,6 @@ public class SystemPropertyProvider {
     private static final String LOG_FILE_NAME = AgentConfigImpl.SYSTEM_PROPERTY_ROOT + AgentConfigImpl.LOG_FILE_NAME;
     private static final String NEW_RELIC_SYSTEM_PROPERTY_ROOT = "newrelic.";
 
-
     private final Map<String, String> envVars;
     private final Map<String, String> envVarToSystemPropKeyMap;
     private final Map<String, String> newRelicSystemProps;
@@ -85,7 +84,8 @@ private Map<String, Object> createNewRelicSystemPropertiesWithoutPrefix() {
         addNewRelicSystemProperties(nrProps, systemProps.getAllSystemProperties());
         return Collections.unmodifiableMap(nrProps);
     }
-    private Map<String, Object> createNewRelicEnvVarsWithoutPrefix(){
+
+    private Map<String, Object> createNewRelicEnvVarsWithoutPrefix() {
         Map<String, Object> nrEnv = new HashMap<>();
         addNewRelicEnvProperties(nrEnv, environmentFacade.getAllEnvProperties());
         return Collections.unmodifiableMap(nrEnv);
@@ -110,12 +110,15 @@ private void addNewRelicEnvProperties(Map<String, Object> nrProps, Map<String, S
                 } else {
                     addPropertyWithoutEnvPrefix(nrProps, envVar.toLowerCase(), entry.getValue());
                 }
-            } else if (envVar.startsWith(AgentConfigImpl.SYSTEM_PROPERTY_ROOT)){
-                //because some newrelic.config properties get passed as environment variables
-                addPropertyWithoutSystemPropRoot(nrProps, envVar, entry.getValue());
+            } else if (envVar.startsWith(AgentConfigImpl.SYSTEM_PROPERTY_ROOT)) {
+                Agent.LOG.log(Level.WARNING,
+                        "The agent only supports environment variable configurations consisting of" +
+                                " alphanumeric characters and underscores. Use {0} instead.",
+                        formatNewRelicEnvVarPrefix(envVar));
             }
         }
     }
+
     private void addPropertyWithoutEnvPrefix(Map<String, Object> nrProps, String key, Object value) {
         nrProps.put(key.substring(NEW_RELIC_PREFIX_ENV.length()), value);
     }
@@ -139,21 +142,22 @@ private String getenv(String key) {
             return val;
         }
         //check if current key needs to be converted from NR config prop to NR env var
-        String removeConfigKey = addNewRelicEnvVarPrefix(key.replace("newrelic.config", "new.relic"));
-        return environmentFacade.getenv(removeConfigKey);
+
+        return environmentFacade.getenv(formatNewRelicEnvVarPrefix(key));
     }
 
-    private String addNewRelicEnvVarPrefix(String key) {
+    private String formatNewRelicEnvVarPrefix(String key) {
         // Replace any dots and dashes with underscores to allow config to be set as environment variables. We are replacing dashes here because
         // our instrumentation modules have dashes in their names and we want to be able to allow those to be disabled via environment variables.
-        return key.replaceAll("[.-]", "_").toUpperCase();
+        return key.replace("newrelic.config", "new.relic")
+                .replaceAll("[.-]", "_")
+                .toUpperCase();
     }
 
     public String getSystemProperty(String prop) {
         return systemProps.getSystemProperty(prop);
     }
 
-
     /**
      * Get a map of the New Relic system properties (any property starting with newrelic.)
      */

newrelic-agent/src/test/java/com/newrelic/agent/config/SystemPropertyProviderTest.java

@@ -16,6 +16,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 
 public class SystemPropertyProviderTest {
     @Test
@@ -50,25 +51,6 @@ public void testSystemPropertyProviderGeneralEnvVars() {
         assertEquals("logfile.log", provider.getNewRelicEnvVarsWithoutPrefix().get("log_file_name"));
     }
 
-    @Test
-    public void testSystemPropertyProviderGeneralEnvProps() {
-        //to cover for a case where config properties get passed as environment variables.
-        Map<String, String> envs = new HashMap<>(System.getenv());
-        envs.put("newrelic.config.process_host.display_name", "hello");
-        envs.put("newrelic.config.app_name", "people");
-        envs.put("newrelic.config.log_file_name", "logfile.log");
-
-        SystemPropertyProvider provider = new SystemPropertyProvider(
-                new SaveSystemPropertyProviderRule.TestSystemProps(),
-                new SaveSystemPropertyProviderRule.TestEnvironmentFacade(envs)
-        );
-
-        assertNotNull("Properties can not be null", provider.getNewRelicEnvVarsWithoutPrefix().get("process_host.display_name"));
-        assertEquals("hello", provider.getNewRelicEnvVarsWithoutPrefix().get("process_host.display_name"));
-        assertEquals("people", provider.getNewRelicEnvVarsWithoutPrefix().get("app_name"));
-        assertEquals("logfile.log", provider.getNewRelicEnvVarsWithoutPrefix().get("log_file_name"));
-    }
-
     @Test
     public void testEnvironmentVariable() {
         Map<String, String> envs = new HashMap<>();
@@ -89,7 +71,8 @@ public void testEnvironmentVariable() {
         assertEquals("hello", provider.getEnvironmentVariable("NEW_RELIC_PROCESS_HOST_DISPLAY_NAME"));
         assertEquals("people", provider.getEnvironmentVariable("NEW_RELIC_APP_NAME"));
         assertEquals("10.96.0.1", provider.getEnvironmentVariable("KUBERNETES_SERVICE_HOST"));
-        assertEquals("true", provider.getEnvironmentVariable("newrelic.config.distributed_tracing.enabled"));
+        // Environment variables with system property style dot notation should not be read by the agent
+        assertNull(provider.getNewRelicEnvVarsWithoutPrefix().get("distributed_tracing.enabled"));
     }
 
     @Test