CORDA-3307: Fix for underscore variables (#5682)

* Revert "Revert "CORDA-3307 - add support for environment variables in linux (#5523)" (#5643)"

This reverts commit 03ab258fc2.

* Env variables with underscore are now validated using schema validation and checking for unknown key errors.

* Resolving comments from PR review.

* Fix for deprecated import.

* Reworked logic according to PR review.

* Resolved bad string parsing problems where the json structure could be broken if some symbols were included in the key or value.

docs/source/changelog.rst

@@ -88,6 +88,9 @@ Unreleased
   Note that it's a responsibility of a client application to handle RPC reconnection in case this happens.
   See :ref:`setting_jvm_args` and :ref:`memory_usage_and_tuning` for further details.
 
+* Environment variables and system properties can now be provided with underscore separators instead of dots. Neither are case sensitive.
+  See :ref:`overriding config values <corda_configuration_file_overriding_config>` for more information.
+
 .. _changelog_v4.1:
 
 Version 4.1

docs/source/corda-configuration-file.rst

@@ -39,6 +39,8 @@ To alter this behaviour, the ``on-unknown-config-keys`` command-line argument ca
 Overriding values from node.conf
 --------------------------------
 
+.. _corda_configuration_file_overriding_config:
+
 Environment variables
   For example: ``${NODE_TRUST_STORE_PASSWORD}`` would be replaced by the contents of environment variable ``NODE_TRUST_STORE_PASSWORD`` (see: :ref:`hiding-sensitive-data` section).
 
@@ -54,6 +56,11 @@ JVM options
 .. note:: If the same field is overriden by both an environment variable and system property, the system property
    takes precedence.
 
+.. note:: Underscores can be used in instead of dots. For example overriding the ``p2pAddress`` with an environment variable can be done
+   by specifying ``CORDA_P2PADDRESS=host:port``. Variables and properties are not case sensitive. Corda will warn you if a variable
+   prefixed with ``CORDA`` cannot be mapped to a valid property. Shadowing occurs when two properties
+   of the same type with the same key are defined. For example having ``CORDA_P2PADDRESS=host:port`` and ``corda_p2paddress=host1:port1``
+   will raise an exception on startup. This is to prevent hard to spot mistakes.
 Configuration file fields
 -------------------------
 

node/src/integration-test/kotlin/net/corda/node/NodeConfigParsingTests.kt

@@ -0,0 +1,110 @@
+package net.corda.node
+
+import net.corda.core.utilities.getOrThrow
+import net.corda.node.logging.logFile
+import net.corda.testing.driver.DriverParameters
+import net.corda.testing.driver.NodeParameters
+import net.corda.testing.driver.driver
+import net.corda.testing.driver.internal.incrementalPortAllocation
+import org.assertj.core.api.Assertions.assertThatThrownBy
+import org.junit.Test
+import org.junit.Assert.assertTrue
+
+
+class NodeConfigParsingTests {
+
+    @Test
+    fun `config is overriden by underscore variable`() {
+        val portAllocator = incrementalPortAllocation()
+        val sshPort = portAllocator.nextPort()
+
+        driver(DriverParameters(
+                environmentVariables = mapOf("corda_sshd_port" to sshPort.toString()),
+                startNodesInProcess = false,
+                portAllocation = portAllocator)) {
+            val hasSsh = startNode().get()
+                    .logFile()
+                    .readLines()
+                    .filter { it.contains("SSH server listening on port") }
+                    .any { it.contains(sshPort.toString()) }
+            assertTrue(hasSsh)
+        }
+    }
+
+    @Test
+    fun `config is overriden by case insensitive underscore variable`() {
+        val portAllocator = incrementalPortAllocation()
+        val sshPort = portAllocator.nextPort()
+
+        driver(DriverParameters(
+                environmentVariables = mapOf("CORDA_sshd_port" to sshPort.toString()),
+                startNodesInProcess = false,
+                portAllocation = portAllocator)) {
+            val hasSsh = startNode().get()
+                    .logFile()
+                    .readLines()
+                    .filter { it.contains("SSH server listening on port") }
+                    .any { it.contains(sshPort.toString()) }
+            assertTrue(hasSsh)
+        }
+    }
+
+    @Test
+    fun `config is overriden by case insensitive dot variable`() {
+        val portAllocator = incrementalPortAllocation()
+        val sshPort = portAllocator.nextPort()
+
+        driver(DriverParameters(
+                environmentVariables = mapOf("CORDA.sshd.port" to sshPort.toString(),
+                                             "corda.devMode" to true.toString()),
+                startNodesInProcess = false,
+                portAllocation = portAllocator)) {
+            val hasSsh = startNode(NodeParameters()).get()
+                    .logFile()
+                    .readLines()
+                    .filter { it.contains("SSH server listening on port") }
+                    .any { it.contains(sshPort.toString()) }
+            assertTrue(hasSsh)
+        }
+    }
+
+    @Test
+    fun `shadowing is forbidden`() {
+        val portAllocator = incrementalPortAllocation()
+        val sshPort = portAllocator.nextPort()
+
+        driver(DriverParameters(
+                environmentVariables = mapOf(
+                        "CORDA_sshd_port" to sshPort.toString(),
+                        "corda.sshd.port" to sshPort.toString()),
+                startNodesInProcess = false,
+                portAllocation = portAllocator,
+                notarySpecs = emptyList())) {
+
+            assertThatThrownBy {
+                startNode().getOrThrow()
+            }
+        }
+    }
+
+    @Test
+    fun `bad keys are ignored and warned for`() {
+        val portAllocator = incrementalPortAllocation()
+        driver(DriverParameters(
+                environmentVariables = mapOf(
+                        "corda_bad_key" to "2077"),
+                startNodesInProcess = false,
+                portAllocation = portAllocator,
+                notarySpecs = emptyList())) {
+
+                val hasWarning = startNode()
+                        .getOrThrow()
+                        .logFile()
+                        .readLines()
+                        .any {
+                            it.contains("(property or environment variable) cannot be mapped to an existing Corda")
+                        }
+                assertTrue(hasWarning)
+        }
+    }
+}

node/src/main/kotlin/net/corda/node/services/config/ConfigUtilities.kt

@@ -4,10 +4,13 @@ import com.typesafe.config.Config
 import com.typesafe.config.ConfigFactory
 import com.typesafe.config.ConfigParseOptions
 import net.corda.cliutils.CordaSystemUtils
+import net.corda.common.configuration.parsing.internal.Configuration
 import net.corda.core.identity.CordaX500Name
 import net.corda.core.internal.createDirectories
 import net.corda.core.internal.div
 import net.corda.core.internal.exists
+import net.corda.node.internal.Node
+import net.corda.node.services.config.schema.v1.V1NodeConfigurationSpec
 import net.corda.nodeapi.internal.DEV_CA_KEY_STORE_PASS
 import net.corda.nodeapi.internal.config.FileBasedCertificateStoreSupplier
 import net.corda.nodeapi.internal.config.MutualSslConfiguration
@@ -27,6 +30,7 @@ operator fun Config.plus(overrides: Map<String, Any?>): Config = ConfigFactory.p
 object ConfigHelper {
 
     private const val CORDA_PROPERTY_PREFIX = "corda."
+    private const val UPPERCASE_PROPERTY_PREFIX = "CORDA."
 
     private val log = LoggerFactory.getLogger(javaClass)
     fun loadConfig(baseDirectory: Path,
@@ -68,10 +72,48 @@ object ConfigHelper {
     }
 
     private fun Config.cordaEntriesOnly(): Config {
-        return ConfigFactory.parseMap(toProperties()
-            .filterKeys { (it as String).startsWith(CORDA_PROPERTY_PREFIX) }
-            .mapKeys { (it.key as String).removePrefix(CORDA_PROPERTY_PREFIX) }
-        )
+        val cordaPropOccurrences = mutableSetOf<String>()
+        val badKeyConversions = mutableSetOf<String>()
+
+        return ConfigFactory.parseMap(
+                toProperties()
+                .mapKeys {
+                    var newKey = (it.key as String)
+                                .replace('_', '.')
+                                .replace(UPPERCASE_PROPERTY_PREFIX, CORDA_PROPERTY_PREFIX)
+
+                    if (!newKey.startsWith(CORDA_PROPERTY_PREFIX)) {
+                        return@mapKeys newKey
+                    }
+
+                    newKey = newKey.substring(CORDA_PROPERTY_PREFIX.length)
+
+                    if (cordaPropOccurrences.contains(newKey))
+                    {
+                        throw ShadowingException(it.key.toString(), newKey)
+                    }
+
+                    cordaPropOccurrences.add(newKey)
+
+                    newKey.let { key ->
+                        val cfg = ConfigFactory.parseMap(mapOf(key to it.value))
+                        val result = V1NodeConfigurationSpec.validate(cfg, Configuration.Validation.Options(strict = true))
+
+                        val isInvalidProperty = result.errors.any { err -> err is Configuration.Validation.Error.Unknown }
+                        if (isInvalidProperty) {
+                            Node.printWarning(
+                                    "${it.key} (property or environment variable) cannot be mapped to an existing Corda" +
+                                            " config property and thus won't be used as a config override!" +
+                                            " It won't be passed as a config override! If that was the intention " +
+                                            " double check the spelling and ensure there is such config key.")
+                            badKeyConversions.add(key)
+                        }
+
+                        CORDA_PROPERTY_PREFIX + key
+                    }
+                }.filterKeys { it.startsWith(CORDA_PROPERTY_PREFIX) }
+                .mapKeys { it.key.substring(CORDA_PROPERTY_PREFIX.length) }
+                .filterKeys { !badKeyConversions.contains(it) })
     }
 }
 

node/src/main/kotlin/net/corda/node/services/config/ShadowingException.kt

@@ -0,0 +1,7 @@
+package net.corda.node.services.config
+
+import com.typesafe.config.ConfigException
+
+class ShadowingException(definedProperty : String, convertedProperty : String)
+    : ConfigException(
+        "Environment variable $definedProperty is shadowing another property transformed to $convertedProperty")

testing/node-driver/src/main/kotlin/net/corda/testing/driver/Driver.kt

@@ -201,7 +201,8 @@ fun <A> driver(defaultParameters: DriverParameters = DriverParameters(), dsl: Dr
                     inMemoryDB = defaultParameters.inMemoryDB,
                     cordappsForAllNodes = uncheckedCast(defaultParameters.cordappsForAllNodes),
                     djvmBootstrapSource = defaultParameters.djvmBootstrapSource,
-                    djvmCordaSource = defaultParameters.djvmCordaSource
+                    djvmCordaSource = defaultParameters.djvmCordaSource,
+                    environmentVariables = defaultParameters.environmentVariables
             ),
             coerce = { it },
             dsl = dsl
@@ -261,7 +262,8 @@ data class DriverParameters(
         val inMemoryDB: Boolean = true,
         val cordappsForAllNodes: Collection<TestCordapp>? = null,
         val djvmBootstrapSource: Path? = null,
-        val djvmCordaSource: List<Path> = emptyList()
+        val djvmCordaSource: List<Path> = emptyList(),
+        val environmentVariables : Map<String, String> = emptyMap()
 ) {
     constructor(cordappsForAllNodes: Collection<TestCordapp>) : this(isDebug = false, cordappsForAllNodes = cordappsForAllNodes)
 
@@ -301,7 +303,8 @@ data class DriverParameters(
 
             // These fields have been added in v4.4
             djvmBootstrapSource = null,
-            djvmCordaSource = emptyList()
+            djvmCordaSource = emptyList(),
+            environmentVariables = emptyMap()
     )
 
     constructor(
@@ -420,6 +423,7 @@ data class DriverParameters(
     fun withCordappsForAllNodes(cordappsForAllNodes: Collection<TestCordapp>?): DriverParameters = copy(cordappsForAllNodes = cordappsForAllNodes)
     fun withDjvmBootstrapSource(djvmBootstrapSource: Path?): DriverParameters = copy(djvmBootstrapSource = djvmBootstrapSource)
     fun withDjvmCordaSource(djvmCordaSource: List<Path>): DriverParameters = copy(djvmCordaSource = djvmCordaSource)
+    fun withEnvironmentVariables(variables : Map<String, String>): DriverParameters = copy(environmentVariables = variables)
 
     fun copy(
             isDebug: Boolean,
@@ -515,9 +519,9 @@ data class DriverParameters(
             notaryCustomOverrides = notaryCustomOverrides,
             inMemoryDB = inMemoryDB,
             cordappsForAllNodes = cordappsForAllNodes,
-
             // These fields have been added in v4.4
             djvmBootstrapSource = djvmBootstrapSource,
-            djvmCordaSource = djvmCordaSource
+            djvmCordaSource = djvmCordaSource,
+            environmentVariables = environmentVariables
     )
 }

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/DriverDSLImpl.kt

@@ -122,7 +122,8 @@ class DriverDSLImpl(
         val inMemoryDB: Boolean,
         val cordappsForAllNodes: Collection<TestCordappInternal>?,
         val djvmBootstrapSource: Path?,
-        val djvmCordaSource: List<Path>
+        val djvmCordaSource: List<Path>,
+        val environmentVariables : Map<String, String>
 ) : InternalDriverDSL {
 
     private var _executorService: ScheduledExecutorService? = null
@@ -335,9 +336,11 @@ class DriverDSLImpl(
         } else {
             startOutOfProcessMiniNode(
                     config,
+                    arrayOf(
                         "initial-registration",
                         "--network-root-truststore=${rootTruststorePath.toAbsolutePath()}",
                         "--network-root-truststore-password=$rootTruststorePassword"
+                    )
             ).map { config }
         }
     }
@@ -504,7 +507,7 @@ class DriverDSLImpl(
             } else {
                 // TODO The config we use here is uses a hardocded p2p port which changes when the node is run proper
                 // This causes two node info files to be generated.
-                startOutOfProcessMiniNode(config, "generate-node-info").map {
+                startOutOfProcessMiniNode(config, arrayOf("generate-node-info")).map {
                     // Once done we have to read the signed node info file that's been generated
                     val nodeInfoFile = config.corda.baseDirectory.list { paths ->
                         paths.filter { it.fileName.toString().startsWith(NodeInfoFilesCopier.NODE_INFO_FILE_NAME_PREFIX) }.findFirst().get()
@@ -590,8 +593,7 @@ class DriverDSLImpl(
      * Start the node with the given flag which is expected to start the node for some function, which once complete will
      * terminate the node.
      */
-    @Suppress("SpreadOperator")
-    private fun startOutOfProcessMiniNode(config: NodeConfig, vararg extraCmdLineFlag: String): CordaFuture<Unit> {
+    private fun startOutOfProcessMiniNode(config: NodeConfig, extraCmdLineFlag: Array<String> = emptyArray()): CordaFuture<Unit> {
         val debugPort = if (isDebug) debugPortAllocation.nextPort() else null
         val process = startOutOfProcessNode(
                 config,
@@ -603,7 +605,8 @@ class DriverDSLImpl(
                 "512m",
                 null,
                 ZonedDateTime.now().format(DateTimeFormatter.ofPattern("HHmmss.SSS")),
-            *extraCmdLineFlag
+                environmentVariables,
+                extraCmdLineFlag
         )
 
         return poll(executorService, "$extraCmdLineFlag (${config.corda.myLegalName})") {
@@ -672,7 +675,8 @@ class DriverDSLImpl(
                     systemProperties,
                     parameters.maximumHeapSize,
                     parameters.logLevelOverride,
-                identifier
+                    identifier,
+                    environmentVariables
             )
 
             // Destroy the child process when the parent exits.This is needed even when `waitForAllNodesToFinish` is
@@ -847,7 +851,8 @@ class DriverDSLImpl(
                 maximumHeapSize: String,
                 logLevelOverride: String?,
                 identifier: String,
-            vararg extraCmdLineFlag: String
+                environmentVariables : Map<String,String>,
+                extraCmdLineFlag: Array<String> = emptyArray()
         ): Process {
             log.info("Starting out-of-process Node ${config.corda.myLegalName.organisation}, " +
                     "debug port is " + (debugPort ?: "not enabled") + ", " +
@@ -892,7 +897,7 @@ class DriverDSLImpl(
                     "--base-directory=${config.corda.baseDirectory}",
                     "--logging-level=$loggingLevel",
                     "--no-local-shell").also {
-                it += extraCmdLineFlag
+                it.addAll(extraCmdLineFlag)
             }.toList()
 
             val bytemanJvmArgs = {
@@ -927,7 +932,8 @@ class DriverDSLImpl(
                     workingDirectory = config.corda.baseDirectory,
                     maximumHeapSize = maximumHeapSize,
                     classPath = cp,
-                identifier = identifier
+                    identifier = identifier,
+                    environmentVariables = environmentVariables
             )
         }
 
@@ -1155,7 +1161,8 @@ fun <DI : DriverDSL, D : InternalDriverDSL, A> genericDriver(
                 inMemoryDB = defaultParameters.inMemoryDB,
                 cordappsForAllNodes = uncheckedCast(defaultParameters.cordappsForAllNodes),
                 djvmBootstrapSource = defaultParameters.djvmBootstrapSource,
-                    djvmCordaSource = defaultParameters.djvmCordaSource
+                djvmCordaSource = defaultParameters.djvmCordaSource,
+                environmentVariables = defaultParameters.environmentVariables
             )
     )
     val shutdownHook = addShutdownHook(driverDsl::shutdown)
@@ -1252,6 +1259,7 @@ fun <A> internalDriver(
         cordappsForAllNodes: Collection<TestCordappInternal>? = null,
         djvmBootstrapSource: Path? = null,
         djvmCordaSource: List<Path> = emptyList(),
+        environmentVariables: Map<String, String> = emptyMap(),
         dsl: DriverDSLImpl.() -> A
 ): A {
     return genericDriver(
@@ -1273,7 +1281,8 @@ fun <A> internalDriver(
                 inMemoryDB = inMemoryDB,
                 cordappsForAllNodes = cordappsForAllNodes,
                 djvmBootstrapSource = djvmBootstrapSource,
-                    djvmCordaSource = djvmCordaSource
+                djvmCordaSource = djvmCordaSource,
+                environmentVariables = environmentVariables
             ),
             coerce = { it },
             dsl = dsl

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/ProcessUtilities.kt

@@ -5,15 +5,26 @@ import java.io.File
 import java.nio.file.Path
 
 object ProcessUtilities {
+    @Suppress("LongParameterList")
     inline fun <reified C : Any> startJavaProcess(
             arguments: List<String>,
             classPath: List<String> = defaultClassPath,
             workingDirectory: Path? = null,
             jdwpPort: Int? = null,
             extraJvmArguments: List<String> = emptyList(),
-            maximumHeapSize: String? = null
+            maximumHeapSize: String? = null,
+            environmentVariables: Map<String, String> = emptyMap()
     ): Process {
-        return startJavaProcess(C::class.java.name, arguments, classPath, workingDirectory, jdwpPort, extraJvmArguments, maximumHeapSize)
+        return startJavaProcess(
+                C::class.java.name,
+                arguments,
+                classPath,
+                workingDirectory,
+                jdwpPort,
+                extraJvmArguments,
+                maximumHeapSize,
+                environmentVariables = environmentVariables
+        )
     }
 
     @Suppress("LongParameterList")
@@ -25,7 +36,8 @@ object ProcessUtilities {
             jdwpPort: Int? = null,
             extraJvmArguments: List<String> = emptyList(),
             maximumHeapSize: String? = null,
-            identifier: String = ""
+            identifier: String = "",
+            environmentVariables: Map<String,String> = emptyMap()
     ): Process {
         val command = mutableListOf<String>().apply {
             add(javaPath)
@@ -38,6 +50,7 @@ object ProcessUtilities {
         }
         return ProcessBuilder(command).apply {
             inheritIO()
+            environment().putAll(environmentVariables)
             environment()["CLASSPATH"] = classPath.joinToString(File.pathSeparator)
             if (workingDirectory != null) {
                 // An identifier may be handy if the same process started, killed and then re-started. Without the identifier

testing/node-driver/src/main/kotlin/net/corda/testing/node/internal/RPCDriver.kt

@@ -124,6 +124,7 @@ fun <A> rpcDriver(
         cordappsForAllNodes: Collection<TestCordappInternal>? = null,
         djvmBootstrapSource: Path? = null,
         djvmCordaSource: List<Path> = emptyList(),
+        environmentVariables: Map<String, String> = emptyMap(),
         dsl: RPCDriverDSL.() -> A
 ): A {
     return genericDriver(
@@ -146,7 +147,8 @@ fun <A> rpcDriver(
                             inMemoryDB = inMemoryDB,
                             cordappsForAllNodes = cordappsForAllNodes,
                             djvmBootstrapSource = djvmBootstrapSource,
-                            djvmCordaSource = djvmCordaSource
+                            djvmCordaSource = djvmCordaSource,
+                            environmentVariables = environmentVariables
                     ), externalTrace
             ),
             coerce = { it },