fix: only pass NRIA_PASSTHROUGH_ENVIRONMENT variables to v4 integrati…

…ons (#751)

build/windows/test.ps1

@@ -14,6 +14,9 @@ if (-not $?)
     exit -1
 }
 
+Write-Output "--- Setting gopath"
+$env:GOPATH = go env GOPATH
+
 Write-Output "--- Running tests"
 
 go test -ldflags '-X github.com/newrelic/infrastructure-agent/internal/integrations/v4/integration.minimumIntegrationIntervalOverride=2s' $workspace\pkg\... $workspace\cmd\... $workspace\internal\... $workspace\test\...

internal/integrations/v4/executor/executor.go

@@ -9,7 +9,6 @@ import (
 	"bytes"
 	"context"
 	"io"
-	"os"
 	"os/exec"
 	"sync"
 
@@ -142,7 +141,6 @@ func forwardCmdOutput(buffer io.Reader, fwd chan<- []byte, errors chan<- error)
 
 func (r *Executor) buildCommand(ctx context.Context) *exec.Cmd {
 	cmd := r.userAwareCmd(ctx)
-	cmd.Env = os.Environ()
 	for key, val := range r.Cfg.BuildEnv() {
 		cmd.Env = append(cmd.Env, key+"="+val)
 	}

internal/integrations/v4/executor/executor_test.go

@@ -273,6 +273,69 @@ func TestRunnable_Execute_NoVerboseSet(t *testing.T) {
 	assert.Equal(t, "VERBOSE=", testhelp.ChannelRead(to.Stderr))
 }
 
+func TestRunnable_BuildCommandWithNriaPassthroughEnvironment(t *testing.T) {
+	defer leaktest.Check(t)()
+
+	tests := []struct {
+		name           string
+		cfgEnv         map[string]string
+		osEnv          map[string]string
+		passthrough    []string
+		expectedCmdEnv []string
+	}{
+		{
+			name:           "no passthrough variables",
+			cfgEnv:         map[string]string{"PREFIX": "hello"},
+			osEnv:          map[string]string{"SOME_VAR": "some value", "ANOTHER_VAR": "another value"},
+			passthrough:    nil,
+			expectedCmdEnv: []string{"PREFIX=hello"},
+		},
+		{
+			name:           "one passthrough variable",
+			cfgEnv:         map[string]string{"PREFIX": "hello"},
+			osEnv:          map[string]string{"SOME_VAR": "some value", "ANOTHER_VAR": "another value"},
+			passthrough:    []string{"ANOTHER_VAR"},
+			expectedCmdEnv: []string{"PREFIX=hello", "ANOTHER_VAR=another value"},
+		},
+		{
+			name:           "multiple passthrough variable",
+			cfgEnv:         map[string]string{"PREFIX": "hello"},
+			osEnv:          map[string]string{"SOME_VAR": "some value", "ANOTHER_VAR": "another value"},
+			passthrough:    []string{"SOME_VAR", "ANOTHER_VAR"},
+			expectedCmdEnv: []string{"PREFIX=hello", "SOME_VAR=some value", "ANOTHER_VAR=another value"},
+		},
+		{
+			name:           "passthrough variable has precedence over integration one",
+			cfgEnv:         map[string]string{"PREFIX": "hello", "SOME_VAR": "integration value"},
+			osEnv:          map[string]string{"SOME_VAR": "some value", "ANOTHER_VAR": "another value"},
+			passthrough:    []string{"SOME_VAR", "ANOTHER_VAR"},
+			expectedCmdEnv: []string{"PREFIX=hello", "SOME_VAR=some value", "ANOTHER_VAR=another value"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// GIVEN a working runnable that is configured with CLI arguments and env vars AND passthrough env variables
+			cfg := execConfig(t)
+			cfg.Environment = tt.cfgEnv
+			cfg.Passthrough = tt.passthrough
+			r := FromCmdSlice(testhelp.Command(fixtures.BasicCmd, "world"), cfg)
+
+			// AND os ENV variables are set
+			for k, v := range tt.osEnv {
+				err := os.Setenv(k, v)
+				assert.Nil(t, err)
+			}
+			// WHEN building the command
+			cmd := r.buildCommand(context.Background())
+
+			// THEN only os env variables present in passthrough should be passed to command
+			// and have precedence over the integration ones
+			assert.ElementsMatch(t, tt.expectedCmdEnv, cmd.Env)
+		})
+	}
+}
+
 func execConfig(t require.TestingT) *Config {
 	d, err := os.Getwd()
 	require.NoError(t, err)

internal/integrations/v4/runner/group_test.go

@@ -29,6 +29,8 @@ import (
 
 var terminatedQueue = make(chan string)
 
+var passthroughEnv = []string{"GOCACHE", "GOPATH", "HOME", "PATH", "LOCALAPPDATA"}
+
 func TestGroup_Run(t *testing.T) {
 	defer leaktest.Check(t)()
 
@@ -79,7 +81,7 @@ func TestGroup_Run_Inventory(t *testing.T) {
 				Labels: map[string]string{"foo": "bar", "ou": "yea"}},
 		},
 	}, nil)
-	gr, _, err := NewGroup(loader, integration.InstancesLookup{}, nil, te, cmdrequest.NoopHandleFn, configrequest.NoopHandleFn, "", terminatedQueue)
+	gr, _, err := NewGroup(loader, integration.InstancesLookup{}, passthroughEnv, te, cmdrequest.NoopHandleFn, configrequest.NoopHandleFn, "", terminatedQueue)
 	require.NoError(t, err)
 
 	// WHEN the integration is executed
@@ -128,7 +130,7 @@ func TestGroup_Run_Inventory_OverridePrefix(t *testing.T) {
 				InventorySource: "custom/inventory"},
 		},
 	}, nil)
