From 8b12fa8fcc520d6c5ac5ba57f1362b15a779d799 Mon Sep 17 00:00:00 2001 From: Ruben Ruiz de Gauna Date: Thu, 23 Sep 2021 07:51:31 +0200 Subject: [PATCH] fix: only pass NRIA_PASSTHROUGH_ENVIRONMENT variables to v4 integrations --- build/windows/test.ps1 | 3 + internal/integrations/v4/executor/executor.go | 2 - .../integrations/v4/executor/executor_test.go | 63 +++++++++++++++++++ internal/integrations/v4/runner/group_test.go | 6 +- .../integrations/v4/runner/runner_test.go | 4 +- pkg/integrations/v4/manager_test.go | 56 ++++++++++------- 6 files changed, 104 insertions(+), 30 deletions(-) diff --git a/build/windows/test.ps1 b/build/windows/test.ps1 index 5b105d1ca..d8ca6c122 100644 --- a/build/windows/test.ps1 +++ b/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\... diff --git a/internal/integrations/v4/executor/executor.go b/internal/integrations/v4/executor/executor.go index 6ba8fbedc..3118ca126 100644 --- a/internal/integrations/v4/executor/executor.go +++ b/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) } diff --git a/internal/integrations/v4/executor/executor_test.go b/internal/integrations/v4/executor/executor_test.go index 5688d4e99..392c95209 100644 --- a/internal/integrations/v4/executor/executor_test.go +++ b/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) diff --git a/internal/integrations/v4/runner/group_test.go b/internal/integrations/v4/runner/group_test.go index 13396e05e..1ae151b3c 100644 --- a/internal/integrations/v4/runner/group_test.go +++ b/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 diff --git a/internal/integrations/v4/runner/runner_test.go b/internal/integrations/v4/runner/runner_test.go index a2637e60f..831e2a77c 100644 --- a/internal/integrations/v4/runner/runner_test.go +++ b/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) diff --git a/pkg/integrations/v4/manager_test.go b/pkg/integrations/v4/manager_test.go index 63366f1ec..1a9c614eb 100644 --- a/pkg/integrations/v4/manager_test.go +++ b/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())