Fail boot if definition file is invalid JSON

and `definitions.skip_if_unchanged` is set to `true`. References #2610, #6418. Closes #8372. (cherry picked from commit 0c6f8aa) (cherry picked from commit 7e7431a) # Conflicts: # deps/rabbit/src/rabbit_definitions.erl # deps/rabbit/test/definition_import_SUITE.erl
rabbitmq · May 28, 2023 · be05098 · be05098
1 parent 9a3e563
commit be05098
Show file tree

Hide file tree

Showing 3 changed files with 133 additions and 10 deletions.
diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl
@@ -44,7 +44,8 @@
 -export([import_raw/1, import_raw/2, import_parsed/1, import_parsed/2,
          import_parsed_with_hashing/1, import_parsed_with_hashing/2,
          apply_defs/2, apply_defs/3,
-         should_skip_if_unchanged/0]).
+         should_skip_if_unchanged/0
+        ]).
 
 -export([all_definitions/0]).
 -export([
@@ -53,7 +54,13 @@
   list_exchanges/0, list_queues/0, list_bindings/0,
   is_internal_parameter/1
 ]).
--export([decode/1, decode/2, args/1]).
+-export([decode/1, decode/2, args/1, validate_definitions/1]).
+
+%% for tests
+-export([
+    maybe_load_definitions_from_local_filesystem_if_unchanged/3,
+    maybe_load_definitions_from_pluggable_source_if_unchanged/2
+]).
 
 -import(rabbit_misc, [pget/2, pget/3]).
 -import(rabbit_data_coercion, [to_binary/1]).
@@ -97,6 +104,21 @@ maybe_load_definitions() ->
         {error, E} -> {error, E}
     end.
 
+validate_definitions(Defs) when is_list(Defs) ->
+    lists:foldl(fun(_Body, false) ->
+                     false;
+                   (Body, true) ->
+                       case decode(Body) of
+                           {ok, _Map}    -> true;
+                           {error, _Err} -> false
+                       end
+                end, true, Defs);
+validate_definitions(Body) when is_binary(Body) ->
+    case decode(Body) of
+        {ok, _Map}    -> true;
+        {error, _Err} -> false
+    end.
+
 -spec import_raw(Body :: binary() | iolist()) -> ok | {error, term()}.
 import_raw(Body) ->
     rabbit_log:info("Asked to import definitions. Acting user: ~s", [?INTERNAL_USER]),
@@ -271,6 +293,7 @@ maybe_load_definitions_from_local_filesystem(App, Key) ->
                     rabbit_log:debug("Will re-import definitions even if they have not changed"),
                     Mod:load(IsDir, Path);
                 true ->
+<<<<<<< HEAD
                     Algo = rabbit_definitions_hashing:hashing_algorithm(),
                     rabbit_log:debug("Will import definitions only if definition file/directory has changed, hashing algo: ~ts", [Algo]),
                     CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
@@ -282,9 +305,27 @@ maybe_load_definitions_from_local_filesystem(App, Key) ->
                             rabbit_log:debug("Hash value of imported definitions has changed to ~s", [binary:part(rabbit_misc:hexify(UpdatedHash), 0, 12)]),
                             rabbit_definitions_hashing:store_global_hash(UpdatedHash)
                     end
+=======
+                    maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, IsDir, Path)
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
             end
     end.
 
+maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, IsDir, Path) ->
+    Algo = rabbit_definitions_hashing:hashing_algorithm(),
+    rabbit_log:debug("Will import definitions only if definition file/directory has changed, hashing algo: ~ts", [Algo]),
+    CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
+    rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+    case Mod:load_with_hashing(IsDir, Path, CurrentHash, Algo) of
+        {error, Err} ->
+            {error, Err};
+        CurrentHash ->
+            rabbit_log:info("Hash value of imported definitions matches current contents");
+        UpdatedHash ->
+            rabbit_log:debug("Hash value of imported definitions has changed to ~ts", [binary:part(rabbit_misc:hexify(UpdatedHash), 0, 12)]),
+            rabbit_definitions_hashing:store_global_hash(UpdatedHash)
+    end.
+
 maybe_load_definitions_from_pluggable_source(App, Key) ->
     case application:get_env(App, Key) of
         undefined  -> ok;
