[uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes #247

Shastick · 2023-10-12T08:06:00Z

This should address Issue #244.

It also updates the markdown doc for this check, which was erroneously referencing NET0500 instead of NET0210

BenjaminPelletier · 2023-10-12T19:01:47Z

monitoring/uss_qualifier/scenarios/astm/netrid/common/misbehavior.py

@@ -167,7 +167,7 @@ def _evaluate_and_test_authentication(
        no flights were yet returned by the authenticated queries.
        """

-        with self.check("Missing credentials") as check:
+        with self.check("Missing credentials", [self._dss.participant_id]) as check:


When we query all_flights below, that operation first queries the DSS to identify which Service Providers need to be queried, and then it queries all the Service Providers. The unauthenticated session is then used to repeat the uss_flight_queries (the queries to all the Service Providers). If one of the Service Providers incorrectly allows an unauthenticated request, the provider of the DSS (self._dss.participant_id) isn't the participant that failed the requirement. Likewise, if the unauthenticated queries to each Service Provider are all correctly rejected, we haven't shown the DSS provider's compliance to the requirement. The participants identified in a check should correspond to the participants we are verifying comply (or rather, don't fail to comply) with a requirement. In this case, we're checking whether each Service Provider complies with the requirement to only allow authenticated requests.

The with syntax for the check is just an abbreviation for "if I didn't record_failed in this block, then record_passed when I exit the block". So, when the block exits, we will currently record that the DSS provider passed this check if no USSs failed the check. If any USSs failed the check, we will not record that the DSS provided passed this check. I don't think that's what we want.

I think we want to keep the with check blocks as small as practical in general -- ideally just a single if statement. It's not necessarily bad to perform a lot of actions in the check block, but I think it does make it more challenging to reason about what will be indicated in the report under various circumstances.

Indeed, I went over this a bit too quickly.

I updated the PR so the check is now within the for-loop. The check is now also attached to the server_id with which the request was tagged.

I was considering adding a separate check stating "DSS can successfully be queried", but the misbheavior.py test is actually about testing SP's so I figured I'd rather not add a check mentioning the DSS?

edit: though I just noticed that the query is not properly tagged anyway, and that there's a bit more work required to map queries back to the relevant USS

I was considering adding a separate check stating "DSS can successfully be queried", but the misbheavior.py test is actually about testing SP's so I figured I'd rather not add a check mentioning the DSS?

Whenever someone might fail to meet a requirement, especially one that would affect the behavior of the test, it's probably worth making a check for that. In this case, the DSS provider is important to the test and we probably want fail something if we can't get information from the DSS. So, I think it's worth having an explicit check that applies to the DSS provider which fails if we can't get the information (and therefore can't proceed with the test), even though the DSS provider is not the primary focus of this test.

Shastick · 2023-10-13T09:31:17Z

Taking this back to draft until I can properly define which participant a query belongs to.

Shastick · 2023-10-13T13:02:42Z

@BenjaminPelletier this is ready for another pass of review.

This PR grew a little bit: I found out that quite a few queries were getting attributed to the wrong participants in rid.all_flights.

The suggested change is to solve this with some additional configuration information.

The test suite might fail because there are some unattributed queries that are detected in the aggregated checks, and I still need to check if these can be ignored or not (they involve example.com and the v22 endpoints when the v19 checks were run)

Shastick · 2023-10-13T17:50:28Z

monitoring/monitorlib/fetch/rid.py

@@ -1163,7 +1164,7 @@ def all_flights(
    session: UTMClientSession,
    dss_base_url: str = "",
    enhanced_details: bool = False,
-    server_id: Optional[str] = None,
+    dss_server_id: Optional[str] = None,


Note: did a few renames here and there to make certain things clearer.

Shastick · 2023-10-13T17:51:51Z

monitoring/uss_qualifier/resources/netrid/service_providers.py

@@ -41,25 +41,27 @@ class NetRIDServiceProvidersSpecification(ImplicitDict):

 class NetRIDServiceProvider(object):
    participant_id: str
-    base_url: str
-    client: infrastructure.UTMClientSession
+    injection_base_url: str


Here the rename is to indicate that this is the URL used to inject flights, nothing more.

(Any place I found where this is set, this was the case, but please correct me if I got the intent wrong).

The rename should reduce the chances that anyone gets tempted by using this to match queries to SP's :)

Shastick · 2023-10-13T17:52:47Z

monitoring/uss_qualifier/resources/netrid/service_providers.py

-    base_url: str
-    client: infrastructure.UTMClientSession
+    injection_base_url: str
+    flights_injection_client: infrastructure.UTMClientSession


Same for the client/session: I guess it might work to query other endpoints on an SP but it does not seem to be its purpose.

…eruss#244

mickmis · 2023-10-20T10:31:53Z

I made a pass on this PR to in order to finalize it. I ended up doing a bit more though:

create unauthenticated session through overriding correct audience
rename all the server_id to participant_id for the sake of consistency; I took the opportunity of doing that in this PR, that's been bothering me for a while (sorry, it does add noise to the PR)
factor away telemetry match + tolerance constants in monitorlib.geo package
factor away part of telemetry polling from both scenarios in the virtual observer
refactor/cleanup parts of the scenarios (notably use the fetch functions even for unauthenticated queries)
factor away/cleanup inject_flights + injected_flights_errors in injection.py
clarify some things related to setting participant IDs in display_data_evaluator
fix attribution of query type in uss_flights

~~(note: at the time of writing this the PR is still not ready for review and need some fixes)~~

There is actually more I wanted to do because I find that display_data_evaluator.py became a bit confusing over the time IMO, but that's enough for now I believe.

BenjaminPelletier · 2023-10-20T17:32:10Z

monitoring/uss_qualifier/scenarios/astm/netrid/display_data_evaluator.py

+        injected_flights, observed_flights
+    )
+
+    # TODO: a better approach here would be to separately map flights URL to participant IDs based on all TelemetryMapping encountered, and set retroactively the participant ID on all queries


…some refactorings and fixes (#247) * [uss_qualifier] attach participant id to net0210 check, fix Issue #244 * rename server_id -> participant_id * factor out polling; refactor some parts of scenarios * add aud override in NoAuth * fix participant_id setter * fix several issues * fix small issues * add TODO --------- Co-authored-by: Mickaël Misbach <[email protected]> Co-authored-by: Mickaël Misbach <[email protected]> 9753ad1

Shastick force-pushed the fix-#244 branch from 3247082 to 3d305b3 Compare October 12, 2023 08:06

Shastick requested a review from BenjaminPelletier October 12, 2023 09:54

BenjaminPelletier reviewed Oct 12, 2023

View reviewed changes

Shastick force-pushed the fix-#244 branch 2 times, most recently from 8f53270 to cbe7e08 Compare October 13, 2023 09:23

Shastick requested a review from BenjaminPelletier October 13, 2023 09:26

Shastick marked this pull request as draft October 13, 2023 09:30

Shastick force-pushed the fix-#244 branch 4 times, most recently from f721f83 to 9e27793 Compare October 13, 2023 12:59

Shastick force-pushed the fix-#244 branch from 9e27793 to 6e28475 Compare October 13, 2023 17:46

Shastick commented Oct 13, 2023

View reviewed changes

[uss_qualifier] attach participant id to net0210 check, fix Issue int…

e12193c

…eruss#244

Shastick force-pushed the fix-#244 branch from 6e28475 to e12193c Compare October 13, 2023 17:56

mickmis added 6 commits October 20, 2023 10:25

rename server_id -> participant_id

ebc73de

factor out polling; refactor some parts of scenarios

2d94d2e

Merge remote-tracking branch 'interuss/main' into fix-interuss#244

e39aa47

add aud override in NoAuth

37a25bd

fix participant_id setter

2ed9e29

fix several issues

bf64618

mickmis added 2 commits October 20, 2023 14:05

fix small issues

23db813

add TODO

da4f862

mickmis marked this pull request as ready for review October 20, 2023 12:19

mickmis changed the title ~~[uss_qualifier] attach participant id to net0210 check, fix Issue #244~~ [uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes Oct 20, 2023

mickmis linked an issue Oct 20, 2023 that may be closed by this pull request

Test participant(s) not indicated for NET0210 #244

Closed

Merge branch 'main' into fix-interuss#244

433eb55

BenjaminPelletier approved these changes Oct 20, 2023

View reviewed changes

BenjaminPelletier merged commit 9753ad1 into interuss:main Oct 20, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes #247

[uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes #247

Shastick commented Oct 12, 2023

BenjaminPelletier Oct 12, 2023

Shastick Oct 13, 2023 •

edited

Loading

BenjaminPelletier Oct 13, 2023

Shastick commented Oct 13, 2023

Shastick commented Oct 13, 2023

Shastick Oct 13, 2023

Shastick Oct 13, 2023

Shastick Oct 13, 2023

mickmis commented Oct 20, 2023 •

edited

Loading

BenjaminPelletier Oct 20, 2023

[uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes #247

[uss_qualifier/netrid] fix #244; rename server_id to participant_id; some refactorings and fixes #247

Conversation

Shastick commented Oct 12, 2023

BenjaminPelletier Oct 12, 2023

Choose a reason for hiding this comment

Shastick Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

BenjaminPelletier Oct 13, 2023

Choose a reason for hiding this comment

Shastick commented Oct 13, 2023

Shastick commented Oct 13, 2023

Shastick Oct 13, 2023

Choose a reason for hiding this comment

Shastick Oct 13, 2023

Choose a reason for hiding this comment

Shastick Oct 13, 2023

Choose a reason for hiding this comment

mickmis commented Oct 20, 2023 • edited Loading

BenjaminPelletier Oct 20, 2023

Choose a reason for hiding this comment

Shastick Oct 13, 2023 •

edited

Loading

mickmis commented Oct 20, 2023 •

edited

Loading