Copilot commented on code in PR #215: URL: https://github.com/apache/skywalking-rover/pull/215#discussion_r3658447279
########## pkg/accesslog/collector/ztunnel/ztunnel_offsets.go: ########## @@ -0,0 +1,477 @@ +// 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 ztunnel + +import ( + "context" + "fmt" + "github.com/apache/skywalking-rover/pkg/tools/host" + ztunneltool "github.com/apache/skywalking-rover/pkg/tools/ztunnel" + "os/exec" + "sort" + "sync" + "time" +) Review Comment: This import block is not gofmt-compliant (std imports should be grouped separately from third-party imports). If the repo enforces gofmt or linting, this will fail CI. Please run gofmt (or reorder imports into standard/third-party groups) for this and other newly added Go files with similarly ordered imports. ########## bpf/accesslog/ambient/ztunnel.c: ########## @@ -102,3 +114,80 @@ int connection_result_new(struct pt_regs* ctx) { bpf_perf_event_output(ctx, &ztunnel_lb_socket_mapping_event_queue, BPF_F_CURRENT_CPU, event, sizeof(*event)); return 0; } + +// ConnectionResult::record_internal(&mut self, res) is the function that WRITES the ztunnel +// access log line, so &self holds exactly what that line carries: the peer addresses, the +// HBONE target(the real backend pod), and the whole CommonTrafficLabels - which includes the +// source/destination principals and clusters. Reading it here replaces tailing the access log +// file entirely, and does so a poll interval earlier(no kubelet write + tailer read in between). +// +// It is the most stable probe point available: present in every ztunnel from 1.24 through +// master and in the vendor rebuilds, and - unlike the ConnectionResult constructors, which +// return a large struct through a hidden sret pointer that shifts the argument registers on +// x86-64 but not on AArch64 - it returns unit, so &self is in PARM1 on BOTH architectures with +// no arch-conditional register juggling. +// +// record_internal is generic over the error type, so Rust may emit several monomorphized +// copies; user space attaches to every matching symbol(see the attach helper in ztunnel.go). +SEC("uprobe/connection_result_record_internal") +int connection_result_record_internal(struct pt_regs* ctx) { + void *self = (void *)PT_REGS_PARM1(ctx); + if (self == NULL) { + return 0; + } + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct ztunnel_offsets_config_t *cfg = bpf_map_lookup_elem(&ztunnel_offsets_config_map, &pid); + + // FAST PATH: once user space has resolved the offsets and written them here, extract only the + // named fields and ship the ~88 byte compact event instead of the 520 byte window. The identity + // strings live behind pointers in refcounted heap buffers the probe cannot copy, so their raw + // addresses are shipped for user space to follow via /proc/<pid>/mem, exactly as it does today. + if (cfg != NULL && cfg->valid) { + struct ztunnel_connection_result_compact_t *ev = create_ztunnel_connection_result_compact_event(); + if (ev == NULL) { + return 0; + } + // clear the reused per-CPU slot so no stale field from a previous event leaks through + __builtin_memset(ev, 0, sizeof(*ev)); + ev->pid = pid; + // reuse the same SocketAddr decoder the mapping probes use; the event slot is memset to 0 + // so a failed read leaves ip/port zero, and user space treats a zero src as "not a usable + // peer"(decodeCompact), so no separate BPF-side zero check is needed here + get_socket_addr_ip_in_ztunnel(true, (void *)((char *)self + cfg->src), &ev->src_ip, &ev->src_port); + get_socket_addr_ip_in_ztunnel(true, (void *)((char *)self + cfg->dst), &ev->dst_ip, &ev->dst_port); + ev->has_direction = cfg->has_direction ? 1 : 0; + if (cfg->has_direction) { + bpf_probe_read_user(&ev->reporter, sizeof(ev->reporter), (void *)((char *)self + cfg->reporter)); + bpf_probe_read_user(&ev->security_policy, sizeof(ev->security_policy), (void *)((char *)self + cfg->security_policy)); + } Review Comment: The compact path gates *both* `reporter` and `security_policy` reads on `has_direction`, but user space currently derives `has_direction` from `Reporter != OffsetAbsent` (not from both offsets). That means a configuration with `reporter` present but `security_policy == -1` can still execute `self + (-1)` for the security policy read. Recommended fix: gate the reads independently (e.g., only read `reporter` when `cfg->reporter >= 0` and only read `security_policy` when `cfg->security_policy >= 0`), and treat `has_direction` as “direction known” rather than “both bytes meaningful” (or add a separate flag if you need both). -- 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]
