New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

agent: Add agent process supervisor tests #20741

Merged

averche merged 95 commits into main from agent-process-supervisor-tests

May 30, 2023

Contributor

averche commented May 24, 2023 •

edited

This introduces 6 integration tests for agent running in process supervisor mode (#20739):

simple test to ensure environment variables are injected correctly
test when the child process exits early
test when the child process exits early with an error
test sending sigterm to the child process
test sending sigusr1 to the child process
test child process ignoring signals

This PR is part of a larger effort to implement secrets as environment variable support in agent (VLT-253)

This PR was started in #20722 by @dhuckins

dhuckins and others added 30 commits

May 8, 2023 11:16


          added exec and env_template config/parsing

92d738a


          add tests

6d331c8


          we can reuse ctconfig here

aacd796


          do not create a non-nil map

ef2f4c0


          check defaults

18ea7d4


          Merge branch 'main' of github.com:hashicorp/vault into agent-runner-e…

…nv-var


          Apply suggestions from code review

efbcfb9

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>


          Apply suggestions from code review

baa4700

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>


          first go of exec server

78bc24b

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          convert to list

1a09e78

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          convert to list

af3b535

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          refactor

bcf3f17

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          sig test

654b0a1

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          add failing example

ed56bf0

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Merge branch 'agent-runner-env-var-config' into agent-runner-env-var

545ac2a


          refactor for config changes

27a8a50

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Merge branch 'main' of github.com:hashicorp/vault into agent-runner-e…

deb0270

…nv-var


          Merge branch 'main' of github.com:hashicorp/vault into agent-runner-e…

4d14b06

…nv-var-config


          add test for invalid signal

a83a3ab

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Merge branch 'agent-runner-env-var-config' into agent-runner-env-var

cf2827d


          account for auth token changes

7f4011e

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          only start the runner once we have a token

df1f0b2


          test case

118404f

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          tests in diff branch

3bf8057

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Apply suggestions from code review

e6035ee

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>


          fix rename

c7041c6

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Update command/agent/exec/exec.go

9bdb47e

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>


          apply suggestions from code review

5dd1aa0

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>


          Merge branch 'agent-runner-env-var' of github.com:hashicorp/vault int…

6147b75

…o agent-runner-env-var


          cleanup

71dac97

Signed-off-by: Daniel Huckins <dhuckins@users.noreply.github.com>

averche requested a review from a team as a code owner

May 24, 2023 21:05


          Merge branch 'main' into agent-process-supervisor-tests

vercel bot deployed to Preview

May 24, 2023 21:16

View deployment

VioletHynes self-requested a review

May 25, 2023 12:47

VioletHynes reviewed

View reviewed changes

command/agent/exec/test-app/main.go Show resolved Hide resolved

command/agent/exec/exec_test.go Outdated Show resolved Hide resolved

command/agent/exec/exec_test.go Show resolved Hide resolved

command/agent/exec/exec_test.go

+                              "request_id": "8af096e9-518c-7351-eff5-5ba20554b21f",
+                              "lease_id": "",
+                              "renewable": false,
+                              "lease_duration": 0,

Contributor

VioletHynes May 25, 2023

I'd like to see a test on a dynamic secret, as well as static (it would likely make more sense on a real Vault server, as mentioned above).

Contributor Author

averche May 25, 2023

Dynamic secret might be a bit difficult to set up on a real server. I'm thinking we can use fake server that simply scrambles the response on each request.

Contributor

VioletHynes May 25, 2023

You could request a token? That should be easy enough to set up, I think. For example, while TestAgent_Cache_DynamicSecret uses the API proxy subsystem of Agent, I don't see why it couldn't be doable using templating?

command/agent/exec/exec_test.go

+              			}},
+              			testAppArgs:       []string{"--stop-after", "10s"},
+              			testAppStopSignal: syscall.SIGTERM,
+              			testAppPort:       34001,

Contributor

VioletHynes May 25, 2023

Instead of static ports, could we use something similar to generateListenerAddress in agent_test to try and avoid collisions?

Contributor Author

averche May 25, 2023

