chore(license): treat license file API as an upload (5.0)

Improving the HTTP API for license after PR feedback.
2022-08-02 17:25:33 -03:00 · 2022-08-02 17:25:33 -03:00 · 69ec76f586
parent 1454ef72d7
commit 69ec76f586
6 changed files with 131 additions and 145 deletions
--- a/lib-ee/emqx_license/i18n/emqx_license_http_api.conf
+++ b/lib-ee/emqx_license/i18n/emqx_license_http_api.conf
@ -10,10 +10,21 @@ emqx_license_http_api {
    }
  }

-  desc_license_upload_api {
+  desc_license_file_api {
    desc {
-      en: "Upload a license file or key"
-      zh: "上传许可证文件或密钥"
+      en: "Upload a license file"
+      zh: "上传一个许可证文件"
+    }
+    label: {
+      en: "Update license"
+      zh: "更新许可证"
+    }
+  }
+
+  desc_license_key_api {
+    desc {
+      en: "Update a license key"
+      zh: "更新一个许可证密钥"
    }
    label: {
      en: "Update license"
--- a/lib-ee/emqx_license/src/emqx_license.erl
+++ b/lib-ee/emqx_license/src/emqx_license.erl
@ -22,6 +22,7 @@
    read_license/0,
    read_license/1,
    update_file/1,
+    update_file_contents/1,
    update_key/1,
    license_dir/0,
    save_and_backup_license/1
@ -70,15 +71,20 @@ relative_license_path() ->
 update_file(Filename) when is_binary(Filename); is_list(Filename) ->
    case file:read_file(Filename) of
        {ok, Contents} ->
+            update_file_contents(Contents);
+        {error, Error} ->
+            {error, Error}
+    end.
+
+-spec update_file_contents(binary() | string()) ->
+    {ok, emqx_config:update_result()} | {error, emqx_config:update_error()}.
+update_file_contents(Contents) when is_binary(Contents) ->
    Result = emqx_conf:update(
        ?CONF_KEY_PATH,
        {file, Contents},
        #{rawconf_with_defaults => true, override_to => local}
    ),
-            handle_config_update_result(Result);
-        {error, Error} ->
-            {error, Error}
-    end.
+    handle_config_update_result(Result).

 -spec update_key(binary() | string()) ->
    {ok, emqx_config:update_result()} | {error, emqx_config:update_error()}.
--- a/lib-ee/emqx_license/src/emqx_license_http_api.erl
+++ b/lib-ee/emqx_license/src/emqx_license_http_api.erl
@ -18,11 +18,11 @@

 -export([
    '/license'/2,
-    '/license/upload'/2
+    '/license/key'/2,
+    '/license/file'/2
 ]).

 -define(BAD_REQUEST, 'BAD_REQUEST').
-define(NOT_FOUND, 'NOT_FOUND').

 namespace() -> "license_http_api".

@ -32,7 +32,8 @@ api_spec() ->
 paths() ->
    [
        "/license",
-        "/license/upload"
+        "/license/key",
+        "/license/file"
    ].

 schema("/license") ->
@ -54,15 +55,36 @@ schema("/license") ->
            }
        }
    };
-schema("/license/upload") ->
+schema("/license/file") ->
    #{
-        'operationId' => '/license/upload',
+        'operationId' => '/license/file',
        post => #{
            tags => [<<"license">>],