@@ -296,6 +337,7 @@ maybe_load_definitions_from_pluggable_source(App, Key) ->
                     {error, "definition import source is configured but definitions.import_backend is not set"};
                 ModOrAlias ->
                     Mod = normalize_backend_module(ModOrAlias),
+<<<<<<< HEAD
                     case should_skip_if_unchanged() of
                         false ->
                             rabbit_log:debug("Will use module ~s to import definitions", [Mod]),
@@ -313,9 +355,35 @@ maybe_load_definitions_from_pluggable_source(App, Key) ->
                                     rabbit_definitions_hashing:store_global_hash(UpdatedHash)
                             end
                     end
+=======
+                    maybe_load_definitions_from_pluggable_source_if_unchanged(Mod, Proplist)
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
             end
     end.
 
+maybe_load_definitions_from_pluggable_source_if_unchanged(Mod, Proplist) ->
+    case should_skip_if_unchanged() of
+        false ->
+            rabbit_log:debug("Will use module ~ts to import definitions", [Mod]),
+            Mod:load(Proplist);
+        true ->
+            rabbit_log:debug("Will use module ~ts to import definitions (if definition file/directory/source has changed)", [Mod]),
+            CurrentHash = rabbit_definitions_hashing:stored_global_hash(),
+            rabbit_log:debug("Previously stored hash value of imported definitions: ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+            Algo = rabbit_definitions_hashing:hashing_algorithm(),
+            case Mod:load_with_hashing(Proplist, CurrentHash, Algo) of
+                {error, Err} ->
+                    {error, Err};
+                CurrentHash ->
+                    rabbit_log:info("Hash value of imported definitions matches current contents");
+                UpdatedHash ->
+                    rabbit_log:debug("Hash value of imported definitions has changed to ~ts...", [binary:part(rabbit_misc:hexify(CurrentHash), 0, 12)]),
+                    rabbit_definitions_hashing:store_global_hash(UpdatedHash)
+            end
+    end.
+
+
+
 normalize_backend_module(local_filesystem) ->
     rabbit_definitions_import_local_filesystem;
 normalize_backend_module(local) ->

diff --git a/deps/rabbit/src/rabbit_definitions_import_local_filesystem.erl b/deps/rabbit/src/rabbit_definitions_import_local_filesystem.erl
@@ -35,7 +35,7 @@
 
 -import(rabbit_misc, [pget/2, pget/3]).
 -import(rabbit_data_coercion, [to_binary/1]).
--import(rabbit_definitions, [import_raw/1]).
+-import(rabbit_definitions, [import_raw/1, decode/1, validate_definitions/1]).
 
 %%
 %% API
@@ -84,12 +84,18 @@ load_with_hashing(IsDir, Path, PreviousHash, Algo) when is_boolean(IsDir) ->
         [] ->
             rabbit_definitions_hashing:hash(Algo, undefined);
         Defs ->
-            case rabbit_definitions_hashing:hash(Algo, Defs) of
-                PreviousHash -> PreviousHash;
-                Other        ->
-                    rabbit_log:debug("New hash: ~ts", [rabbit_misc:hexify(Other)]),
-                    _ = load_from_local_path(IsDir, Path),
-                    Other
+            case validate_definitions(Defs) of
+                true ->
+                    case rabbit_definitions_hashing:hash(Algo, Defs) of
+                        PreviousHash -> PreviousHash;
+                        Other        ->
+                            rabbit_log:debug("New hash: ~ts", [rabbit_misc:hexify(Other)]),
+                            _ = load_from_local_path(IsDir, Path),
+                            Other
+                    end;
+                false ->
+                    rabbit_log:error("Failed to parse a definition file, path: ~p", [Path]),
+                    {error, not_json}
             end
     end.
 

diff --git a/deps/rabbit/test/definition_import_SUITE.erl b/deps/rabbit/test/definition_import_SUITE.erl
@@ -51,7 +51,8 @@ groups() ->
                                import_case16,
                                import_case17,
                                import_case18,
-                               import_case19
+                               import_case19,
+                               import_case20
                               ]},
 
         {boot_time_import_using_classic_source, [], [
@@ -294,6 +295,15 @@ import_case19(Config) ->
             {skip, "Should not run in mixed version environments"}
     end.
 
+import_case20(Config) ->
+    case rabbit_ct_helpers:is_mixed_versions() of
+        false ->
+            import_invalid_file_case_if_unchanged(Config, "failing_case20");
+        true ->
+            %% skip the test in mixed version mode
+            {skip, "Should not run in mixed version environments"}
+    end.
+
 export_import_round_trip_case1(Config) ->
     case rabbit_ct_helpers:is_mixed_versions() of
       false ->
@@ -378,6 +388,11 @@ import_invalid_file_case(Config, CaseName) ->
     rabbit_ct_broker_helpers:rpc(Config, 0, ?MODULE, run_invalid_import_case, [CasePath]),
     ok.
 
+import_invalid_file_case_if_unchanged(Config, CaseName) ->
+    CasePath = filename:join(?config(data_dir, Config), CaseName ++ ".json"),
+    rabbit_ct_broker_helpers:rpc(Config, 0, ?MODULE, run_invalid_import_case_if_unchanged, [CasePath]),
+    ok.
+
 import_from_directory_case(Config, CaseName) ->
     import_from_directory_case_expect(Config, CaseName, ok).
 
@@ -396,16 +411,26 @@ import_raw(Config, Body) ->
     case rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_definitions, import_raw, [Body]) of
         ok -> ok;
         {error, E} ->
+<<<<<<< HEAD
             ct:pal("Import of JSON definitions ~p failed: ~p~n", [Body, E]),
             ct:fail({failure, Body, E})
+=======
+            ct:pal("Import of JSON definitions ~tp failed: ~tp~n", [Body, E]),
+            ct:fail({expected_failure, Body, E})
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
     end.
 
 import_parsed(Config, Body) ->
     case rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_definitions, import_parsed, [Body]) of
         ok -> ok;
         {error, E} ->