-	gr, _, err := NewGroup(loader, integration.InstancesLookup{}, nil, te, cmdrequest.NoopHandleFn, configrequest.NoopHandleFn, "", terminatedQueue)
+	gr, _, err := NewGroup(loader, integration.InstancesLookup{}, passthroughEnv, te, cmdrequest.NoopHandleFn, configrequest.NoopHandleFn, "", terminatedQueue)
 	require.NoError(t, err)
 
 	// WHEN the integration is executed

internal/integrations/v4/runner/runner_test.go

@@ -52,7 +52,7 @@ func Test_runner_Run(t *testing.T) {
 	e := &testemit.RecordEmitter{}
 	r := NewRunner(def, e, nil, nil, cmdrequest.NoopHandleFn, configrequest.NoopHandleFn, nil)
 
-	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
+	ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
 	defer cancel()
 
 	r.Run(ctx, nil, nil)
@@ -169,7 +169,7 @@ func Test_runner_Run_handlesCfgProtocol(t *testing.T) {
 	r := NewRunner(def, e, nil, nil, cmdrequest.NoopHandleFn, mockHandleFn, nil)
 
 	// WHEN the runner executes the binary and handle the payload.
-	ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
+	ctx, cancel := context.WithTimeout(context.Background(), 1500*time.Millisecond)
 	defer cancel()
 	r.Run(ctx, nil, nil)
 

pkg/integrations/v4/manager_test.go

@@ -144,6 +144,8 @@ integrations:
       STDOUT_TYPE: cfgreq
     exec: ` + getExe(testhelp.GoRun(fixtures.CfgReqGoFile)) + "\n"
 
+var passthroughEnv = []string{"GOCACHE", "GOPATH", "HOME", "PATH", "LOCALAPPDATA"}
+
 var (
 	definitionQ          = make(chan integration.Definition, 1000)
 	configEntryQ         = make(chan configrequest.Entry, 1000)
@@ -161,7 +163,7 @@ func TestManager_StartIntegrations(t *testing.T) {
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager loads and executes the integrations in the folder
 	ctx, cancel := context.WithCancel(context.Background())
@@ -196,7 +198,7 @@ integrations:
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager loads and executes the integration
 	ctx, cancel := context.WithCancel(context.Background())
@@ -221,7 +223,7 @@ integrations:
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager loads and executes the integration
 	ctx, cancel := context.WithCancel(context.Background())
@@ -298,7 +300,7 @@ func TestManager_Config_EmbeddedYAML(t *testing.T) {
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager loads and executes the integrations in the folder
 	ctx, cancel := context.WithCancel(context.Background())
@@ -323,7 +325,7 @@ func TestManager_HotReload_Add(t *testing.T) {
 	defer removeTempFiles(t, dir)
 
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go mgr.Start(ctx)
@@ -357,7 +359,7 @@ func TestManager_HotReload_Modify(t *testing.T) {
 	defer removeTempFiles(t, dir)
 
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go mgr.Start(ctx)
@@ -404,7 +406,7 @@ func TestManager_HotReload_ModifyLinkFile(t *testing.T) {
 	require.NoError(t, err)
 
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go mgr.Start(ctx)
@@ -453,7 +455,7 @@ func TestManager_HotReload_Delete(t *testing.T) {
 	defer removeTempFiles(t, dir)
 
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go mgr.Start(ctx)
@@ -502,7 +504,7 @@ integrations:
 	mgr := NewManager(Configuration{
 		ConfigFolders:          []string{configDir},
 		DefinitionFolders:      []string{niDir},
-		PassthroughEnvironment: []string{niDir},
+		PassthroughEnvironment: []string{"VALUE"},
 	}, emitter, instancesLookupReturning(execPath), definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
@@ -737,7 +739,8 @@ func TestManager_EnableFeature_WhenFeatureOnOHICfgAndAgentCfgIsDisabledAndEnable
 	// AND an integrations manager and with no feature within agent config
 	e := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
+		ConfigFolders:          []string{dir},
+		PassthroughEnvironment: passthroughEnv,
 		//AgentFeatures: map[string]bool{"docker_enabled": false},
 	}, e, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
@@ -768,8 +771,9 @@ func TestManager_EnableFeatureFromAgentConfig(t *testing.T) {
 	// AND an integrations manager and with feature enabled within agent config
 	e := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
-		AgentFeatures: map[string]bool{"docker_enabled": true},
+		ConfigFolders:          []string{dir},
+		AgentFeatures:          map[string]bool{"docker_enabled": true},
+		PassthroughEnvironment: passthroughEnv,
 	}, e, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// AND the manager starts
@@ -794,8 +798,9 @@ func TestManager_CCDisablesAgentEnabledFeature(t *testing.T) {
 	// AND an integrations manager and OHI enabled (ie via feature agent config)
 	e := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
-		AgentFeatures: map[string]bool{"docker_enabled": true},
+		ConfigFolders:          []string{dir},
+		AgentFeatures:          map[string]bool{"docker_enabled": true},
+		PassthroughEnvironment: passthroughEnv,
 	}, e, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// AND manager loads and executes the integrations in the folder
@@ -829,7 +834,8 @@ func TestManager_CCDisablesPreviouslyEnabledFeature(t *testing.T) {
 	// AND an integrations manager and OHI enabled (ie via feature agent config)
 	e := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
+		ConfigFolders:          []string{dir},
+		PassthroughEnvironment: passthroughEnv,
 	}, e, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// AND manager loads and executes the integrations in the folder
@@ -873,7 +879,7 @@ func TestManager_WhenFileExists(t *testing.T) {
 	defer removeTempFiles(t, dir)
 
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager loads and executes the integrations in the folder
 	ctx, cancel := context.WithCancel(context.Background())
@@ -917,8 +923,9 @@ func TestManager_StartWithVerbose(t *testing.T) {
 	// AND an integrations manager and with feature enabled within agent config
 	emitter := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
-		Verbose:       1,
+		ConfigFolders:          []string{dir},
+		PassthroughEnvironment: passthroughEnv,
+		Verbose:                1,
 	}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// AND the manager starts
@@ -948,8 +955,9 @@ func TestManager_StartWithVerboseFalse(t *testing.T) {
 	// AND an integrations manager and with feature enabled within agent config
 	emitter := &testemit.RecordEmitter{}
 	mgr := NewManager(Configuration{
-		ConfigFolders: []string{dir},
-		Verbose:       0,
+		ConfigFolders:          []string{dir},
+		PassthroughEnvironment: passthroughEnv,
+		Verbose:                0,
 	}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// AND the manager starts
@@ -1003,7 +1011,7 @@ func TestManager_anIntegrationCanSpawnAnotherOne(t *testing.T) {
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager executes the integration
 	ctx, cancel := context.WithCancel(context.Background())
@@ -1025,7 +1033,7 @@ func TestManager_cfgProtocolSpawnIntegrationV3Payload(t *testing.T) {
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager executes the integration
 	ctx, cancel := context.WithCancel(context.Background())
@@ -1047,7 +1055,7 @@ func TestManager_cfgProtocolSpawnIntegrationV4Payload(t *testing.T) {
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager executes the integration
 	ctx, cancel := context.WithCancel(context.Background())
@@ -1072,7 +1080,7 @@ func TestManager_cfgProtocolSpawnedIntegrationCannotSpawnIntegration(t *testing.
 
 	// AND an integrations manager
 	emitter := &testemit.RecordEmitter{}
-	mgr := NewManager(Configuration{ConfigFolders: []string{dir}}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
+	mgr := NewManager(Configuration{ConfigFolders: []string{dir}, PassthroughEnvironment: passthroughEnv}, emitter, integration.ErrLookup, definitionQ, terminateDefinitionQ, configEntryQ, track.NewTracker(nil))
 
 	// WHEN the manager executes the integration
 	ctx, cancel := context.WithCancel(context.Background())