From 678fb51c8f54983761a8bdb957ab8bd67a491f95 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 8 Oct 2025 22:58:21 -0400 Subject: [PATCH 1/8] Fix data race in host infrastructure proxy context map This commit fixes a data race in the host infrastructure where proxyContextMap was being accessed concurrently without synchronization. The race occurred between: - runEnvoy() writing to the map (line 92) - Close() reading from the map (line 37) - stopEnvoy() reading and deleting from the map (lines 125, 129) Added a sync.RWMutex to protect all accesses to proxyContextMap: - Use RLock for reads in Close() and CreateOrUpdateProxyInfra() - Use Lock for writes in runEnvoy() and stopEnvoy() - Extract map keys before iteration in Close() to avoid holding the lock while calling stopEnvoy() This ensures thread-safe access to the proxy context map during concurrent infrastructure operations. Signed-off-by: Adrian Cole --- internal/infrastructure/host/infra.go | 3 +++ internal/infrastructure/host/proxy_infra.go | 23 +++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/internal/infrastructure/host/infra.go b/internal/infrastructure/host/infra.go index 27c782cdc3f..68ff9d09246 100644 --- a/internal/infrastructure/host/infra.go +++ b/internal/infrastructure/host/infra.go @@ -11,6 +11,7 @@ import ( "io" "os" "path/filepath" + "sync" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/envoygateway/config" @@ -46,6 +47,8 @@ type Infra struct { // proxyContextMap store the context of each running proxy by its name for lifecycle management. proxyContextMap map[string]*proxyContext + // mu protects proxyContextMap from concurrent access. + mu sync.RWMutex // TODO: remove this field once it supports the configurable homeDir sdsConfigPath string diff --git a/internal/infrastructure/host/proxy_infra.go b/internal/infrastructure/host/proxy_infra.go index b730573373e..e1ee66ed8db 100644 --- a/internal/infrastructure/host/proxy_infra.go +++ b/internal/infrastructure/host/proxy_infra.go @@ -34,7 +34,14 @@ type proxyContext struct { // Close implements the Manager interface. func (i *Infra) Close() error { + i.mu.RLock() + names := make([]string, 0, len(i.proxyContextMap)) for name := range i.proxyContextMap { + names = append(names, name) + } + i.mu.RUnlock() + + for _, name := range names { i.stopEnvoy(name) } return nil @@ -53,7 +60,10 @@ func (i *Infra) CreateOrUpdateProxyInfra(ctx context.Context, infra *ir.Infra) e proxyInfra := infra.GetProxyInfra() proxyName := utils.GetHashedName(proxyInfra.Name, 64) // Return directly if the proxy is running. - if _, ok := i.proxyContextMap[proxyName]; ok { + i.mu.RLock() + _, running := i.proxyContextMap[proxyName] + i.mu.RUnlock() + if running { return nil } @@ -91,7 +101,9 @@ func (i *Infra) CreateOrUpdateProxyInfra(ctx context.Context, infra *ir.Infra) e func (i *Infra) runEnvoy(ctx context.Context, envoyVersion, name string, args []string) { pCtx, cancel := context.WithCancel(ctx) exit := make(chan struct{}, 1) + i.mu.Lock() i.proxyContextMap[name] = &proxyContext{cancel: cancel, exit: exit} + i.mu.Unlock() go func() { // Run blocks until pCtx is done or the process exits where the latter doesn't happen when // Envoy successfully starts up. So, this will not return until pCtx is done in practice. @@ -124,11 +136,18 @@ func (i *Infra) DeleteProxyInfra(_ context.Context, infra *ir.Infra) error { // stopEnvoy stops the Envoy process by its name. It will block until the process completely stopped. func (i *Infra) stopEnvoy(proxyName string) { - if pCtx, ok := i.proxyContextMap[proxyName]; ok { + i.mu.Lock() + pCtx, ok := i.proxyContextMap[proxyName] + i.mu.Unlock() + + if ok { pCtx.cancel() // Cancel causes the Envoy process to exit. <-pCtx.exit // Wait for the Envoy process to completely exit. close(pCtx.exit) // Close the channel to avoid leaking. + + i.mu.Lock() delete(i.proxyContextMap, proxyName) + i.mu.Unlock() } } From f95e3894f6a3a12b03ae16a0a087372b86a8ae8c Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 8 Oct 2025 23:23:30 -0400 Subject: [PATCH 2/8] use syncmap Signed-off-by: Adrian Cole --- internal/infrastructure/host/infra.go | 5 +-- internal/infrastructure/host/proxy_infra.go | 31 +++++-------- .../infrastructure/host/proxy_infra_test.go | 43 ++++++++++--------- 3 files changed, 33 insertions(+), 46 deletions(-) diff --git a/internal/infrastructure/host/infra.go b/internal/infrastructure/host/infra.go index 68ff9d09246..3536b1800c4 100644 --- a/internal/infrastructure/host/infra.go +++ b/internal/infrastructure/host/infra.go @@ -46,9 +46,7 @@ type Infra struct { EnvoyGateway *egv1a1.EnvoyGateway // proxyContextMap store the context of each running proxy by its name for lifecycle management. - proxyContextMap map[string]*proxyContext - // mu protects proxyContextMap from concurrent access. - mu sync.RWMutex + proxyContextMap sync.Map // TODO: remove this field once it supports the configurable homeDir sdsConfigPath string @@ -82,7 +80,6 @@ func NewInfra(runnerCtx context.Context, cfg *config.Server, logger logging.Logg HomeDir: defaultHomeDir, Logger: logger, EnvoyGateway: cfg.EnvoyGateway, - proxyContextMap: make(map[string]*proxyContext), sdsConfigPath: defaultLocalCertPathDir, defaultEnvoyImage: egv1a1.DefaultEnvoyProxyImage, Stdout: cfg.Stdout, diff --git a/internal/infrastructure/host/proxy_infra.go b/internal/infrastructure/host/proxy_infra.go index e1ee66ed8db..a97d2e59a87 100644 --- a/internal/infrastructure/host/proxy_infra.go +++ b/internal/infrastructure/host/proxy_infra.go @@ -34,12 +34,12 @@ type proxyContext struct { // Close implements the Manager interface. func (i *Infra) Close() error { - i.mu.RLock() - names := make([]string, 0, len(i.proxyContextMap)) - for name := range i.proxyContextMap { - names = append(names, name) - } - i.mu.RUnlock() + // Collect all proxy names first to avoid holding locks during stopEnvoy calls + var names []string + i.proxyContextMap.Range(func(key, value interface{}) bool { + names = append(names, key.(string)) + return true + }) for _, name := range names { i.stopEnvoy(name) @@ -60,10 +60,7 @@ func (i *Infra) CreateOrUpdateProxyInfra(ctx context.Context, infra *ir.Infra) e proxyInfra := infra.GetProxyInfra() proxyName := utils.GetHashedName(proxyInfra.Name, 64) // Return directly if the proxy is running. - i.mu.RLock() - _, running := i.proxyContextMap[proxyName] - i.mu.RUnlock() - if running { + if _, loaded := i.proxyContextMap.Load(proxyName); loaded { return nil } @@ -101,9 +98,7 @@ func (i *Infra) CreateOrUpdateProxyInfra(ctx context.Context, infra *ir.Infra) e func (i *Infra) runEnvoy(ctx context.Context, envoyVersion, name string, args []string) { pCtx, cancel := context.WithCancel(ctx) exit := make(chan struct{}, 1) - i.mu.Lock() - i.proxyContextMap[name] = &proxyContext{cancel: cancel, exit: exit} - i.mu.Unlock() + i.proxyContextMap.Store(name, &proxyContext{cancel: cancel, exit: exit}) go func() { // Run blocks until pCtx is done or the process exits where the latter doesn't happen when // Envoy successfully starts up. So, this will not return until pCtx is done in practice. @@ -136,18 +131,12 @@ func (i *Infra) DeleteProxyInfra(_ context.Context, infra *ir.Infra) error { // stopEnvoy stops the Envoy process by its name. It will block until the process completely stopped. func (i *Infra) stopEnvoy(proxyName string) { - i.mu.Lock() - pCtx, ok := i.proxyContextMap[proxyName] - i.mu.Unlock() - + value, ok := i.proxyContextMap.LoadAndDelete(proxyName) if ok { + pCtx := value.(*proxyContext) pCtx.cancel() // Cancel causes the Envoy process to exit. <-pCtx.exit // Wait for the Envoy process to completely exit. close(pCtx.exit) // Close the channel to avoid leaking. - - i.mu.Lock() - delete(i.proxyContextMap, proxyName) - i.mu.Unlock() } } diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index aa275fac6c3..48d99ea2150 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -47,13 +47,12 @@ func newMockInfra(t *testing.T, cfg *config.Server) *Infra { require.NoError(t, err) infra := &Infra{ - HomeDir: homeDir, - Logger: logging.DefaultLogger(io.Discard, egv1a1.LogLevelInfo), - EnvoyGateway: cfg.EnvoyGateway, - proxyContextMap: make(map[string]*proxyContext), - sdsConfigPath: proxyDir, - Stdout: io.Discard, - Stderr: io.Discard, + HomeDir: homeDir, + Logger: logging.DefaultLogger(io.Discard, egv1a1.LogLevelInfo), + EnvoyGateway: cfg.EnvoyGateway, + sdsConfigPath: proxyDir, + Stdout: io.Discard, + Stderr: io.Discard, } return infra } @@ -104,11 +103,10 @@ func TestInfra_runEnvoy_stopEnvoy(t *testing.T) { stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} i := &Infra{ - proxyContextMap: make(map[string]*proxyContext), - HomeDir: tmpdir, - Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), - Stdout: stdout, - Stderr: stderr, + HomeDir: tmpdir, + Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), + Stdout: stdout, + Stderr: stderr, } // Ensures that run -> stop will successfully stop the envoy and we can // run it again without any issues. @@ -118,9 +116,11 @@ func TestInfra_runEnvoy_stopEnvoy(t *testing.T) { "admin: {address: {socket_address: {address: '127.0.0.1', port_value: 9901}}}", } i.runEnvoy(t.Context(), "", "test", args) - require.Len(t, i.proxyContextMap, 1) + _, ok := i.proxyContextMap.Load("test") + require.True(t, ok, "expected proxy context to be stored") i.stopEnvoy("test") - require.Empty(t, i.proxyContextMap) + _, ok = i.proxyContextMap.Load("test") + require.False(t, ok, "expected proxy context to be removed") // If the cleanup didn't work, the error due to "address already in use" will be tried to be written to the nil logger, // which will panic. } @@ -167,11 +167,10 @@ func TestInfra_runEnvoy_OutputRedirection(t *testing.T) { stderr := buffers[1] i := &Infra{ - proxyContextMap: make(map[string]*proxyContext), - HomeDir: tmpdir, - Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), - Stdout: stdout, - Stderr: stderr, + HomeDir: tmpdir, + Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), + Stdout: stdout, + Stderr: stderr, } // Run envoy with an invalid config to force it to write to stderr and exit quickly @@ -181,7 +180,8 @@ func TestInfra_runEnvoy_OutputRedirection(t *testing.T) { } i.runEnvoy(t.Context(), "", "test", args) - require.Len(t, i.proxyContextMap, 1) + _, ok := i.proxyContextMap.Load("test") + require.True(t, ok, "expected proxy context to be stored") // Wait a bit for envoy to fail require.Eventually(t, func() bool { @@ -189,7 +189,8 @@ func TestInfra_runEnvoy_OutputRedirection(t *testing.T) { }, 5*time.Second, 100*time.Millisecond, "expected output to be written to buffers") i.stopEnvoy("test") - require.Empty(t, i.proxyContextMap) + _, ok = i.proxyContextMap.Load("test") + require.False(t, ok, "expected proxy context to be removed") // Verify that output was captured in buffers (either stdout or stderr should have content) totalOutput := stdout.Len() + stderr.Len() From 47e1494372521c3ebee7b0ec718a98fc8b57b182 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 8 Oct 2025 23:39:59 -0400 Subject: [PATCH 3/8] cover Signed-off-by: Adrian Cole --- .../infrastructure/host/proxy_infra_test.go | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index 48d99ea2150..0978d2036ca 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -126,6 +126,53 @@ func TestInfra_runEnvoy_stopEnvoy(t *testing.T) { } } +func TestInfra_Close(t *testing.T) { + tmpdir := t.TempDir() + // Ensures that all the required binaries are available. + err := func_e.Run(t.Context(), []string{"--version"}, api.HomeDir(tmpdir)) + require.NoError(t, err) + + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + i := &Infra{ + HomeDir: tmpdir, + Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), + Stdout: stdout, + Stderr: stderr, + } + + // Start multiple proxies + ports := []string{"9901", "9902", "9903"} + for idx := range 3 { + args := []string{ + "--config-yaml", + "admin: {address: {socket_address: {address: '127.0.0.1', port_value: " + ports[idx] + "}}}", + } + name := "test-" + ports[idx] + i.runEnvoy(t.Context(), "", name, args) + } + + // Verify all proxies are running + count := 0 + i.proxyContextMap.Range(func(key, value interface{}) bool { + count++ + return true + }) + require.Equal(t, 3, count, "expected 3 proxies to be running") + + // Close should stop all proxies + err = i.Close() + require.NoError(t, err) + + // Verify all proxies are stopped + count = 0 + i.proxyContextMap.Range(func(key, value interface{}) bool { + count++ + return true + }) + require.Equal(t, 0, count, "expected all proxies to be stopped") +} + func TestExtractSemver(t *testing.T) { tests := []struct { image string From 49e7588def787e9f7ca5038d176d871bfbfc472d Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 8 Oct 2025 23:47:12 -0400 Subject: [PATCH 4/8] polish Signed-off-by: Adrian Cole --- internal/infrastructure/host/proxy_infra_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index 0978d2036ca..3c74a08b241 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -7,6 +7,7 @@ package host import ( "bytes" + "fmt" "io" "path" "testing" @@ -132,24 +133,20 @@ func TestInfra_Close(t *testing.T) { err := func_e.Run(t.Context(), []string{"--version"}, api.HomeDir(tmpdir)) require.NoError(t, err) - stdout := &bytes.Buffer{} - stderr := &bytes.Buffer{} i := &Infra{ HomeDir: tmpdir, - Logger: logging.DefaultLogger(stdout, egv1a1.LogLevelInfo), - Stdout: stdout, - Stderr: stderr, + Logger: logging.DefaultLogger(io.Discard, egv1a1.LogLevelInfo), + Stdout: io.Discard, + Stderr: io.Discard, } // Start multiple proxies - ports := []string{"9901", "9902", "9903"} for idx := range 3 { args := []string{ "--config-yaml", - "admin: {address: {socket_address: {address: '127.0.0.1', port_value: " + ports[idx] + "}}}", + "admin: {address: {socket_address: {address: '127.0.0.1', port_value: 0}}}", } - name := "test-" + ports[idx] - i.runEnvoy(t.Context(), "", name, args) + i.runEnvoy(t.Context(), "", fmt.Sprintf("test-%d", idx), args) } // Verify all proxies are running From d3c84f7ef6265329036f5274773fa4278dd69206 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 9 Oct 2025 11:46:41 -0400 Subject: [PATCH 5/8] feedback Signed-off-by: Adrian Cole --- internal/infrastructure/host/proxy_infra.go | 18 +++++++++++------- .../infrastructure/host/proxy_infra_test.go | 9 ++++++--- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/internal/infrastructure/host/proxy_infra.go b/internal/infrastructure/host/proxy_infra.go index a97d2e59a87..ca811bd011a 100644 --- a/internal/infrastructure/host/proxy_infra.go +++ b/internal/infrastructure/host/proxy_infra.go @@ -12,6 +12,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" func_e "github.com/tetratelabs/func-e" "github.com/tetratelabs/func-e/api" @@ -34,16 +35,19 @@ type proxyContext struct { // Close implements the Manager interface. func (i *Infra) Close() error { - // Collect all proxy names first to avoid holding locks during stopEnvoy calls - var names []string - i.proxyContextMap.Range(func(key, value interface{}) bool { - names = append(names, key.(string)) + var wg sync.WaitGroup + + // Stop any Envoy subprocesses in parallel + i.proxyContextMap.Range(func(key, value any) bool { + wg.Add(1) + go func(name string) { + defer wg.Done() + i.stopEnvoy(name) + }(key.(string)) return true }) - for _, name := range names { - i.stopEnvoy(name) - } + wg.Wait() return nil } diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index 3c74a08b241..dee80f8f3cb 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -11,6 +11,7 @@ import ( "io" "path" "testing" + "testing/synctest" "time" "github.com/stretchr/testify/require" @@ -157,9 +158,11 @@ func TestInfra_Close(t *testing.T) { }) require.Equal(t, 3, count, "expected 3 proxies to be running") - // Close should stop all proxies - err = i.Close() - require.NoError(t, err) + // Close should stop all proxies and not leak goroutines + synctest.Test(t, func(t *testing.T) { + err := i.Close() + require.NoError(t, err) + }) // Verify all proxies are stopped count = 0 From 6b0d9e175bcec4e7b0f06594885b7e216a63392a Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 9 Oct 2025 11:51:28 -0400 Subject: [PATCH 6/8] polish Signed-off-by: Adrian Cole --- internal/infrastructure/host/proxy_infra_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index dee80f8f3cb..4967439e645 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -165,12 +165,12 @@ func TestInfra_Close(t *testing.T) { }) // Verify all proxies are stopped - count = 0 - i.proxyContextMap.Range(func(key, value interface{}) bool { - count++ - return true + found := false + i.proxyContextMap.Range(func(key, value any) bool { + found = true + return false }) - require.Equal(t, 0, count, "expected all proxies to be stopped") + require.False(t, found, "expected all proxies to be stopped") } func TestExtractSemver(t *testing.T) { From 1ed6120181afc0ce955307fc5f368e5fe68a3069 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 9 Oct 2025 19:04:11 -0400 Subject: [PATCH 7/8] synctest_is_go125 Signed-off-by: Adrian Cole --- internal/infrastructure/host/proxy_infra_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index 4967439e645..600011158ba 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -11,7 +11,6 @@ import ( "io" "path" "testing" - "testing/synctest" "time" "github.com/stretchr/testify/require" @@ -158,11 +157,9 @@ func TestInfra_Close(t *testing.T) { }) require.Equal(t, 3, count, "expected 3 proxies to be running") - // Close should stop all proxies and not leak goroutines - synctest.Test(t, func(t *testing.T) { - err := i.Close() - require.NoError(t, err) - }) + // Close should stop all proxies + err = i.Close() + require.NoError(t, err) // Verify all proxies are stopped found := false From 8bbb4f19e2f2ce406b17aec5f7dc0c5b769474f7 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 13 Oct 2025 05:33:17 -0400 Subject: [PATCH 8/8] coverage Signed-off-by: Adrian Cole --- .../infrastructure/host/proxy_infra_test.go | 230 ++++++++++++++++-- 1 file changed, 216 insertions(+), 14 deletions(-) diff --git a/internal/infrastructure/host/proxy_infra_test.go b/internal/infrastructure/host/proxy_infra_test.go index 600011158ba..b0745f92c78 100644 --- a/internal/infrastructure/host/proxy_infra_test.go +++ b/internal/infrastructure/host/proxy_infra_test.go @@ -9,7 +9,9 @@ import ( "bytes" "fmt" "io" + "os" "path" + "path/filepath" "testing" "time" @@ -24,9 +26,10 @@ import ( "github.com/envoyproxy/gateway/internal/infrastructure/common" "github.com/envoyproxy/gateway/internal/ir" "github.com/envoyproxy/gateway/internal/logging" + "github.com/envoyproxy/gateway/internal/utils" "github.com/envoyproxy/gateway/internal/utils/file" "github.com/envoyproxy/gateway/internal/xds/bootstrap" - "github.com/envoyproxy/gateway/test/utils" + testutils "github.com/envoyproxy/gateway/test/utils" ) func newMockInfra(t *testing.T, cfg *config.Server) *Infra { @@ -65,31 +68,188 @@ func TestInfraCreateProxy(t *testing.T) { // TODO: add more tests once it supports configurable homeDir and runDir. testCases := []struct { - name string - expect bool - infra *ir.Infra + name string + infra *ir.Infra + expectedError string }{ { - name: "nil cfg", - expect: false, - infra: nil, + name: "nil cfg", + infra: nil, + expectedError: "infra ir is nil", }, { - name: "nil proxy", - expect: false, + name: "nil proxy", infra: &ir.Infra{ Proxy: nil, }, + expectedError: "infra proxy ir is nil", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = infra.CreateOrUpdateProxyInfra(t.Context(), tc.infra) - if tc.expect { - require.NoError(t, err) + actual := infra.CreateOrUpdateProxyInfra(t.Context(), tc.infra) + require.EqualError(t, actual, tc.expectedError) + }) + } +} + +func TestInfra_CreateOrUpdateProxyInfra_Success(t *testing.T) { + tmpdir := t.TempDir() + // Ensures that all the required binaries are available. + err := func_e.Run(t.Context(), []string{"--version"}, api.HomeDir(tmpdir)) + require.NoError(t, err) + + cfg, err := config.New(io.Discard, io.Discard) + require.NoError(t, err) + infra := newMockInfra(t, cfg) + + testCases := []struct { + name string + proxyName string + expectProxyLoaded bool + }{ + { + name: "create new proxy", + proxyName: "test-proxy", + expectProxyLoaded: true, + }, + { + name: "idempotent - proxy already exists", + proxyName: "test-proxy-idempotent", + expectProxyLoaded: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + infraIR := &ir.Infra{ + Proxy: &ir.ProxyInfra{ + Name: tc.proxyName, + Namespace: "default", + Config: &egv1a1.EnvoyProxy{ + Spec: egv1a1.EnvoyProxySpec{ + Logging: egv1a1.ProxyLogging{ + Level: map[egv1a1.ProxyLogComponent]egv1a1.LogLevel{ + egv1a1.LogComponentDefault: egv1a1.LogLevelInfo, + }, + }, + }, + }, + }, + } + + hashedName := utils.GetHashedName(tc.proxyName, 64) + t.Cleanup(func() { infra.stopEnvoy(hashedName) }) + + // First call should create the proxy + actual := infra.CreateOrUpdateProxyInfra(t.Context(), infraIR) + require.NoError(t, actual) + + // Verify proxy context was stored + _, loaded := infra.proxyContextMap.Load(hashedName) + require.Equal(t, tc.expectProxyLoaded, loaded) + + // Second call should be idempotent (early return) + actual = infra.CreateOrUpdateProxyInfra(t.Context(), infraIR) + require.NoError(t, actual) + + // Verify proxy is still loaded + _, loaded = infra.proxyContextMap.Load(hashedName) + require.Equal(t, tc.expectProxyLoaded, loaded) + }) + } +} + +func TestInfra_DeleteProxyInfra(t *testing.T) { + tmpdir := t.TempDir() + // Ensures that all the required binaries are available. + err := func_e.Run(t.Context(), []string{"--version"}, api.HomeDir(tmpdir)) + require.NoError(t, err) + + cfg, err := config.New(io.Discard, io.Discard) + require.NoError(t, err) + infra := newMockInfra(t, cfg) + + testCases := []struct { + name string + setupProxy bool + proxyName string + infraIR *ir.Infra + expectedError string + expectRemoved bool + }{ + { + name: "delete existing proxy", + setupProxy: true, + proxyName: "test-proxy-delete", + infraIR: &ir.Infra{ + Proxy: &ir.ProxyInfra{ + Name: "test-proxy-delete", + Namespace: "default", + Config: &egv1a1.EnvoyProxy{ + Spec: egv1a1.EnvoyProxySpec{ + Logging: egv1a1.ProxyLogging{ + Level: map[egv1a1.ProxyLogComponent]egv1a1.LogLevel{ + egv1a1.LogComponentDefault: egv1a1.LogLevelInfo, + }, + }, + }, + }, + }, + }, + expectedError: "", + expectRemoved: true, + }, + { + name: "delete non-existent proxy", + setupProxy: false, + proxyName: "non-existent-proxy", + infraIR: &ir.Infra{ + Proxy: &ir.ProxyInfra{ + Name: "non-existent-proxy", + Namespace: "default", + Config: &egv1a1.EnvoyProxy{}, + }, + }, + expectedError: "", + expectRemoved: false, + }, + { + name: "nil infra", + setupProxy: false, + infraIR: nil, + expectedError: "infra ir is nil", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var hashedName string + if tc.setupProxy { + // Create a proxy first + actual := infra.CreateOrUpdateProxyInfra(t.Context(), tc.infraIR) + require.NoError(t, actual) + + hashedName = utils.GetHashedName(tc.proxyName, 64) + t.Cleanup(func() { infra.stopEnvoy(hashedName) }) + + _, loaded := infra.proxyContextMap.Load(hashedName) + require.True(t, loaded, "proxy should be loaded before deletion") + } + + // Delete the proxy + actual := infra.DeleteProxyInfra(t.Context(), tc.infraIR) + if tc.expectedError != "" { + require.EqualError(t, actual, tc.expectedError) } else { - require.Error(t, err) + require.NoError(t, actual) + } + + // Verify deletion + if tc.expectRemoved { + _, loaded := infra.proxyContextMap.Load(hashedName) + require.False(t, loaded, "proxy should be removed after deletion") } }) } @@ -206,7 +366,7 @@ func TestInfra_runEnvoy_OutputRedirection(t *testing.T) { require.NoError(t, err) // Create separate buffers for stdout and stderr - buffers := utils.DumpLogsOnFail(t, "stdout", "stderr") + buffers := testutils.DumpLogsOnFail(t, "stdout", "stderr") stdout := buffers[0] stderr := buffers[1] @@ -323,6 +483,48 @@ func TestGetEnvoyVersion(t *testing.T) { // TestTopologyInjectorDisabledInHostMode verifies we don't cause a 15+ second // startup delay in standalone mode as Envoy waits for endpoint discovery. // See: https://github.com/envoyproxy/gateway/issues/7080 +func TestNewInfra(t *testing.T) { + // This test verifies successful creation of Infra using defaults. + // NewInfra uses hardcoded paths like defaultHomeDir and defaultLocalCertPathDir, + // so we can only test the success case where those directories exist. + cfg, err := config.New(io.Discard, io.Discard) + require.NoError(t, err) + + actual, err := NewInfra(t.Context(), cfg, logging.DefaultLogger(io.Discard, egv1a1.LogLevelInfo)) + require.NoError(t, err) + require.NotNil(t, actual) + require.Equal(t, defaultHomeDir, actual.HomeDir) + require.Equal(t, defaultLocalCertPathDir, actual.sdsConfigPath) + require.NotNil(t, actual.Logger) + require.NotNil(t, actual.EnvoyGateway) + require.Equal(t, egv1a1.DefaultEnvoyProxyImage, actual.defaultEnvoyImage) + require.NotNil(t, actual.Stdout) + require.NotNil(t, actual.Stderr) +} + +func TestCreateSdsConfig(t *testing.T) { + dir := t.TempDir() + // Create required cert files + require.NoError(t, file.WriteDir([]byte("test ca"), dir, XdsTLSCaFilename)) + require.NoError(t, file.WriteDir([]byte("test cert"), dir, XdsTLSCertFilename)) + require.NoError(t, file.WriteDir([]byte("test key"), dir, XdsTLSKeyFilename)) + + actual := createSdsConfig(dir) + require.NoError(t, actual) + + // Verify CA config was created + caConfigPath := filepath.Join(dir, common.SdsCAFilename) + actualCAConfig, err := os.ReadFile(caConfigPath) + require.NoError(t, err) + require.NotEmpty(t, actualCAConfig) + + // Verify cert config was created + certConfigPath := filepath.Join(dir, common.SdsCertFilename) + actualCertConfig, err := os.ReadFile(certConfigPath) + require.NoError(t, err) + require.NotEmpty(t, actualCertConfig) +} + func TestTopologyInjectorDisabledInHostMode(t *testing.T) { testCases := []struct { name string