Make protocol and port parameters optional for policy based routing

[szvincze]

Currently proto and port fields of PolicyRoute struct are mandatory in ConnectionContext.

For this reason, if the user sets source port as any which means port range of 0-65535, then the whole range should be requested, which would end up quite a lot of policy routes.

Therefore these fields are expected to be optional.

If it implies change of the API, backward compatibility shall be considered.

[denis-tingaikin]

In my mind, if proto and port fields are any then we don't use policy-based routing.

[denis-tingaikin]

Probably am missing something... @szvincze Could you please clarify the use case and also how is ANY used in terms of vWire?

[edwarnicke]

@szvincze Normally I'd suggest treating the zero value as 'any' ... but it appears IP proto value of 0 is assigned to IPv6 Hop By Hop Option.

TCP/UDP port zero is reserved.

Given that, how would you feel about interpreting a zero port as 'any'.

For proto, would it make sense to its a little unclear how to proceed due to the unfortunate choice made of proto value 0 being assigned. I feel like the default' behavior if you don't set anything in the rule should be 'ANY' ... but we can't do that simply with leaving proto value 0. It feels like we may need some sort of bool to indicate that we should treat proto 0 as something other than ANY.

Thoughts?

[szvincze]

@edwarnicke, @denis-tingaikin Yes, we were thinking about similar kind of solution you described, with the additional bool.

[edwarnicke]

@szvincze Thoughts on naming of the bool?

[szvincze]

@edwarnicke Good question. First thought was any_port_indicator.

[edwarnicke]

@szvincze

@edwarnicke Good question. First thought was any_port_indicator.

I meant for the proto... for ports I think we are covered as port 0 is reserved (specifically so we can do things like interpret it as 'any' if we so choose).

[szvincze]

I meant for the proto... My fault. Of course, I wanted to write any_proto_indicator.

[edwarnicke]

@szvincze Sounds good :)

[LionelJouin]

Here are 2 proposals to solve the issue:

message PolicyRoute {
    string from  = 1;
    uint32 proto = 2;
    Range dest_port  = 3;
    repeated Route routes = 4;
}

message Range {
    uint32 start = 1;
    uint32 end = 2;
}

The user could let dest_port as nil, or he could add the range he wants using the new range message. If the user wants to have only 1 port in the policy, he could set start and end to the same value.

message PolicyRoute {
    string from  = 1;
    uint32 proto = 2;
    string dest_port  = 3;
    repeated Route routes = 4;
}

This is the one we discussed during the meeting. We would need to have a parser for the range.

We also have renamed the port property, so there will be no confusion between the destination and source (which could be extended later) port ranges.

Here is where the implementation of the IP rules should be modified: https://github.com/networkservicemesh/sdk-kernel/blob/2292537418c7c312ef8c2d0de2344e1f52040ef5/pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule/common.go#L102

[tedlean]

Hi

To keep it fully flexible for the future and not risk ending up in an "NBC-corner". I will suggest to use strings for everything.

We do not need to support all notations, like ranges, list etc. from day one. We can start by only supporting "empty" and a single string-integers. Later on if/when needed we can extend by adding support for a range notation etc. without causing an NBC.

Also I think there is an issue only having the dest_port specified:

For incoming traffic (where NSC acts as server), the reply traffic, which is the subject for PBR, the dest_port will normally be the ephemeral port chosen by peer client. This is of little interest for PBR. For this scenario PBR on src_port would be better.

For outgoing traffic (where the NSC acts as client) it will be the dest_port in the request that is of interest for PBR.

Consequently it should be possible to specify both dest_port or src_port depending on the traffic scenario.

So my fully flexible proposal would look like this:

message PolicyRoute { string src_ip = 1; string proto = 2; string dest_port = 3; string src_port = 4 repeated Route routes = 5; }

@edwarnicke @denis-tingaikin and all others of course, any comments?

BTW: The only usecase we (in Ericsson) need to have support for in NSM release 1.3 is being able to specify the src_ip .. nothing else. Next up would be proto. We do envision use-cases where src- and dst-port would be needed, but that is a bit further away as of now. Still this could turn overnight if required by an important customer.

[edwarnicke]

@tedlean This looks good to me API wise (other than a mild preference for dst_port rather than dest_port). Is @ljkiraly about to pick up the implementation?

[tedlean]

yeah.. "dst_port" of course. I think it will be @LionelJouin that starts this task.

[edwarnicke]

@LionelJouin Let me know if you need any help!

[LionelJouin]

Sure thank you

[edwarnicke]

@LionelJouin One quick pointer, I think you will find the bulk of the germane code here: https://github.com/networkservicemesh/sdk-kernel/tree/main/pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule