Skip to content

Add client config param IgnoreTURNResolveErrors #455

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amanakin
Copy link
Contributor

@amanakin amanakin commented May 7, 2025

Description

In some configurations we don't use the TURN address at the client side (e.g. proxy connection, see #450). But now at the client setup the provided turn address must be resolvable.

Adding client config param IgnoreTURNResolveErrors that allows omit resolving errors when we don't need turn address.

Also discussed here: pion/ice#773

Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.57%. Comparing base (51f0d62) to head (752326c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   67.43%   67.57%   +0.13%     
==========================================
  Files          43       43              
  Lines        3083     3087       +4     
==========================================
+ Hits         2079     2086       +7     
+ Misses        843      841       -2     
+ Partials      161      160       -1     
Flag Coverage Δ
go 67.57% <100.00%> (+0.13%) ⬆️
wasm 26.36% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amanakin amanakin force-pushed the add_no_resolve_error_param branch from 20a5223 to a170ed1 Compare May 7, 2025 14:57
@amanakin amanakin force-pushed the add_no_resolve_error_param branch from d94f5e4 to 752326c Compare May 7, 2025 15:17
@@ -47,6 +47,8 @@ type ClientConfig struct {
Conn net.PacketConn // Listening socket (net.PacketConn)
Net transport.Net
LoggerFactory logging.LoggerFactory

IgnoreTURNResolveErrors bool // TURN server address is not required for some configurations (e.g. proxy)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add a config, there are many reason, but it feels like a single use flag.
What if we make a new constructor NewClientWithResolver and essentially make the nested condition after if len(config.TURNServerAddr) > 0 { customizable, we can then make the current NewClient call this new constructor with the old resolver logic.

Copy link
Member

Choose a reason for hiding this comment

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

Even better, we can use Pion options, pattern, and make NewClientWithOptions and make a custom option WithResolver.

Copy link
Member

Choose a reason for hiding this comment

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

I can make a PR for this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoeTurki So you mean something like

if len(config.TURNServerAddr) > 0 {
	turnServ, err = config.ResolveTURN(config.TURNServerAddr)
	if err != nil {
		return nil, err
	}
}

And then in some cases use there WithResolveTURN(...) that will return nil, nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoeTurki Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants