Merge pull request #11466 from zhongwencool/hocon-converter-not-running

fix: hocon converter not working when make_serializable is true

apps/emqx/src/emqx_metrics_worker.erl

@@ -495,7 +495,7 @@ terminate(_Reason, #state{metric_ids = MIDs}) ->
 
 stop(Name) ->
     try
-        gen_server:stop(Name)
+        gen_server:stop(Name, normal, 10000)
     catch
         exit:noproc ->
             ok;

apps/emqx/src/emqx_schema.erl

@@ -2314,18 +2314,7 @@ ciphers_schema(Default) ->
         hoconsc:array(string()),
         #{
             default => default_ciphers(Default),
-            converter => fun
-                (undefined) ->
-                    [];
-                (<<>>) ->
-                    [];
-                ("") ->
-                    [];
-                (Ciphers) when is_binary(Ciphers) ->
-                    binary:split(Ciphers, <<",">>, [global]);
-                (Ciphers) when is_list(Ciphers) ->
-                    Ciphers
-            end,
+            converter => fun converter_ciphers/2,
             validator =>
                 case Default =:= quic of
                     %% quic has openssl statically linked
@@ -2336,6 +2325,14 @@ ciphers_schema(Default) ->
         }
     ).
 
+converter_ciphers(undefined, _Opts) ->
+    [];
+converter_ciphers(<<>>, _Opts) ->
+    [];
+converter_ciphers(Ciphers, _Opts) when is_list(Ciphers) -> Ciphers;
+converter_ciphers(Ciphers, _Opts) when is_binary(Ciphers) ->
+    binary:split(Ciphers, <<",">>, [global]).
+
 default_ciphers(Which) ->
     lists:map(
         fun erlang:iolist_to_binary/1,
@@ -3125,9 +3122,10 @@ quic_feature_toggle(Desc) ->
             importance => ?IMPORTANCE_HIDDEN,
             required => false,
             converter => fun
-                (true) -> 1;
-                (false) -> 0;
-                (Other) -> Other
+                (Val, #{make_serializable := true}) -> Val;
+                (true, _Opts) -> 1;
+                (false, _Opts) -> 0;
+                (Other, _Opts) -> Other
             end
         }
     ).

apps/emqx/test/emqx_listeners_update_SUITE.erl

@@ -116,6 +116,122 @@ t_update_conf(_Conf) ->
     ?assert(is_running('wss:default')),
     ok.
 
