From 88e6c2eedf00a9517aa24f9912d8b47b7e98acf4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 13 Jun 2026 01:52:58 +0000 Subject: [PATCH] Tighten host_query comments to match project style https://claude.ai/code/session_01Nibp24Pgk2JMue8VyWHq7o --- host_query_server.go | 135 +++++++++++++++----------------------- host_query_server_test.go | 40 +++++------ 2 files changed, 73 insertions(+), 102 deletions(-) diff --git a/host_query_server.go b/host_query_server.go index 9211d3d9..49e77da3 100644 --- a/host_query_server.go +++ b/host_query_server.go @@ -22,20 +22,17 @@ import ( "github.com/slackhq/nebula/config" ) -// hostQueryServer owns the local host query API: a small HTTP+JSON listener -// on a unix socket or tcp address that lets other programs on this machine -// resolve a vpn address to its certificate identity (name, groups, networks) -// for making authorization decisions. It mirrors the lifecycle shape of -// statsServer: constructor wires the reload callback, reload records config, -// Start builds and runs the runtime, Stop tears it down. +// hostQueryServer is a small http+json listener on a unix socket or tcp address that lets other +// programs on this machine resolve a vpn address to its certificate identity (name, groups, networks) +// for making authorization decisions. Lifecycle works like statsServer: the constructor wires the +// reload callback, reload records config, Start runs the runtime, Stop tears it down type hostQueryServer struct { l *slog.Logger ctx context.Context hostMap *HostMap pki *PKI - // enabled mirrors `host_query.enabled`. Start consults it so callers - // don't need to know the gating rules. + // enabled mirrors `host_query.enabled` so callers of Start don't need to know the gating rules enabled atomic.Bool runMu sync.Mutex @@ -43,35 +40,28 @@ type hostQueryServer struct { run *hostQueryRuntime // non-nil while a runtime is live } -// hostQueryRuntime is the live state owned by a single Start invocation. -// Start stashes a pointer under runMu; Stop and Start's own exit path use -// pointer equality to tell "my runtime" apart from one that replaced it -// after a reload. +// hostQueryRuntime is the live state owned by a single Start invocation. Stop and Start's exit path +// use pointer equality to tell "my runtime" apart from one that replaced it after a reload type hostQueryRuntime struct { server *http.Server listener net.Listener } -// hostQueryConfig is the snapshot of host_query config that drives the -// runtime. It is comparable with == so reload can detect "no change" cheaply. +// hostQueryConfig is a snapshot of the host_query config section, comparable with == so reload can +// detect "no change" cheaply type hostQueryConfig struct { enabled bool listen string // raw config value, for error messages network string // "unix" or "tcp" addr string // socket path or host:port - // socketMode is the file mode applied to the unix socket after bind. + // file mode applied to the unix socket after bind socketMode fs.FileMode } -// newHostQueryServerFromConfig builds a hostQueryServer, applies the initial -// config, and registers a reload callback. The reload callback is registered -// before the initial config is applied so a SIGHUP can later enable, fix, or -// disable the listener even if the initial application failed. -// -// Construction never binds the listener; that happens in Start, so config -// tests are side effect free. Start is safe to call unconditionally: it -// no-ops when the host query API is disabled. The returned pointer is always -// non-nil, even on error. +// newHostQueryServerFromConfig builds a hostQueryServer and applies the initial config. The reload +// callback is registered first so a SIGHUP can later enable, fix, or disable the listener even if +// the initial config was bad. Nothing binds until Start, so config tests are side effect free. +// The returned pointer is always non-nil, even on error func newHostQueryServerFromConfig(ctx context.Context, l *slog.Logger, pki *PKI, hostMap *HostMap, c *config.C) (*hostQueryServer, error) { h := &hostQueryServer{ l: l, @@ -92,14 +82,9 @@ func newHostQueryServerFromConfig(ctx context.Context, l *slog.Logger, pki *PKI, return h, nil } -// reload records the latest config. On the initial call it only records it; -// Control.Start is what launches the first runtime via hostQueryStart. On -// later calls it reconciles the running runtime with the new config: -// -// - newly enabled -> spawn Start -// - newly disabled -> Stop the runtime -// - config changed (still enabled) -> Stop the old, Start the new -// - no change -> no-op +// reload records the latest config. The initial call only records it, Control.Start launches the +// first runtime via hostQueryStart. Later calls reconcile the running listener with the new config: +// enable, disable, or restart when the listen config changed func (h *hostQueryServer) reload(c *config.C, initial bool) error { newCfg, err := loadHostQueryConfig(c) if err != nil { @@ -127,9 +112,8 @@ func (h *hostQueryServer) reload(c *config.C, initial bool) error { return nil } -// Start binds the listener from the latest config and serves until Stop is -// called or ctx fires. Safe to call when the host query API is disabled or -// already running (both no-op). +// Start binds the listener from the latest config and serves until Stop is called or ctx fires. +// Safe to call when disabled or already running (both no-op) func (h *hostQueryServer) Start() { if !h.enabled.Load() { return @@ -143,8 +127,7 @@ func (h *hostQueryServer) Start() { cfg := *h.runCfg ln, err := h.listen(cfg) if err != nil { - // Drop the cached config so a SIGHUP with the same config re-triggers - // Start once the user fixes the underlying problem. + // drop the cached config so a SIGHUP with the same config retries the bind h.runCfg = nil h.runMu.Unlock() h.l.Error("Failed to start host query listener", "listen", cfg.listen, "error", err) @@ -162,19 +145,17 @@ func (h *hostQueryServer) Start() { h.l.Info("Starting host query listener", "network", cfg.network, "addr", ln.Addr()) cleanExit := h.serve(srv, ln) - // A Stop that raced our bind shut the server down before Serve could - // adopt the listener; closing it again is harmless and guarantees a unix - // socket file gets unlinked. + // A Stop that raced our bind shut the server down before Serve could adopt the listener; + // closing it again is harmless and guarantees a unix socket file gets unlinked _ = ln.Close() - // Clear our runtime only if nothing has replaced it. Stop races through - // here too but leaves h.run == nil, so the pointer check skips. + // Clear our runtime only if nothing has replaced it. Stop races through here too but leaves + // h.run == nil, so the pointer check skips h.runMu.Lock() if h.run == rt { h.run = nil - // A listener that exited with an error leaves runCfg cached as if it - // were applied. Drop it so a SIGHUP with the same config re-triggers - // Start once the user fixes the underlying problem. + // an error exit leaves runCfg cached as if it were applied, drop it so a SIGHUP with the + // same config re-triggers Start once the user fixes the underlying problem if !cleanExit { h.runCfg = nil } @@ -182,13 +163,11 @@ func (h *hostQueryServer) Start() { h.runMu.Unlock() } -// serve runs srv.Serve and ensures ctx cancellation unblocks it. Returns true -// if the listener exited cleanly (Stop, ctx cancellation, or any other -// http.ErrServerClosed path), false on an unexpected error. +// serve runs srv.Serve and ensures ctx cancellation unblocks it. Returns true if the listener +// exited cleanly (Stop, ctx cancellation), false on an unexpected error func (h *hostQueryServer) serve(srv *http.Server, ln net.Listener) bool { - // Per-invocation watcher: ctx cancellation triggers a server shutdown - // which in turn unblocks Serve. Closing `done` on exit keeps the watcher - // from outliving this call. + // ctx cancellation triggers a server shutdown which in turn unblocks Serve, closing `done` on + // exit keeps the watcher from outliving this call done := make(chan struct{}) go func() { select { @@ -211,7 +190,7 @@ func (h *hostQueryServer) serve(srv *http.Server, ln net.Listener) bool { return false } -// Stop tears down the active runtime, if any. Idempotent. +// Stop tears down the active runtime, if any. Idempotent func (h *hostQueryServer) Stop() { h.runMu.Lock() rt := h.run @@ -227,9 +206,8 @@ func (h *hostQueryServer) Stop() { } } -// listen binds the configured address. For unix sockets it also clears a -// stale socket file left by an unclean exit and applies the configured file -// mode. +// listen binds the configured address. For unix sockets it also clears a stale socket file left by +// an unclean exit and applies the configured file mode func (h *hostQueryServer) listen(cfg hostQueryConfig) (net.Listener, error) { if cfg.network == "unix" { return h.listenUnix(cfg) @@ -249,9 +227,8 @@ func (h *hostQueryServer) listenUnix(cfg hostQueryConfig) (net.Listener, error) if fi.Mode()&os.ModeSocket == 0 { return nil, fmt.Errorf("host_query.listen path %s exists and is not a socket, refusing to replace it", cfg.addr) } - // A normal shutdown unlinks the socket (unlink-on-close), so a file - // here means a previous process exited uncleanly. Remove it so the - // bind below can succeed. + // a normal shutdown unlinks the socket, so a file here means a previous process exited + // uncleanly, remove it so the bind below can succeed if err = os.Remove(cfg.addr); err != nil { return nil, fmt.Errorf("failed to remove stale socket %s: %w", cfg.addr, err) } @@ -261,9 +238,8 @@ func (h *hostQueryServer) listenUnix(cfg hostQueryConfig) (net.Listener, error) if err != nil { return nil, err } - // The socket is briefly live with umask-derived permissions before this - // chmod lands; tolerated because connections accepted in that window - // still only reach this read-only API. + // The socket is briefly live with umask-derived permissions before this chmod lands, tolerated + // because connections accepted in that window still only reach this read-only API if err = os.Chmod(cfg.addr, cfg.socketMode); err != nil { _ = ln.Close() return nil, fmt.Errorf("failed to set mode on socket %s: %w", cfg.addr, err) @@ -278,10 +254,9 @@ func (h *hostQueryServer) certState() *CertState { return h.pki.getCertState() } -// handleHost serves GET /v1/host?addr=, answering with the identity -// of the host that owns the address: a peer with an active tunnel, or this -// node itself. addr may include a port, which is ignored, so clients can pass -// a connection's remote address through without parsing it. +// handleHost serves GET /v1/host?addr=, answering with the identity of the host that +// owns the address: a peer with an active tunnel, or this node itself. addr may include a port, +// which is ignored, so clients can pass a connection's remote address through without parsing it func (h *hostQueryServer) handleHost(w http.ResponseWriter, r *http.Request) { q := r.URL.Query().Get("addr") if q == "" { @@ -302,7 +277,7 @@ func (h *hostQueryServer) handleHost(w http.ResponseWriter, r *http.Request) { h.writeHostIdentity(w, crt) } -// handleSelf serves GET /v1/self, answering with this node's own identity. +// handleSelf serves GET /v1/self, answering with this node's own identity func (h *hostQueryServer) handleSelf(w http.ResponseWriter, r *http.Request) { var crt cert.Certificate if cs := h.certState(); cs != nil { @@ -327,10 +302,9 @@ func (h *hostQueryServer) writeHostIdentity(w http.ResponseWriter, crt cert.Cert } } -// findCertificateForVpnAddr answers "who owns this vpn address": ourselves -// (from local cert state, since the hostmap never carries an entry for this -// node) or a peer with an active tunnel. Returns nil when the address is -// unknown or the tunnel is mid-teardown. +// findCertificateForVpnAddr answers "who owns this vpn address": ourselves (from local cert state, +// the hostmap never carries an entry for this node) or a peer with an active tunnel. Returns nil +// when the address is unknown or the tunnel is mid-teardown func findCertificateForVpnAddr(cs *CertState, hostMap *HostMap, ip netip.Addr) cert.Certificate { if cs != nil && cs.myVpnAddrsTable != nil && cs.myVpnAddrsTable.Contains(ip) { return cs.getCertificate(cs.initiatingVersion) @@ -347,8 +321,8 @@ func findCertificateForVpnAddr(cs *CertState, hostMap *HostMap, ip netip.Addr) c return cc.Certificate } -// hostIdentity is the JSON document served for both /v1/host and /v1/self. -// Every field is derived from the authenticated certificate alone. +// hostIdentity is the json document served for both /v1/host and /v1/self, every field is derived +// from the authenticated certificate alone type hostIdentity struct { Name string `json:"name"` VpnAddrs []netip.Addr `json:"vpnAddrs"` @@ -368,8 +342,7 @@ func newHostIdentity(crt cert.Certificate) (hostIdentity, error) { return hostIdentity{}, err } - // Slices are always allocated so they marshal as [] rather than null; - // consumers iterate groups without a presence check. + // slices are always allocated so they marshal as [] rather than null networks := crt.Networks() id := hostIdentity{ Name: crt.Name(), @@ -395,10 +368,9 @@ func writeJSONError(w http.ResponseWriter, status int, msg string) { _ = json.NewEncoder(w).Encode(map[string]string{"error": msg}) } -// parseQueryAddrParam parses the addr query parameter, accepting a bare -// address or an address with a port (`192.168.100.7:54321`, `[fd00::1]:443`) -// so callers can pass a connection's RemoteAddr straight through. The result -// is unmapped: 4in6 addresses (::ffff:a.b.c.d) are normalized to ipv4. +// parseQueryAddrParam parses the addr query parameter, accepting a bare address or an address with +// a port (`192.168.100.7:54321`, `[fd00::1]:443`) so callers can pass a connection's RemoteAddr +// straight through. The result is unmapped, 4in6 addresses (::ffff:a.b.c.d) become ipv4 func parseQueryAddrParam(s string) (netip.Addr, error) { if ip, err := netip.ParseAddr(s); err == nil { return ip.Unmap(), nil @@ -430,7 +402,7 @@ func loadHostQueryConfig(c *config.C) (hostQueryConfig, error) { cfg.addr = addr if network == "unix" { - // Read as a string so YAML can't reinterpret the octal literal. + // read as a string so yaml can't reinterpret the octal literal modeStr := c.GetString("host_query.socket_mode", "0600") mode, err := strconv.ParseUint(modeStr, 8, 32) if err != nil || fs.FileMode(mode)&^fs.ModePerm != 0 { @@ -441,9 +413,8 @@ func loadHostQueryConfig(c *config.C) (hostQueryConfig, error) { return cfg, nil } -// parseHostQueryListen splits the host_query.listen config value into a -// network and address for net.Listen: `unix:///abs/path.sock` selects a unix -// socket, anything else must be a tcp host:port. +// parseHostQueryListen splits the host_query.listen config value into a network and address for +// net.Listen: `unix:///abs/path.sock` selects a unix socket, anything else must be a tcp host:port func parseHostQueryListen(listen string) (network string, addr string, err error) { if path, ok := strings.CutPrefix(listen, "unix://"); ok { if !filepath.IsAbs(path) { diff --git a/host_query_server_test.go b/host_query_server_test.go index e4323950..4859847b 100644 --- a/host_query_server_test.go +++ b/host_query_server_test.go @@ -56,17 +56,17 @@ func Test_parseHostQueryListen(t *testing.T) { func Test_loadHostQueryConfig(t *testing.T) { c := config.NewC(nil) - // Absent section: disabled, no error. + // absent section means disabled, no error cfg, err := loadHostQueryConfig(c) require.NoError(t, err) assert.False(t, cfg.enabled) - // Enabled without a listen address is an error. + // enabled without a listen address is an error setHostQueryConfig(c, true, "", "") _, err = loadHostQueryConfig(c) require.Error(t, err) - // Unix socket gets the default mode. + // a unix socket gets the default mode setHostQueryConfig(c, true, "unix:///tmp/hq.sock", "") cfg, err = loadHostQueryConfig(c) require.NoError(t, err) @@ -83,7 +83,7 @@ func Test_loadHostQueryConfig(t *testing.T) { _, err = loadHostQueryConfig(c) require.Error(t, err) - // Mode bits beyond the permission bits are rejected. + // mode bits beyond the permission bits are rejected setHostQueryConfig(c, true, "unix:///tmp/hq.sock", "10600") _, err = loadHostQueryConfig(c) require.Error(t, err) @@ -117,8 +117,8 @@ func newTestHostQueryServer(t *testing.T) (*hostQueryServer, *config.C) { return h, config.NewC(nil) } -// addTestPeer creates a certificate for a peer owning each addr (as a /24 or -// /64) and inserts it into the hostmap as an established tunnel. +// addTestPeer creates a certificate for a peer owning each addr (as a /24 or /64) and inserts it +// into the hostmap as an established tunnel func addTestPeer(t *testing.T, hm *HostMap, name string, addrs []netip.Addr, unsafeNetworks []netip.Prefix, groups []string) cert.Certificate { t.Helper() networks := make([]netip.Prefix, 0, len(addrs)) @@ -173,7 +173,7 @@ func TestHostQueryServer_handleHost(t *testing.T) { []netip.Prefix{netip.MustParsePrefix("192.168.50.0/24")}, []string{"eng", "ssh"}) addTestPeer(t, h.hostMap, "groupless", []netip.Addr{netip.MustParseAddr("10.0.0.77")}, nil, nil) - // An established peer comes back with its full identity. + // an established peer comes back with its full identity code, body := getHost(t, h, "10.0.0.99") require.Equal(t, http.StatusOK, code) assert.Equal(t, "laptop-alice", body["name"]) @@ -186,7 +186,7 @@ func TestHostQueryServer_handleHost(t *testing.T) { assert.NotEmpty(t, body["notBefore"]) assert.NotEmpty(t, body["notAfter"]) - // Empty cert slices marshal as [] rather than null. + // empty cert slices marshal as [] rather than null code, body = getHost(t, h, "10.0.0.77") require.Equal(t, http.StatusOK, code) require.NotNil(t, body["groups"]) @@ -194,15 +194,15 @@ func TestHostQueryServer_handleHost(t *testing.T) { require.NotNil(t, body["unsafeNetworks"]) assert.Empty(t, body["unsafeNetworks"]) - // A port in addr is ignored so RemoteAddr can be passed through directly, - // including the bracketed v6 and 4in6 forms. + // a port in addr is ignored so RemoteAddr can be passed through directly, including the + // bracketed v6 and 4in6 forms for _, q := range []string{"10.0.0.99:54321", "[fd00::99]:443", "::ffff:10.0.0.99"} { code, body = getHost(t, h, q) require.Equal(t, http.StatusOK, code, "addr=%q", q) assert.Equal(t, "laptop-alice", body["name"], "addr=%q", q) } - // Our own address answers from the local cert state. + // our own address answers from the local cert state code, body = getHost(t, h, "10.0.0.1") require.Equal(t, http.StatusOK, code) assert.Equal(t, "self", body["name"]) @@ -211,7 +211,7 @@ func TestHostQueryServer_handleHost(t *testing.T) { assert.Equal(t, http.StatusNotFound, code) assert.NotEmpty(t, body["error"]) - // A tunnel mid-teardown (no peer cert) is treated as unknown. + // a tunnel mid-teardown (no peer cert) is treated as unknown h.hostMap.unlockedAddHostInfo(&HostInfo{ ConnectionState: &ConnectionState{}, vpnAddrs: []netip.Addr{netip.MustParseAddr("10.0.0.66")}, @@ -247,7 +247,7 @@ func TestHostQueryServer_handleSelf(t *testing.T) { assert.Equal(t, "lighthouse", body["name"]) assert.Equal(t, []any{"10.0.0.1"}, body["vpnAddrs"]) - // No cert state available should be an error, not a panic. + // no cert state available should be an error, not a panic h.pki = nil w = httptest.NewRecorder() h.handleSelf(w, r) @@ -267,7 +267,7 @@ func unixHTTPClient(path string) *http.Client { } } -// waitForServe polls until a GET /v1/self through client succeeds. +// waitForServe polls until a GET /v1/self through client succeeds func waitForServe(t *testing.T, client *http.Client) { t.Helper() waitFor(t, func() bool { @@ -370,7 +370,7 @@ func TestHostQueryServer_staleSocket(t *testing.T) { h, _ := newTestHostQueryServer(t) sock := filepath.Join(t.TempDir(), "hq.sock") - // Simulate an unclean exit: a leftover socket file with no listener. + // simulate an unclean exit, a leftover socket file with no listener stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: sock, Net: "unix"}) require.NoError(t, err) stale.SetUnlinkOnClose(false) @@ -407,7 +407,7 @@ func TestHostQueryServer_reload(t *testing.T) { sock1 := filepath.Join(dir, "hq1.sock") sock2 := filepath.Join(dir, "hq2.sock") - // Initial reload only records config; Control.Start launches the runtime. + // initial reload only records config, Control.Start is what launches the runtime setHostQueryConfig(c, false, "unix://"+sock1, "") require.NoError(t, h.reload(c, true)) assert.False(t, h.enabled.Load()) @@ -415,12 +415,12 @@ func TestHostQueryServer_reload(t *testing.T) { assert.Nil(t, h.run) h.runMu.Unlock() - // Enabling via reload spawns the listener. + // enabling via reload spawns the listener setHostQueryConfig(c, true, "unix://"+sock1, "") require.NoError(t, h.reload(c, false)) waitForServe(t, unixHTTPClient(sock1)) - // Changing the listen path restarts on the new address. + // changing the listen path restarts on the new address setHostQueryConfig(c, true, "unix://"+sock2, "") require.NoError(t, h.reload(c, false)) waitForServe(t, unixHTTPClient(sock2)) @@ -429,7 +429,7 @@ func TestHostQueryServer_reload(t *testing.T) { return os.IsNotExist(err) }) - // Reloading an unchanged config does not restart the runtime. + // reloading an unchanged config does not restart the runtime h.runMu.Lock() rt := h.run h.runMu.Unlock() @@ -438,7 +438,7 @@ func TestHostQueryServer_reload(t *testing.T) { assert.Same(t, rt, h.run) h.runMu.Unlock() - // Disabling stops the listener. + // disabling stops the listener setHostQueryConfig(c, false, "unix://"+sock2, "") require.NoError(t, h.reload(c, false)) assert.False(t, h.enabled.Load())