-            summary => <<"Upload license">>,
-            description => ?DESC("desc_license_upload_api"),
+            summary => <<"Upload license file">>,
+            description => ?DESC("desc_license_file_api"),
+            'requestBody' => emqx_dashboard_swagger:file_schema(filename),
+            responses => #{
+                200 => emqx_dashboard_swagger:schema_with_examples(
+                    map(),
+                    #{
+                        sample_license_info => #{
+                            value => sample_license_info_response()
+                        }
+                    }
+                ),
+                400 => emqx_dashboard_swagger:error_codes([?BAD_REQUEST], <<"Bad license file">>)
+            }
+        }
+    };
+schema("/license/key") ->
+    #{
+        'operationId' => '/license/key',
+        post => #{
+            tags => [<<"license">>],
+            summary => <<"Update license key">>,
+            description => ?DESC("desc_license_key_api"),
            'requestBody' => emqx_dashboard_swagger:schema_with_examples(
-                emqx_license_schema:license_type(),
+                emqx_license_schema:key_license(),
                #{
                    license_key => #{
                        summary => <<"License key string">>,
@ -71,14 +93,6 @@ schema("/license/upload") ->
                            <<"connection_low_watermark">> => "75%",
                            <<"connection_high_watermark">> => "80%"
                        }
-                    },
-                    license_file => #{
-                        summary => <<"Path to a license file">>,
-                        value => #{
-                            <<"file">> => <<"/path/to/license">>,
-                            <<"connection_low_watermark">> => "75%",
-                            <<"connection_high_watermark">> => "80%"
-                        }
                    }
                }
            ),
@ -91,8 +105,7 @@ schema("/license/upload") ->
                        }
                    }
                ),
-                400 => emqx_dashboard_swagger:error_codes([?BAD_REQUEST], <<"Bad license key">>),
-                404 => emqx_dashboard_swagger:error_codes([?NOT_FOUND], <<"File not found">>)
+                400 => emqx_dashboard_swagger:error_codes([?BAD_REQUEST], <<"Bad license file">>)
            }
        }
    }.
@ -117,37 +130,26 @@ error_msg(Code, Msg) ->
    License = maps:from_list(emqx_license_checker:dump()),
    {200, License}.

-'/license/upload'(post, #{body := #{<<"file">> := Filepath}}) ->
-    case emqx_license:update_file(Filepath) of
-        {error, enoent} ->
-            ?SLOG(error, #{
-                msg => "license_file_not_found",
-                path => Filepath
-            }),
-            {404, error_msg(?NOT_FOUND, <<"File not found">>)};
-        {error, Error} when is_atom(Error) ->
-            ?SLOG(error, #{
-                msg => "bad_license_file",
-                reason => Error,
-                path => Filepath
-            }),
-            {400, error_msg(?BAD_REQUEST, emqx_misc:explain_posix(Error))};
+'/license/file'(post, #{body := #{<<"filename">> := #{type := _} = File}}) ->
+    [{_Filename, Contents}] = maps:to_list(maps:without([type], File)),
+    case emqx_license:update_file_contents(Contents) of
        {error, Error} ->
            ?SLOG(error, #{
                msg => "bad_license_file",
-                reason => Error,
-                path => Filepath
+                reason => Error
            }),
            {400, error_msg(?BAD_REQUEST, <<"Bad license file">>)};
        {ok, _} ->
            ?SLOG(info, #{
-                msg => "updated_license_file",
-                path => Filepath
+                msg => "updated_license_file"
            }),
            License = maps:from_list(emqx_license_checker:dump()),
            {200, License}
    end;
