Make upnp lease duration configurable #868

lcruz99 · 2024-10-11T15:26:51Z

Problem

User is not able to set a custom UPnP lease duration.

Solution

This PR adds a UPnP telio feature with lease duration field.

☑️ Definition of Done checklist

Commit history is clean (requirements)
README.md is updated
Functionality is covered by unit or integration tests

packgron · 2024-10-16T11:31:50Z

crates/telio-traversal/src/endpoint_providers/upnp.rs

@@ -36,7 +36,7 @@ const GET_INTERFACE_TIMEOUT_S: Duration = Duration::from_secs(2);

 const EPHEMERAL_PORT_RANGE: std::ops::RangeInclusive<u16> = 49152..=65535;

-const DEFAULT_LEASE_DURATION_S: u32 = 600;
+const LEASE_RENEWAL_LIMIT_S: u32 = 600;


What does this const controll, we have what we set as wanted, but i don't entirely get there this comes from.
At what poin we should update lease ?

We have FeaturesUPnP::lease_duration_s which sets the actual lease duration for the UPnP port mappings. This LEASE_RENEWAL_LIMIT_S is the value which when the lease duration falls below it, we should renew the mappings by sending a new mapping request to the UPnP device.

E.g: We set the mappings lease duration to 3600s, when we check the lease duration left - every upnp_interval tick time - and there's less than 600s left for them to expire, we simple renew the lease for another FeaturesUPnP::lease_duration_s which is 3600s.

Thinking this better, maybe this const can be correlated with the FeaturesUPnP::lease_duration_s, something like 1/5 of FeaturesUPnP::lease_duration_s

Yeh, that was what i was thinking also, maybe min(1/10, 1)

It has to be higher than the tick time (upnp_interval) though, otherwise it will get expired. I'll try to come up with something.

tomaszklak · 2024-10-16T11:33:46Z

crates/telio-traversal/src/endpoint_providers/upnp.rs

+                telio_log_info!("Error validating endpoint: {}", e);

