Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2362, Add source routing APIs to SB #2367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Jun 1, 2023

Checklist (Please check before submitting)

Describe the contribution
Add two new APIs that allow more control over the message routing. These allow the caller to directly specify the route (MsgId) and size of the content, and the message will be delivered based on the passed in values vs. the values inside the message itself.

This restructures the existing/historical API calls to use the same underlying mechanism. Send/Receive actions have a transaction object associated and this tracks the status and events. Common event reporting is done based on this transaction object.

Fixes #2362

Testing performed
Build and run CFE including functional test app
Build and run sanity tests

Expected behavior changes
Adds two new APIs. Existing APIs should be backward compatible, with some possible changes to the expected event IDs that might be generated under some off-nominal conditions.

System(s) tested on
Debian

Additional context
The new API allows the passed object to be any arbitrary data -- it does not need to be a CFE/CCSDS message at all. Therefore this could be more suitable for things like CFDP PDU packets, etc. as it would not need to be forced to look like a CMD/TLM message.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jun 1, 2023
@jphickey
Copy link
Contributor Author

jphickey commented Jun 1, 2023

Still have some (known) coverage and test workflow issues to solve in here, but wanted to get this going for CCB review today.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@jphickey jphickey force-pushed the fix-2362-sb-sourceroute branch 4 times, most recently from 4408c1c to 9a81cb6 Compare June 2, 2023 18:22
Add two new APIs that allow more control over the message routing.
These allow the caller to directly specify the route (MsgId) and size of
the content, and the message will be delivered based on the passed in
values vs. the values inside the message itself.

This restructures the existing/historical API calls to use the same
underlying mechanism.  Send/Receive actions have a transaction object
associated and this tracks the status and events.  Common event
reporting is done based on this transaction object.
@skliper
Copy link
Contributor

skliper commented Jun 12, 2023

I don't understand the driver behind adding complexity to SB to handle blobs of data that aren't self identifiable, this doesn't seem like a "natural" fit for pub/sub. Why not just wrap it with CCSDS if pub/sub is necessary, or implement point-to-point if it isn't? Simple tools for simple jobs vs trying to expand SB to do everything. Is this introducing a pattern of extending a core service beyond original intent, and if so what are the bounds? There's no longer embedded size, identification, common handling supported by binary blob messages, it's not clear why such an extended change makes sense in the open source context. Maybe just add a library that does wrapping/unwrapping for you and can be used by anyone who needs to send blobs, or some other way to avoid such major rewrites? My concern is now receivers need to know if it's a blob or not, which none of the standard receivers handle, so as soon as this is introduced there's an interoperability issue all over (not every subscribed MID can be checked for size or whatever).

@jphickey
Copy link
Contributor Author

Yeah, arbitrary data blobs are not really the emphasis/driving need behind this. Its more about the routing flexibility.

We already had the notion of the message itself as well as its "envelope" -- the CFE_SB_BuffertD_t that wrapped every message as it traversed the bus. The latter was used for routing (mostly) but mainly for convenience, some code would still peek at the message. This change makes the envelope address the authoritative one - that is, once a message gets put into its envelope, that envelope address becomes the one (and only) thing that is used for routing on the bus.

The fact that this permitted the message to be a binary blob was a side benefit - if nothing else, proving that delivery of a random binary blob works ensures that the delivery is being done based only on its envelope information.

This opens up a whole bunch of possibilities for future features:

  • Messages can be sent through intermediate delivery agent(s) - in case the dest is not directly reachable on the same SB subscription domain - they can go through one or more such hops if needed, on the way to their internal destination. This is the current need of the stakeholders and the driving reason behind this PR
  • Envelope size could be bigger than the internal message size, with the extra space for things like a CRC and/or cryptographic signature for authentication. This is also currently needed by the stakeholders, although there are other was of doing this that have been used, it is more complex and have been a big maintenance issue. This would simplify.
  • Envelope content could be encrypted (might not be useful till we get isolated memory spaces, but still...). Although this is not currently needed by the stakeholders right now, its been on the roadmap for some time.

If the concern has to do with the (possibility) of sending a binary blob and confusing a subscriber, we could conceivably restrict content to "Valid messages" even with this new API, and add some checks that the message attempting to be sent seems like an actual message. I think all we could do is confirm that the CFE_MSG_GetSize() and CFE_MSG_GetMsgId() return reasonable values, which is really the only checks the currently calls do (that is, today one could generate a message that is complete garbage but as long as these two calls output values that are within range, the blob can be sent). That isn't a particularly robust test of a message sanity, but its all we can do, and all we ever really did up to this point even with the current APIs.

@skliper
Copy link
Contributor

skliper commented Jun 13, 2023

While I get you could do these things all within SB using the SB internal "envelope" concept, I don't agree with such a temporary nature or adding complexity to SB to do things that also could require changes to every sender/receiver. This concept seems to tie directly with SB, where it's dependent on these new APIs, only SB has the envelope information, it's gone as soon as delivered, etc. If the envelope is part of the message (wrapped) that dependency is eliminated, the CCSDS spacepacket header is already an envelope and you can layer as needed to add all the functionality above yet maintain independences from SB "envelope" magic. Wrapping w/ additional protocols (or encapsulating protocols in CCSDS space packets) has been common practice and eliminates the strict dependency on SB to get this all done. You could keep the SB/MSG independence and just update MSG to handle whatever envelope/wrapper information is needed and it can be customized/localized to the specific projects that need it. If folks really want this to work seamlessly across the common apps I just don't see how adding APIs and requiring special handling on the receiving side is the preferred approach.

@jphickey jphickey removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add source-routing feature to Software Bus
2 participants