correct NOTIFY config reload dying on error

main/Main.hs

@@ -1,4 +1,5 @@
 {-# LANGUAGE CPP            #-}
+{-# LANGUAGE LambdaCase     #-}
 {-# LANGUAGE MultiWayIf     #-}
 {-# LANGUAGE NamedFieldPuns #-}
 
@@ -66,7 +67,7 @@ main = do
   CLI{cliCommand, cliPath} <- readCLIShowHelp env
 
   -- build the 'AppConfig' from the config file path
-  conf <- readValidateConfig mempty env cliPath
+  conf <- either panic identity <$> readConfig mempty env cliPath
 
   -- These are config values that can't be reloaded at runtime. Reloading some of them would imply restarting the web server.
   let
@@ -103,10 +104,10 @@ main = do
   -- Config that can change at runtime
   refConf <- newIORef conf
 
-  let configRereader = reReadConfig pool gucConfigEnabled env cliPath refConf
+  let configRereader startingUp = reReadConfig startingUp pool gucConfigEnabled env cliPath refConf
 
   -- re-read and override the config if db-load-guc-config is true
-  when gucConfigEnabled configRereader
+  when gucConfigEnabled $ configRereader True
 
   case cliCommand of
     CmdDumpConfig ->
@@ -148,13 +149,13 @@ main = do
 
   -- Re-read the config on SIGUSR2
   void $ installHandler sigUSR2 (
-    Catch $ configRereader >> putStrLn ("Config reloaded" :: Text)
+    Catch $ configRereader False
     ) Nothing
 #endif
 
   -- reload schema cache + config on NOTIFY
   when dbChannelEnabled $
-    listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker configRereader
+    listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker $ configRereader False
 
   -- ask for the OS time at most once per second
   getTime <- mkAutoUpdate defaultUpdateSettings {updateAction = getCurrentTime}
@@ -310,7 +311,7 @@ loadSchemaCache pool actualPgVersion refConf refDbStructure = do
   It uses the connectionWorker in case the LISTEN connection dies.
 -}
 listener :: ByteString -> Text -> P.Pool -> IORef AppConfig -> IORef (Maybe DbStructure) -> MVar ConnectionStatus -> IO () -> IO () -> IO ()
-listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker configRereader = start
+listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker configLoader = start
   where
     start = do
       connStatus <- takeMVar mvarConnectionStatus -- takeMVar makes the thread wait if the MVar is empty(until there's a connection).
@@ -321,14 +322,13 @@ listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWo
             Right db -> do
               putStrLn $ "Listening for notifications on the " <> dbChannel <> " channel"
               let channelToListen = N.toPgIdentifier dbChannel
-                  cfLoader = configRereader >> putStrLn ("Config reloaded" :: Text)
                   scLoader = void $ loadSchemaCache pool actualPgVersion refConf refDbStructure -- It's not necessary to check the loadSchemaCache success here. If the connection drops, the thread will die and proceed to recover below.
               N.listen db channelToListen
               N.waitForNotifications (\_ msg ->
-                if | BS.null msg            -> scLoader -- reload the schema cache
-                   | msg == "reload schema" -> scLoader -- reload the schema cache
-                   | msg == "reload config" -> cfLoader -- reload the config
-                   | otherwise              -> pure ()  -- Do nothing if anything else than an empty message is sent
+                if | BS.null msg            -> scLoader      -- reload the schema cache
+                   | msg == "reload schema" -> scLoader      -- reload the schema cache
+                   | msg == "reload config" -> configLoader  -- reload the config
+                   | otherwise              -> pure ()       -- Do nothing if anything else than an empty message is sent
                 ) db
             _ -> die errorMessage)
           (\_ -> do -- if the thread dies, we try to recover
@@ -341,12 +341,19 @@ listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWo
     retryMessage = "Retrying listening for notifications on the " <> dbChannel <> " channel.." :: Text
 
 -- | Re-reads the config at runtime.
--- | If it panics(config path was changed, invalid setting), it'll show an error but won't kill the main thread.
-reReadConfig :: P.Pool -> Bool -> Environment -> Maybe FilePath -> IORef AppConfig -> IO ()
-reReadConfig pool gucConfigEnabled env path refConf = do
+reReadConfig :: Bool -> P.Pool -> Bool -> Environment -> Maybe FilePath -> IORef AppConfig -> IO ()
+reReadConfig startingUp pool gucConfigEnabled env path refConf = do
   dbSettings <- if gucConfigEnabled then loadDbSettings pool else pure []
-  conf <- readValidateConfig dbSettings env path
-  atomicWriteIORef refConf conf
+  readConfig dbSettings env path >>= \case
+    Left err   ->
+      if startingUp
+        then panic err -- die on invalid config if the program is starting up
+        else hPutStrLn stderr $ "Failed config reload. " <> err
+    Right conf -> do
+      atomicWriteIORef refConf conf
+      if startingUp
+        then pass
+        else putStrLn ("Config reloaded" :: Text)
 
 -- | Dump DbStructure schema to JSON
 dumpSchema :: P.Pool -> AppConfig -> IO LBS.ByteString

src/PostgREST/Config.hs

@@ -30,7 +30,7 @@ module PostgREST.Config ( prettyVersion
                         , Environment
                         , readCLIShowHelp
                         , readEnvironment
-                        , readValidateConfig
+                        , readConfig
                         )
        where
 
@@ -44,6 +44,7 @@ import Control.Lens            (preview)
 import Control.Monad           (fail)
 import Crypto.JWT              (JWKSet, StringOrURI, stringOrUri)
 import Data.Aeson              (encode, toJSON)
+import Data.Either.Combinators (mapLeft)
 import Data.List               (lookup)
 import Data.List.NonEmpty      (fromList, toList)
 import Data.Maybe              (fromJust)
@@ -52,7 +53,6 @@ import Data.Text               (dropEnd, dropWhileEnd, filter,
                                 intercalate, pack, replace, splitOn,
                                 strip, stripPrefix, take, toLower,
                                 toTitle, unpack)
-import Data.Text.IO            (hPutStrLn)
 import Data.Version            (versionBranch)
 import Development.GitRev      (gitHash)
 import Numeric                 (readOct, showOct)
@@ -330,22 +330,18 @@ instance JustIfMaybe a (Maybe a) where
   justIfMaybe a = Just a
 
 -- | Parse the config file
-readAppConfig :: [(Text, Text)] -> Environment -> Maybe FilePath -> IO AppConfig
+readAppConfig :: [(Text, Text)] -> Environment -> Maybe FilePath -> IO (Either Text AppConfig)
 readAppConfig dbSettings env optPath = do
   -- Now read the actual config file
   conf <- case optPath of
-    Just cfgPath -> catches (C.load cfgPath)
-      [ Handler (\(ex :: IOError)    -> exitErr $ "Cannot open config file:\n\t" <> show ex)
-      , Handler (\(C.ParseError err) -> exitErr $ "Error parsing config file:\n" <> err)
+    Just cfgPath -> C.load cfgPath `catches`
+      [ Handler (\(ex :: IOError)    -> panic $ "Cannot open config file: " <> show ex)
+      , Handler (\(C.ParseError err) -> panic $ "Error parsing config file: " <> err)
       ]
     -- if no filename provided, start with an empty map to read config from environment
     Nothing -> return M.empty
 
-  case C.runParser parseConfig conf of
-    Left err ->
-      exitErr $ "Error in config:\n\t" <> err
-    Right appConf ->
-      return appConf
+  pure $ mapLeft ("Error in config: " <>) $ C.runParser parseConfig conf
 
   where
     parseConfig =
@@ -395,7 +391,7 @@ readAppConfig dbSettings env optPath = do
     parseSocketFileMode :: C.Key -> C.Parser C.Config FileMode
     parseSocketFileMode k =
       optString k >>= \case
-        Nothing -> pure $ 432 -- return default 660 mode if no value was provided
+        Nothing -> pure 432 -- return default 660 mode if no value was provided
         Just fileModeText ->
           case (readOct . unpack) fileModeText of
             []              ->
@@ -518,16 +514,14 @@ readAppConfig dbSettings env optPath = do
     splitOnCommas (C.String s) = strip <$> splitOn "," s
     splitOnCommas _            = []
 
-    exitErr :: Text -> IO a
-    exitErr err = do
-      hPutStrLn stderr err
-      exitFailure
-
--- | Parse the AppConfig and validate it. Overrides the config options from env vars or db settings. Panics on invalid config options.
-readValidateConfig :: [(Text, Text)] -> Environment -> Maybe FilePath -> IO AppConfig
-readValidateConfig dbSettings env path = do
-  conf <- loadDbUriFile =<< loadSecretFile =<< readAppConfig dbSettings env path
-  return $ conf { configJWKS = parseSecret <$> configJwtSecret conf}
+-- | Reads the config and overrides its parameters from files, env vars or db settings.
+readConfig :: [(Text, Text)] -> Environment -> Maybe FilePath -> IO (Either Text AppConfig)
+readConfig dbSettings env path =
+  readAppConfig dbSettings env path >>= \case
+    Left err -> pure $ Left err
+    Right appConf -> do
+      conf <- loadDbUriFile =<< loadSecretFile appConf
+      pure $ Right $ conf { configJWKS = parseSecret <$> configJwtSecret conf}
 
 type Environment = M.Map [Char] Text
 

test/fixtures/schema.sql

@@ -1955,3 +1955,15 @@ begin
   perform pg_notify('pgrst', 'reload config');
   perform pg_notify('pgrst', 'reload schema');
 end $_$ volatile security definer language plpgsql ;
+
+create or replace function test.invalid_role_claim_key_reload() returns void as $_$
+begin
+  alter role postgrest_test_authenticator set pgrst."jwt-role-claim-key" = 'test';
+  perform pg_notify('pgrst', 'reload config');
+end $_$ volatile security definer language plpgsql ;
+
+create or replace function test.reset_invalid_role_claim_key() returns void as $_$
+begin
+  alter role postgrest_test_authenticator set pgrst."jwt-role-claim-key" = '."a"."role"';
+  perform pg_notify('pgrst', 'reload config');
+end $_$ volatile security definer language plpgsql ;

test/io-tests/test_io.py

@@ -154,7 +154,7 @@ def run(configpath=None, stdin=None, env=None, port=None):
         if configpath:
             command.append(configpath)
 
-        process = subprocess.Popen(command, stdin=subprocess.PIPE, env=env)
+        process = subprocess.Popen(command, stdin=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
 
         try:
             process.stdin.write(stdin or b"")
@@ -590,3 +590,19 @@ def test_max_rows_notify_reload(defaultenv):
 
         # reset max-rows config on the db
         postgrest.session.post("/rpc/reset_max_rows_config")
+
+def invalid_role_claim_key_notify_reload(defaultenv):
+    "NOTIFY reload config should show an error if role-claim-key is invalid"
+
+    env = {
+        **defaultenv,
+        "PGRST_DB_LOAD_GUC_CONFIG": "true",
+        "PGRST_DB_CHANNEL_ENABLED": "true",
+    }
+
+    with run(env=env) as postgrest:
+        postgrest.session.post("/rpc/invalid_role_claim_key_reload")
+
+        assert "failed to parse role-claim-key value" in str(postgrest.process.stderr.readline())
+
+        postgrest.session.post("/rpc/reset_invalid_role_claim_key")