+<<<<<<< HEAD
             ct:pal("Import of parsed definitions ~p failed: ~p~n", [Body, E]),
             ct:fail({failure, Body, E})
+=======
+            ct:pal("Import of parsed definitions ~tp failed: ~tp~n", [Body, E]),
+            ct:fail({expected_failure, Body, E})
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
     end.
 
 export(Config) ->
@@ -430,20 +455,44 @@ run_import_case(Path) ->
    case rabbit_definitions:import_raw(Body) of
      ok -> ok;
      {error, E} ->
+<<<<<<< HEAD
        ct:pal("Import case ~p failed: ~p~n", [Path, E]),
        ct:fail({failure, Path, E})
+=======
+       ct:pal("Import case ~tp failed: ~tp~n", [Path, E]),
+       ct:fail({expected_failure, Path, E})
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
    end.
 
 run_invalid_import_case(Path) ->
    {ok, Body} = file:read_file(Path),
+<<<<<<< HEAD
    ct:pal("Successfully loaded a definition to import from ~p~n", [Path]),
    case rabbit_definitions:import_raw(Body) of
      ok ->
        ct:pal("Expected import case ~p to fail~n", [Path]),
        ct:fail({failure, Path});
+=======
+   ct:pal("Successfully loaded a definition file at ~tp~n", [Path]),
+   case rabbit_definitions:import_raw(Body) of
+     ok ->
+       ct:pal("Expected import case ~tp to fail~n", [Path]),
+       ct:fail({expected_failure, Path});
+>>>>>>> 7e7431a06b (Fail boot if definition file is invalid JSON)
      {error, _E} -> ok
    end.
 
+run_invalid_import_case_if_unchanged(Path) ->
+    Mod = rabbit_definitions_import_local_filesystem,
+    ct:pal("Successfully loaded a definition to import from ~tp~n", [Path]),
+    case rabbit_definitions:maybe_load_definitions_from_local_filesystem_if_unchanged(Mod, false, Path) of
+        ok ->
+            ct:pal("Expected import case ~tp to fail~n", [Path]),
+            ct:fail({expected_failure, Path});
+        {error, _E} -> ok
+    end.
+
+
 queue_lookup(Config, VHost, Name) ->
     rabbit_ct_broker_helpers:rpc(Config, 0, rabbit_amqqueue, lookup, [rabbit_misc:r(VHost, queue, Name)]).