chore: apply review suggestions

2023-04-21 11:54:11 +08:00 · 2023-04-21 11:54:11 +08:00 · 1db38de71f
parent f831a0b827
commit 1db38de71f
3 changed files with 98 additions and 117 deletions
--- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl
+++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl
@ -263,10 +263,9 @@ transform_header_name(Headers) ->
    ).

 check_ssl_opts(Conf) ->
-    case get_conf_val("url", Conf) of
-        undefined ->
-            ok;
-        Url ->
+    case is_backend_http(Conf) of
+        true ->
+            Url = get_conf_val("url", Conf),
            {BaseUrl, _Path, _Query} = parse_url(Url),
            case BaseUrl of
                <<"https://", _/binary>> ->
@ -278,14 +277,15 @@ check_ssl_opts(Conf) ->
                    end;
                <<"http://", _/binary>> ->
                    ok
-            end
+            end;
+        false ->
+            ok
    end.

 check_headers(Conf) ->
-    case get_conf_val("headers", Conf) of
-        undefined ->
-            ok;
-        Headers ->
+    case is_backend_http(Conf) of
+        true ->
+            Headers = get_conf_val("headers", Conf),
            case to_bin(get_conf_val("method", Conf)) of
                <<"post">> ->
                    ok;
@ -294,7 +294,15 @@ check_headers(Conf) ->
                        false -> ok;
                        true -> <<"HTTP GET requests cannot include content-type header.">>
                    end
-            end
+            end;
+        false ->
+            ok
+    end.
+
+is_backend_http(Conf) ->
+    case get_conf_val("backend", Conf) of
+        http -> true;
+        _ -> false
    end.

 parse_url(Url) ->
--- a/apps/emqx_conf/test/emqx_conf_schema_tests.erl
+++ b/apps/emqx_conf/test/emqx_conf_schema_tests.erl
@ -6,27 +6,27 @@

 -include_lib("eunit/include/eunit.hrl").