+t_update_empty_ssl_options_conf(_Conf) ->
+    Raw = emqx:get_raw_config(?LISTENERS),
+    Raw1 = emqx_utils_maps:deep_put(
+        [<<"tcp">>, <<"default">>, <<"bind">>], Raw, <<"127.0.0.1:1883">>
+    ),
+    Raw2 = emqx_utils_maps:deep_put(
+        [<<"ssl">>, <<"default">>, <<"bind">>], Raw1, <<"127.0.0.1:8883">>
+    ),
+    Raw3 = emqx_utils_maps:deep_put(
+        [<<"ws">>, <<"default">>, <<"bind">>], Raw2, <<"0.0.0.0:8083">>
+    ),
+    Raw4 = emqx_utils_maps:deep_put(
+        [<<"wss">>, <<"default">>, <<"bind">>], Raw3, <<"127.0.0.1:8084">>
+    ),
+    Raw5 = emqx_utils_maps:deep_put(
+        [<<"ssl">>, <<"default">>, <<"ssl_options">>, <<"cacertfile">>], Raw4, <<"">>
+    ),
+    Raw6 = emqx_utils_maps:deep_put(
+        [<<"wss">>, <<"default">>, <<"ssl_options">>, <<"cacertfile">>], Raw5, <<"">>
+    ),
+    Raw7 = emqx_utils_maps:deep_put(
+        [<<"wss">>, <<"default">>, <<"ssl_options">>, <<"ciphers">>], Raw6, <<"">>
+    ),
+    Raw8 = emqx_utils_maps:deep_put(
+        [<<"ssl">>, <<"default">>, <<"ssl_options">>, <<"ciphers">>],
+        Raw7,
+        <<"TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256">>
+    ),
+    ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, Raw8)),
+    ?assertMatch(
+        #{
+            <<"tcp">> := #{<<"default">> := #{<<"bind">> := <<"127.0.0.1:1883">>}},
+            <<"ssl">> := #{
+                <<"default">> := #{
+                    <<"bind">> := <<"127.0.0.1:8883">>,
+                    <<"ssl_options">> := #{
+                        <<"cacertfile">> := <<"">>,
+                        <<"ciphers">> := <<"TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256">>
+                    }
+                }
+            },
+            <<"ws">> := #{<<"default">> := #{<<"bind">> := <<"0.0.0.0:8083">>}},
+            <<"wss">> := #{
+                <<"default">> := #{
+                    <<"bind">> := <<"127.0.0.1:8084">>,
+                    <<"ssl_options">> := #{
+                        <<"cacertfile">> := <<"">>,
+                        <<"ciphers">> := <<"">>
+                    }
+                }
+            }
+        },
+        emqx:get_raw_config(?LISTENERS)
+    ),
+    BindTcp = {{127, 0, 0, 1}, 1883},
+    BindSsl = {{127, 0, 0, 1}, 8883},
+    BindWs = {{0, 0, 0, 0}, 8083},
+    BindWss = {{127, 0, 0, 1}, 8084},
+    ?assertMatch(
+        #{
+            tcp := #{default := #{bind := BindTcp}},
+            ssl := #{
+                default := #{
+                    bind := BindSsl,
+                    ssl_options := #{
+                        cacertfile := <<"">>,
+                        ciphers := ["TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256"]
+                    }
+                }
+            },
+            ws := #{default := #{bind := BindWs}},
+            wss := #{
+                default := #{
+                    bind := BindWss,
+                    ssl_options := #{
+                        cacertfile := <<"">>,
+                        ciphers := []
+                    }
+                }
+            }
+        },
+        emqx:get_config(?LISTENERS)
+    ),
+    ?assertError(not_found, current_conns(<<"tcp:default">>, {{0, 0, 0, 0}, 1883})),
+    ?assertError(not_found, current_conns(<<"ssl:default">>, {{0, 0, 0, 0}, 8883})),
+
+    ?assertEqual(0, current_conns(<<"tcp:default">>, BindTcp)),
+    ?assertEqual(0, current_conns(<<"ssl:default">>, BindSsl)),
+
+    ?assertEqual({0, 0, 0, 0}, proplists:get_value(ip, ranch:info('ws:default'))),
+    ?assertEqual({127, 0, 0, 1}, proplists:get_value(ip, ranch:info('wss:default'))),
+    ?assert(is_running('ws:default')),
+    ?assert(is_running('wss:default')),
+
+    Raw9 = emqx_utils_maps:deep_put(
+        [<<"ssl">>, <<"default">>, <<"ssl_options">>, <<"ciphers">>], Raw7, [
+            "TLS_AES_256_GCM_SHA384",
+            "TLS_AES_128_GCM_SHA256",
+            "TLS_CHACHA20_POLY1305_SHA256"
+        ]
+    ),
+    ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, Raw9)),
+
+    BadRaw = emqx_utils_maps:deep_put(
+        [<<"ssl">>, <<"default">>, <<"ssl_options">>, <<"keyfile">>], Raw4, <<"">>
+    ),
+    ?assertMatch(
+        {error,
+            {bad_ssl_config, #{
+                reason := pem_file_path_or_string_is_required,
+                which_options := [[<<"keyfile">>]]
+            }}},
+        emqx:update_config(?LISTENERS, BadRaw)
+    ),
+    ok.
+
 t_add_delete_conf(_Conf) ->
     Raw = emqx:get_raw_config(?LISTENERS),
     %% add

apps/emqx_bridge_gcp_pubsub/src/emqx_bridge_gcp_pubsub.erl

@@ -18,7 +18,7 @@
 ]).
 -export([
     service_account_json_validator/1,
-    service_account_json_converter/1
+    service_account_json_converter/2
 ]).
 
 %% emqx_bridge_enterprise "unofficial" API
@@ -105,7 +105,7 @@ fields(connector_config) ->
                 #{
                     required => true,
                     validator => fun ?MODULE:service_account_json_validator/1,
-                    converter => fun ?MODULE:service_account_json_converter/1,
+                    converter => fun ?MODULE:service_account_json_converter/2,
                     sensitive => true,
                     desc => ?DESC("service_account_json")
                 }
