Fix node config reading in gRPC config reload on SIGHUP

cardano-node/src/Cardano/Node/Run.hs

@@ -281,7 +281,7 @@ handleNodeWithTracers cmdPc nc p@(SomeConsensusProtocol blockType runP) = do
                                       then DisabledBlockForging
                                       else EnabledBlockForging))
 
-      handleSimpleNode blockType runP tracers nc networkMagic
+      handleSimpleNode blockType runP tracers nc cmdPc networkMagic
         (\nk -> do
             setNodeKernel nodeKernelData nk
             traceWith (nodeStateTracer tracers) NodeKernelOnline)
@@ -327,7 +327,7 @@ handleNodeWithTracers cmdPc nc p@(SomeConsensusProtocol blockType runP) = do
 
       -- We ignore peer logging thread if it dies, but it will be killed
       -- when 'handleSimpleNode' terminates.
-      handleSimpleNode blockType runP tracers nc networkMagic
+      handleSimpleNode blockType runP tracers nc cmdPc networkMagic
         (\nk -> do
             setNodeKernel nodeKernelData nk
             traceWith (nodeStateTracer tracers) NodeKernelOnline)
@@ -400,13 +400,16 @@ handleSimpleNode
   -> Api.ProtocolInfoArgs blk
   -> Tracers RemoteAddress LocalAddress blk IO
   -> NodeConfiguration
+  -> PartialNodeConfiguration
+  -- ^ Original CLI configuration, used for SIGHUP config reload so CLI
+  -- overrides are preserved when re-reading the YAML file.
   -> NetworkMagic
   -> (NodeKernel IO RemoteAddress LocalConnectionId blk -> IO ())
   -- ^ Called on the 'NodeKernel' after creating it, but before the network
   -- layer is initialised.  This implies this function must not block,
   -- otherwise the node won't actually start.
   -> IO ()
-handleSimpleNode blockType runP tracers nc networkMagic onKernel = do
+handleSimpleNode blockType runP tracers nc cmdPc networkMagic onKernel = do
   logStartupWarnings
 
   logDeprecatedLedgerDBOptions
@@ -534,7 +537,7 @@ handleSimpleNode blockType runP tracers nc networkMagic onKernel = do
               (readTVar ledgerPeerSnapshotPathVar)
               (readTVar useLedgerVar)
               (writeTVar ledgerPeerSnapshotVar)
-            updateRpcConfiguration (startupTracer tracers) (ncConfigFile nc) rpcConfigVar
+            updateRpcConfiguration (startupTracer tracers) cmdPc rpcConfigVar
             traceWith (startupTracer tracers) (BlockForgingUpdate NotEffective)
           )
           Nothing
@@ -579,7 +582,7 @@ handleSimpleNode blockType runP tracers nc networkMagic onKernel = do
               rnNodeKernelHook = \registry nodeKernel -> do
                 -- reinstall `SIGHUP` handler
                 installSigHUPHandler (startupTracer tracers) (Consensus.kesAgentTracer $ consensusTracers tracers)
-                                     blockType nc networkMagic nodeKernel localRootsVar publicRootsVar useLedgerVar
+                                     blockType nc cmdPc networkMagic nodeKernel localRootsVar publicRootsVar useLedgerVar
                                      useBootstrapVar ledgerPeerSnapshotPathVar ledgerPeerSnapshotVar
                                      rpcConfigVar
                 rnNodeKernelHook nodeArgs registry nodeKernel
@@ -674,6 +677,7 @@ installSigHUPHandler :: Tracer IO (StartupTrace blk)
                      -> Tracer IO KESAgentClientTrace
                      -> Api.BlockType blk
                      -> NodeConfiguration
+                     -> PartialNodeConfiguration -- ^ original CLI configuration
                      -> NetworkMagic
                      -> NodeKernel IO RemoteAddress (ConnectionId LocalAddress) blk
                      -> StrictTVar IO [(HotValency, WarmValency, Map RelayAccessPoint (LocalRootConfig PeerTrustable))]
@@ -685,9 +689,9 @@ installSigHUPHandler :: Tracer IO (StartupTrace blk)
                      -> StrictTVar IO RpcConfig
                      -> IO ()
 #ifndef UNIX
