eclipse-uprotocol/up-cpp

Bug in URI Validator forcing to set service_instance_id to be 1

Closed this issue · 4 comments

It is perfectly valid if the ue_id (32bits) has the upper 16 bits (the instance_id portion) set to 0 meaning we are not caring about the instance id however isValidRpcMethod(), isValidRpcResponse(), isValidPublishTopic() all will fail if the upper 16 bits are 0 because uses_wildcards() returns true when the upper 16 bits is 0.

Examples of the issue can be found in https://github.com/eclipse-uprotocol/up-cpp/blob/main/test/coverage/datamodel/UUriValidatorTest.cpp#L43.

I confirmed that the specifications is ambiguous to this point so I will also add an issue to up-spec to fix this, eclipse-uprotocol/up-spec#217

There has been some fairly extensive discussion on this. There are a few issues here.

First, overloading the value of 0 to be both a valid instance ID and the wildcard filter for instance IDs creates a scenario where developers could easily and unintentionally create invalid configurations (such as having two instances with ID 0 and 1 - instance 0 would receive all messages for all instances and instance 1 would receive only its messages). There's no clear way for us to enforce a rule that if 0 is used as an instance ID, other values should not be used (for a given entity ID).

Second, it was mentioned that an instance ID of 0 cannot be used directly in the SOME/IP transport format, and so an instance ID of 0 is always rewritten to be 1 in SOME/IP. This creates an ambiguous mapping since both instance ID 0 and instance ID 1 could be represented as instance ID 1 over SOME/IP. This once again invites misconfiguration and unclear behavior.

Third, we want to preserve instance ID 0 as a valid instance ID since protobuf will pack integers into smaller fields to avoid sending unnecessary 0 bytes. Many entities may have only one instance, so in aggregate this one byte per message savings is non-trivial. This means we can't solve the above issues by stating that valid instance IDs start at 1 and reserve 0 to be the wildcard filter value.

Given those issues, my recommendation is to update the spec as follows:

  1. Make the instance ID wildcard value 0xFFFF. Since the filters will rarely be sent on the wire, this preserves 0 as a normal instance ID. This also has benefits when doing pattern matching with things like bloom filters, and is consistent with the pattern used in all other numeric UUri fields.

  2. Disallow 0xFFFF as an instance ID value in message attributes (just like all the other URI field wildcards).

  3. For uProtocol to SOME/IP mapping, always add 1 to the instance ID. For SOME/IP to uProtocol mapping, always subtract 1 from the instance ID. 0xFFFF would be allowed in SOME/IP messages since it represents a uProtocol instance ID of 0xFFFE

Labeling this bug as blocked until agreement is reached on whether a spec update is needed.

The spec is being updated. We will need to update up-cpp's handling of instance ID to treat 0xFFFF as the wildcard value now. See eclipse-uprotocol/up-spec#222

This fix should target the v1.0_up-v1.6.0 branch