Tighten host_query comments to match project style

https://claude.ai/code/session_01Nibp24Pgk2JMue8VyWHq7o

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
+// hostQueryServer is a small http+json listener on a unix socket or tcp address that lets other
-// on a unix socket or tcp address that lets other programs on this machine
+// programs on this machine resolve a vpn address to its certificate identity (name, groups, networks)
-// resolve a vpn address to its certificate identity (name, groups, networks)
+// for making authorization decisions. Lifecycle works like statsServer: the constructor wires the
-// for making authorization decisions. It mirrors the lifecycle shape of
+// reload callback, reload records config, Start runs the runtime, Stop tears it down
-// statsServer: constructor wires the reload callback, reload records config,
-// Start builds and 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
+	// enabled mirrors `host_query.enabled` so callers of Start don't need to know the gating rules
-	// 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.
+// hostQueryRuntime is the live state owned by a single Start invocation. Stop and Start's exit path
-// Start stashes a pointer under runMu; Stop and Start's own exit path use
+// use pointer equality to tell "my runtime" apart from one that replaced it after a reload
-// 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
+// hostQueryConfig is a snapshot of the host_query config section, comparable with == so reload can
-// runtime. It is comparable with == so reload can detect "no change" cheaply.
+// 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
+// newHostQueryServerFromConfig builds a hostQueryServer and applies the initial config. The reload
-// config, and registers a reload callback. The reload callback is registered
+// callback is registered first so a SIGHUP can later enable, fix, or disable the listener even if
-// before the initial config is applied so a SIGHUP can later enable, fix, or
+// the initial config was bad. Nothing binds until Start, so config tests are side effect free.
-// disable the listener even if the initial application failed.
+// The returned pointer is always non-nil, even on error
-//
-// 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.
 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;
+// reload records the latest config. The initial call only records it, Control.Start launches the
-// Control.Start is what launches the first runtime via hostQueryStart. On
+// first runtime via hostQueryStart. Later calls reconcile the running listener with the new config:
-// later calls it reconciles the running runtime with the new config:
+// enable, disable, or restart when the listen config changed
-//
-//   - newly enabled -> spawn Start
-//   - newly disabled -> Stop the runtime
-//   - config changed (still enabled) -> Stop the old, Start the new
-//   - no change -> no-op
 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
+// Start binds the listener from the latest config and serves until Stop is called or ctx fires.
-// called or ctx fires. Safe to call when the host query API is disabled or
+// Safe to call when disabled or already running (both no-op)
-// 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
+		// drop the cached config so a SIGHUP with the same config retries the bind
-		// Start once the user fixes the underlying problem.
 		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
+	// A Stop that raced our bind shut the server down before Serve could adopt the listener;
-	// adopt the listener; closing it again is harmless and guarantees a unix
+	// closing it again is harmless and guarantees a unix socket file gets unlinked
-	// socket file gets unlinked.
 	_ = ln.Close()
 
-	// Clear our runtime only if nothing has replaced it. Stop races through
+	// Clear our runtime only if nothing has replaced it. Stop races through here too but leaves
-	// here too but leaves h.run == nil, so the pointer check skips.
+	// 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
+		// an error exit leaves runCfg cached as if it were applied, drop it so a SIGHUP with the
-		// were applied. Drop it so a SIGHUP with the same config re-triggers
+		// same config re-triggers Start once the user fixes the underlying problem
-		// 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
+// serve runs srv.Serve and ensures ctx cancellation unblocks it. Returns true if the listener
-// if the listener exited cleanly (Stop, ctx cancellation, or any other
+// exited cleanly (Stop, ctx cancellation), false on an unexpected error
-// http.ErrServerClosed path), 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
+	// ctx cancellation triggers a server shutdown which in turn unblocks Serve, closing `done` on
-	// which in turn unblocks Serve. Closing `done` on exit keeps the watcher
+	// exit keeps the watcher from outliving this call
-	// 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
+// listen binds the configured address. For unix sockets it also clears a stale socket file left by
-// stale socket file left by an unclean exit and applies the configured file
+// an unclean exit and applies the configured file mode
-// 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
+		// a normal shutdown unlinks the socket, so a file here means a previous process exited
-		// here means a previous process exited uncleanly. Remove it so the
+		// uncleanly, remove it so the bind below can succeed
-		// 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
+	// The socket is briefly live with umask-derived permissions before this chmod lands, tolerated
-	// chmod lands; tolerated because connections accepted in that window
+	// because connections accepted in that window still only reach this read-only API
-	// 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=<vpn addr>, answering with the identity
+// handleHost serves GET /v1/host?addr=<vpn addr>, answering with the identity of the host that
-// of the host that owns the address: a peer with an active tunnel, or this
+// owns the address: a peer with an active tunnel, or this node itself. addr may include a port,
-// node itself. addr may include a port, which is ignored, so clients can pass
+// which is ignored, so clients can pass a connection's remote address through without parsing it
-// 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
+// findCertificateForVpnAddr answers "who owns this vpn address": ourselves (from local cert state,
-// (from local cert state, since the hostmap never carries an entry for this
+// the hostmap never carries an entry for this node) or a peer with an active tunnel. Returns nil
-// node) or a peer with an active tunnel. Returns nil when the address is
+// when the address is unknown or the tunnel is mid-teardown
-// 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.
+// hostIdentity is the json document served for both /v1/host and /v1/self, every field is derived
-// Every field is derived from the authenticated certificate alone.
+// 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;
+	// slices are always allocated so they marshal as [] rather than null
-	// consumers iterate groups without a presence check.
 	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
+// parseQueryAddrParam parses the addr query parameter, accepting a bare address or an address with
-// address or an address with a port (`192.168.100.7:54321`, `[fd00::1]:443`)
+// a port (`192.168.100.7:54321`, `[fd00::1]:443`) so callers can pass a connection's RemoteAddr
-// so callers can pass a connection's RemoteAddr straight through. The result
+// straight through. The result is unmapped, 4in6 addresses (::ffff:a.b.c.d) become ipv4
-// is unmapped: 4in6 addresses (::ffff:a.b.c.d) are normalized to 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
+// parseHostQueryListen splits the host_query.listen config value into a network and address for
-// network and address for net.Listen: `unix:///abs/path.sock` selects a unix
+// net.Listen: `unix:///abs/path.sock` selects a unix socket, anything else must be a tcp host:port
-// 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) {

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
+// addTestPeer creates a certificate for a peer owning each addr (as a /24 or /64) and inserts it
-// /64) and inserts it into the hostmap as an established tunnel.
+// 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,
+	// a port in addr is ignored so RemoteAddr can be passed through directly, including the
-	// including the bracketed v6 and 4in6 forms.
+	// 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())