-installSigHUPHandler _ _ _ _ _ _ _ _ _ _ _ _ _ = return ()
+installSigHUPHandler _ _ _ _ _ _ _ _ _ _ _ _ _ _ = return ()
 #else
-installSigHUPHandler startupTracer kesAgentTracer blockType nc networkMagic nodeKernel localRootsVar
+installSigHUPHandler startupTracer kesAgentTracer blockType nc cmdPc networkMagic nodeKernel localRootsVar
                      publicRootsVar useLedgerVar useBootstrapPeersVar ledgerPeerSnapshotPathVar ledgerPeerSnapshotVar
                      rpcConfigVar =
   void $ Signals.installHandler
@@ -704,7 +708,7 @@ installSigHUPHandler startupTracer kesAgentTracer blockType nc networkMagic node
                (readTVar ledgerPeerSnapshotPathVar)
                (readTVar useLedgerVar)
                (writeTVar ledgerPeerSnapshotVar)
-      updateRpcConfiguration startupTracer (ncConfigFile nc) rpcConfigVar
+      updateRpcConfiguration startupTracer cmdPc rpcConfigVar
     )
     Nothing
 #endif
@@ -881,21 +885,22 @@ rpcServerLoop startupTracer rpcTracer rpcConfigVar networkMagic = go
       atomically . modifyTVar rpcConfigVar $ \config -> config{isEnabled = Identity False}
 
 #ifdef UNIX
--- | Reload RPC configuration from the configuration file
-updateRpcConfiguration :: Tracer IO (StartupTrace blk) -- ^ tracer tracing the configuration reload
-                       -> ConfigYamlFilePath -- ^ node configuration file, to reload configuration from
-                       -> StrictTVar IO RpcConfig -- ^ TVar storing RPC configuration
+-- | Reload RPC configuration by re-reading the YAML config file and merging
+-- with CLI overrides, exactly like startup does via 'buildNodeConfiguration'.
+updateRpcConfiguration :: Tracer IO (StartupTrace blk)
+                       -- ^ tracer for configuration reload events
+                       -> PartialNodeConfiguration
+                       -- ^ original CLI configuration, so CLI overrides are
+                       -- preserved when re-reading the YAML file
+                       -> StrictTVar IO RpcConfig
+                       -- ^ TVar storing RPC configuration
                        -> IO ()
-updateRpcConfiguration tracer configFilePath rpcConfigVar = do
-  result <- fmap (join . first Exception.displayException)
-              . try @Exception.SomeException
-              . fmap makeNodeConfiguration
-              . parseNodeConfigurationFP
-              $ Just configFilePath
+updateRpcConfiguration tracer cmdPc rpcConfigVar = do
+  result <- try @Exception.SomeException $ buildNodeConfiguration cmdPc
   case result of
     Left err ->
       -- reload failure, we don't do anything this time
-      traceWith tracer (RpcConfigUpdateError $ pack err)
+      traceWith tracer (RpcConfigUpdateError $ pack (Exception.displayException err))
     Right NodeConfiguration{ncRpcConfig=newConfig} ->
       join . atomically $ do
         oldConfig <- readTVar rpcConfigVar

cardano-node/test/Test/Cardano/Node/POM.hs

@@ -8,6 +8,8 @@ module Test.Cardano.Node.POM
   ) where
 
 
+import           Cardano.Api (File (..))
+
 import           Cardano.Crypto.ProtocolMagic (RequiresNetworkMagic (..))
 import           Cardano.Network.ConsensusMode (ConsensusMode (..))
 import           Cardano.Network.Diffusion.Configuration (defaultNumberOfBigLedgerPeers)
@@ -18,7 +20,7 @@ import           Cardano.Node.Configuration.POM
 import           Cardano.Node.Configuration.Socket
 import           Cardano.Node.Handlers.Shutdown
 import           Cardano.Node.Types
-import           Cardano.Rpc.Server.Config (makeRpcConfig)
+import           Cardano.Rpc.Server.Config (RpcConfigF (..), makeRpcConfig)
 import           Cardano.Tracing.Config (PartialTraceOptions (..), defaultPartialTraceConfiguration,
                    partialTraceSelectionToEither)
 import           Ouroboros.Consensus.Node (NodeDatabasePaths (..))
