integrate with multicast menu api #2

Laur04 · 2022-04-10T00:24:53Z

Multicast Menu is now hosted at http://18.209.44.34 (DNS/SSL pending).

These changes are designed to replace the current method of adding streams to MM with the new API. The API also allows for the removal of streams, with the intent that the translator cleans up after itself. I've made these changes by adding two dictionaries, one to track activity and one to track the unique access code used to control each stream. A loop counter was also added to assist in tracking activity.

These changes will work as is, but a crucial step in full functionality is going to be allowing access to this server from the above IP address.

This relates to the concept of an "inside_request." This is very similar to a typical translation, except the sender is MM itself. This is intended to allow someone to upload a file to MM -> MM streams the file to the translation server -> TS makes an API call (with the "inside_request" parameter set) -> MM matches that API call to the file that it sent and updates s-g accordingly.

This will not work until MM can initiate that request to TS, i.e. it is whitelisted.

These changes are largely untested. On the MM side, the API calls will work (I dummy tested them). However, I don't know the best way to go about testing this without disrupting the current operation.

Thoughts?

jvmk

Hi Lauren,

My apologies for the slow response.

First of all, great work! Thank you!

I've added some in-line comments with some thoughts/notes/ideas. Please note: these are just ideas/thoughts and are not to be understood as "mandatory in order to approve the PR". Let me know what you think, and we take it from there :).

BTW, we could also consider using public/private key based authentication between the TS and the MM. Something like: (i) the MM issues a challenge to the TS whenever the TS wants to delete a stream, (ii) the TS solves the challenge, signs the result with its private key, and sends the signed result to the MM, (iii) the MM verifies the authenticity and correctness of the result using TS's public key. Might be way overkill though - we probably don't need to worry too much about MM entry manipulation from outside sources at this point :).

As for whitelisting the MM IP at the TS: reach out to Andrew Gallo to have him whitelist it, if you haven't already.

I wouldn't worry too much about disrupting the current setup; it's just experimental AFAIK, so I don't see any issue in you killing the translator that's running on Andrew's server, fetching your changes, and then starting it again to try out the changes. But maybe check with Lenny just in case.

Best,
Janus

P.S. feel free to ping me on Slack if you want to discuss this in more depth over a Zoom call.

constants.py

translator.py

jvmk · 2022-05-12T04:34:13Z

translator.py

+        for src_addr in self._fib:
+            if self._loop_counter - self._atb[src_addr] > 20:
+                # Remove this stream from the FIB
+                self._allocated_mcast_addrs.remove(self._fib[src_addr])


I think we need to add locking when modifying this set and the FIB (and your two new dicts) to avoid race conditions as we're doing some modifications in worker threads. This is tricky (and I'm not sure I got locking right myself in the first version).

jvmk · 2022-05-12T04:40:18Z

translator.py

+                # Clean up dictionaries
+                del self._fib[src_addr]
+                del self._atb[src_addr]
+                del self._acm[src_addr]


I think there is a potential race condition here. The access code comes from the response from the MM, which is fetched asynchronously and may thus be in-flight when clean-up is triggered. Thus you can end up in a situation where you'll be trying to remove a key from _acm that have not yet been inserted (but is just about to be). Hope that makes sense.

I don't think this is an issue, but let me know if my reasoning is flawed here.

When a stream is added to Multicast Menu (when it is first identified) the key is put in _acm in a worker thread just after the stream is added to _fib. This code that attempts to remove that entry from _acm (and _atb and _fib) won't run until at least 20 seconds after the stream is added. Additionally, the workers for adding and removing from MM are pulled from the same threadpool, meaning (I think?) that the add task is certain to be run before the remove task because it gets called first.

Laur04 · 2022-07-08T15:52:19Z

Okay, sorry this took so long to get back to. I updated the PR with everything that you commented on. More specifically:

Switched Multicast Menu UID to a command-line argument that effectively toggles whether the translator will attempt to add streams to Multicast Menu
Updated Multicast Menu related constants
Created a buffer for removing items from fib in _identify_inactive_streams()
Changed _loop_counter structure to check for inactive streams every 30 loops
Changed inactive stream identification to rely on individual streams' last seen time instead of global loop count
Attempted to add locking to _identify_inactive_streams()
Updated documentation to reflect changes

When it comes to locking, I'm far from an expert, so I just copied your setup. Let me know what your thoughts are on the updated changes.

jvmk

Mostly aesthetics :)

jvmk · 2023-07-18T06:22:48Z

README.md

@@ -100,6 +100,7 @@ usage: translator.py [-h] [--unicast-nif-ip UNICAST_NIF_IP]
                     [--unicast-port UNICAST_PORT]
                     [--multicast-addr-space MULTICAST_ADDR_SPACE]
                     [--multicast-port MULTICAST_PORT]
+                     [--mm_uid MULTICAST_MENU_UID]


In the example above it is python3 translator.py --mm-uid SAMPLE_MM_UID, so I think this should be [--mm-uid MM_UID] (i.e., --mm-uid, not --mm_uid, and corresponding all caps placeholder (which will use _, not -). Alternatively just replace this entire section with the output of python3 translator.py --help (and update the example above with the option name you pick)

jvmk · 2023-07-18T06:23:41Z

README.md

@@ -125,6 +126,10 @@ optional arguments:
                        port number will be used for all translated flows.
                        Thus, a translated flow is identified solely by its
                        assigned multicast IP address (group). Default: 9002
+  --multicastmenu_uid MULTICAST_MENU_UID


In the example above it is python3 translator.py --mm-uid SAMPLE_MM_UID, so I think this should be --mm-uid MM_UID ? Copy paste from python3 translator.py --help after you've settled on an option name.

jvmk · 2023-07-18T06:27:45Z

translator.py

@@ -317,15 +360,20 @@ def _add_to_multicast_menu(self, mcast_dst_ip, email, description):
        'multicast IP address (group). Default: %(default)d'
    ap.add_argument(f'--{mcast_port_argnmame}', type=int, default=constants.DEFAULT_MULTICAST_PORT, help=h)

+    mm_uid = 'multicastmenu-uid'


In the example above it is python3 translator.py --mm-uid SAMPLE_MM_UID, so I think this should be mm_uid = 'mm-uid' ?

jvmk · 2023-07-18T06:29:51Z

translator.py

@@ -317,15 +360,20 @@ def _add_to_multicast_menu(self, mcast_dst_ip, email, description):
        'multicast IP address (group). Default: %(default)d'
    ap.add_argument(f'--{mcast_port_argnmame}', type=int, default=constants.DEFAULT_MULTICAST_PORT, help=h)

+    mm_uid = 'multicastmenu-uid'
+    h = 'UID to use when identifying the translator account on Multicast Menu. Default: %(default)d'


The default value is None, so you need to format it as a string (not a digit) in the help text (Default: %(default)d --> Default: %(default)s). I think you should see an error (TypeError: %d format: a real number is required, not NoneType) when you do python3 translator.py --help if you use digit formatting here.

Alternatively, we can also make this a required argument (by changing it to a positional argument or by setting required=True) if it doesn't make sense to run the translator without this value present. But I like that it is optional as someone might want to run the translator without necessarily publishing information about streams to MM.

jvmk · 2023-07-18T07:29:17Z

translator.py

                    if mcast_addr is not None:
                        # Forward the payload to the multicast address allocated for this client.
                        self._mcast_sckt.sendto(payload, (str(mcast_addr), self._mcast_dst_port))
+                        # Update Activity Information Base
+                        self._atb[src_addr] = datetime.now()


I think we need to add locking around this line as it is a mutating operation and you run self._identify_inactive_streams on a different thread, which also manipulates this dict

jvmk · 2023-07-18T07:37:28Z

translator.py

                    else:
                        # mcast_addr will be None if we've run out of addresses.
                        # TODO silently discard the packet or communicate this to the client?
                        print(f'WARNING: run out of multicast addresses, cannot serve {src_addr}')
                except socket.timeout:
                    # No data available to read during this iteration.
                    print('read timeout: nothing to be translated this iteration')
+                # Determine if any streams are newly inactive
+                if self._loop_counter == 30:
+                    self._mcastmenu_threadpool.submit(self._identify_inactive_streams)


Add todo comment here: # TODO move this work to a different thread(pool) dedicated to handling clean up

_mcastmenu_threadpool is for sending network requests to the MM. Not much difference from a functional perspective (so no need to change this now), but just to keep things clean.

jvmk · 2023-07-18T07:42:08Z

translator.py

+        Loop through the streams in the FIB and determine which have not been seen in a while.
+        """
+        try:
+            self._lock.acquire()


add todo comment # TODO figure out how to do more granular locking to avoid blocking translation thread for a long time

jvmk · 2023-07-18T07:53:20Z

translator.py

+
+                        resp_post = requests.post(constants.MULTICASTMENU_REMOVE_URL, data=options)
+
+                        if resp_post.status_code != 201:


HTTP status code 201 indicates resource creation, but we're doing the opposite here (deletion). Don't change this here if the API currently returns 201 for this operation - just a heads up in case it's a typo here.

Laur04 requested review from jvmk and natzberg April 10, 2022 00:24

Laur04 linked an issue Apr 10, 2022 that may be closed by this pull request

Reclaim multicast addresses #1

Open

Laur04 force-pushed the main branch from 3010cb1 to c185ab3 Compare April 11, 2022 19:06

Laur04 changed the title ~~intergrate with multicast menu api~~ integrate with multicast menu api Apr 11, 2022

jvmk reviewed May 12, 2022

View reviewed changes

Laur04 force-pushed the main branch from c185ab3 to 4f77b7d Compare July 8, 2022 15:50

Laur04 requested a review from jvmk July 8, 2022 15:52

Laur04 force-pushed the main branch from 4f77b7d to 28233f8 Compare July 13, 2022 17:10

integrate with multicast menu

28233f8

Laur04 force-pushed the main branch from 537e84f to 51e5c82 Compare July 27, 2022 16:40

add print statement for inside requests

51e5c82

jvmk suggested changes Jul 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate with multicast menu api #2

integrate with multicast menu api #2

Laur04 commented Apr 10, 2022 •

edited

Loading

jvmk left a comment

jvmk May 12, 2022

jvmk May 12, 2022

Laur04 Jul 8, 2022

Laur04 commented Jul 8, 2022

jvmk left a comment

jvmk Jul 18, 2023 •

edited

Loading

jvmk Jul 18, 2023 •

edited

Loading

jvmk Jul 18, 2023

jvmk Jul 18, 2023 •

edited

Loading

jvmk Jul 18, 2023

jvmk Jul 18, 2023

jvmk Jul 18, 2023

jvmk Jul 18, 2023


		resp_post = requests.post(constants.MULTICASTMENU_REMOVE_URL, data=options)

		if resp_post.status_code != 201:

integrate with multicast menu api #2

Are you sure you want to change the base?

integrate with multicast menu api #2

Conversation

Laur04 commented Apr 10, 2022 • edited Loading

jvmk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laur04 commented Jul 8, 2022

jvmk left a comment

Choose a reason for hiding this comment

jvmk Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

jvmk Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvmk Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laur04 commented Apr 10, 2022 •

edited

Loading

jvmk Jul 18, 2023 •

edited

Loading

jvmk Jul 18, 2023 •

edited

Loading

jvmk Jul 18, 2023 •

edited

Loading