Reject duplicate P4Runtime election IDs from multiple controllers

[pudelkoM]

P4Service currently does not reject duplicate election IDs from controllers. This leads to a data corruption when a slave controller sends a MastershipUpdate with the same election ID as the current master:

• 3 Controllers connected: 1 (1111, slave), 2 (2222, master), 3 (1212, slave)
• C1 sends arb. update with election id 2222, same as current master
• Call entry: AddOrModifyController 1, 2222, ipv4:127.0.0.1:56994.
• Controller set at beginning of call: Controller (2, 2222, ipv4:127.0.0.1:56994), Controller (3, 1212, ipv4:127.0.0.1:56994), Controller (1, 1111, ipv4:127.0.0.1:56994), .
• C1 gets deleted from set: Controller (2, 2222, ipv4:127.0.0.1:56994), Controller (3, 1212, ipv4:127.0.0.1:56994), .
• Try to insert C1 into set, but election ID already exists, C1 lost: Controller (2, 2222, ipv4:127.0.0.1:56994), Controller (3, 1212, ipv4:127.0.0.1:56994), .
• Stratum falsely logs C1 as new master: Controller (connection_id: 1, election_id: 2222, uri: ipv4:127.0.0.1:56994) is connected as MASTER for node (aka device) with ID 123123123.
• C2 receives update: Controller #2 mastership update: arbitration { device_id: 123123123 election_id { low: 2222 } status { } }
• C3 receives update: Controller #3 mastership update: arbitration { device_id: 123123123 election_id { low: 2222 } status { code: 6 message: "You are not my master!" } }
• No updates sent to C1 and the stream is still open, but not recognized by Stratum as a controller anymore
• C1 might thinks it's master, but all calls will lead to ERR_PERMISSION_DENIED errors

This happens because we store Controllers in a set with a custom comparison function. Controllers are not hashed, but only compared by their election ID. Thus, two controllers with the same election ID are not possible.

stratum/stratum/hal/lib/common/p4_service.h

Lines 235 to 240 in 34a75cd

	// Map from node ID to the set of Controller instances corresponding to the
	// external controller clients connected to that node. The Controller
	// instances for each node are sorted such that the master (Controller
	// with highest election_id) is the first element.
	std::map<uint64, std::set<Controller, ControllerComp>> node_id_to_controllers_
	GUARDED_BY(controller_lock_);

stratum/stratum/hal/lib/common/p4_service.h

Lines 81 to 87 in 34a75cd

	// Custom comparator for Controller class.
	struct ControllerComp {
	bool operator()(const Controller& x, const Controller& y) const {
	// To make sure controller with the highest election_id is the 1st element
	return x.election_id() > y.election_id();
	}
	};

This PR changes this by checking for insertion success. Failure indicates a duplicate and we close the stream, all according to spec:

P4RT Spec - Section 5.3:

Otherwise, if the MasterArbitrationUpdate message is received from an already connected controller:

If the election_id is set and is already used by another controller (excluding the controller making the request) for the same (device_id, role_id), the P4Runtime server shall terminate the stream by returning an INVALID_ARGUMENT error.

Additional considerations

Stratum is not fully P4RT spec compliant, with or without this fix. While this change brings us closer, a gap remains. At some point we should consider delegating P4RT related code to an external, well tested library.

Outright closing the stream without telling the offending controller the currently highest election ID puts the controller an an interesting situation:

• To become master it has to know the current ID, which is only broadcast to backups
• To become backup it has to find a lower, free election ID by guessing or bruteforce

[codecov]

Codecov Report

Merging #857 (333d7f6) into main (05fa2e6) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##             main     #857   +/-   ##
=======================================
  Coverage   78.55%   78.56%           
=======================================
  Files         334      334           
  Lines       30054    30057    +3     
=======================================
+ Hits        23610    23613    +3     
  Misses       6444     6444           

Impacted Files	Coverage Δ
stratum/hal/lib/common/p4_service.h	100.00% <ø> (ø)
stratum/hal/lib/common/p4_service.cc	82.93% <80.00%> (+0.09%)	⬆️

[pudelkoM]

@pierventre FYI

[pierventre]

@pudelkoM regarding your additional comments, I agree on using a well tested library. I am not fully sure about your final remarks - in theory an ONOS cluster should not need this and each instance can potentially guess the election id of the other instances. Happy to discuss today. Thanks a lot for this hot fix.

[pierventre]

tested - works great

[bocon13]

LGTM