fix(cfg)!: make sure we use correct relays #2778

Arqu · 2024-10-02T20:29:32Z

Description

This takes us back to only relying on the env var for forcing staging relays. However for tests it should still remain active with the test/feature combo by default.

Breaking Changes

TEST_DNS_NODE_ORIGIN is removed
iroh now defaults to using prod relays unless IROH_FORCE_STAGING_RELAYS is set to a non empty value

Notes & open questions

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-10-02T21:09:57Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 148bac0

iroh-net/src/discovery/dns.rs

iroh-net/src/discovery/pkarr.rs

iroh-net/src/endpoint.rs

iroh/src/node/builder.rs

matheus23

This takes us back to only relying on the env var for forcing staging relays. However for tests it should still remain active with the test/feature combo by default.

I don't think this is how it should be.

I agree with dig, we should drop the #[cfg(..)] completely, and solely rely on std::env::var(FORCE_STAGING_RELAYS).is_ok().

There's two permutations of this idea that I'm on the fence about:

we could require this env variable to be present at compile time, i.e. use std::env!("IROH_FORCE_STAGING_RELAYS"). This might be annoying if we try to test with, let's say, a pre-built iroh-doctor binary.
we could match on cfg!(test) || std::env::var(FORCE_STAGING_RELAYS).is_ok(). This way we make sure that any testing code always uses staging relays. This shouldn't be too bad. But also I still don't think this would be right to do. I want it to be possible to run the test env as similar to prod as possible, if I want to.

About insecure_skip_relay_cert_verify: This should just be gated on #[cfg(feature = "test-utils")] IMO.
But whether or not it's exposed is actually a concern outside of the scope of this PR, so this PR shouldn't change when this function actually is exposed.

matheus23

You match on Ok(value) = std::env::var(...) if !value.is_empty(), but in the docs you write "if the ... env variable is set".

I think you should simply use std::env::var(ENV_FORCE_STAGING_RELAYS).is_ok().
If the env variable is not set, then std::env::var returns Err(NotPresent), so checking for is_ok() should be fine IMO.

With your current setup, this would not force staging relays:

$ IROH_FORCE_STAGING_RELAYS="" cargo run # ...

iroh-net/src/discovery/dns.rs

iroh-net/src/discovery/pkarr.rs

github-actions · 2024-10-17T20:56:31Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2778/docs/iroh/

Last updated: 2024-10-21T10:59:45Z

matheus23 · 2024-10-18T08:27:11Z

iroh-net/src/discovery/pkarr.rs

+        let pkarr_relay = match std::env::var(ENV_FORCE_STAGING_RELAYS) {
+            Ok(value) if !value.is_empty() => N0_DNS_PKARR_RELAY_STAGING,
+            _ => N0_DNS_PKARR_RELAY_PROD,
+        };


This should probably also do the cfg(test) dance as the other things, do, right?

@flub the N0_DNS_PKARR_RELAY_PROD values are distinct from the dns records ie. have /pkarr added. Do you want me to add a TEST_DNS_PKARR_RELAY similar to how it is on dns.rs right now?

Hmm no, I think TEST_DNS_NODE_ORIGIN is a special case: It uses the .test TLD which has special meaning in DNS resolvers!

We don't have any equivalent for any of the other infra identifiers.

So from my recollection TEST_DNS_NODE_ORIGIN was added so that it uses a special URL which will not be resolved by any normal resolvers and will only be resolved if you start a local resolver in the test using a helper function. This stops tests from hitting any external DNS server.

IIRC we basically didn't have any requests and this was more a precaution when we were dealing with the relays as well. (Though in spirit I do agree with it)

matheus23 · 2024-10-18T08:27:25Z

iroh-net/src/discovery/pkarr.rs

+        #[cfg(not(test))]
        let pkarr_relay = N0_DNS_PKARR_RELAY_PROD;
-        #[cfg(any(test, feature = "test-utils"))]
+        #[cfg(test)]
        let pkarr_relay = N0_DNS_PKARR_RELAY_STAGING;


We should probably also match on the env var here, right?

matheus23 · 2024-10-18T08:30:19Z

iroh-net/src/endpoint.rs