-'/license/upload'(post, #{body := #{<<"key">> := Key}}) ->
+'/license/file'(post, _Params) ->
+    {400, error_msg(?BAD_REQUEST, <<"Invalid request params">>)}.
+
+'/license/key'(post, #{body := #{<<"key">> := Key}}) ->
    case emqx_license:update_key(Key) of
        {error, Error} ->
            ?SLOG(error, #{
@ -160,5 +162,5 @@ error_msg(Code, Msg) ->
            License = maps:from_list(emqx_license_checker:dump()),
            {200, License}
    end;
-'/license/upload'(post, _Params) ->
+'/license/key'(post, _Params) ->
    {400, error_msg(?BAD_REQUEST, <<"Invalid request params">>)}.
--- a/lib-ee/emqx_license/src/emqx_license_schema.erl
+++ b/lib-ee/emqx_license/src/emqx_license_schema.erl
@ -15,7 +15,9 @@
 -export([roots/0, fields/1, validations/0, desc/1]).

 -export([
-    license_type/0
+    license_type/0,
+    key_license/0,
+    file_license/0
 ]).

 roots() ->
@ -99,10 +101,16 @@ validations() ->

 license_type() ->
    hoconsc:union([
-        hoconsc:ref(?MODULE, key_license),
-        hoconsc:ref(?MODULE, file_license)
+        key_license(),
+        file_license()
    ]).

+key_license() ->
+    hoconsc:ref(?MODULE, key_license).
+
+file_license() ->
+    hoconsc:ref(?MODULE, file_license).
+
 check_license_watermark(Conf) ->
    case hocon_maps:get("license.connection_low_watermark", Conf) of
        undefined ->
--- a/lib-ee/emqx_license/test/emqx_license_SUITE.erl
+++ b/lib-ee/emqx_license/test/emqx_license_SUITE.erl
@ -16,6 +16,9 @@ all() ->
    emqx_common_test_helpers:all(?MODULE).

 init_per_suite(Config) ->
+    %% hack needed to avoid inter-suite flakiness when using meck...
+    ok = cover:stop(),
+    {ok, _} = cover:start(),
    _ = application:load(emqx_conf),
    emqx_config:save_schema_mod_and_names(emqx_license_schema),
    emqx_common_test_helpers:start_apps([emqx_license], fun set_special_configs/1),
@ -141,7 +144,9 @@ setup_test(TestCase, Config) when
                    emqx_config:put([license], LicConfig),
                    RawConfig = #{<<"type">> => file, <<"file">> => LicensePath},
                    emqx_config:put_raw([<<"license">>], RawConfig),
-                    ok = meck:new(emqx_license, [non_strict, passthrough, no_history, no_link]),
+                    ok = meck:new(emqx_license_parser, [
+                        non_strict, passthrough, no_history, no_link
+                    ]),
                    meck:expect(
                        emqx_license_parser,
                        parse,
--- a/lib-ee/emqx_license/test/emqx_license_http_api_SUITE.erl
+++ b/lib-ee/emqx_license/test/emqx_license_http_api_SUITE.erl
@ -21,23 +21,12 @@ all() ->
 init_per_suite(Config) ->
    _ = application:load(emqx_conf),
    emqx_config:save_schema_mod_and_names(emqx_license_schema),
-    ok = meck:new(emqx_license_parser, [non_strict, passthrough, no_history, no_link]),
-    ok = meck:expect(
-        emqx_license_parser,
-        parse,
-        fun(X) ->
-            emqx_license_parser:parse(
-                X,
-                emqx_license_test_lib:public_key_pem()
-            )
-        end
-    ),
    emqx_common_test_helpers:start_apps([emqx_license, emqx_dashboard], fun set_special_configs/1),
    Config.

 end_per_suite(_) ->
-    emqx_common_test_helpers:stop_apps([emqx_license, emqx_dashboard]),
    ok = meck:unload([emqx_license_parser]),
+    emqx_common_test_helpers:stop_apps([emqx_license, emqx_dashboard]),
    Config = #{type => file, file => emqx_license_test_lib:default_license()},
    emqx_config:put([license], Config),
    RawConfig = #{<<"type">> => file, <<"file">> => emqx_license_test_lib:default_license()},
@ -51,7 +40,19 @@ set_special_configs(emqx_license) ->
    Config = #{type => key, key => LicenseKey},
    emqx_config:put([license], Config),
    RawConfig = #{<<"type">> => key, <<"key">> => LicenseKey},
-    emqx_config:put_raw([<<"license">>], RawConfig);
+    emqx_config:put_raw([<<"license">>], RawConfig),
+    ok = meck:new(emqx_license_parser, [non_strict, passthrough, no_history, no_link]),
+    ok = meck:expect(
+        emqx_license_parser,
+        parse,
+        fun(X) ->
+            emqx_license_parser:parse(
+                X,
+                emqx_license_test_lib:public_key_pem()
+            )
+        end
+    ),
+    ok;
 set_special_configs(_) ->
    ok.

@ -88,6 +89,14 @@ assert_untouched_license() ->
        get_license()
    ).

+multipart_formdata_request(Uri, File) ->
+    emqx_dashboard_api_test_helpers:multipart_formdata_request(
+        Uri,
+        _Username = <<"license_admin">>,
+        _Fields = [],
+        [File]
+    ).
+
 %%------------------------------------------------------------------------------
 %% Testcases
 %%------------------------------------------------------------------------------
@ -114,13 +123,9 @@ t_license_info(_Config) ->

 t_license_upload_file_success(_Config) ->
    NewKey = emqx_license_test_lib:make_license(#{max_connections => "999"}),
-    Path = "/tmp/new.lic",
-    ok = file:write_file(Path, NewKey),
-    try
-        Res = request(
-            post,
-            uri(["license", "upload"]),
-            #{file => Path}
+    Res = multipart_formdata_request(
+        uri(["license", "file"]),
+        {filename, "emqx.lic", NewKey}
    ),
    ?assertMatch({ok, 200, _}, Res),
    {ok, 200, Payload} = Res,
@ -142,59 +147,12 @@ t_license_upload_file_success(_Config) ->
        #{max_connections := 999},
        get_license()
    ),
-        ok
-    after
-        ok = file:delete(Path),
-        ok
-    end.
-
-t_license_upload_file_not_found(_Config) ->
-    Res = request(
-        post,
-        uri(["license", "upload"]),
-        #{file => "/tmp/inexistent.lic"}
-    ),
-
-    ?assertMatch({ok, 404, _}, Res),
-    {ok, 404, Payload} = Res,
-    ?assertEqual(
-        #{
-            <<"code">> => <<"NOT_FOUND">>,
-            <<"message">> => <<"File not found">>
-        },
-        emqx_json:decode(Payload, [return_maps])
-    ),
-    assert_untouched_license(),
-    ok.
-
-t_license_upload_file_reading_error(_Config) ->
-    %% eisdir
-    Path = "/tmp/",
-    Res = request(
-        post,
-        uri(["license", "upload"]),
-        #{file => Path}
-    ),
-    ?assertMatch({ok, 400, _}, Res),
-    {ok, 400, Payload} = Res,
-    ?assertEqual(
-        #{
-            <<"code">> => <<"BAD_REQUEST">>,
-            <<"message">> => <<"Illegal operation on a directory">>
-        },
-        emqx_json:decode(Payload, [return_maps])
-    ),
-    assert_untouched_license(),
    ok.

 t_license_upload_file_bad_license(_Config) ->
-    Path = "/tmp/bad.lic",
-    ok = file:write_file(Path, <<"bad key">>),
-    try
-        Res = request(
-            post,
-            uri(["license", "upload"]),
-            #{file => Path}
+    Res = multipart_formdata_request(
+        uri(["license", "file"]),
+        {filename, "bad.lic", <<"bad key">>}
    ),
    ?assertMatch({ok, 400, _}, Res),
    {ok, 400, Payload} = Res,
@ -206,17 +164,13 @@ t_license_upload_file_bad_license(_Config) ->
        emqx_json:decode(Payload, [return_maps])
    ),
    assert_untouched_license(),
-        ok
-    after
-        ok = file:delete(Path),
-        ok
-    end.
+    ok.

 t_license_upload_key_success(_Config) ->
    NewKey = emqx_license_test_lib:make_license(#{max_connections => "999"}),
    Res = request(
        post,
-        uri(["license", "upload"]),
+        uri(["license", "key"]),
        #{key => NewKey}
    ),
    ?assertMatch({ok, 200, _}, Res),
@ -245,7 +199,7 @@ t_license_upload_key_bad_key(_Config) ->
    BadKey = <<"bad key">>,
    Res = request(
        post,
-        uri(["license", "upload"]),
+        uri(["license", "key"]),
        #{key => BadKey}
    ),
    ?assertMatch({ok, 400, _}, Res),