From 599620f6ab7c46eee730704e2d5ee5f3664681e4 Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Mon, 3 Aug 2026 17:53:14 -0500 Subject: [PATCH] Cancel the context when Main hands back no Control --- main.go | 7 +++-- main_test.go | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 main_test.go diff --git a/main.go b/main.go index 2ef2031b..82783217 100644 --- a/main.go +++ b/main.go @@ -22,9 +22,12 @@ type m = map[string]any func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, deviceFactory overlay.DeviceFactory) (retcon *Control, reterr error) { ctx, cancel := context.WithCancel(context.Background()) - // Automatically cancel the context if Main returns an error, to signal all created goroutines to quit. + // The goroutines started below stop only when this context does, and only a caller holding the + // Control can arrange that. Cancel whenever we are not handing one back, which covers an error + // and a config test alike: a config test used to leave the lighthouse query worker, and a + // hostname resolver per dns named static host, running for the life of the process. defer func() { - if reterr != nil { + if retcon == nil { cancel() } }() diff --git a/main_test.go b/main_test.go new file mode 100644 index 00000000..6f7fbd68 --- /dev/null +++ b/main_test.go @@ -0,0 +1,82 @@ +package nebula + +import ( + "fmt" + "net/netip" + "os" + "path/filepath" + "testing" + "time" + + "github.com/slackhq/nebula/cert" + cert_test "github.com/slackhq/nebula/cert_test" + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" +) + +// TestMain_ConfigTestReleasesItsGoroutines pins the rule that Main only leaves goroutines running +// when it hands back a Control to stop them with. +// +// A config test gets no Control, so anything it started had nothing to stop it: the lighthouse +// query worker, and a hostname resolver per dns named static host, ran for the life of the +// process. That matters to every embedder that validates a config in process rather than by +// exec'ing, dnclient and the apple clients included, because they do it on each config load and +// the leak accumulates. +func TestMain_ConfigTestReleasesItsGoroutines(t *testing.T) { + defer goleak.VerifyNone(t, goleak.IgnoreCurrent()) + + l := test.NewLogger() + dir := t.TempDir() + + before := time.Now().Add(-time.Hour) + after := time.Now().Add(time.Hour) + ca, _, caKey, caPEM := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, before, after, nil, nil, nil) + networks := []netip.Prefix{netip.MustParsePrefix("10.0.0.1/24")} + _, _, keyPEM, certPEM := cert_test.NewTestCert( + cert.Version2, cert.Curve_CURVE25519, ca, caKey, "config-test", before, after, networks, nil, nil) + + caPath := filepath.Join(dir, "ca.pem") + certPath := filepath.Join(dir, "cert.pem") + keyPath := filepath.Join(dir, "key.pem") + require.NoError(t, os.WriteFile(caPath, caPEM, 0o600)) + require.NoError(t, os.WriteFile(certPath, certPEM, 0o600)) + require.NoError(t, os.WriteFile(keyPath, keyPEM, 0o600)) + + // A static host by address, not by name: the query worker is the goroutine under test and a + // hostname would drag a real dns lookup into a unit test. + configBody := fmt.Sprintf(` +pki: + ca: %s + cert: %s + key: %s +static_host_map: + "10.0.0.2": ["192.0.2.1:4242"] +lighthouse: + hosts: + - "10.0.0.2" +listen: + host: 127.0.0.1 + port: 0 +tun: + disabled: true +firewall: + outbound: + - port: any + proto: any + host: any + inbound: + - port: any + proto: any + host: any +`, caPath, certPath, keyPath) + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte(configBody), 0o600)) + + c := config.NewC(l) + require.NoError(t, c.Load(dir)) + + ctrl, err := Main(c, true, "config-test", l, nil) + require.NoError(t, err) + require.Nil(t, ctrl, "a config test hands back nothing to stop, so it must stop itself") +}