Merge pull request #6590 from IntersectMBO/mgalazyn/fix/grpc-config-reload

Fix node config reading in gRPC config reload on SIGHUP

Author: carbolymer

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

@@ -265,7 +265,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)
@@ -305,13 +305,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
@@ -439,7 +442,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
@@ -484,7 +487,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
@@ -579,6 +582,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))]
@@ -590,9 +594,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
@@ -609,7 +613,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
@@ -786,21 +790,18 @@ 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
+-- | 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, merged with re-read YAML on reload
                        -> 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           Ouroboros.Consensus.Node (NodeDatabasePaths (..))
 import           Ouroboros.Consensus.Node.Genesis (disableGenesisConfig)
 import           Ouroboros.Consensus.Storage.LedgerDB.Args
@@ -29,12 +31,14 @@ 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)
 
-import           Hedgehog (Property, discover, withTests, (===))
+import           Hedgehog (Property, discover, (===))
 import qualified Hedgehog
+import qualified Hedgehog.Extras as H
 import           Hedgehog.Internal.Property (evalEither, failWith)
 
 
@@ -45,7 +49,7 @@ import           Hedgehog.Internal.Property (evalEither, failWith)
 
 prop_sanityCheck_POM :: Property
 prop_sanityCheck_POM =
-   withTests 1 . Hedgehog.property $ do
+   H.propertyOnce $ do
     let combinedPartials = defaultPartialNodeConfiguration
                              <> testPartialYamlConfig
                              <> testPartialCliConfig
@@ -290,6 +294,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.
+-- Documents the config precedence that issue #6589 depends on: CLI flags
+-- must survive a YAML-only re-read.
+prop_rpcReload_cliEnabledYamlSilent :: Property
+prop_rpcReload_cliEnabledYamlSilent =
+  H.propertyOnce $ 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 =
+  H.propertyOnce $ 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 =
+  H.propertyOnce $ 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 =
+  H.propertyOnce $ 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