[RFC PATCH 00/20] Context typed filters

[Asbjørn Sloth Tønnesen]

Hi Ondrej,

Thanks for the feedback.

On 1/6/20 3:16 AM, Ondrej Zajicek wrote:
> Thanks for the patchset, sorry for not giving feedback sooner, was on
> vacation.

No problem, I was expecting that. I am also first responding now, since I
have been under the weather for the past few days.


> If i understand it correctly, the main point of this patchset is to
> introduce 'typed symbols', which have multiple values based of network
> type of processed route.

Correct. The patches allows for explicitly typed constants, filters and
functions. If a filter or function uses a typed symbol, it will implicitly
be typed itself. Once the context is know that version of the filter will
be generated.


> I must say that i do not like the approach taken in the patch, seems to
> me that for intended purpose it is too intrusive change and the feature
> is too idiosyncratic - i don't recall any language where a constant may
> represent multiple unrelated values based on context.

I agree that the patch set is a little intrusive, but I decided that it
was good enough for a PoC and to get the discussion started, hence
labeling it as an RFC patch.


In bird 1 it was possible to have bird4.conf and bird6.conf share common
configuration files, but have 2 different files defining symbols.

In bird 2 this ends out with a lot of symbols ending in "4" or "6",
and quite a few case statements or to do address family specific processing.
The case statements could be hidden behind a case-function per data type.

function get_typed_pfx_set(prefix set v4; prefix set v6)
{
         case net.type {
                 NET_IP4: return v4;
                 NET_IP6: return v6;
         }
}

Even this leads to long lines like: get_typed_pfx_set(AS65000_prefixes4, AS65000_prefixes6),
including the need to pass both prefix sets, through generic filtering functions.
Overall leading to an idiosyncratic config.

I would hope that people would only use this feature to store related values,
however I have only enforced that they must have the same data type.


> It is also over-specific (dispatch hardcoded for net_type, while there
> are other, context-specific properties, like protocol type).
I made the syntax and symbol part very net_type specific, since I'm not aware
of other context parameters where it would make a lot of sense.

I specifically choose not allow a fallback value, ie. calling cf_error
if a symbol is not defined for a given net_type.

With protocol typing I see it as more useful for protocol specific
overrides, so maybe a fallback value should be allowed?

Does anybody else have any feedback on this?


> One way to make it less intrusive would be to keep symbol table the same
> (just symbol->value mapping) and do the change just in 'value' part.
I don't have a problem with converting it to a linked list of versions of the symbol.


> You have this example:
> 
> define default_route:ipv4 = 0.0.0.0/0;
> define default_route:ipv6 = ::/0;
> 
> This could be done by using function instead of constant:
> 
> function default_route()
> {
>    case net.type
>    {
>      NET_IP4: return 0.0.0.0/0;
>      NET_IP6: return ::/0;
>    }
> }
 >
> I agree that it is more cumbersome than your version.

I used default route mostly as an easy to understand example, the big
benefit comes with prefix filtering hence the reference to bgpfilterguide.


> Seems to me that your intended objective could be achieved, while keeping
> it universal and reasonable, by several independent changes:
> 
> 1) Allow 'lazy-evaluated constants', that could depend on context. These
> are essentially zero-argument functions in disguise.
 >
> 2) Allow something like if/case expressions (instead of just statements).
> 
> 3) (optional) partially-evaluate/adapt filters based on context.

When we originally discussed this idea during RIPE78 in Reykjavik, we also
talked about that it should be possible to optimize it parse time.

Therefore I wanted to explore the feasibility of doing that, the shortcut
I took here, by only having a thin adaptation layer, is that I had to mark
it as never constant. That could be fixed by moving most of the linearization
work to the adaptation state, leaving only a loop check the current
linearization stage.

The benefit of doing the adaptation at parse time, means that non-existent
variants of a symbol can be revealed before a new config is loaded.

The redundant case statements during run-time is not an issue for us,
but I imagine it could be to some of the route server operators.


> So one could wrote something like:
> 
> define default_route = case net.type {
>    NET_IP4: 0.0.0.0/0;
>    NET_IP6: ::/0;
> }
This syntax could work, but should just result in the entries being put in
a linked-list under the symbol, so that it can be guaranteed that they all have
the same type.

I prefer to be able to define constants as several individual statements.

If constants can be defined for other parameters than net_type, should then
also be possible to do it multi-dimensional? Fx. for ipv4 in protocol x.


> (The remaining wart is a current style of enum constants, like 'NET_IP4'.)

I'm aware that net.type uses the C constants, but I choose to use `net_type`,
since it is consistent with table and channel type definitions.


-- 
Best regards
Asbjørn Sloth Tønnesen