Skip to content
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.

Sign up for GitHub

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

Add UDP, Time, & Duration to Connectivity Report #327

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add UDP, Time, & Duration to Connectivity Report #327

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5d398b
update: add udp report and duration to reports
amircybersec Nov 11, 2024
cd78174
Merge remote-tracking branch 'origin/main' into amir-tc-udp
amircybersec Nov 11, 2024
b45e18b
fix: use providers
amircybersec Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 78 additions & 9 deletions x/examples/test-connectivity/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ type connectivityReport struct {
Test testReport `json:"test"`
DNSQueries []dnsReport `json:"dns_queries,omitempty"`
TCPConnections []tcpReport `json:"tcp_connections,omitempty"`
UDPConnections []udpReport `json:"udp_connections,omitempty"`
}

type testReport struct {
Expand All @@ -68,10 +69,21 @@ type dnsReport struct {
}

type tcpReport struct {
Hostname string `json:"hostname"`
IP string `json:"ip"`
Port string `json:"port"`
Error string `json:"error"`
Hostname string `json:"hostname"`
IP string `json:"ip"`
Port string `json:"port"`
Error string `json:"error"`
Time time.Time `json:"time"`
Duration int64 `json:"duration_ms"`
}

type udpReport struct {
Hostname string `json:"hostname"`
IP string `json:"ip"`
Port string `json:"port"`
Error string `json:"error"`
Time time.Time `json:"time"`
Duration int64 `json:"duration_ms"`
}

type errorJSON struct {
Expand All @@ -80,7 +92,8 @@ type errorJSON struct {
// Posix error, when available
PosixError string `json:"posix_error,omitempty"`
// TODO: remove IP addresses
Msg string `json:"msg,omitempty"`
Msg string `json:"msg,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to note that these two fields may contain Personally Identifiable Information (PII). We need to figure out a way to not leak that before this is actually used in production with user probes.

We should probably allowlist specific errors as we learn about them. Though we need the full message to learn about them.

MsgFull string `json:"msg_full,omitempty"`
}

func makeErrorRecord(result *connectivity.ConnectivityError) *errorJSON {
Expand All @@ -91,6 +104,7 @@ func makeErrorRecord(result *connectivity.ConnectivityError) *errorJSON {
record.Op = result.Op
record.PosixError = result.PosixError
record.Msg = unwrapAll(result.Err).Error()
record.MsgFull = result.Err.Error()
return record
}

Expand Down Expand Up @@ -120,7 +134,9 @@ func init() {
}
func newTCPTraceDialer(
onDNS func(ctx context.Context, domain string) func(di httptrace.DNSDoneInfo),
onDial func(ctx context.Context, network, addr string, connErr error)) transport.StreamDialer {
onDial func(ctx context.Context, network, addr string, connErr error),
Copy link
Contributor

Choose a reason for hiding this comment

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

This design puts the onus on the user to match dial start and end. That also complicates the testing code.

Please change the design so that we have a single onDial that is called on the beginning of the dial, and returns the onDialDone handler, like we do for DNS.

In the case of TCP/UDP, you will need a map (net, addr) -> done handler that you can create here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fortuna I am going to iterate on the proposed design. Ideally the user should just run the test and get report with a clean API that is part of the SDK (/x) and test-connectivity code would just use it.

onDialStart func(ctx context.Context, network, addr string),
) transport.StreamDialer {
dialer := &transport.TCPDialer{}
var onDNSDone func(di httptrace.DNSDoneInfo)
return transport.FuncStreamDialer(func(ctx context.Context, addr string) (transport.StreamConn, error) {
Expand All @@ -134,6 +150,9 @@ func newTCPTraceDialer(
onDNSDone = nil
}
},
ConnectStart: func(network, addr string) {
onDialStart(ctx, network, addr)
},
ConnectDone: func(network, addr string, connErr error) {
onDial(ctx, network, addr, connErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should lookup the table to retrieve and call the onDone handler.

},
Expand All @@ -143,7 +162,10 @@ func newTCPTraceDialer(
}

func newUDPTraceDialer(
onDNS func(ctx context.Context, domain string) func(di httptrace.DNSDoneInfo)) transport.PacketDialer {
onDNS func(ctx context.Context, domain string) func(di httptrace.DNSDoneInfo),
onDial func(ctx context.Context, network, addr string, connErr error),
onDialStart func(ctx context.Context, network, addr string),
) transport.PacketDialer {
dialer := &transport.UDPDialer{}
var onDNSDone func(di httptrace.DNSDoneInfo)
return transport.FuncPacketDialer(func(ctx context.Context, addr string) (net.Conn, error) {
Expand All @@ -157,6 +179,12 @@ func newUDPTraceDialer(
onDNSDone = nil
}
},
ConnectStart: func(network, addr string) {
onDialStart(ctx, network, addr)
},
ConnectDone: func(network, addr string, connErr error) {
onDial(ctx, network, addr, connErr)
},
})
return dialer.DialPacket(ctx, addr)
})
Expand Down Expand Up @@ -240,6 +268,8 @@ func main() {
var mu sync.Mutex
dnsReports := make([]dnsReport, 0)
tcpReports := make([]tcpReport, 0)
udpReports := make([]udpReport, 0)
var connectStart = make(map[string]time.Time)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to the newTraceDialers

providers := configurl.NewDefaultProviders()
onDNS := func(ctx context.Context, domain string) func(di httptrace.DNSDoneInfo) {
dnsStart := time.Now()
Expand Down Expand Up @@ -274,6 +304,8 @@ func main() {
Hostname: hostname,
IP: ip,
Port: port,
Time: connectStart[network+"|"+addr].UTC().Truncate(time.Second),
Duration: time.Since(connectStart[network+"|"+addr]).Milliseconds(),
}
if connErr != nil {
report.Error = connErr.Error()
Expand All @@ -282,10 +314,46 @@ func main() {
tcpReports = append(tcpReports, report)
mu.Unlock()
}
return newTCPTraceDialer(onDNS, onDial).DialStream(ctx, addr)
onDialStart := func(ctx context.Context, network, addr string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should save the start time, and return the onDone handler, which can refer to the start time directly.

mu.Lock()
connectStart[network+"|"+addr] = time.Now()
mu.Unlock()
}

return newTCPTraceDialer(onDNS, onDial, onDialStart).DialStream(ctx, addr)
})

providers.PacketDialers.BaseInstance = transport.FuncPacketDialer(func(ctx context.Context, addr string) (net.Conn, error) {
return newUDPTraceDialer(onDNS).DialPacket(ctx, addr)
hostname, _, err := net.SplitHostPort(addr)
if err != nil {
return nil, err
}
onDialStart := func(ctx context.Context, network, addr string) {
mu.Lock()
connectStart[network+"|"+addr] = time.Now()
mu.Unlock()
}
onDial := func(ctx context.Context, network, addr string, connErr error) {
ip, port, err := net.SplitHostPort(addr)
if err != nil {
return
}
report := udpReport{
Hostname: hostname,
IP: ip,
Port: port,
Time: connectStart[network+"|"+addr].UTC().Truncate(time.Second),
Duration: time.Since(connectStart[network+"|"+addr]).Milliseconds(),
}
if connErr != nil {
report.Error = connErr.Error()
}
mu.Lock()
udpReports = append(udpReports, report)
mu.Unlock()
}

return newUDPTraceDialer(onDNS, onDial, onDialStart).DialPacket(ctx, addr)
})

switch proto {
Expand Down Expand Up @@ -336,6 +404,7 @@ func main() {
},
DNSQueries: dnsReports,
TCPConnections: tcpReports,
UDPConnections: udpReports,
}
if reportCollector != nil {
err = reportCollector.Collect(context.Background(), r)
Expand Down
Loading