@@ -398,7 +398,9 @@ service_account_json_validator(Map) ->
             {error, #{missing_keys => MissingKeys}}
     end.
 
-service_account_json_converter(Map) when is_map(Map) ->
+service_account_json_converter(Val, #{make_serializable := true}) ->
+    Val;
+service_account_json_converter(Map, _Opts) when is_map(Map) ->
     ExpectedKeys = [
         <<"type">>,
         <<"project_id">>,
@@ -407,7 +409,7 @@ service_account_json_converter(Map) when is_map(Map) ->
         <<"client_email">>
     ],
     maps:with(ExpectedKeys, Map);
-service_account_json_converter(Val) ->
+service_account_json_converter(Val, _Opts) ->
     Val.
 
 consumer_topic_mapping_validator(_TopicMapping = []) ->

apps/emqx_prometheus/src/emqx_prometheus_schema.erl

@@ -26,7 +26,7 @@
     fields/1,
     desc/1,
     translation/1,
-    convert_headers/1,
+    convert_headers/2,
     validate_push_gateway_server/1
 ]).
 
@@ -61,7 +61,7 @@ fields("prometheus") ->
                 #{
                     default => #{},
                     required => false,
-                    converter => fun ?MODULE:convert_headers/1,
+                    converter => fun ?MODULE:convert_headers/2,
                     desc => ?DESC(headers)
                 }
             )},
@@ -155,9 +155,11 @@ fields("prometheus") ->
 desc("prometheus") -> ?DESC(prometheus);
 desc(_) -> undefined.
 
-convert_headers(<<>>) ->
+convert_headers(Headers, #{make_serializable := true}) ->
+    Headers;
+convert_headers(<<>>, _Opts) ->
     [];
-convert_headers(Headers) when is_map(Headers) ->
+convert_headers(Headers, _Opts) when is_map(Headers) ->
     maps:fold(
         fun(K, V, Acc) ->
             [{binary_to_list(K), binary_to_list(V)} | Acc]
@@ -165,7 +167,7 @@ convert_headers(Headers) when is_map(Headers) ->
         [],
         Headers
     );
-convert_headers(Headers) when is_list(Headers) ->
+convert_headers(Headers, _Opts) when is_list(Headers) ->
     Headers.
 
 validate_push_gateway_server(Url) ->

apps/emqx_prometheus/test/emqx_prometheus_api_SUITE.erl

@@ -75,13 +75,22 @@ t_prometheus_api(_) ->
             <<"vm_statistics_collector">> := _,
             <<"vm_system_info_collector">> := _,
             <<"vm_memory_collector">> := _,
-            <<"vm_msacc_collector">> := _
+            <<"vm_msacc_collector">> := _,
+            <<"headers">> := _
         },
         Conf
     ),
     #{<<"enable">> := Enable} = Conf,
     ?assertEqual(Enable, undefined =/= erlang:whereis(emqx_prometheus)),
-    NewConf = Conf#{<<"interval">> => <<"2s">>, <<"vm_statistics_collector">> => <<"enabled">>},
+
+    NewConf = Conf#{
+        <<"interval">> => <<"2s">>,
+        <<"vm_statistics_collector">> => <<"enabled">>,
+        <<"headers">> => #{
+            <<"test-str1">> => <<"test-value">>,
+            <<"test-str2">> => <<"42">>
+        }
+    },
     {ok, Response2} = emqx_mgmt_api_test_util:request_api(put, Path, "", Auth, NewConf),
 
     Conf2 = emqx_utils_json:decode(Response2, [return_maps]),
@@ -106,6 +115,25 @@ t_prometheus_api(_) ->
         ]
     ),
 
+    ?assertMatch(
+        #{
+            <<"headers">> := #{
+                <<"test-str1">> := <<"test-value">>,
+                <<"test-str2">> := <<"42">>
+            }
+        },
+        emqx_config:get_raw([prometheus])
+    ),
+    ?assertMatch(
+        #{
+            headers := [
+                {"test-str2", "42"},
+                {"test-str1", "test-value"}
+            ]
+        },
+        emqx_config:get([prometheus])
+    ),
+
     NewConf1 = Conf#{<<"enable">> => (not Enable)},
     {ok, _Response3} = emqx_mgmt_api_test_util:request_api(put, Path, "", Auth, NewConf1),
     ?assertEqual((not Enable), undefined =/= erlang:whereis(emqx_prometheus)),

changes/ce/fix-11466.en.md

@@ -0,0 +1 @@
+Fixed a crash that occurred when setting the `ssl_options.ciphers` configuration option to an empty string ("").