I had a similar concern (port collisions when running the tests from multiple PR's) but if I understand correctly, GHA should run each PR in an isolated/containerized environment so it should not be an issue

command/agent/exec/exec_test.go

+              	}()
+              	for name, testCase := range testCases {
+              		t.Run(name, func(t *testing.T) {

Contributor

VioletHynes May 25, 2023

Can we use t.Parallel() on the next line here (and at the start of the function)? I can't see anything that might conflict, but I could be missing something

Contributor Author

averche May 25, 2023

I've tried t.Parallel but it's failing the tests -- possibly something related to the data race.

Contributor Author

averche May 25, 2023

Unfortunately, the tests currently can't be run in parallel since there is a shared setup (a common test app that is built ahead of time). This would probably work better in a test suite/fixture, but these don't seem to be used in Vault.

Contributor

VioletHynes commented May 25, 2023

Oh, as an FYI, there's a data race test failure, in case you've not seen it:

WARNING: DATA RACE
Write at 0x00c000c46cc0 by goroutine 163:
  github.com/hashicorp/consul-template/child.(*Child).kill.func1()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:439 +0x86
  runtime.deferreturn()
      /opt/hostedtoolcache/go/1.20.4/x64/src/runtime/panic.go:476 +0x32
  github.com/hashicorp/consul-template/child.(*Child).internalStop()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:296 +0x1d5
  github.com/hashicorp/consul-template/child.(*Child).Stop()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:274 +0x1877
  github.com/hashicorp/vault/command/agent/exec.(*Server).Run()
      /home/runner/work/vault/vault/command/agent/exec/exec.go:131 +0x1857
  github.com/hashicorp/vault/command/agent/exec.TestServer_Run.func2.1()
      /home/runner/work/vault/vault/command/agent/exec/exec_test.go:203 +0x67

Previous read at 0x00c000c46cc0 by goroutine 184:
  github.com/hashicorp/consul-template/child.(*Child).kill.func2()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:457 +0x84

Goroutine 163 (running) created at:
  github.com/hashicorp/vault/command/agent/exec.TestServer_Run.func2()
      /home/runner/work/vault/vault/command/agent/exec/exec_test.go:202 +0xbbe
  testing.tRunner()
      /opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.20.4/x64/src/testing/testing.go:1629 +0x47

Goroutine 184 (running) created at:
  github.com/hashicorp/consul-template/child.(*Child).kill()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:455 +0x4ab
  github.com/hashicorp/consul-template/child.(*Child).internalStop()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:296 +0x1d5
  github.com/hashicorp/consul-template/child.(*Child).Stop()
      /home/runner/go/pkg/mod/github.com/hashicorp/consul-template@v0.32.0/child/child.go:274 +0x1877
  github.com/hashicorp/vault/command/agent/exec.(*Server).Run()
      /home/runner/work/vault/vault/command/agent/exec/exec.go:131 +0x1857
  github.com/hashicorp/vault/command/agent/exec.TestServer_Run.func2.1()
      /home/runner/work/vault/vault/command/agent/exec/exec_test.go:203 +0x67
==================
2023-05-24T21:12:28.749Z [INFO]  exec server stopped
    testing.go:1446: race detected during execution of test

=== FAIL: command/agent/exec TestServer_Run (71.43s)

Contributor Author

averche commented May 25, 2023

We've been looking into this data race and trying to figure it out, gives us a concern about the existing code 😄

averche added 2 commits

May 25, 2023 16:41


          fix exitCh race condition

b2364b6


          skip the zombie proc test

Contributor Author

averche commented May 25, 2023

The data race seems to be internal to consul-template. I've created an issue hashicorp/consul-template#1753 with a minimal example of failure.


          test doc and cleanup

487cca7

VioletHynes reviewed

View reviewed changes

command/agent/exec/test-app/main.go Show resolved Hide resolved

VioletHynes approved these changes

View reviewed changes

averche added 5 commits

May 26, 2023 11:19


          comment at the top of main.go

c05b2f1


          more comments

10dee8d


          revert to testAppStartedCh


          fix exitCh race condition

38d833c


          Merge branch 'agent-fix-exit-ch-issue' into agent-process-supervisor-…

9b7b782

…tests

averche changed the base branch from main to agent-fix-exit-ch-issue

May 26, 2023 18:00

vercel bot deployed to Preview

May 26, 2023 18:05

View deployment

averche added the backport/1.14.x label

Base automatically changed from agent-fix-exit-ch-issue to main

May 30, 2023 16:23


          Merge branch 'main' into agent-process-supervisor-tests

e7dc3e3

vercel bot deployed to Preview

May 30, 2023 16:32

View deployment

averche enabled auto-merge (squash)

May 30, 2023 16:58


          Shorter wait time to complete tests faster

bba2160

averche merged commit 21eccf8 into main

92 of 93 checks passed

averche deleted the agent-process-supervisor-tests branch

May 30, 2023 18:06

hc-github-team-secure-vault-core mentioned this pull request

Backport of agent: Add agent process supervisor tests into release/1.14.x #20866

Merged

dhuckins mentioned this pull request

agent runner env var test #20722

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment