Merge remote-tracking branch 'couchbase/trinity' into phoenix

* couchbase/trinity:
  MB-67857: ns_node_disco should depend on ns_node_disco_events
  MB-67857: Fix flakiness of menelaus_cbauth cbauth_sync_t
  MB-66812: Use sha-256 hash instead of full VC for metakv rev

Change-Id: I6dc031b97c12f8394f80d45c88ad51ce9a47421b

Author: Peter-Searby

apps/ns_server/src/menelaus_cbauth.erl

@@ -673,7 +673,7 @@ setup_t() ->
     fake_chronicle_kv:setup(),
     %% Test setups return a map of pids for later shutdown in the teardown
     PidMap = mock_helpers:setup_mocks([json_rpc_events,
-                                       ns_node_disco_events,
+                                       ns_node_disco,
                                        user_storage_events,
                                        ssl_service_events,
                                        json_rpc_connection_sup,
@@ -716,7 +716,9 @@ start_fake_json_rpc_connection(Label) ->
     gen_event:notify(json_rpc_events,
                      {started, Label, [internal,
                                        {heartbeat, ?HEARTBEAT_TIME_S}],
-                      self()}).
+                      self()}),
+    %% Wait for initial notify to be handled, to avoid racy tests
+    meck:wait(1, menelaus_cbauth_worker, notify, ['_', '_'], ?LONG_TIMEOUT).
 
 cbauth_init_t() ->
     %% UpdateDB gets called once almost immediately
@@ -776,9 +778,6 @@ cbauth_stats_t() ->
     [] = stats().
 
 cbauth_many_notify_t() ->
-    %% Wait for initial notify to be handled
-    meck:wait(1, menelaus_cbauth_worker, notify, ['_', '_'],
-              ?LONG_TIMEOUT),
     meck:wait(1, json_rpc_connection, perform_call,
               ['_', "AuthCacheSvc.UpdateDB", '_', '_'], ?LONG_TIMEOUT),
     %% Ensure that the next perform_call doesn't immediately return

apps/ns_server/src/menelaus_metakv.erl

@@ -52,7 +52,7 @@ handle_normal_get(Req, Key) ->
                 true ->
                     menelaus_util:reply_json(Req, [], 404);
                 false ->
-                    Rev = base64:encode(erlang:term_to_binary(VC)),
+                    Rev = base64:encode(metakv:convert_vc_to_opaque_hash(VC)),
                     Val = base64:encode(Val0),
                     menelaus_util:reply_json(Req, {[{rev, Rev},
                                                     {value, Val}]})
@@ -70,8 +70,7 @@ handle_mutate(Req, Key, Value, Params) ->
                           missing
                   end;
               XRev ->
-                  XRevB = list_to_binary(XRev),
-                  binary_to_term(XRevB)
+                  list_to_binary(XRev)
           end,
     Sensitive = proplists:get_value("sensitive", Params) =:= "true",
     case metakv:mutate(Key, Value,
@@ -142,7 +141,7 @@ handle_iterate(Req, Path, Continuous) ->
 output_kv(HTTPRes, {K, V}) ->
     write_chunk(HTTPRes, null, K, base64:encode(V), false);
 output_kv(HTTPRes, {K, V, VC, Sensitive}) ->
-    Rev0 = base64:encode(erlang:term_to_binary(VC)),
+    Rev0 = base64:encode(metakv:convert_vc_to_opaque_hash(VC)),
     {Rev, Value} = case V of
                        ?DELETED_MARKER ->
                            {null, null};

apps/ns_server/src/metakv.erl

@@ -20,7 +20,8 @@
          ns_config_get/2,
          mutate/2, mutate/3,
          iterate_matching/1, iterate_matching/2, iterate_matching/3,
-         check_continuous_allowed/1]).
+         check_continuous_allowed/1,
+         convert_vc_to_opaque_hash/1]).
 
 %% Exported APIs
 
@@ -180,12 +181,17 @@ ns_config_mutation(Key, Value, Params) ->
                   false ->
                       RV = ns_config:run_txn(
                              fun (Cfg, SetFn) ->
-                                     OldData = ns_config:search_with_vclock(Cfg, K),
+                                     OldData = ns_config:search_with_vclock(Cfg,
+                                                                            K),
                                      OldValue = get_old_value(OldData),
                                      NewValue = add_sensitive(Sensitive,
                                                               OldValue, Value),
-                                     OldVC = get_old_vclock(OldData),
-                                     case Rev =:= OldVC of
+                                     OldVCHash =
+                                         case get_old_vclock(OldData) of
+                                             missing -> missing;
+                                             VC -> convert_vc_to_opaque_hash(VC)
+                                         end,
+                                     case Rev =:= OldVCHash of
                                          true ->
                                              {commit, SetFn(K, NewValue, Cfg)};
                                          false ->
@@ -351,3 +357,7 @@ ns_config_matching_kvs(Key, KVList) ->
     %% This will retain the behaviour as it existed
     %% before this code was moved here from menelaus_metakv.erl.
     [{K, V} || {K, V} <- KVList, Filter(K)].
+
+%% Takes in a vector clock and returns an opaque SHA256 hash of that clock.
+convert_vc_to_opaque_hash(VC) ->
+    crypto:hash(sha256, erlang:term_to_binary(VC)).

apps/ns_server/test/mock_helpers.erl

@@ -156,15 +156,14 @@ compat_mode_events(PidMap) ->
     PidMap#{?FUNCTION_NAME => CompatModeEventsPid}.
 
 ns_node_disco(PidMap) ->
-    setup_mocks([dist_manager], PidMap),
+    PidMap0 = setup_mocks([dist_manager, ns_node_disco_events], PidMap),
     {ok, NsNodeDiscoPid} = ns_node_disco:start_link(),
-    PidMap#{?FUNCTION_NAME => NsNodeDiscoPid}.
+    PidMap0#{?FUNCTION_NAME => NsNodeDiscoPid}.
 
 ns_node_disco_events(PidMap) ->
-    PidMap0 = setup_mocks([ns_node_disco], PidMap),
     {ok, NSNodeDiscoEventsPid} = gen_event:start_link({local,
                                                        ?FUNCTION_NAME}),
-    PidMap0#{?FUNCTION_NAME => NSNodeDiscoEventsPid}.
+    PidMap#{?FUNCTION_NAME => NSNodeDiscoEventsPid}.
 
 json_rpc_events(PidMap) ->
     {ok, JsonRpcEventsPid} = gen_event:start_link({local, ?FUNCTION_NAME}),