Implement Active TCP-ICE #394

ashellunts · 2021-10-24T10:37:56Z

No description provided.

codecov · 2021-10-24T10:39:10Z

Codecov Report

Base: 78.23% // Head: 77.13% // Decreases project coverage by -1.10% ⚠️

Coverage data is based on head (1542ccb) compared to base (c0be5d1).
Patch coverage: 35.48% of modified lines in pull request are covered.

❗ Current head 1542ccb differs from pull request most recent head ed64aab. Consider uploading reports for the commit ed64aab to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   78.23%   77.13%   -1.10%     
==========================================
  Files          36       36              
  Lines        4273     4348      +75     
==========================================
+ Hits         3343     3354      +11     
- Misses        723      779      +56     
- Partials      207      215       +8

Flag	Coverage Δ
go	`77.13% <35.48%> (-1.10%)`	⬇️
wasm	`?`

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

Impacted Files	Coverage Δ
agent_config.go	`82.14% <ø> (ø)`
selection.go	`80.09% <0.00%> (-1.11%)`	⬇️
agent.go	`79.40% <17.02%> (-3.02%)`	⬇️
candidate_base.go	`86.84% <33.33%> (-1.00%)`	⬇️
tcp_packet_conn.go	`75.12% <40.00%> (-0.90%)`	⬇️
gather.go	`63.81% <47.45%> (-2.55%)`	⬇️
candidate_host.go	`93.47% <100.00%> (+0.62%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ashellunts · 2021-10-24T10:43:13Z

@dgunay were you able to make a successful connection?

agent_config.go

Sean-Der · 2021-10-24T17:46:42Z

agent.go

@@ -130,6 +131,10 @@ type Agent struct {
 	insecureSkipVerify bool

 	proxyDialer proxy.Dialer
+
+	tcpConnections map[string]net.Conn


I would store the tcpConnections inside the Active TCP candidate. Would love to avoid putting more state in the Agent itself

Does Chrome expose the ports it dialed on?

ashellunts · 2021-11-06T14:53:45Z

TCP connection is ready and some messages are sent over the connection. But after first messages a connection freezes.
Here are logs:

artur@hp:~/git/ice$ go test -v -run TestActiveTCP
=== RUN   TestActiveTCP
[::]:7686
ice INFO: 2021/11/06 15:30:13 Listening TCP on [::]:7686
ice DEBUG: 15:30:13.695553 gather.go:178: GetConn by ufrag: FEtegJZjVRZGzPra
ice INFO: 2021/11/06 15:30:13 Ignoring remote candidate with tcpType active: tcp4 host 192.168.1.205:0
ice DEBUG: 15:30:13.696273 agent.go:455: Started agent: isControlling? true, remoteUfrag: "FEtegJZjVRZGzPra", remotePwd: "ftOozEzvZGmnzadZAWssKPCcgjcSWMxe"
ice INFO: 2021/11/06 15:30:13 Setting new connection state: Checking
ice DEBUG: 15:30:13.697976 agent.go:832: count of local candidates 1
ice DEBUG: 15:30:13.698086 agent.go:655: artur, addPair: local tcp4 host 192.168.1.205:0, remote tcp4 host 192.168.1.205:7686
ice DEBUG: 15:30:13.698121 agent.go:657: artur, addressToConnect 192.168.1.205:7686
ice DEBUG: 15:30:13.698477 agent.go:455: Started agent: isControlling? false, remoteUfrag: "SmxZXyxixwugeuYO", remotePwd: "vuSCEihcZbhTAruaftQdOjGwwLTGjyTx"
ice INFO: 2021/11/06 15:30:13 Setting new connection state: Checking
ice DEBUG: 15:30:13.698633 tcp_mux.go:96: Accepted connection from: 192.168.1.205:48332 to 192.168.1.205:7686
ice WARNING: 2021/11/06 15:30:13 pingAllCandidates called with no candidate pairs. Connection is not possible yet.
ice DEBUG: 15:30:13.698976 agent.go:664: artur, socket connected, local 192.168.1.205:48332, remote 192.168.1.205:7686
ice INFO: 2021/11/06 15:30:13 AddConn: tcp 192.168.1.205:7686
ice DEBUG: 15:30:13.700550 tcp_mux.go:186: msg attr: USERNAME: 0x46457465674a5a6a56525a477a5072613a536d785a58797869787775676575594f
ice DEBUG: 15:30:13.700761 tcp_mux.go:186: msg attr: ICE-CONTROLLING: 0xfd3cd86a20f35b19
ice DEBUG: 15:30:13.700822 tcp_mux.go:186: msg attr: PRIORITY: 0x7edfffff
ice DEBUG: 15:30:13.700864 tcp_mux.go:186: msg attr: MESSAGE-INTEGRITY: 0xd2b0404f181edfd6b04dd6a9b88fbdcaef27269e
ice DEBUG: 15:30:13.700901 tcp_mux.go:186: msg attr: FINGERPRINT: 0x623cd098
ice DEBUG: 15:30:13.700942 tcp_mux.go:197: Ufrag: FEtegJZjVRZGzPra
ice INFO: 2021/11/06 15:30:13 AddConn: tcp 192.168.1.205:48332
ice DEBUG: 15:30:13.701497 agent.go:1134: adding a new peer-reflexive candidate: 192.168.1.205:48332 
ice DEBUG: 15:30:13.701601 agent.go:832: count of local candidates 1

May be connection works only in 1 direction yet?

ashellunts · 2021-11-06T14:58:59Z

@Sean-Der please take a look on new implementation.

ashellunts · 2021-11-06T15:18:59Z

I see that pair nomination is not performed.

ashellunts · 2021-11-08T16:30:42Z

Finally first working version (that goes to connected state).

Sean-Der · 2021-11-08T16:46:12Z

holy shit! amazing work @ashellunts this is so exciting :)

I should have some time later today. sorry about the delay. This is really great. Will make clients better, and really excited to see coverage for servers improve.

ashellunts · 2021-11-08T17:10:53Z

holy shit! amazing work @ashellunts this is so exciting :)

I should have some time later today. sorry about the delay. This is really great. Will make clients better, and really excited to see coverage for servers improve.

Thanks. 2 days debugging finally worked out :D

ashellunts · 2021-11-08T17:12:33Z

For now it works only if you have only 1 network interface 😭

Sean-Der · 2021-11-09T17:27:30Z

agent_config.go

@@ -50,7 +50,8 @@ func defaultCandidateTypes() []CandidateType {
 // AgentConfig collects the arguments to ice.Agent construction into
 // a single structure, for future-proofness of the interface
 type AgentConfig struct {
-	Urls []*URL
+	activeTCP bool


DisableActiveTCP

By default we should enable Active TCP

Sean-Der · 2021-11-09T17:31:01Z

@ashellunts This looks really good! If you can fix the linter and test failures I think we can merge this really soon :)

agent.go

Sean-Der · 2021-11-09T17:30:23Z

agent.go

-		a.log.Infof("Ignoring remote candidate with tcpType active: %s", c)
-		return nil
-	}
+	// if c.TCPType() == TCPTypeActive {


Should this be deleted?

candidate_base.go

dgunay · 2021-12-05T00:18:25Z

@Sean-Der addressed some comments and linter errors. There are still a few concurrency bugs it looks like.

ashellunts · 2021-12-05T07:56:45Z

@dgunay Thank you so much for continuing the work!

dgunay · 2022-03-15T08:26:53Z

EDIT: nevermind, I've clearly forgotten how this PR works, it already initiates TCP connections - I'll keep digging

dgunay · 2022-03-15T09:53:36Z

Best I can tell for now is that none of the candidates are being nominated.

stv0g · 2023-02-04T10:10:07Z

I am excited to see that you picked up the PR @ashellunts

Let me know if I can support with testing, reviewing or something else :)

ashellunts · 2023-02-04T10:34:43Z

Thanks. I will appreciate if you can read PR changes and maybe notice something completely wrong.

Sometime ago I had the test passing. Now it doesn't work. And I am debugging to understand what is going on.

agent.go

stv0g · 2023-02-04T10:59:54Z

Thanks. I will appreciate if you can read PR changes and maybe notice something completely wrong.

I will dig into the RFC a bit more into detail later.

Something which confused me a bit: Does an agent always pick the active or the passive role? I thought it would make more sense to active & passive candidates gathered by the same agent.

ashellunts · 2023-02-04T13:55:53Z

Not sure. But for me it looks logical to have either passive agent or active one.

stv0g · 2023-02-04T15:06:27Z

Not sure. But for me it looks logical to have either passive agent or active one.

I've had a look at RFC6544, which is mainly concerned about active/passive/so TCP candidates for ICE. Not about active/passive agents.
I think we do not need to have an extra activeTCP flag as there the agent is already configured NetworkTypes and CandidateTypes to limit type of candidates or transport protocols.

We might want to think about maybe adding TCPCandidateTypes option which with the following options:

TCPCandidateTypeActive
TCPCandidateTypePassive
TCPCandidateTypeSimultaneousOpen

The reason why I am proposing this change is, that in my application I have might have situations in which each side of the P2P connection is/is-not dialable from the outside. So determine the role is not feasible beforehand and would require a complex role-conflict resolution (just like for Lite ICE agents).

Hence, I think its easier to generate all types of TCP candidates in the first place. And allow the user to limit those by the settings which I mentioned above.

ashellunts · 2023-02-04T19:28:02Z

Not sure. But for me it looks logical to have either passive agent or active one.

I've had a look at RFC6544, which is mainly concerned about active/passive/so TCP candidates for ICE. Not about active/passive agents. I think we do not need to have an extra activeTCP flag as there the agent is already configured NetworkTypes and CandidateTypes to limit type of candidates or transport protocols.

We might want to think about maybe adding TCPCandidateTypes option which with the following options:

TCPCandidateTypeActive

TCPCandidateTypePassive

TCPCandidateTypeSimultaneousOpen

The reason why I am proposing this change is, that in my application I have might have situations in which each side of the P2P connection is/is-not dialable from the outside. So determine the role is not feasible beforehand and would require a complex role-conflict resolution (just like for Lite ICE agents).

Hence, I think its easier to generate all types of TCP candidates in the first place. And allow the user to limit those by the settings which I mentioned above.

Thank you for the feedback. Sounds good.

ashellunts · 2023-05-09T19:20:08Z

Created another PR instead of this one: #565

ashellunts requested review from Sean-Der and dgunay October 24, 2021 10:37

pionbot force-pushed the active-tcp-ice branch from 7d62c0f to a6ff55d Compare October 24, 2021 10:38

Sean-Der reviewed Oct 24, 2021

View reviewed changes

agent_config.go Outdated Show resolved Hide resolved

Sean-Der reviewed Oct 24, 2021

View reviewed changes

ashellunts force-pushed the active-tcp-ice branch from a6ff55d to a565945 Compare November 6, 2021 14:50

pionbot force-pushed the active-tcp-ice branch from a565945 to 1604431 Compare November 6, 2021 14:50

ashellunts force-pushed the active-tcp-ice branch 2 times, most recently from 79392b9 to bc02898 Compare November 8, 2021 16:53

Sean-Der reviewed Nov 9, 2021

View reviewed changes

Sean-Der requested changes Nov 9, 2021

View reviewed changes

pionbot force-pushed the active-tcp-ice branch from e2c4f68 to 8e558f8 Compare December 5, 2021 00:15

dgunay mentioned this pull request Mar 15, 2022

vnet: Support TCP pion/transport#27

Open

dgunay mentioned this pull request Sep 3, 2022

Implement Active TCP-ICE #392

Closed

dgunay removed their request for review October 22, 2022 01:42

ashellunts force-pushed the active-tcp-ice branch from 6df0a1f to eb3aaee Compare November 2, 2022 07:10

ashellunts force-pushed the active-tcp-ice branch from 6a4f362 to 278b539 Compare February 4, 2023 09:52

pionbot force-pushed the active-tcp-ice branch from 278b539 to 7da92da Compare February 4, 2023 09:53

stv0g requested changes Feb 4, 2023

View reviewed changes

agent.go Show resolved Hide resolved

agent.go Show resolved Hide resolved

ashellunts force-pushed the active-tcp-ice branch from 1542ccb to 1190af5 Compare February 4, 2023 14:10

pionbot force-pushed the active-tcp-ice branch from 1190af5 to 887ae3a Compare February 4, 2023 14:10

[WIP] Active TCP

ed64aab

ashellunts force-pushed the active-tcp-ice branch from 887ae3a to ed64aab Compare February 4, 2023 19:25

ashellunts mentioned this pull request May 7, 2023

Active TCP candidates? #562

Closed

ashellunts closed this May 9, 2023

ashellunts deleted the active-tcp-ice branch May 24, 2023 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Active TCP-ICE #394

Implement Active TCP-ICE #394

ashellunts commented Oct 24, 2021

codecov bot commented Oct 24, 2021 •

edited

Loading

ashellunts commented Oct 24, 2021

Sean-Der Oct 24, 2021

ashellunts commented Nov 6, 2021

ashellunts commented Nov 6, 2021

ashellunts commented Nov 6, 2021

ashellunts commented Nov 8, 2021

Sean-Der commented Nov 8, 2021

ashellunts commented Nov 8, 2021

ashellunts commented Nov 8, 2021

Sean-Der Nov 9, 2021

Sean-Der commented Nov 9, 2021

Sean-Der Nov 9, 2021

dgunay commented Dec 5, 2021

ashellunts commented Dec 5, 2021

dgunay commented Mar 15, 2022 •

edited

Loading

dgunay commented Mar 15, 2022

stv0g commented Feb 4, 2023

ashellunts commented Feb 4, 2023

stv0g commented Feb 4, 2023

ashellunts commented Feb 4, 2023

stv0g commented Feb 4, 2023 •

edited

Loading

ashellunts commented Feb 4, 2023

ashellunts commented May 9, 2023

Implement Active TCP-ICE #394

Implement Active TCP-ICE #394

Conversation

ashellunts commented Oct 24, 2021

codecov bot commented Oct 24, 2021 • edited Loading

Codecov Report

ashellunts commented Oct 24, 2021

Sean-Der Oct 24, 2021

Choose a reason for hiding this comment

ashellunts commented Nov 6, 2021

ashellunts commented Nov 6, 2021

ashellunts commented Nov 6, 2021

ashellunts commented Nov 8, 2021

Sean-Der commented Nov 8, 2021

ashellunts commented Nov 8, 2021

ashellunts commented Nov 8, 2021

Sean-Der Nov 9, 2021

Choose a reason for hiding this comment

Sean-Der commented Nov 9, 2021

Sean-Der Nov 9, 2021

Choose a reason for hiding this comment

dgunay commented Dec 5, 2021

ashellunts commented Dec 5, 2021

dgunay commented Mar 15, 2022 • edited Loading

dgunay commented Mar 15, 2022

stv0g commented Feb 4, 2023

ashellunts commented Feb 4, 2023

stv0g commented Feb 4, 2023

ashellunts commented Feb 4, 2023

stv0g commented Feb 4, 2023 • edited Loading

ashellunts commented Feb 4, 2023

ashellunts commented May 9, 2023

codecov bot commented Oct 24, 2021 •

edited

Loading

dgunay commented Mar 15, 2022 •

edited

Loading

stv0g commented Feb 4, 2023 •

edited

Loading