Copilot commented on code in PR #213:
URL: https://github.com/apache/skywalking-rover/pull/213#discussion_r3587411137
##########
pkg/accesslog/common/connection.go:
##########
@@ -277,6 +443,163 @@ func (c *ConnectionManager) buildRemoteAddress(e
*events.SocketConnectEvent, soc
return c.buildAddressFromRemote(socket.DestIP, socket.DestPort)
}
+// recordConnectionResolveResult records the final remote address resolve
result of the
+// connection, it MUST be called when the connection is being deleted from the
manager,
+// at that point all resolvers are finalized: the conntrack rewriting happens
on the
+// address building, and the ztunnel correlation could attach the real
destination on
+// any later flush, so judging any earlier would mis-count the resolved
connections
+func (c *ConnectionManager) recordConnectionResolveResult(connection
*ConnectionInfo) {
+ if connection == nil || connection.RPCConnection == nil {
+ return
+ }
+ remote := connection.RPCConnection.GetRemote()
+ if remote == nil || remote.GetIp() == nil {
+ // the remote address is resolved to a local monitored process,
not a raw IP
+ return
+ }
+ if connection.RPCConnection.GetAttachment() != nil {
+ // the ztunnel correlation attached the real destination
+ c.ztunnelResolvedRemoteCount.Add(1)
+ return
+ }
+ if connection.Socket != nil && connection.Socket.ConnTrackResolved {
+ // the conntrack query already rewrote the address to the real
peer(e.g. pod IP),
+ // the address is sent as a raw IP but the backend could
resolve it
+ c.conntrackResolvedRemoteCount.Add(1)
+ return
+ }
+ // ask the resolution-aware flusher(s) WHY this connection ended up
unresolved, so the summary
+ // can group the raw-IP socket pairs by the environment/source that
failed to provide a mapping
+ reason := "unresolved"
+ for _, l := range c.flushListeners {
+ if r, ok := l.(ResolutionAwareFlusher); ok {
+ if rs := r.UnresolvedReason(connection); rs != "" {
+ reason = rs
+ break
+ }
+ }
+ }
+ src := "unknown"
+ if connection.Socket != nil {
+ src = fmt.Sprintf("%s:%d", connection.Socket.SrcIP,
connection.Socket.SrcPort)
+ }
+ c.recordUnresolvedRemote(reason, fmt.Sprintf("%s->%s:%d", src,
remote.GetIp().GetHost(), remote.GetIp().GetPort()))
+}
+
+// recordUnresolvedRemote records a socket pair(src->dst) that cannot be
resolved by any of the
+// resolvers(local process, conntrack, ztunnel correlation) - it would reach
the backend as a raw
+// IP - together with the categorized reason, for the periodic summary. The
per-reason totals are
+// always counted; the per-pair map is bounded so a flood of distinct pairs
cannot grow it without
+// limit.
+func (c *ConnectionManager) recordUnresolvedRemote(reason, socketPair string) {
+ c.unresolvedRemoteCount.Add(1)
+ c.unresolvedRemoteLock.Lock()
+ defer c.unresolvedRemoteLock.Unlock()
+ c.unresolvedByReason[reason]++
+ key := reason + " " + socketPair
+ if _, exist := c.unresolvedRemotes[key]; !exist &&
len(c.unresolvedRemotes) >= unresolvedRemoteMaxTrack {
+ return
+ }
+ c.unresolvedRemotes[key]++
+}
+
+// drainUnresolvedRemotes returns, since the last report, the top un-resolved
socket pairs(each
+// prefixed with its reason) and the full per-reason breakdown, and resets the
tracking maps.
+func (c *ConnectionManager) drainUnresolvedRemotes() (topPairs, byReason
string) {
+ c.unresolvedRemoteLock.Lock()
+ pairs := c.unresolvedRemotes
+ reasons := c.unresolvedByReason
+ c.unresolvedRemotes = make(map[string]int64)
+ c.unresolvedByReason = make(map[string]int64)
+ c.unresolvedRemoteLock.Unlock()
+
+ sortByCount := func(m map[string]int64, topN int) string {
+ if len(m) == 0 {
+ return "none"
+ }
+ type kv struct {
+ k string
+ v int64
+ }
+ sorted := make([]kv, 0, len(m))
+ for k, v := range m {
+ sorted = append(sorted, kv{k: k, v: v})
+ }
+ sort.Slice(sorted, func(i, j int) bool { return sorted[i].v >
sorted[j].v })
+ if topN > 0 && len(sorted) > topN {
+ sorted = sorted[:topN]
+ }
+ items := make([]string, 0, len(sorted))
+ for _, s := range sorted {
+ items = append(items, fmt.Sprintf("%s=%d", s.k, s.v))
+ }
+ return strings.Join(items, ", ")
+ }
+ return sortByCount(pairs, unresolvedRemoteTopCount),
sortByCount(reasons, 0)
+}
+
+// startUnresolvedRemoteReporter periodically reports the summary of remote
addresses
+// that cannot be resolved(by neither the conntrack nor the ztunnel
correlation) at the info level
+func (c *ConnectionManager) startUnresolvedRemoteReporter(ctx context.Context)
{
+ go func() {
+ ticker := time.NewTicker(unresolvedRemoteReportInterval)
+ defer ticker.Stop()
+ var lastUnresolvedCount int64
+ for {
+ select {
+ case <-ticker.C:
+ unresolvedCount :=
c.unresolvedRemoteCount.Load()
+ if unresolvedCount == lastUnresolvedCount {
+ continue
+ }
+ lastUnresolvedCount = unresolvedCount
+ conntrackStats := "not available"
+ if c.connectTracker != nil {
+ conntrackStats =
c.connectTracker.StatsString()
+ }
+ topPairs, byReason := c.drainUnresolvedRemotes()
+ log.Infof("remote address resolve summary:
total remote addresses built: %d, "+
+ "resolved by conntrack: %d, resolved by
ztunnel correlation: %d, unresolved(sent as raw IP): %d, "+
+ "conntrack stats: {%s}, unresolved by
reason: {%s}, "+
+ "open->resolution latency buckets {<1s:
%d, 1-5s: %d, 5-15s: %d, >=15s: %d, max: %dms}, "+
+ "top unresolved socket pairs since last
report: %s",
+ c.remoteAddressBuildCount.Load(),
c.conntrackResolvedRemoteCount.Load(),
+ c.ztunnelResolvedRemoteCount.Load(),
unresolvedCount,
+ conntrackStats, byReason,
+ c.resolutionLatencyUnder1s.Load(),
c.resolutionLatency1to5s.Load(),
+ c.resolutionLatency5to15s.Load(),
c.resolutionLatencyOver15s.Load(),
+ c.resolutionLatencyMaxMilli.Load(),
topPairs)
Review Comment:
The periodic "remote address resolve summary" log mixes cumulative and
interval metrics: `unresolvedRemoteCount` is cumulative since start, but
`unresolved by reason` / `top unresolved socket pairs since last report` are
drained-and-reset per interval. This makes the log line internally inconsistent
and can mislead operational debugging (e.g., reasons/pairs appear to drop while
the unresolved count only increases). Consider logging a per-interval delta
(and optionally also the cumulative total) so all fields describe the same time
window.
##########
pkg/tools/netns/netns.go:
##########
@@ -0,0 +1,68 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package netns
+
+import (
+ "fmt"
+ "os"
+ "runtime"
+
+ "golang.org/x/sys/unix"
+)
+
+// RunInNetNS executes fn with the current OS thread switched into the network
+// namespace referenced by netnsPath(e.g. /proc/<pid>/ns/net), and restores the
+// original network namespace afterwards.
+//
+// Requires CAP_SYS_ADMIN. Everything that must happen inside the target
+// namespace(creating sockets, dialing, reading responses) should be done
+// inside fn, since only the calling OS thread is switched.
Review Comment:
The function comment says the original network namespace is restored
afterwards, but on a restore failure `RunInNetNS` returns while intentionally
keeping the OS thread locked. At that point the caller goroutine will continue
running pinned to a thread that may still be in the target netns, which is a
surprising and potentially dangerous contract. At minimum, document that a
restore failure leaves the goroutine locked to its OS thread and it should exit
promptly to avoid doing further work in the wrong namespace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
