Merge pull request #6946 from zmstone/refactor-license-simplify-parser-error

refactor: treat throw exception as `{error, Reason}` return

apps/emqx/src/emqx_config_handler.erl

@@ -90,6 +90,7 @@ get_raw_cluster_override_conf() ->
 
 -spec init(term()) -> {ok, state()}.
 init(_) ->
+    process_flag(trap_exit, true),
     {ok, #{handlers => #{?MOD => ?MODULE}}}.
 
 handle_call({add_handler, ConfKeyPath, HandlerName}, _From, State = #{handlers := Handlers}) ->
@@ -102,24 +103,8 @@ handle_call({add_handler, ConfKeyPath, HandlerName}, _From, State = #{handlers :
 
 handle_call({change_config, SchemaModule, ConfKeyPath, UpdateArgs}, _From,
             #{handlers := Handlers} = State) ->
-    Reply = try
-        case process_update_request(ConfKeyPath, Handlers, UpdateArgs) of
-            {ok, NewRawConf, OverrideConf, Opts} ->
-                check_and_save_configs(SchemaModule, ConfKeyPath, Handlers, NewRawConf,
-                    OverrideConf, UpdateArgs, Opts);
-            {error, Result} ->
-                {error, Result}
-        end
-    catch Error:Reason:ST ->
-        ?SLOG(error, #{
-            msg => "change_config_failed",
-            exception => Error,
-            reason => Reason,
-            stacktrace => ST
-        }),
-        {error, Reason}
-    end,
-    {reply, Reply, State};
+    Result = handle_update_request(SchemaModule, ConfKeyPath, Handlers, UpdateArgs),
+    {reply, Result, State};
 handle_call(get_raw_cluster_override_conf, _From, State) ->
     Reply = emqx_config:read_override_conf(#{override_to => cluster}),
     {reply, Reply, State};
@@ -165,6 +150,30 @@ deep_put_handler2(Key, KeyPath, Handlers, Mod) ->
             Error
     end.
 
+handle_update_request(SchemaModule, ConfKeyPath, Handlers, UpdateArgs) ->
+    try
+        do_handle_update_request(SchemaModule, ConfKeyPath, Handlers, UpdateArgs)
+    catch
+        throw : Reason ->
+            {error, Reason};
+        Error : Reason : ST ->
+            ?SLOG(error, #{msg => "change_config_failed",
+                           exception => Error,
+                           reason => Reason,
+                           stacktrace => ST
+                          }),
+            {error, config_update_crashed}
+    end.
+
+do_handle_update_request(SchemaModule, ConfKeyPath, Handlers, UpdateArgs) ->
+    case process_update_request(ConfKeyPath, Handlers, UpdateArgs) of
+        {ok, NewRawConf, OverrideConf, Opts} ->
+            check_and_save_configs(SchemaModule, ConfKeyPath, Handlers, NewRawConf,
+                                   OverrideConf, UpdateArgs, Opts);
+        {error, Result} ->
+            {error, Result}
+    end.
+
 process_update_request(ConfKeyPath, _Handlers, {remove, Opts}) ->
     OldRawConf = emqx_config:get_root_raw(ConfKeyPath),
     BinKeyPath = bin_path(ConfKeyPath),

apps/emqx/src/emqx_misc.erl

@@ -48,6 +48,7 @@
         , ipv6_probe/1
         , gen_id/0
         , gen_id/1
+        , explain_posix/1
         ]).
 
 -export([ bin2hexstr_A_F/1
@@ -314,6 +315,41 @@ clamp(Val, Min, Max) ->
        true -> Val
     end.
 
+%% @doc https://www.erlang.org/doc/man/file.html#posix-error-codes
+explain_posix(eacces) -> "Permission denied";
+explain_posix(eagain) -> "Resource temporarily unavailable";
+explain_posix(ebadf) -> "Bad file number";
+explain_posix(ebusy) -> "File busy";
+explain_posix(edquot) -> "Disk quota exceeded";
+explain_posix(eexist) -> "File already exists";
+explain_posix(efault) -> "Bad address in system call argument";
+explain_posix(efbig) -> "File too large";
+explain_posix(eintr) -> "Interrupted system call";
+explain_posix(einval) -> "Invalid argument argument file/socket";
+explain_posix(eio) -> "I/O error";
+explain_posix(eisdir) -> "Illegal operation on a directory";
+explain_posix(eloop) -> "Too many levels of symbolic links";
+explain_posix(emfile) -> "Too many open files";
+explain_posix(emlink) -> "Too many links";
+explain_posix(enametoolong) -> "Filename too long";
+explain_posix(enfile) -> "File table overflow";
+explain_posix(enodev) -> "No such device";
+explain_posix(enoent) -> "No such file or directory";
+explain_posix(enomem) -> "Not enough memory";
+explain_posix(enospc) -> "No space left on device";
+explain_posix(enotblk) -> "Block device required";
+explain_posix(enotdir) -> "Not a directory";
+explain_posix(enotsup) -> "Operation not supported";
+explain_posix(enxio) -> "No such device or address";
+explain_posix(eperm) -> "Not owner";
+explain_posix(epipe) -> "Broken pipe";
+explain_posix(erofs) -> "Read-only file system";
+explain_posix(espipe) -> "Invalid seek";
+explain_posix(esrch) -> "No such process";
+explain_posix(estale) -> "Stale remote file handle";
+explain_posix(exdev) -> "Cross-domain link";
+explain_posix(NotPosix) -> NotPosix.
+
 %%------------------------------------------------------------------------------
 %% Internal Functions
 %%------------------------------------------------------------------------------

apps/emqx_authz/src/emqx_authz_file.erl

@@ -41,15 +41,14 @@ init(#{path := Path} = Source) ->
     Rules = case file:consult(Path) of
                 {ok, Terms} ->
                     [emqx_authz_rule:compile(Term) || Term <- Terms];
-                {error, eacces} ->
-                    ?SLOG(alert, #{msg => "insufficient_permissions_to_read_file", path => Path}),
-                    error(eaccess);
-                {error, enoent} ->
-                    ?SLOG(alert, #{msg => "file_does_not_exist", path => Path}),
-                    error(enoent);
+                {error, Reason} when is_atom(Reason) ->
+                    ?SLOG(alert, #{msg => failed_to_read_acl_file,
+                                   path => Path,
+                                   explain => emqx_misc:explain_posix(Reason)}),
+                    throw(failed_to_read_acl_file);
                 {error, Reason} ->
-                    ?SLOG(alert, #{msg => "failed_to_read_file", path => Path, reason => Reason}),
-                    error(Reason)
+                    ?SLOG(alert, #{msg => bad_acl_file_content, path => Path, reason => Reason}),
+                    throw(bad_acl_file_content)
             end,
     Source#{annotations => #{rules => Rules}}.
 

apps/emqx_authz/test/emqx_authz_file_SUITE.erl

@@ -75,12 +75,12 @@ t_invalid_file(_Config) ->
     ok = file:write_file(<<"acl.conf">>, <<"{{invalid term">>),
 
     ?assertMatch(
-       {error, {1, erl_parse, _}},
+       {error, bad_acl_file_content},
        emqx_authz:update(?CMD_REPLACE, [raw_file_authz_config()])).
 
 t_nonexistent_file(_Config) ->
     ?assertEqual(
-       {error, enoent},
+       {error, failed_to_read_acl_file},
        emqx_authz:update(?CMD_REPLACE,
                          [maps:merge(raw_file_authz_config(),
                                      #{<<"path">> => <<"nonexistent.conf">>})

apps/emqx_conf/include/emqx_conf.hrl

@@ -9,7 +9,7 @@
 
 -record(cluster_rpc_mfa, {
     tnx_id :: pos_integer(),
-    mfa :: mfa(),
+    mfa :: {module(), atom(), [any()]},
     created_at :: calendar:datetime(),
     initiator :: node()
 }).

apps/emqx_conf/src/emqx_cluster_rpc.erl

@@ -228,7 +228,7 @@ catch_up(#{node := Node, retry_interval := RetryMs} = State, SkipResult) ->
                         {atomic, ok} -> catch_up(State, false);
                         Error ->
                             ?SLOG(error, #{
-                                msg => "failed to commit applied call",
+                                msg => "failed_to_commit_applied_call",
                                 applied_id => NextId,
                                 error => Error}),
                             RetryMs
@@ -359,28 +359,34 @@ apply_mfa(TnxId, {M, F, A}) ->
     Res =
         try erlang:apply(M, F, A)
         catch
-            Class:Reason:Stacktrace ->
+            throw : Reason ->
+                {error, #{reason => Reason}};
+            Class : Reason : Stacktrace ->
                 {error, #{exception => Class, reason => Reason, stacktrace => Stacktrace}}
         end,
-    Meta = #{tnx_id => TnxId, module => M, function => F, args => ?TO_BIN(A)},
+    %% Do not log args as it might be sensitive information
+    Meta = #{tnx_id => TnxId, entrypoint => format_mfa(M, F, length(A))},
     IsSuccess = is_success(Res),
-    log_and_alarm(IsSuccess, Res, Meta, TnxId),
+    log_and_alarm(IsSuccess, Res, Meta),
     {IsSuccess, Res}.
 
+format_mfa(M, F, A) ->
+    iolist_to_binary([atom_to_list(M), ":", atom_to_list(F), "/", integer_to_list(A)]).
+
 is_success(ok) -> true;
 is_success({ok, _}) -> true;
 is_success(_) -> false.
 
-log_and_alarm(true, Res, Meta, TnxId) ->
-    OkMeta = Meta#{msg => <<"succeeded to apply MFA">>, result => Res},
-    ?SLOG(debug, OkMeta),
-    Message = ["cluster_rpc_apply_failed:", integer_to_binary(TnxId)],
-    emqx_alarm:deactivate(cluster_rpc_apply_failed, OkMeta#{result => ?TO_BIN(Res)}, Message);
-log_and_alarm(false, Res, Meta, TnxId) ->
-    NotOkMeta = Meta#{msg => <<"failed to apply MFA">>, result => Res},
-    ?SLOG(error, NotOkMeta),
-    Message = ["cluster_rpc_apply_failed:", integer_to_binary(TnxId)],
-    emqx_alarm:activate(cluster_rpc_apply_failed, NotOkMeta#{result => ?TO_BIN(Res)}, Message).
+log_and_alarm(true, Res, Meta) ->
+    ?SLOG(debug, Meta#{msg => "cluster_rpc_apply_ok", result => Res}),
+    do_alarm(deactivate, Res, Meta);
+log_and_alarm(false, Res, Meta) ->
+    ?SLOG(error, Meta#{msg => "cluster_rpc_apply_failed", result => Res}),
+    do_alarm(activate, Res, Meta).
+
+do_alarm(Fun, Res, #{tnx_id := Id} = Meta) ->
+    AlarmMsg = ["cluster_rpc_apply_failed=", integer_to_list(Id)],
+    emqx_alarm:Fun(cluster_rpc_apply_failed, Meta#{result => ?TO_BIN(Res)}, AlarmMsg).
 
 wait_for_all_nodes_commit(TnxId, Delay, Remain) ->
     case lagging_node(TnxId) of

apps/emqx_conf/src/emqx_conf_cli.erl

@@ -73,7 +73,7 @@ admins(_) ->
           {"cluster_call status", "status"},
           {"cluster_call skip [node]", "increase one commit on specific node"},
           {"cluster_call tnxid <TnxId>", "get detailed about TnxId"},
-          {"cluster_call  fast_forward [node] [tnx_id]", "fast forwards to tnx_id" }
+          {"cluster_call fast_forward [node] [tnx_id]", "fast forwards to tnx_id" }
       ]).
 
 status() ->

lib-ee/emqx_license/src/emqx_license.erl

@@ -118,10 +118,10 @@ do_update({file, Filename}, _Conf) ->
                 {ok, _License} ->
                     #{<<"file">> => Filename};
                 {error, Reason} ->
-                    error(Reason)
+                    erlang:throw(Reason)
             end;
         {error, Reason} ->
-            error({invalid_license_file, Reason})
+            erlang:throw({invalid_license_file, Reason})
     end;
 
 do_update({key, Content}, _Conf) when is_binary(Content); is_list(Content) ->
@@ -129,7 +129,7 @@ do_update({key, Content}, _Conf) when is_binary(Content); is_list(Content) ->
         {ok, _License} ->
             #{<<"key">> => Content};
         {error, Reason} ->
-            error(Reason)
+            erlang:throw(Reason)
     end.
 
 check_max_clients_exceeded(MaxClients) ->

lib-ee/emqx_license/src/emqx_license_parser.erl

@@ -98,7 +98,7 @@ max_connections(#{module := Module, data := LicenseData}) ->
 %%--------------------------------------------------------------------
 
 do_parse(_Content, _Key, [], Errors) ->
-    {error, {unknown_format, lists:reverse(Errors)}};
+    {error, lists:reverse(Errors)};
 
 do_parse(Content, Key, [Module | Modules], Errors) ->
     try Module:parse(Content, Key) of
@@ -107,7 +107,7 @@ do_parse(Content, Key, [Module | Modules], Errors) ->
         {error, Error} ->
             do_parse(Content, Key, Modules, [{Module, Error} | Errors])
     catch
-        _Class:Error:_Stk ->
+        _Class : Error ->
             do_parse(Content, Key, Modules, [{Module, Error} | Errors])
     end.
 

lib-ee/emqx_license/src/emqx_license_parser_v20220101.erl

@@ -24,12 +24,14 @@
 %%------------------------------------------------------------------------------
 
 parse(Content, Key) ->
-    [EncodedPayload, EncodedSignature] = binary:split(Content, <<".">>),
-    Payload = base64:decode(EncodedPayload),
-    Signature = base64:decode(EncodedSignature),
-    case verify_signature(Payload, Signature, Key) of
-        true -> parse_payload(Payload);
-        false -> {error, invalid_signature}
+    case do_parse(Content) of
+        {ok, {Payload, Signature}} ->
+            case verify_signature(Payload, Signature, Key) of
+                true -> parse_payload(Payload);
+                false -> {error, invalid_signature}
+            end;
+        {error, Reason} ->
+            {error, Reason}
     end.
 
 dump(#{type := Type,
@@ -67,6 +69,17 @@ max_connections(#{max_connections := MaxConns}) ->
 %% Private functions
 %%------------------------------------------------------------------------------
 
+do_parse(Content) ->
+    try
+        [EncodedPayload, EncodedSignature] = binary:split(Content, <<".">>),
+        Payload = base64:decode(EncodedPayload),
+        Signature = base64:decode(EncodedSignature),
+        {ok, {Payload, Signature}}
+    catch
+        _ : _ ->
+            {error, bad_license_format}
+    end.
+
 verify_signature(Payload, Signature, Key) ->
     RSAPublicKey = public_key:der_decode('RSAPublicKey', Key),
     public_key:verify(Payload, ?DIGEST_TYPE, Signature, RSAPublicKey).

lib-ee/emqx_license/test/emqx_license_SUITE.erl

@@ -68,7 +68,7 @@ t_update_file(_Config) ->
 
     ok = file:write_file("license_with_invalid_content.lic", <<"bad license">>),
     ?assertMatch(
-       {error, {unknown_format, _}},
+       {error, [_ | _]},
        emqx_license:update_file("license_with_invalid_content.lic")),
 
     ?assertMatch(
@@ -77,7 +77,7 @@ t_update_file(_Config) ->
 
 t_update_value(_Config) ->
     ?assertMatch(
-       {error, {unknown_format, _}},
+       {error, [_ | _]},
        emqx_license:update_key("invalid.license")),
 
     {ok, LicenseValue} = file:read_file(emqx_license_test_lib:default_license()),

lib-ee/emqx_license/test/emqx_license_parser_SUITE.erl

@@ -45,8 +45,7 @@ t_parse(_Config) ->
     %% invalid version
     ?assertMatch(
        {error,
-        {unknown_format,
-         [{emqx_license_parser_v20220101,invalid_version}]}},
+        [{emqx_license_parser_v20220101,invalid_version}]},
        emqx_license_parser:parse(
          emqx_license_test_lib:make_license(
            ["220101",
@@ -63,8 +62,7 @@ t_parse(_Config) ->
     %% invalid field number
     ?assertMatch(
        {error,
-        {unknown_format,
-         [{emqx_license_parser_v20220101,invalid_field_number}]}},
+        [{emqx_license_parser_v20220101,invalid_field_number}]},
        emqx_license_parser:parse(
          emqx_license_test_lib:make_license(
            ["220111",
@@ -80,12 +78,11 @@ t_parse(_Config) ->
 
     ?assertMatch(
        {error,
-        {unknown_format,
          [{emqx_license_parser_v20220101,
            [{type,invalid_license_type},
             {customer_type,invalid_customer_type},
             {date_start,invalid_date},
-            {days,invalid_int_value}]}]}},
+            {days,invalid_int_value}]}]},
        emqx_license_parser:parse(
          emqx_license_test_lib:make_license(
            ["220111",
@@ -125,21 +122,20 @@ t_parse(_Config) ->
 
     ?assertMatch(
        {error,
-        {unknown_format,
-         [{emqx_license_parser_v20220101,invalid_signature}]}},
+         [{emqx_license_parser_v20220101,invalid_signature}]},
        emqx_license_parser:parse(
          iolist_to_binary([LicensePart, <<".">>, SignaturePart]),
          public_key_encoded())),
 
     %% totally invalid strings as license
     ?assertMatch(
-       {error, {unknown_format, _}},
+       {error, [_ | _]},
        emqx_license_parser:parse(
          <<"badlicense">>,
          public_key_encoded())),
 
     ?assertMatch(
-       {error, {unknown_format, _}},
+       {error, [_ | _]},
        emqx_license_parser:parse(
          <<"bad.license">>,
          public_key_encoded())).

scripts/buildx.sh

@@ -11,7 +11,6 @@
 ## ./scripts/buildx.sh --profile emqx --pkgtype tgz --arch arm64 --builder ghcr.io/emqx/emqx-builder/4.4-4:24.1.5-3-debian10
 
 set -euo pipefail
-set -x
 
 help() {
     echo
@@ -91,6 +90,8 @@ if [ -z "${PROFILE:-}" ]    ||
     exit 1
 fi
 
+set -x
+
 if [ -z "${WITH_ELIXIR:-}" ]; then
   WITH_ELIXIR=no
 fi