IP and policy removal not working on connection update

[LionelJouin]

After some testing with this change: networkservicemesh/sdk#1231. I found it was possible to add new IP addresses and policy routes by just requesting (updating) a new time without having to close the connection.

The issue I noticed is that it is not possible to remove any IP or policy route while they are removed from the IP context in the request.

[denis-tingaikin]

I feel this is totally related to networkservicemesh/cmd-forwarder-vpp#441

To resolve the issue we should rework each chain element from sdk-vpp and add a chec on change.
If we have a change then we need to delete previosly created things.
otherwise do as we do currently.

[denis-tingaikin]

cc @edwarnicke

[LionelJouin]

Right, it is related. Just in my use case, the NSC is intentionally changing the content of the IP context. From my tests, the ipadress and iprule chain elements are working fine for adding but not for removing.

[edwarnicke]

@LionelJouin That likely is related to needing to update the chain elements that do that work.

@denis-tingaikin One other thing to watch carefully is that we are only doing the work on the return stroke of the call in those chain elements. If the NSE is OK allowing the NSC to update IP context... that's fine... but its not OK to act on NSC requests to update IPContext until the NSE has returned those updated (meaning it thinks they are valid).

@LionelJouin Does that make sense to you?

[LionelJouin]

yes, it's clear to me.

[denis-tingaikin]

@LionelJouin networkservicemesh/sdk#1234 is merged. Could you test is it solves the problem?

[LionelJouin]

I tried it, it works fine, It solved this issue: networkservicemesh/sdk#1231

[edwarnicke]

@LionelJouin and does it also solve your issue here with IP policy removal on connection update? Or is that still open?

[LionelJouin]

no, it is not solved, this issue is still open (#426).

[LionelJouin]

@edwarnicke @denis-tingaikin How the forwarder resiliency could be handled? Handling IP addresses would be easy since it is possible to get the IP addresses by checking the interface directly and then compare them to the ones in the IPContext to find which ones have to be added or removed. But I believe this would be more complex for the policy routes, how the forwarder could remember which rule/table belongs to which policy? (in case the forwarder crashes/restarts/...)

[glazychev-art]

Hello @LionelJouin,
Maybe I misunderstood you. Is the problem still present after this fix? - #437

[LionelJouin]

Hi, yes, I think so. For instance, if the forwarder restarts, then the policies map will be emptied, and then the forwarder will loose track of which ip tables/rules it was managing previously.

[glazychev-art]

Ah.. got it.
So, if forwarder restarts - client starts healing with the connection, where we should have our rules. And I think we can restore them in forwarder.
At this line we can check tables map, and if it is empty for a given connection Id - try to find added table IDs using netlink.RuleList().

[edwarnicke]

@LionelJouin One other thing to keep in mind is that if your NSE wants to trigger a change... it should probably use:

https://github.com/networkservicemesh/api/blob/main/pkg/api/networkservice/connection.proto#L36

Which is to say:

1. NSE sends a ConnectionEvent over Monitor towards the NSE with State set to REFRESH_REQUESTED
2. This will cause the NSC to send a new Request
3. The NSE can return a changed Connection.ConnectionContext

Basically... this way you don't have to wait around for a Refresh to happen at Expiration.

[glazychev-art]

@LionelJouin
As far as I can see, all main PRs have been merged.
Can we close this issue?

[LionelJouin]

There is still 2e2 tests to develop and other properties to support. We could keep this issue, otherwise I would open new ones for the e2e and properties

[edwarnicke]

@LionelJouin Agree on tracking the other issues. I'd suggest opening new issues and referencing this one.