+    match std::env::var(ENV_FORCE_STAGING_RELAYS) {
+        Ok(value) if !value.is_empty() => RelayMode::Staging,
+        _ => RelayMode::Default,


Err. I'm a little confused.
Do we want to match on both env vars and cfg(test) now, or not?
Usage now seems somewhat inconsistent.

Perhaps we should just have a single fn use_staging_infra() -> bool somewhere instead of the ENV_FORCE_STAGING_RELAYS constant.
Then we can make sure our use is consistent.

matheus23 · 2024-10-18T08:38:18Z

iroh/src/node/builder.rs

+        #[cfg(not(test))]
        let relay_mode = RelayMode::Default;
-        #[cfg(any(test, feature = "test-utils"))]
+        #[cfg(test)]
        let relay_mode = RelayMode::Staging;


IMO This should additionally test for the env variable here, too.

matheus23 · 2024-10-18T08:38:32Z

iroh/src/node/builder.rs

+        #[cfg(not(test))]
        let relay_mode = RelayMode::Default;
-        #[cfg(any(test, feature = "test-utils"))]
+        #[cfg(test)]
        let relay_mode = RelayMode::Staging;


(same here)

iroh/src/node/builder.rs

Arqu · 2024-10-18T18:56:22Z

iroh-net/src/relay.rs

+
+/// Returns `true` if the use of staging relays is forced.
+pub fn force_staging_infra() -> bool {
+    matches!(std::env::var(ENV_FORCE_STAGING_RELAYS), Ok(value) if !value.is_empty())


We can now handle the cfg(test) case here if we want to specifically. But as Frando mentioned, this only works in the tested crate.

Arqu · 2024-10-18T18:58:08Z

Revamped the thing again. This is the "slim" version.

No fluffy matching in each file, single generic function
No cfg(test) shenanigans
No TEST_urls
Works only by env var

matheus23 · 2024-10-21T07:31:17Z

iroh-net/src/discovery/dns.rs

+        match force_staging_infra() {
+            true => Self::new(N0_DNS_NODE_ORIGIN_STAGING.to_string()),
+            false => Self::new(N0_DNS_NODE_ORIGIN_PROD.to_string()),


Suggested change

match force_staging_infra() {

true => Self::new(N0_DNS_NODE_ORIGIN_STAGING.to_string()),

false => Self::new(N0_DNS_NODE_ORIGIN_PROD.to_string()),

if force_staging_infra() {

Self::new(N0_DNS_NODE_ORIGIN_STAGING.to_string())

} else {

Self::new(N0_DNS_NODE_ORIGIN_PROD.to_string())

I guess it'd be unusual to see a match on booleans. But honestly, it's shorter, and I'd almost say the match reads better in this case? I'm somewhat undecided :D

Yeah, I just followed like the others (where I let x = match ...) but technically here it is a simple if else. Updated to that 🤷

lol, I saw the other uses and was like "ah well, it's probably fine", but now we have it inconsistent 🙈
Anyhow, we can update another time.

matheus23

I'm happy with this :)

(Btw: I didn't know we already set the IROH_FORCE_STAGING_RELAYS env var in testing CI! :D)

Arqu requested review from flub and dignifiedquire October 2, 2024 20:30

Arqu self-assigned this Oct 2, 2024

Arqu requested a review from matheus23 October 2, 2024 20:30

flub reviewed Oct 3, 2024

View reviewed changes

matheus23 requested changes Oct 16, 2024

View reviewed changes

matheus23 reviewed Oct 17, 2024

View reviewed changes

iroh-net/src/discovery/dns.rs Outdated Show resolved Hide resolved

iroh-net/src/discovery/pkarr.rs Outdated Show resolved Hide resolved

Arqu requested review from matheus23 and flub October 17, 2024 21:03

Arqu force-pushed the arqu/fix_staging_relay_matrix branch from 556f174 to 3b9044e Compare October 18, 2024 07:15

matheus23 reviewed Oct 18, 2024

View reviewed changes

iroh/src/node/builder.rs Outdated Show resolved Hide resolved

Arqu commented Oct 18, 2024

View reviewed changes

Arqu added 9 commits October 19, 2024 13:04

fix(cfg): make sure we use correct relays

81a88b8

cleanup

34a26e1

cleanup

f42b36c

CR

f0a8cd0

more CR

bc38a37

fix docs

432b96c

more cleanup

b4514ff

fix docs issue

9dd4997

another revamp

8d39c93

Arqu force-pushed the arqu/fix_staging_relay_matrix branch from 4ea7554 to 8d39c93 Compare October 19, 2024 11:06

cleanup

e0cde90

Arqu changed the title ~~fix(cfg): make sure we use correct relays~~ fix(cfg)!: make sure we use correct relays Oct 19, 2024

matheus23 reviewed Oct 21, 2024

View reviewed changes

matheus23 approved these changes Oct 21, 2024

View reviewed changes

minor cleanup

4f6adc4

Arqu enabled auto-merge October 21, 2024 10:57

Arqu added this pull request to the merge queue Oct 21, 2024

Merged via the queue into main with commit 844b146 Oct 21, 2024
26 of 27 checks passed

Arqu deleted the arqu/fix_staging_relay_matrix branch October 21, 2024 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cfg)!: make sure we use correct relays #2778

fix(cfg)!: make sure we use correct relays #2778

Arqu commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

matheus23 left a comment

matheus23 left a comment

github-actions bot commented Oct 17, 2024 •

edited

Loading

matheus23 Oct 18, 2024

Arqu Oct 18, 2024

matheus23 Oct 18, 2024

flub Oct 18, 2024

Arqu Oct 18, 2024

matheus23 Oct 18, 2024

matheus23 Oct 18, 2024

matheus23 Oct 18, 2024

matheus23 Oct 18, 2024

Arqu Oct 18, 2024

Arqu commented Oct 18, 2024

matheus23 Oct 21, 2024

matheus23 Oct 21, 2024

Arqu Oct 21, 2024

matheus23 Oct 21, 2024

matheus23 left a comment

fix(cfg)!: make sure we use correct relays #2778

fix(cfg)!: make sure we use correct relays #2778

Conversation

Arqu commented Oct 2, 2024 • edited Loading

Description

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Oct 2, 2024 • edited Loading

matheus23 left a comment

Choose a reason for hiding this comment

matheus23 left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024 • 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

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

Arqu commented Oct 18, 2024

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

matheus23 left a comment

Choose a reason for hiding this comment

Arqu commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading