Fix and clarify relay behaviour

Leave it up to a handle_request callback to decide whether or
not to filter the peer from which the incoming request was sent.
Reply with 3002 (DIAMETER_UNABLE_TO_DELIVER) on anything but an
answer from the peer.

2 files changed, 55 insertions, 68 deletions

diff --git a/lib/diameter/doc/src/diameter_app.xml b/lib/diameter/doc/src/diameter_app.xml
index fc359b9d1d..a3d9a8eeac 100644
--- a/lib/diameter/doc/src/diameter_app.xml
+++ b/lib/diameter/doc/src/diameter_app.xml
@@ -467,11 +467,11 @@ callback returned false.</p>
 <v>Packet  = packet()</v>
 <v>SvcName = term()</v>
 <v>Peer    = peer()</v>
-<v>Action  = Reply | {relay, Opts} | discard | {eval, Action, ContF}</v>
+<v>Action  = Reply | {relay, Opts} | discard | {eval, Action, PostF}</v>
 <v>Reply   = {reply, message()}
            | {protocol_error, 3000..3999}</v>
 <v>Opts    = diameter:call_opts()</v>
-<v>ContF   = diameter:evaluable()</v>
+<v>PostF   = diameter:evaluable()</v>
 </type>
 <desc>
 <p>
@@ -559,26 +559,28 @@ will cause the request process in question to fail.</p>
 <tag><c>{relay, Opts}</c></tag>
 <item>
 <p>
-Relay a request to another peer.
-The appropriate Route-Record AVP will be added to the relayed request
-by diameter and <seealso marker="#pick_peer">pick_peer/4</seealso>
-and <seealso marker="#prepare_request">prepare_request/3</seealso>
-callback will take place just as if <seealso
+Relay a request to another peer in the role of a Diameter relay agent.
+If a routing loop is detected then the request is answered with
+3005 (DIAMETER_LOOP_DETECTED).
+Otherwise a Route-Record AVP (containing the sending peer's Origin-Host) is
+added to the request and <seealso marker="#pick_peer">pick_peer/4</seealso>
+and subsequent callbacks take place just as if <seealso
 marker="diameter#call">diameter:call/4</seealso> had been called
 explicitly.
-However, returning a <c>relay</c> tuple also causes the End-to-End
-Identifier to be preserved in the header of the relayed request as
-required by RFC 3588.</p>
+The End-to-End Identifier of the incoming request is preserved in the
+header of the relayed request.</p>
 
 <p>
-The returned <c>Opts</c> should not specify <c>detach</c> and
-the <seealso marker="#handle_answer">handle_answer/4</seealso>
-callback following from a relayed request must return its first
+The returned <c>Opts</c> should not specify <c>detach</c>.
+A subsequent <seealso marker="#handle_answer">handle_answer/4</seealso>
+callback for the relayed request must return its first
 argument, the <c>diameter_packet</c> record containing the answer
 message.
 Note that the <c>extra</c> option can be specified to supply arguments
-that can distinguish the relay case from others if so desired,
-although the form of the request message may be sufficient.</p>
+that can distinguish the relay case from others if so desired.
+Any other return value (for example, from a
+<seealso marker="#handle_error">handle_error/4</seealso> callback)
+causes the request to be answered with 3002 (DIAMETER_UNABLE_TO_DELIVER).</p>
 </item>
 
 <tag><c>discard</c></tag>
@@ -587,18 +589,18 @@ although the form of the request message may be sufficient.</p>
 Discard the request.</p>
 </item>
 
-<tag><c>{eval, Action, ContF}</c></tag>
+<tag><c>{eval, Action, PostF}</c></tag>
 <item>
 <p>
 Handle the request as if <c>Action</c> has been returned and then
-evaluate <c>ContF</c> in the request process.</p>
+evaluate <c>PostF</c> in the request process.</p>
 </item>
 
 </taglist>
 
 <p>
-Note that diameter will respond to protocol errors in an incoming
-request without invoking <c>handle_request/3</c>.</p>
+Note that protocol errors detected by diameter will result in an
+answer message without <c>handle_request/3</c> being invoked.</p>
 
 </desc>
 </func>
diff --git a/lib/diameter/src/app/diameter_service.erl b/lib/diameter/src/app/diameter_service.erl
index 63b0649dc4..b7b9ac1ef4 100644
--- a/lib/diameter/src/app/diameter_service.erl
+++ b/lib/diameter/src/app/diameter_service.erl
@@ -1685,9 +1685,9 @@ recv_request({Id, Alias}, T, TPid, Apps, Caps, Pkt) ->
 %%   DIAMETER_APPLICATION_UNSUPPORTED   3007
 %%      A request was sent for an application that is not supported.
 
-recv_request(false, {_, OH, OR}, TPid, _, _, Pkt) ->
-    ?LOG({error, application}, Pkt),
-    reply(answer_message({OH, OR, 3007}, collect_avps(Pkt)), ?BASE, TPid, Pkt).
+recv_request(false, T, TPid, _, _, Pkt) ->
+    As = collect_avps(Pkt),
+    protocol_error(3007, T, TPid, Pkt#diameter_packet{avps = As}).
 
 collect_avps(Pkt) ->
     case diameter_codec:collect_avps(Pkt) of
@@ -1706,13 +1706,9 @@ collect_avps(Pkt) ->
 %%      set to an unrecognized value, or that is inconsistent with the
 %%      AVP's definition.
 %%
-recv_request({_, OH, OR}, {TPid, _}, _, #diameter_packet{errors = [Bs | _],
-                                                         bin = Bin,
-                                                         avps = Avps}
-                                        = Pkt)
+recv_request(T, {TPid, _}, _, #diameter_packet{errors = [Bs | _]} = Pkt)
   when is_bitstring(Bs) ->
-    ?LOG({error, invalid_avp_bits}, Bin),
-    reply(answer_message({OH, OR, 3009}, Avps), ?BASE, TPid, Pkt);
+    protocol_error(3009, T, TPid, Pkt);
 
 %% Either we support this application but don't recognize the command
 %% or we're a relay and the command isn't proxiable.
@@ -1722,18 +1718,15 @@ recv_request({_, OH, OR}, {TPid, _}, _, #diameter_packet{errors = [Bs | _],
 %%      recognize or support.  This MUST be used when a Diameter node
 %%      receives an experimental command that it does not understand.
 %%
-recv_request({_, OH, OR},
+recv_request(T,
              {TPid, _},
              #diameter_app{id = Id},
              #diameter_packet{header = #diameter_header{is_proxiable = P},
-                              msg = M,
-                              avps = Avps,
-                              bin = Bin}
+                              msg = M}
              = Pkt)
   when ?APP_ID_RELAY /= Id, undefined == M;
        ?APP_ID_RELAY == Id, not P ->
-    ?LOG({error, command_unsupported}, Bin),
-    reply(answer_message({OH, OR, 3001}, Avps), ?BASE, TPid, Pkt);
+    protocol_error(3001, T, TPid, Pkt);
 
 %% Error bit was set on a request.
 %%
@@ -1742,15 +1735,12 @@ recv_request({_, OH, OR},
 %%      either set to an invalid combination, or to a value that is
 %%      inconsistent with the command code's definition.
 %%
-recv_request({_, OH, OR},
+recv_request(T,
              {TPid, _},
              _,
-             #diameter_packet{header = #diameter_header{is_error = true},
-                              avps = Avps,
-                              bin = Bin}
+             #diameter_packet{header = #diameter_header{is_error = true}}
              = Pkt) ->
-    ?LOG({error, error_bit}, Bin),
-    reply(answer_message({OH, OR, 3008}, Avps), ?BASE, TPid, Pkt);
+    protocol_error(3008, T, TPid, Pkt);
 
 %% A message in a locally supported application or a proxiable message
 %% in the relay application. Don't distinguish between the two since
@@ -1878,7 +1868,7 @@ resend(true, _, _, T, {TPid, _}, Pkt) ->  %% Route-Record loop
 resend(false,
        Opts,
        App,
-       {SvcName, _, _},
+       {SvcName, _, _} = T,
        {TPid, #diameter_caps{origin_host = {_, OH}}},
        #diameter_packet{header = Hdr0,
                         avps = Avps}
@@ -1887,46 +1877,41 @@ resend(false,
     Seq = diameter_session:sequence(),
     Hdr = Hdr0#diameter_header{hop_by_hop_id = Seq},
     Msg = [Hdr, Route | Avps],
-    %% Filter sender as ineligible receiver.
-    reply(call(SvcName, App, Msg, [{filter, {neg, {host, OH}}} | Opts]),
-          TPid,
-          Pkt).
+    resend(call(SvcName, App, Msg, Opts), T, TPid, Pkt).
 %% The incoming request is relayed with the addition of a
-%% Route-Record. Note the requirement on the return from call/4.
-%% This places a requirement on the values returned by the
-%% handle_answer and handle_error callbacks of the application module
-%% in question.
+%% Route-Record. Note the requirement on the return from call/4 below,
+%% which places a requirement on the value returned by the
+%% handle_answer callback of the application module in question.
+%%
+%% Note that there's nothing stopping the request from being relayed
+%% back to the sender. A pick_peer callback may want to avoid this but
+%% a smart peer might recognize the potential loop and choose another
+%% route. A less smart one will probably just relay the request back
+%% again and force us to detect the loop. A pick_peer that wants to
+%% avoid this can specify filter to avoid the possibility.
+%% Eg. {neg, {host, OH} where #diameter_caps{origin_host = {OH, _}}.
 %%
 %% RFC 6.3 says that a relay agent does not modify Origin-Host but
 %% says nothing about a proxy. Assume it should behave the same way.
 
-%% reply/3
+%% resend/4
 %%
 %% Relay a reply to a relayed request.
 
 %% Answer from the peer: reset the hop by hop identifier and send.
-reply(#diameter_packet{bin = B}
-      = Pkt,
-      TPid,
-      #diameter_packet{header = #diameter_header{hop_by_hop_id = Id},
-                       transport_data = TD}) ->
+resend(#diameter_packet{bin = B}
+       = Pkt,
+       _,
+       TPid,
+       #diameter_packet{header = #diameter_header{hop_by_hop_id = Id},
+                        transport_data = TD}) ->
     send(TPid, Pkt#diameter_packet{bin = diameter_codec:hop_by_hop_id(Id, B),
                                    transport_data = TD});
 %% TODO: counters
 
-%% Not. Ignoring the error feels harsh but there is no appropriate
-%% Result-Code for a protocol error (which this isn't really anyway)
-%% and the RFC doesn't provide any guidance how to act. A weakness
-%% here is that we don't deal well with a decode error: the request
-%% will simply timeout on the peer's end. Better would be to just send
-%% the answer (with modified hop by hop identifier) on regardless, at
-%% least in the relay case in which there's no examination of the
-%% answer. In the proxy case it's not clear that the callback won't
-%% examine the answer. Just be quiet here since a decode error causes
-%% the request process to crash (or not depending on the error and
-%% config and/or handle_answer callback).
-reply(_, _, _) ->
-    ok.
+%% Or not: DIAMETER_UNABLE_TO_DELIVER.
+resend(_, T, TPid, Pkt) ->
+    protocol_error(3002, T, TPid, Pkt).
 
 %% is_loop/4
 %%