Fix diameter AVP filter matching #164
Conversation
3b31cc8
to
e9f7bac
Compare
| @@ -74,6 +74,7 @@ It is also possible to use conditions on the top level, conditioning the deletio | |||
|
|
|||
| NOTE: Of course removing an AVP marked as positional/mandatory in the dictionary definion will result in encoding failure. I.e. we are not referring to the M bit in the AVP definition, but APVs marked `<avp>` or `{avp}` in the ABNF definition of the messages. | |||
|
|
|||
| To prepare for not Erlang native configuration inputs, the matching of AVP values in the conditions are made somewhat flexible. The value matching will attempt to match the condition to the type of the AVP value including the single element list OTP diameter uses for optional AVP values. E.g. an IP address condition given in binary `<<"192.168.0.1">>` format is going to match with AVP values in formats `<<"192.168.0.1">>`, `{192,168,0,1}` or in case of optional AVP `[<<"192.168.0.1">>]`, `[{192,168,0,1}]`. Currently matching string, integer and IP address types are supported. | |||
There was a problem hiding this comment.
Is it possible to normalise the input/condition ? Allow Erlang, JSON, xxx input, but change it to binary, integer, tuple? Or is that too complex ? You would have to know what Erlang/OTP Diameter type of that condition is.
There was a problem hiding this comment.
Sure, however it is inconvenient. Some parameters are allowed (and used in the code) in different formats (e.g. ip addresses that can be tuple or binary). It could work if OTP diameter would be somewhat more consistent in the data formats in the prepare request stage (and e.g. do away with the single element list marking of message optional values). I'm not sure we can put that burden to get it right to anyone operationally configuring the filter.
There was a problem hiding this comment.
travelping/ergw#417 adds DIAMETER type annotations to fields in the JSON/YAML config.
We could opt for not striping them out and using them here to check the correct type as well.
BTW: the only type that are somewhat arbitrary are Address and OctetString (from base/diameter.erl):
-type 'OctetString'() :: iolist().
-type 'Address'()
:: inet:ip_address()
| string().
Time could be called annoying, but nothing a config parse can't handle:
-type 'Time'() :: {{integer(), 1..12, 1..31},
{0..23, 0..59, 0..59}}.
Floats are also problematic, but we don't use them.
There was a problem hiding this comment.
Not sure what benefit it'd bring to check the types here in the filter. IMO the AVP structure is coming from code and code errors we should try to catch elsewhere.
Adding the types in the filter config might not be sufficient. Without such format/type matching here, we'd need to make sure to unify the format of Address and OctetString (can be list or binary too) we put in the messages we compose (may not be many places, didn't look yet). Also make sure whenever someone changes code, it stays unified (despite what diameter allows). Whoever makes the configuration should also understand what AVP is message/group optional or mandatory (most will probably look at the M flag in the pcap and get confused 😉) and put it into the config accordingly.
Since we have the message available, we can deduct the used format/type here, sounds safer to me, even if one could argue that in principle it is somewhat less correct.
I guess the generic question for the discussion is: how much of the complexity of the OTP internal diameter implementation we want to expose to this configuration. My preference is : as little as we can get away with. Which is kind of the idea behind this at the moment.
Time format is indeed not too difficult, but I didn't consider it (yet) : just like floats, it sounds too variable by nature to make sense for filtering.
src/ergw_aaa_diameter_srv.erl
Outdated
| AVPVal == binary_to_integer(ConditionVal); | ||
| match_avp_value(ConditionVal, AVPVal) | ||
| when is_binary(ConditionVal) andalso is_atom(AVPVal) -> | ||
| AVPVal == binary_to_existing_atom(ConditionVal, utf8); |
There was a problem hiding this comment.
Might fail with badardg. You have to have it inside ''when''.
There was a problem hiding this comment.
Good point, binary_to_atom/2 should do here. The binary is coming from config, the chance for significant atom table pollution is minimal. Fixing.
test/diameter_avp_filter_SUITE.erl
Outdated
| ], | ||
| {Before, After} = do_filter_test(Filter), | ||
|
|
||
There was a problem hiding this comment.
extra spaces here and at 219 line
|
this PR is not need when we use travelping/ergw#417 |
I'm not (yet) convinced about that ... |
303b6c1
to
ef0bd80
Compare
ef0bd80
to
a089e35
Compare
Condition matching didn't work and expected when there were path branches behind the condition. AVP filter condition value matching made more flexible.