From b0e8e9dc280235102a89c2ebc4d26a3a5e8497b1 Mon Sep 17 00:00:00 2001
From: Thales Macedo Garitezi <thalesmg@gmail.com>
Date: Tue, 8 Nov 2022 13:53:27 -0300
Subject: [PATCH] refactor(crl): use cast for refreshing crls

---
 src/emqx_crl_cache.erl        | 25 +++++++++--------
 test/emqx_crl_cache_SUITE.erl | 53 +++++++++++++++++++++++++++++++----
 2 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/src/emqx_crl_cache.erl b/src/emqx_crl_cache.erl
index 8dab8d5f1..d411bc52e 100644
--- a/src/emqx_crl_cache.erl
+++ b/src/emqx_crl_cache.erl
@@ -66,7 +66,7 @@ start_link(Opts = #{urls := _, refresh_interval := _}) ->
     gen_server:start_link({local, ?MODULE}, ?MODULE, Opts, []).
 
 refresh(URL) ->
-    gen_server:call(?MODULE, {refresh, URL}, ?HTTP_TIMEOUT + 2_000).
+    gen_server:cast(?MODULE, {refresh, URL}).
 
 evict(URL) ->
     gen_server:cast(?MODULE, {evict, URL}).
@@ -81,16 +81,6 @@ init(#{urls := URLs, refresh_interval := RefreshIntervalMS}) ->
                         URLs),
     {ok, State}.
 
-handle_call({refresh, URL}, _From, State0) ->
-    case do_http_fetch_and_cache(URL) of
-        {error, Error} ->
-            ?LOG(error, "failed to fetch crl response for ~p; error: ~p",
-                 [URL, Error]),
-            {reply, error, ensure_timer(URL, State0, ?RETRY_TIMEOUT)};
-        {ok, CRLs} ->
-            ?LOG(debug, "fetched crl response for ~p", [URL]),
-            {reply, {ok, CRLs}, ensure_timer(URL, State0)}
-    end;
 handle_call(Call, _From, State) ->
     {reply, {error, {bad_call, Call}}, State}.
 
@@ -104,6 +94,17 @@ handle_cast({evict, URL}, State0 = #state{refresh_timers = RefreshTimers0}) ->
         #{ url => URL
          }),
     {noreply, State};
+handle_cast({refresh, URL}, State0) ->
+    case do_http_fetch_and_cache(URL) of
+        {error, Error} ->
+            ?tp(crl_refresh_failure, #{error => Error, url => URL}),
+            ?LOG(error, "failed to fetch crl response for ~p; error: ~p",
+                 [URL, Error]),
+            {noreply, ensure_timer(URL, State0, ?RETRY_TIMEOUT)};
+        {ok, _CRLs} ->
+            ?LOG(debug, "fetched crl response for ~p", [URL]),
+            {noreply, ensure_timer(URL, State0)}
+    end;
 handle_cast(_Cast, State) ->
     {noreply, State}.
 
@@ -151,7 +152,7 @@ do_http_fetch_and_cache(URL) ->
                     {error, invalid_crl};
                 CRLs ->
                     ssl_crl_cache:insert(URL, {der, CRLs}),
-                    ?tp(crl_cache_insert, #{url => URL}),
+                    ?tp(crl_cache_insert, #{url => URL, crls => CRLs}),
                     {ok, CRLs}
             end;
         {ok, {{_, Code, _}, _, Body}} ->
diff --git a/test/emqx_crl_cache_SUITE.erl b/test/emqx_crl_cache_SUITE.erl
index 685e76684..f9090d85f 100644
--- a/test/emqx_crl_cache_SUITE.erl
+++ b/test/emqx_crl_cache_SUITE.erl
@@ -280,7 +280,12 @@ t_manual_refresh(Config) ->
     ?assertEqual([], ets:tab2list(Ref)),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
-    ?assertEqual({ok, [CRLDer]}, emqx_crl_cache:refresh(URL)),
+    ok = snabbkaffe:start_trace(),
+    ?wait_async_action(
+       ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
+       #{?snk_kind := crl_cache_insert},
+       5_000),
+    ok = snabbkaffe:stop(),
     ?assertEqual(
        [{"crl.pem", [CRLDer]}],
        ets:tab2list(Ref)),
@@ -293,7 +298,18 @@ t_refresh_request_error(_Config) ->
                 end),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
-    ?assertEqual(error, emqx_crl_cache:refresh(URL)),
+    ?check_trace(
+       ?wait_async_action(
+          ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
+          #{?snk_kind := crl_cache_insert},
+          5_000),
+       fun(Trace) ->
+         ?assertMatch(
+            [#{error := {bad_response, #{code := 404}}}],
+            ?of_kind(crl_refresh_failure, Trace)),
+         ok
+       end),
+    ok = snabbkaffe:stop(),
     ok.
 
 t_refresh_invalid_response(_Config) ->
@@ -303,7 +319,18 @@ t_refresh_invalid_response(_Config) ->
                 end),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
-    ?assertEqual({ok, []}, emqx_crl_cache:refresh(URL)),
+    ?check_trace(
+       ?wait_async_action(
+          ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
+          #{?snk_kind := crl_cache_insert},
+          5_000),
+       fun(Trace) ->
+         ?assertMatch(
+            [#{crls := []}],
+            ?of_kind(crl_cache_insert, Trace)),
+         ok
+       end),
+    ok = snabbkaffe:stop(),
     ok.
 
 t_refresh_http_error(_Config) ->
@@ -313,7 +340,18 @@ t_refresh_http_error(_Config) ->
                 end),
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
-    ?assertEqual(error, emqx_crl_cache:refresh(URL)),
+    ?check_trace(
+       ?wait_async_action(
+          ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
+          #{?snk_kind := crl_cache_insert},
+          5_000),
+       fun(Trace) ->
+         ?assertMatch(
+            [#{error := {http_error, timeout}}],
+            ?of_kind(crl_refresh_failure, Trace)),
+         ok
+       end),
+    ok = snabbkaffe:stop(),
     ok.
 
 t_unknown_messages(_Config) ->
@@ -326,7 +364,12 @@ t_unknown_messages(_Config) ->
 t_evict(_Config) ->
     {ok, _} = emqx_crl_cache:start_link(),
     URL = "http://localhost/crl.pem",
-    {ok, [_]} = emqx_crl_cache:refresh(URL),
+    ok = snabbkaffe:start_trace(),
+    ?wait_async_action(
+       ?assertEqual(ok, emqx_crl_cache:refresh(URL)),
+       #{?snk_kind := crl_cache_insert},
+       5_000),
+    ok = snabbkaffe:stop(),
     Ref = get_crl_cache_table(),
     ?assertMatch([{"crl.pem", _}], ets:tab2list(Ref)),
     snabbkaffe:start_trace(),