-                    telio_log_info!("Deleting an old Upnp endpoint");
-                    let detete = self.igd_gw.delete_endpoint_routes(
+                telio_log_info!("Dropping Upnp endpoint");


seems to me this should be telio_log_error!

is there a reason not to merge those two logs into one?

It was already telio_log_info and I've questioned myself the same, but when there's an error validating the endpoint I consider not critical enough to be an error regarding the library runtime, and it can be easily recovered from.

I think we can, it will just result in a somewhat long log. But I can merge it, what do you think?

in that case this sounds like it should be telio_log_warn!.

imo, lets merge it

tomaszklak · 2024-10-16T11:34:19Z

crates/telio-traversal/src/endpoint_providers/upnp.rs

+                    )
+                    .await
+                {
+                    telio_log_info!("Error dropping Upnp endpoint: {}", e);


This looks like it should be an error log, not info.

This error could be simply the mappings that were already dropped by the UPnP device, as the comment above, I don't consider it worth to be logged as a error, as the runtime is not affected and the mappings won't exist anyway.

Let me know if you disagree, this is something I've also questined myself.

Yeah, in that case it sounds like a telio_log_warn!.

tomaszklak · 2024-10-16T11:36:46Z

src/device.rs

@@ -1352,6 +1352,7 @@ impl Runtime {

            // Create Upnp Endpoint Provider
            let upnp_endpoint_provider = if has_provider(Upnp) {
+                let direct_features = self.features.direct.clone().unwrap_or_default();


I think there is no need for direct_features since direct variable is already in scope here.

good catch, I missed that

tomaszklak · 2024-10-16T12:10:28Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

lcruz99 · 2024-10-16T12:30:23Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

You're right, the commits prefixed with review: will be squashed before merging the PR. I've been doing that to avoid messing up the git history for the reviewers and segregate what's been changed during the review.

tomaszklak · 2024-10-16T12:45:05Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

You're right, the commits prefixed with review: will be squashed before merging the PR. I've been doing that to avoid messing up the git history for the reviewers and segregate what's been changed during the review.

In that case, when is the good time to review the commit history?

lcruz99 · 2024-10-16T12:53:58Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

You're right, the commits prefixed with review: will be squashed before merging the PR. I've been doing that to avoid messing up the git history for the reviewers and segregate what's been changed during the review.

In that case, when is the good time to review the commit history?

I'd say after every issue is resolved and +2 acquired. Then commit history is cleaned and approval by the reviewers unlocked.

tomaszklak · 2024-10-16T13:14:39Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

You're right, the commits prefixed with review: will be squashed before merging the PR. I've been doing that to avoid messing up the git history for the reviewers and segregate what's been changed during the review.

In that case, when is the good time to review the commit history?

I'd say after every issue is resolved and +2 acquired. Then commit history is cleaned and approval by the reviewers unlocked.

I don't think we agreed to this way of working - I would like to see a clean git history (in the state you intend to merge it) before I add any points.

lcruz99 · 2024-10-17T15:42:58Z

I think that 4bb57eb shouldn't be a separate commit but a part of b5caf1c

You're right, the commits prefixed with review: will be squashed before merging the PR. I've been doing that to avoid messing up the git history for the reviewers and segregate what's been changed during the review.

In that case, when is the good time to review the commit history?

I'd say after every issue is resolved and +2 acquired. Then commit history is cleaned and approval by the reviewers unlocked.

I don't think we agreed to this way of working - I would like to see a clean git history (in the state you intend to merge it) before I add any points.

I understand your POV. I propose once the review is done I would clean git history and then get the eventual reviewer points.

tomaszklak

+1

lcruz99 · 2024-10-18T12:26:25Z

it looks like the test is flaky, I have to investigate it and probably make some changes

lcruz99 · 2024-10-18T17:43:40Z

it looks like the test is flaky, I have to investigate it and probably make some changes

test is fixed, I've had to adjust endpoint time variables on the test itself.

lcruz99 requested a review from a team as a code owner October 11, 2024 15:26

lcruz99 force-pushed the LLT-5299_upnp_configurable_lease_duration branch 2 times, most recently from 366199d to 0bb3037 Compare October 11, 2024 15:39

packgron reviewed Oct 16, 2024

View reviewed changes

tomaszklak reviewed Oct 16, 2024

View reviewed changes

lcruz99 force-pushed the LLT-5299_upnp_configurable_lease_duration branch 5 times, most recently from 19047e2 to f8e2b04 Compare October 18, 2024 11:09

tomaszklak previously approved these changes Oct 18, 2024

View reviewed changes

gytsto and others added 5 commits October 18, 2024 18:41

Revert "Update libtelio build ref to v2.4.10"

76a0332

update rust toolchain version for unused-features linter

471ce30

add changelog

6c3e09c

features: Make upnp port lease duration configurable

ba1bebc

natlab: Add upnp lease duration test

a47c919

lcruz99 dismissed tomaszklak’s stale review via a47c919 October 18, 2024 17:41

lcruz99 force-pushed the LLT-5299_upnp_configurable_lease_duration branch from f8e2b04 to a47c919 Compare October 18, 2024 17:41

lcruz99 requested review from Jauler and jjanowsk as code owners October 18, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make upnp lease duration configurable #868

Make upnp lease duration configurable #868

lcruz99 commented Oct 11, 2024 •

edited

Loading

packgron Oct 16, 2024

lcruz99 Oct 16, 2024 •

edited

Loading

lcruz99 Oct 16, 2024

packgron Oct 16, 2024

lcruz99 Oct 16, 2024

tomaszklak Oct 16, 2024

lcruz99 Oct 16, 2024

tomaszklak Oct 16, 2024

tomaszklak Oct 16, 2024

lcruz99 Oct 16, 2024 •

edited

Loading

tomaszklak Oct 16, 2024

tomaszklak Oct 16, 2024

lcruz99 Oct 16, 2024

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 16, 2024 •

edited

Loading

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 16, 2024

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 17, 2024

tomaszklak left a comment

lcruz99 commented Oct 18, 2024

lcruz99 commented Oct 18, 2024 •

edited

Loading

Make upnp lease duration configurable #868

Are you sure you want to change the base?

Make upnp lease duration configurable #868

Conversation

lcruz99 commented Oct 11, 2024 • edited Loading

Problem

Solution

☑️ Definition of Done checklist

Choose a reason for hiding this comment

lcruz99 Oct 16, 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

lcruz99 Oct 16, 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

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 16, 2024 • edited Loading

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 16, 2024

tomaszklak commented Oct 16, 2024

lcruz99 commented Oct 17, 2024

tomaszklak left a comment

Choose a reason for hiding this comment

lcruz99 commented Oct 18, 2024

lcruz99 commented Oct 18, 2024 • edited Loading

lcruz99 commented Oct 11, 2024 •

edited

Loading

lcruz99 Oct 16, 2024 •

edited

Loading

lcruz99 Oct 16, 2024 •

edited

Loading

lcruz99 commented Oct 16, 2024 •

edited

Loading

lcruz99 commented Oct 18, 2024 •

edited

Loading