+%% erlfmt-ignore
 -define(BASE_CONF,
-    ""
-    "\n"
-    "     node {\n"
-    "        name = \"emqx1@127.0.0.1\"\n"
-    "        cookie = \"emqxsecretcookie\"\n"
-    "        data_dir = \"data\"\n"
-    "     }\n"
-    "     cluster {\n"
-    "        name = emqxcl\n"
-    "        discovery_strategy = static\n"
-    "        static.seeds = ~p\n"
-    "        core_nodes = ~p\n"
-    "     }\n"
-    ""
-).
+    """
+             node {
+                name = \"emqx1@127.0.0.1\"
+                cookie = \"emqxsecretcookie\"
+                data_dir = \"data\"
+             }
+             cluster {
+                name = emqxcl
+                discovery_strategy = static
+                static.seeds = ~p
+                core_nodes = ~p
+             }
+    """).
+
 array_nodes_test() ->
    ExpectNodes = ['emqx1@127.0.0.1', 'emqx2@127.0.0.1'],
    lists:foreach(
        fun(Nodes) ->
-            ConfFile = iolist_to_binary(io_lib:format(?BASE_CONF, [Nodes, Nodes])),
+            ConfFile = to_bin(?BASE_CONF, [Nodes, Nodes]),
            {ok, Conf} = hocon:binary(ConfFile, #{format => richmap}),
            ConfList = hocon_tconf:generate(emqx_conf_schema, Conf),
            ClusterDiscovery = proplists:get_value(
@ -47,101 +47,73 @@ array_nodes_test() ->
    ),
    ok.

+%% erlfmt-ignore
+-define(BASE_AUTHN_ARRAY,
+    """
+        authentication = [
+          {backend = \"http\"
+          body {password = \"${password}\", username = \"${username}\"}
+          connect_timeout = \"15s\"
+          enable_pipelining = 100
+          headers {\"content-type\" = \"application/json\"}
+          mechanism = \"password_based\"
+          method = \"~p\"
+          pool_size = 8
+          request_timeout = \"5s\"
+          ssl {enable = ~p, verify = \"verify_peer\"}
+          url = \"~ts\"
+        }
+        ]
+    """
+).
+
+-define(ERROR(Reason),
+    {emqx_conf_schema, [
+        #{
+            kind := validation_error,
+            reason := integrity_validation_failure,
+            result := _,
+            validation_name := Reason
+        }
+    ]}
+).
+
 authn_validations_test() ->
-    BaseConf = iolist_to_binary(io_lib:format(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"])),
-    DisableSSLWithHttps =
-        ""
-        "\n"
-        "authentication = [\n"
-        "{backend = \"http\"\n"
-        "body {password = \"${password}\", username = \"${username}\"}\n"
-        "connect_timeout = \"15s\"\n"
-        "enable_pipelining = 100\n"
-        "headers {\"content-type\" = \"application/json\"}\n"
-        "mechanism = \"password_based\"\n"
-        "method = \"post\"\n"
-        "pool_size = 8\n"
-        "request_timeout = \"5s\"\n"
-        "ssl {enable = false, verify = \"verify_peer\"}\n"
-        "url = \"https://127.0.0.1:8080\"\n"
-        "}\n"
-        "]\n"
-        "",
-    Conf = <<BaseConf/binary, (list_to_binary(DisableSSLWithHttps))/binary>>,
-    {ok, ConfMap} = hocon:binary(Conf, #{format => richmap}),
-    ?assertThrow(
-        {emqx_conf_schema, [
-            #{
-                kind := validation_error,
-                reason := integrity_validation_failure,
-                result := _,
-                validation_name := check_http_ssl_opts
-            }
-        ]},
-        hocon_tconf:generate(emqx_conf_schema, ConfMap)
-    ),
-    BadHeader =
-        ""
-        "\n"
-        "authentication = [\n"
-        "{backend = \"http\"\n"
-        "body {password = \"${password}\", username = \"${username}\"}\n"
-        "connect_timeout = \"15s\"\n"
-        "enable_pipelining = 100\n"
-        "headers {\"content-type\" = \"application/json\"}\n"
-        "mechanism = \"password_based\"\n"
-        "method = \"get\"\n"
-        "pool_size = 8\n"
-        "request_timeout = \"5s\"\n"
-        "ssl {enable = false, verify = \"verify_peer\"}\n"
-        "url = \"http://127.0.0.1:8080\"\n"
-        "}\n"
-        "]\n"
-        "",
-    Conf1 = <<BaseConf/binary, (list_to_binary(BadHeader))/binary>>,
+    BaseConf = to_bin(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"]),
+
+    OKHttps = to_bin(?BASE_AUTHN_ARRAY, [post, true, <<"https://127.0.0.1:8080">>]),
+    Conf0 = <<BaseConf/binary, OKHttps/binary>>,
+    {ok, ConfMap0} = hocon:binary(Conf0, #{format => richmap}),
+    ?assert(is_list(hocon_tconf:generate(emqx_conf_schema, ConfMap0))),
+
+    OKHttp = to_bin(?BASE_AUTHN_ARRAY, [post, false, <<"http://127.0.0.1:8080">>]),
+    Conf1 = <<BaseConf/binary, OKHttp/binary>>,
    {ok, ConfMap1} = hocon:binary(Conf1, #{format => richmap}),
-    ?assertThrow(
-        {emqx_conf_schema, [
-            #{
-                kind := validation_error,
-                reason := integrity_validation_failure,
-                result := _,
-                validation_name := check_http_headers
-            }
-        ]},
-        hocon_tconf:generate(emqx_conf_schema, ConfMap1)
-    ),
-    BadHeader2 =
-        ""
-        "\n"
-        "authentication = \n"
-        "{backend = \"http\"\n"
-        "body {password = \"${password}\", username = \"${username}\"}\n"
-        "connect_timeout = \"15s\"\n"
-        "enable_pipelining = 100\n"
-        "headers {\"content-type\" = \"application/json\"}\n"
-        "mechanism = \"password_based\"\n"
-        "method = \"get\"\n"
-        "pool_size = 8\n"
-        "request_timeout = \"5s\"\n"
-        "ssl {enable = false, verify = \"verify_peer\"}\n"
-        "url = \"http://127.0.0.1:8080\"\n"
-        "}\n"
-        "\n"
-        "",
-    Conf2 = <<BaseConf/binary, (list_to_binary(BadHeader2))/binary>>,
+    ?assert(is_list(hocon_tconf:generate(emqx_conf_schema, ConfMap1))),
+
+    DisableSSLWithHttps = to_bin(?BASE_AUTHN_ARRAY, [post, false, <<"https://127.0.0.1:8080">>]),
+    Conf2 = <<BaseConf/binary, DisableSSLWithHttps/binary>>,
    {ok, ConfMap2} = hocon:binary(Conf2, #{format => richmap}),
    ?assertThrow(
-        {emqx_conf_schema, [
-            #{
-                kind := validation_error,
-                reason := integrity_validation_failure,
-                result := _,
-                validation_name := check_http_headers
-            }
-        ]},
+        ?ERROR(check_http_ssl_opts),
        hocon_tconf:generate(emqx_conf_schema, ConfMap2)
    ),
+
+    BadHeader = to_bin(?BASE_AUTHN_ARRAY, [get, true, <<"https://127.0.0.1:8080">>]),
+    Conf3 = <<BaseConf/binary, BadHeader/binary>>,
+    {ok, ConfMap3} = hocon:binary(Conf3, #{format => richmap}),
+    ?assertThrow(
+        ?ERROR(check_http_headers),
+        hocon_tconf:generate(emqx_conf_schema, ConfMap3)
+    ),
+
+    BadHeaderWithTuple = binary:replace(BadHeader, [<<"[">>, <<"]">>], <<"">>, [global]),
+    Conf4 = <<BaseConf/binary, BadHeaderWithTuple/binary>>,
+    {ok, ConfMap4} = hocon:binary(Conf4, #{format => richmap}),
+    ?assertThrow(
+        ?ERROR(check_http_headers),
+        hocon_tconf:generate(emqx_conf_schema, ConfMap4)
+    ),
    ok.

 doc_gen_test() ->
@ -164,3 +136,6 @@ doc_gen_test() ->
            ok
        end
    }.
+
+to_bin(Format, Args) ->
+    iolist_to_binary(io_lib:format(Format, Args)).
--- a/apps/emqx_prometheus/TODO
+++ b/apps/emqx_prometheus/TODO
@ -1,2 +0,0 @@
-1. Add more VM Metrics
-2. Add more emqx Metrics