From 59048cbaa1cb4e16b24121814ae25d3c04de3586 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Wed, 16 Sep 2026 18:12:36 -0700 Subject: [PATCH 1/3] Clean up tunnel build contexts on shutdown Ensure tunnel proxy build context archives are removed when the reconciler lifetime ends, including ambiguous image creation outcomes. Update integration tests to verify controller-owned cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...ntainer_network_tunnel_proxy_controller.go | 37 ++++++++++++++++--- .../container_network_tunnel_proxy_test.go | 4 +- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/controllers/container_network_tunnel_proxy_controller.go b/controllers/container_network_tunnel_proxy_controller.go index baefe4bb..8c3d0c7e 100644 --- a/controllers/container_network_tunnel_proxy_controller.go +++ b/controllers/container_network_tunnel_proxy_controller.go @@ -1240,7 +1240,11 @@ func (r *ContainerNetworkTunnelProxyReconciler) ensureTunnelProxyPhysicalContain currentImage := apiv2.PhysicalContainerImage{} lookupErr := r.NoCacheClient.Get(ctx, imageName, ¤tImage) if lookupErr == nil { - removeUnusedTunnelProxyBuildContext(imagePlan.BuildContextArchive.Source, ¤tImage, log) + if tunnelProxyPhysicalImageUsesBuildContext(¤tImage, imagePlan.BuildContextArchive.Source) { + r.removeTunnelProxyBuildContextOnShutdown(imagePlan.BuildContextArchive.Source, log) + } else { + removeUnusedTunnelProxyBuildContext(imagePlan.BuildContextArchive.Source, ¤tImage, log) + } return nil } if apimachinery_errors.IsNotFound(lookupErr) { @@ -1254,6 +1258,7 @@ func (r *ContainerNetworkTunnelProxyReconciler) ensureTunnelProxyPhysicalContain ) } + r.removeTunnelProxyBuildContextOnShutdown(imagePlan.BuildContextArchive.Source, log) log.V(1).Info("Created shared tunnel proxy PhysicalContainerImage", "PhysicalContainerImage", imageName) return nil } @@ -1298,11 +1303,7 @@ func removeUnusedTunnelProxyBuildContext( if buildContextSource == "" { return } - if physicalImage != nil && - physicalImage.Spec.Image != nil && - physicalImage.Spec.Image.Build != nil && - physicalImage.Spec.Image.Build.ContextArchive != nil && - physicalImage.Spec.Image.Build.ContextArchive.Source == buildContextSource { + if tunnelProxyPhysicalImageUsesBuildContext(physicalImage, buildContextSource) { return } @@ -1312,6 +1313,30 @@ func removeUnusedTunnelProxyBuildContext( } } +func tunnelProxyPhysicalImageUsesBuildContext( + physicalImage *apiv2.PhysicalContainerImage, + buildContextSource string, +) bool { + return physicalImage != nil && + physicalImage.Spec.Image != nil && + physicalImage.Spec.Image.Build != nil && + physicalImage.Spec.Image.Build.ContextArchive != nil && + physicalImage.Spec.Image.Build.ContextArchive.Source == buildContextSource +} + +func (r *ContainerNetworkTunnelProxyReconciler) removeTunnelProxyBuildContextOnShutdown( + buildContextSource string, + log logr.Logger, +) { + if buildContextSource == "" { + return + } + + context.AfterFunc(r.LifetimeCtx, func() { + removeUnusedTunnelProxyBuildContext(buildContextSource, nil, log) + }) +} + func (r *ContainerNetworkTunnelProxyReconciler) updateClientProxyContainerStatus( ctx context.Context, tunnelProxy *apiv1.ContainerNetworkTunnelProxy, diff --git a/test/integration/container_network_tunnel_proxy_test.go b/test/integration/container_network_tunnel_proxy_test.go index 5ea196be..3b5b4315 100644 --- a/test/integration/container_network_tunnel_proxy_test.go +++ b/test/integration/container_network_tunnel_proxy_test.go @@ -316,7 +316,7 @@ func TestTunnelProxyRunningStatus(t *testing.T) { require.NoError(t, globErr) require.Equal(t, []string{physicalImages.Items[0].Spec.Image.Build.ContextArchive.Source}, buildContextFiles) t.Cleanup(func() { - require.NoError(t, os.Remove(physicalImages.Items[0].Spec.Image.Build.ContextArchive.Source)) + require.NoFileExists(t, physicalImages.Items[0].Spec.Image.Build.ContextArchive.Source) }) require.Equal(t, apiv2.PhysicalContainerImagePhaseReady, physicalImages.Items[0].Status.Phase) require.Equal(t, 1, testContainerOrchestrator.BuildImageCallCount(updatedTunnelProxy.Status.ClientProxyContainerImage)) @@ -509,7 +509,7 @@ func TestTunnelProxyCleanup(t *testing.T) { require.Empty(t, physicalImage.Spec.Image.Build.ContextArchive.RawContents) require.FileExists(t, physicalImage.Spec.Image.Build.ContextArchive.Source) t.Cleanup(func() { - require.NoError(t, os.Remove(physicalImage.Spec.Image.Build.ContextArchive.Source)) + require.NoFileExists(t, physicalImage.Spec.Image.Build.ContextArchive.Source) }) t.Logf("Deleting ContainerNetworkTunnelProxy object '%s'", tunnelProxy.ObjectMeta.Name) From fd81130301f395019cbb01cd41cc96ee836c0d25 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Wed, 16 Sep 2026 18:29:00 -0700 Subject: [PATCH 2/3] Use session folders for tunnel build contexts Make tunnel build context placement explicit so integration controllers write archives into their per-test session folders. Rely on the existing awaited session cleanup instead of an asynchronous controller callback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...ntainer_network_tunnel_proxy_controller.go | 46 ++++++------------- internal/dcpctrl/commands/run_controllers.go | 2 + internal/dcptun/image_builder.go | 15 ++++-- internal/dcptun/image_builder_test.go | 10 ++-- test/integration/advanced_test_env.go | 1 + .../container_network_tunnel_proxy_test.go | 16 +------ test/integration/standard_test_env.go | 3 ++ 7 files changed, 39 insertions(+), 54 deletions(-) diff --git a/controllers/container_network_tunnel_proxy_controller.go b/controllers/container_network_tunnel_proxy_controller.go index 8c3d0c7e..c8a6bb3e 100644 --- a/controllers/container_network_tunnel_proxy_controller.go +++ b/controllers/container_network_tunnel_proxy_controller.go @@ -98,6 +98,10 @@ var ( type ContainerNetworkTunnelProxyReconcilerConfig struct { ProcessExecutor process.Executor // Mandatory + // Directory that owns the lifetime of generated tunnel proxy image build contexts. + // Mandatory. + BuildContextDir string + // The factory function to create a TunnelControlClient used to control the proxy pair. // Normal execution uses "real" gRPC client, tests use a stub since most tests do not run real tunnels. // Mandatory. @@ -132,6 +136,9 @@ func NewContainerNetworkTunnelProxyReconciler( if config.ProcessExecutor == nil { panic("ContainerNetworkTunnelProxyReconcilerConfig.ProcessExecutor must not be nil") } + if config.BuildContextDir == "" { + panic("ContainerNetworkTunnelProxyReconcilerConfig.BuildContextDir must not be empty") + } if config.MakeTunnelControlClient == nil { panic("ContainerNetworkTunnelProxyReconcilerConfig.TunnelControlClientFactory must not be nil") } @@ -1204,7 +1211,7 @@ func (r *ContainerNetworkTunnelProxyReconciler) ensureTunnelProxyPhysicalContain return fmt.Errorf("get PhysicalContainerImage %q: %w", imageName.String(), getErr) } - imagePlan, prepareErr := dcptun.PrepareClientProxyImageBuild(ctx) + imagePlan, prepareErr := dcptun.PrepareClientProxyImageBuild(ctx, r.config.BuildContextDir) if prepareErr != nil { return prepareErr } @@ -1240,11 +1247,7 @@ func (r *ContainerNetworkTunnelProxyReconciler) ensureTunnelProxyPhysicalContain currentImage := apiv2.PhysicalContainerImage{} lookupErr := r.NoCacheClient.Get(ctx, imageName, ¤tImage) if lookupErr == nil { - if tunnelProxyPhysicalImageUsesBuildContext(¤tImage, imagePlan.BuildContextArchive.Source) { - r.removeTunnelProxyBuildContextOnShutdown(imagePlan.BuildContextArchive.Source, log) - } else { - removeUnusedTunnelProxyBuildContext(imagePlan.BuildContextArchive.Source, ¤tImage, log) - } + removeUnusedTunnelProxyBuildContext(imagePlan.BuildContextArchive.Source, ¤tImage, log) return nil } if apimachinery_errors.IsNotFound(lookupErr) { @@ -1258,7 +1261,6 @@ func (r *ContainerNetworkTunnelProxyReconciler) ensureTunnelProxyPhysicalContain ) } - r.removeTunnelProxyBuildContextOnShutdown(imagePlan.BuildContextArchive.Source, log) log.V(1).Info("Created shared tunnel proxy PhysicalContainerImage", "PhysicalContainerImage", imageName) return nil } @@ -1303,7 +1305,11 @@ func removeUnusedTunnelProxyBuildContext( if buildContextSource == "" { return } - if tunnelProxyPhysicalImageUsesBuildContext(physicalImage, buildContextSource) { + if physicalImage != nil && + physicalImage.Spec.Image != nil && + physicalImage.Spec.Image.Build != nil && + physicalImage.Spec.Image.Build.ContextArchive != nil && + physicalImage.Spec.Image.Build.ContextArchive.Source == buildContextSource { return } @@ -1313,30 +1319,6 @@ func removeUnusedTunnelProxyBuildContext( } } -func tunnelProxyPhysicalImageUsesBuildContext( - physicalImage *apiv2.PhysicalContainerImage, - buildContextSource string, -) bool { - return physicalImage != nil && - physicalImage.Spec.Image != nil && - physicalImage.Spec.Image.Build != nil && - physicalImage.Spec.Image.Build.ContextArchive != nil && - physicalImage.Spec.Image.Build.ContextArchive.Source == buildContextSource -} - -func (r *ContainerNetworkTunnelProxyReconciler) removeTunnelProxyBuildContextOnShutdown( - buildContextSource string, - log logr.Logger, -) { - if buildContextSource == "" { - return - } - - context.AfterFunc(r.LifetimeCtx, func() { - removeUnusedTunnelProxyBuildContext(buildContextSource, nil, log) - }) -} - func (r *ContainerNetworkTunnelProxyReconciler) updateClientProxyContainerStatus( ctx context.Context, tunnelProxy *apiv1.ContainerNetworkTunnelProxy, diff --git a/internal/dcpctrl/commands/run_controllers.go b/internal/dcpctrl/commands/run_controllers.go index 95eacce3..ba0073f2 100644 --- a/internal/dcpctrl/commands/run_controllers.go +++ b/internal/dcpctrl/commands/run_controllers.go @@ -30,6 +30,7 @@ import ( "github.com/microsoft/dcp/internal/perftrace" "github.com/microsoft/dcp/internal/proxy" "github.com/microsoft/dcp/internal/statestore" + usvc_io "github.com/microsoft/dcp/pkg/io" "github.com/microsoft/dcp/pkg/kubeconfig" "github.com/microsoft/dcp/pkg/logger" "github.com/microsoft/dcp/pkg/process" @@ -376,6 +377,7 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), controllers.ContainerNetworkTunnelProxyReconcilerConfig{ ProcessExecutor: processExecutor, + BuildContextDir: usvc_io.DcpTempDir(), MakeTunnelControlClient: dcptunproto.NewTunnelControlClient, }, log.WithName("TunnelProxyReconciler"), diff --git a/internal/dcptun/image_builder.go b/internal/dcptun/image_builder.go index 1d1f01ef..5b3ef826 100644 --- a/internal/dcptun/image_builder.go +++ b/internal/dcptun/image_builder.go @@ -52,7 +52,11 @@ type ClientProxyImageBuildPlan struct { // PrepareClientProxyImageBuild creates the build input for the shared tunnel proxy // PhysicalContainerImage. -func PrepareClientProxyImageBuild(ctx context.Context) (ClientProxyImageBuildPlan, error) { +func PrepareClientProxyImageBuild(ctx context.Context, buildContextDir string) (ClientProxyImageBuildPlan, error) { + if buildContextDir == "" { + return ClientProxyImageBuildPlan{}, fmt.Errorf("build context directory is required") + } + dcpTunClientPath, clientPathErr := dcptunClientBinaryPath() if clientPathErr != nil { return ClientProxyImageBuildPlan{}, fmt.Errorf("failed to get path to dcptun client binary: %w", clientPathErr) @@ -69,7 +73,7 @@ func PrepareClientProxyImageBuild(ctx context.Context) (ClientProxyImageBuildPla if digestErr != nil { return ClientProxyImageBuildPlan{}, digestErr } - buildContextArchive, contextErr := setupImageBuildContextArchive(ctx, dcpTunClientPath, dockerfileContent) + buildContextArchive, contextErr := setupImageBuildContextArchive(ctx, buildContextDir, dcpTunClientPath, dockerfileContent) if contextErr != nil { return ClientProxyImageBuildPlan{}, fmt.Errorf("failed to create build context archive: %w", contextErr) } @@ -99,6 +103,7 @@ func clientProxyImageName(clientBinaryHash string) string { func setupImageBuildContextArchive( ctx context.Context, + buildContextDir string, dcpTunClientPath string, dockerfileContent string, ) (*containers.ContainerBuildContextArchive, error) { @@ -110,9 +115,9 @@ func setupImageBuildContextArchive( if randomSuffixErr != nil { return nil, fmt.Errorf("create random build context archive suffix: %w", randomSuffixErr) } - // The archive lives in the DCP session directory, whose lifetime owns its cleanup. - archiveFile, openArchiveErr := usvc_io.CreateNewTempFile( - fmt.Sprintf("dcptun-build-context-%s.tar", randomSuffix), + // The caller-provided directory owns the archive lifetime. + archiveFile, openArchiveErr := usvc_io.CreateNewFile( + filepath.Join(buildContextDir, fmt.Sprintf("dcptun-build-context-%s.tar", randomSuffix)), osutil.PermissionOnlyOwnerReadWrite, ) if openArchiveErr != nil { diff --git a/internal/dcptun/image_builder_test.go b/internal/dcptun/image_builder_test.go index 5d40a5b1..bc1bf967 100644 --- a/internal/dcptun/image_builder_test.go +++ b/internal/dcptun/image_builder_test.go @@ -10,6 +10,7 @@ import ( "context" "io" "os" + "path/filepath" "strings" "testing" @@ -24,8 +25,9 @@ func TestPrepareClientProxyImageBuild(t *testing.T) { t.Parallel() dcppaths.EnableTestPathProbing() + buildContextDir := t.TempDir() - plan, prepareErr := dcptun.PrepareClientProxyImageBuild(context.Background()) + plan, prepareErr := dcptun.PrepareClientProxyImageBuild(context.Background(), buildContextDir) require.NoError(t, prepareErr) const expectedTagPrefix = "dcptun_developer_ms:" @@ -34,6 +36,7 @@ func TestPrepareClientProxyImageBuild(t *testing.T) { require.NotNil(t, plan.BuildContextArchive) require.NotEmpty(t, plan.BuildContextDigest) require.NotEmpty(t, plan.BuildContextArchive.Source) + require.Equal(t, buildContextDir, filepath.Dir(plan.BuildContextArchive.Source)) require.NotEmpty(t, plan.BuildContextArchive.SHA256) require.NotEqual(t, "sha256:"+plan.BuildContextArchive.SHA256, plan.BuildContextDigest) require.Empty(t, plan.BuildContextArchive.RawContents) @@ -42,9 +45,10 @@ func TestPrepareClientProxyImageBuild(t *testing.T) { require.NoError(t, os.Remove(plan.BuildContextArchive.Source)) }) - secondPlan, secondPrepareErr := dcptun.PrepareClientProxyImageBuild(context.Background()) + secondPlan, secondPrepareErr := dcptun.PrepareClientProxyImageBuild(context.Background(), buildContextDir) require.NoError(t, secondPrepareErr) require.Equal(t, plan.BuildContextDigest, secondPlan.BuildContextDigest) + require.Equal(t, buildContextDir, filepath.Dir(secondPlan.BuildContextArchive.Source)) t.Cleanup(func() { require.NoError(t, os.Remove(secondPlan.BuildContextArchive.Source)) }) @@ -77,6 +81,6 @@ func TestPrepareClientProxyImageBuildHonorsCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, prepareErr := dcptun.PrepareClientProxyImageBuild(ctx) + _, prepareErr := dcptun.PrepareClientProxyImageBuild(ctx, t.TempDir()) require.ErrorIs(t, prepareErr, context.Canceled) } diff --git a/test/integration/advanced_test_env.go b/test/integration/advanced_test_env.go index f88ef99d..7033e6b0 100644 --- a/test/integration/advanced_test_env.go +++ b/test/integration/advanced_test_env.go @@ -347,6 +347,7 @@ func StartAdvancedTestEnvironmentWithOptions( if inclCtrl&ContainerNetworkTunnelProxyController != 0 { tprOpts := controllers.ContainerNetworkTunnelProxyReconcilerConfig{ ProcessExecutor: pe, + BuildContextDir: sessionFolder, MakeTunnelControlClient: dcptunproto.NewTunnelControlClient, MaxTunnelPreparationAttempts: 2, } diff --git a/test/integration/container_network_tunnel_proxy_test.go b/test/integration/container_network_tunnel_proxy_test.go index 3b5b4315..12d5c2b7 100644 --- a/test/integration/container_network_tunnel_proxy_test.go +++ b/test/integration/container_network_tunnel_proxy_test.go @@ -185,26 +185,14 @@ func TestTunnelProxyRunningStatus(t *testing.T) { dcppaths.EnableTestPathProbing() const testName = "test-tunnel-proxy-running-status" - buildContextDir, mkdirErr := os.MkdirTemp("", "dcp-tunnel-") - require.NoError(t, mkdirErr) - t.Cleanup(func() { - require.NoError(t, os.RemoveAll(buildContextDir)) - }) - originalDcpTempDir := usvc_io.DcpTempDir - usvc_io.DcpTempDir = func() string { - return buildContextDir - } - t.Cleanup(func() { - usvc_io.DcpTempDir = originalDcpTempDir - }) - includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") defer shutdownTestEnvironment(serverInfo, cancel) testContainerOrchestrator, ok := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, ok) - imagePlan, imagePlanErr := dcptun.PrepareClientProxyImageBuild(ctx) + buildContextDir := teInfo.SessionFolder + imagePlan, imagePlanErr := dcptun.PrepareClientProxyImageBuild(ctx, buildContextDir) require.NoError(t, imagePlanErr) require.NoError(t, os.Remove(imagePlan.BuildContextArchive.Source)) releaseImageBuild := testContainerOrchestrator.BlockBuildImage(imagePlan.Image) diff --git a/test/integration/standard_test_env.go b/test/integration/standard_test_env.go index 7e78fb82..1bccdb2d 100644 --- a/test/integration/standard_test_env.go +++ b/test/integration/standard_test_env.go @@ -42,6 +42,7 @@ type TestEnvironmentInfo struct { StateStore *statestore.Store ResourceLeaseOwner process.ProcessHandle Log logr.Logger + SessionFolder string } type TestEnvironmentOptions struct { @@ -377,6 +378,7 @@ func StartTestEnvironmentWithOptions( tcc = ctrl_testutil.NewTestTunnelControlClient() tprOpts := controllers.ContainerNetworkTunnelProxyReconcilerConfig{ ProcessExecutor: pex, + BuildContextDir: sessionFolder, MakeTunnelControlClient: func(_ grpc.ClientConnInterface) dcptunproto.TunnelControlClient { return tcc }, MaxTunnelPreparationAttempts: 2, } @@ -414,6 +416,7 @@ func StartTestEnvironmentWithOptions( StateStore: stateStore, ResourceLeaseOwner: leaseOwner, Log: log, + SessionFolder: sessionFolder, } return serverInfo, teInfo, nil } From 6fc21fd2e7fa25530dae45a8035a0cceb9401eb4 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Wed, 16 Sep 2026 18:37:43 -0700 Subject: [PATCH 3/3] Default tunnel build context directory Keep the build context directory as an optional test override and use the existing DCP temporary directory by default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- controllers/container_network_tunnel_proxy_controller.go | 4 ++-- internal/dcpctrl/commands/run_controllers.go | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/controllers/container_network_tunnel_proxy_controller.go b/controllers/container_network_tunnel_proxy_controller.go index c8a6bb3e..c0df1f6b 100644 --- a/controllers/container_network_tunnel_proxy_controller.go +++ b/controllers/container_network_tunnel_proxy_controller.go @@ -99,7 +99,7 @@ type ContainerNetworkTunnelProxyReconcilerConfig struct { ProcessExecutor process.Executor // Mandatory // Directory that owns the lifetime of generated tunnel proxy image build contexts. - // Mandatory. + // Defaults to the DCP temporary directory. BuildContextDir string // The factory function to create a TunnelControlClient used to control the proxy pair. @@ -137,7 +137,7 @@ func NewContainerNetworkTunnelProxyReconciler( panic("ContainerNetworkTunnelProxyReconcilerConfig.ProcessExecutor must not be nil") } if config.BuildContextDir == "" { - panic("ContainerNetworkTunnelProxyReconcilerConfig.BuildContextDir must not be empty") + config.BuildContextDir = usvc_io.DcpTempDir() } if config.MakeTunnelControlClient == nil { panic("ContainerNetworkTunnelProxyReconcilerConfig.TunnelControlClientFactory must not be nil") diff --git a/internal/dcpctrl/commands/run_controllers.go b/internal/dcpctrl/commands/run_controllers.go index ba0073f2..95eacce3 100644 --- a/internal/dcpctrl/commands/run_controllers.go +++ b/internal/dcpctrl/commands/run_controllers.go @@ -30,7 +30,6 @@ import ( "github.com/microsoft/dcp/internal/perftrace" "github.com/microsoft/dcp/internal/proxy" "github.com/microsoft/dcp/internal/statestore" - usvc_io "github.com/microsoft/dcp/pkg/io" "github.com/microsoft/dcp/pkg/kubeconfig" "github.com/microsoft/dcp/pkg/logger" "github.com/microsoft/dcp/pkg/process" @@ -377,7 +376,6 @@ func runControllers(log logr.Logger) func(cmd *cobra.Command, _ []string) error mgr.GetAPIReader(), controllers.ContainerNetworkTunnelProxyReconcilerConfig{ ProcessExecutor: processExecutor, - BuildContextDir: usvc_io.DcpTempDir(), MakeTunnelControlClient: dcptunproto.NewTunnelControlClient, }, log.WithName("TunnelProxyReconciler"),