@@ -31,6 +33,7 @@ import           Ouroboros.Network.PeerSelection.PeerSharing (PeerSharing (..))
 import           Ouroboros.Network.TxSubmission.Inbound.V2.Types
 
 import           Data.Bifunctor (first)
+import           Data.Functor.Identity (Identity (..))
 import           Data.Monoid (Last (..))
 import           Data.String
 import           Data.Text (Text)
@@ -303,6 +306,57 @@ eExpectedConfig = do
     , ncTxSubmissionInitDelay = defaultTxSubmissionInitDelay
     }
 
+-- A socket config with a node socket path, needed for RPC-enabled tests
+-- because makeRpcConfig validates that a node socket exists when RPC is enabled.
+testSocketConfigWithPath :: Last SocketConfig
+testSocketConfigWithPath =
+  Last . Just $ SocketConfig mempty mempty mempty (Last . Just $ File "node.socket")
+
+-- | CLI --grpc-enable + YAML silent -> RPC stays enabled.
+-- Verifies the fix for issue #6589: a node started with --grpc-enable must not
+-- have RPC silently disabled on SIGHUP.
+prop_rpcReload_cliEnabledYamlSilent :: Property
+prop_rpcReload_cliEnabledYamlSilent =
+  withTests 1 . Hedgehog.property $ do
+    let cliConfig = testPartialCliConfig
+          { pncRpcConfig = RpcConfig (Last (Just True)) mempty mempty
+          , pncSocketConfig = testSocketConfigWithPath
+          }
+        merged = defaultPartialNodeConfiguration <> testPartialYamlConfig <> cliConfig
+    NodeConfiguration{ncRpcConfig = RpcConfig{isEnabled}} <- evalEither $ makeNodeConfiguration merged
+    isEnabled === Identity True
+
+-- | CLI silent + YAML EnableRpc -> RPC enabled.
+prop_rpcReload_cliSilentYamlEnabled :: Property
+prop_rpcReload_cliSilentYamlEnabled =
+  withTests 1 . Hedgehog.property $ do
+    let yamlConfig = testPartialYamlConfig{pncRpcConfig = RpcConfig (Last (Just True)) mempty mempty}
+        cliConfig = testPartialCliConfig{pncSocketConfig = testSocketConfigWithPath}
+        merged = defaultPartialNodeConfiguration <> yamlConfig <> cliConfig
+    NodeConfiguration{ncRpcConfig = RpcConfig{isEnabled}} <- evalEither $ makeNodeConfiguration merged
+    isEnabled === Identity True
+
+-- | Both CLI and YAML silent -> RPC disabled (default).
+prop_rpcReload_bothSilent :: Property
+prop_rpcReload_bothSilent =
+  withTests 1 . Hedgehog.property $ do
+    let merged = defaultPartialNodeConfiguration <> testPartialYamlConfig <> testPartialCliConfig
+    NodeConfiguration{ncRpcConfig = RpcConfig{isEnabled}} <- evalEither $ makeNodeConfiguration merged
+    isEnabled === Identity False
+
+-- | CLI --grpc-enable wins over YAML EnableRpc: false.
+prop_rpcReload_cliOverridesYaml :: Property
+prop_rpcReload_cliOverridesYaml =
+  withTests 1 . Hedgehog.property $ do
+    let yamlConfig = testPartialYamlConfig{pncRpcConfig = RpcConfig (Last (Just False)) mempty mempty}
+        cliConfig = testPartialCliConfig
+          { pncRpcConfig = RpcConfig (Last (Just True)) mempty mempty
+          , pncSocketConfig = testSocketConfigWithPath
+          }
+        merged = defaultPartialNodeConfiguration <> yamlConfig <> cliConfig
+    NodeConfiguration{ncRpcConfig = RpcConfig{isEnabled}} <- evalEither $ makeNodeConfiguration merged
+    isEnabled === Identity True
+
 -- -----------------------------------------------------------------------------
 
 tests :: IO Bool