Enable and fix AuditOn. #17687
Merged
Enable and fix AuditOn. #17687
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakule
force-pushed
the
jakule/auditon-fix-3
branch
from
40fe59a to
8ac9976
Compare
jakule
force-pushed
the
jakule/auditon-fix-3
branch
from
4d9a647 to
01a0527
Compare
jakule
commented
Nov 11, 2022
| @@ -51,7 +51,8 @@ func NewFixture(t *testing.T) *Fixture { | |||
| require.NoError(t, err) | |||
|
|
|||
| // Find AllocatePortsNum free listening ports to use. | |||
| fixture.Me, _ = user.Current() | |||
| fixture.Me, err = user.Current() | |||
There was a problem hiding this comment.
Unrelated to the fix itself, but it bit me at some point, so I decided to fix it.
|
|
||
| myTerm.Type("\aecho hi\n\r\aexit\n\r\a") | ||
| // let's type "echo hi" followed by "enter" and then "exit" + "enter": | ||
| myTerm.Type("echo hi\n\rexit\n\r") |
There was a problem hiding this comment.
\a suspends the terminal for a second
teleport/integration/terminal_test.go
Line 79 in 1686a71
I don't see the point in waiting.
| @@ -304,13 +304,6 @@ func RunCommand() (errw io.Writer, code int, err error) { | |||
| if err != nil { | |||
| return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err) | |||
| } | |||
| defer func() { | |||
There was a problem hiding this comment.
We don't want to close TTY and PTY before we close the process. This cleanup logic was introduced a few months ago #13491
After this function returns, the process ends anyway, so all file descriptors will be closed anyways.
| @@ -990,6 +990,12 @@ func (s *session) startInteractive(ctx context.Context, ch ssh.Channel, scx *Ser | |||
| scx.Errorf("Received error waiting for the interactive session %v to finish: %v.", s.id, err) | |||
| } | |||
|
|
|||
| if result != nil { | |||
There was a problem hiding this comment.
I moved this block above the select. The result should be returned before we finish the session. Otherwise, we may miss the exit code.
| @@ -560,6 +571,7 @@ func (t *remoteTerminal) PID() int { | |||
| } | |||
|
|
|||
| func (t *remoteTerminal) Close() error { | |||
| t.wg.Wait() | |||
There was a problem hiding this comment.
This maybe controversial, but this t.wg is not being used anywhere, and from what I see we should wait on it here (the same as terminal).
jakule
commented
Nov 11, 2022
| var err error | ||
| defer t.closeTTY() |
There was a problem hiding this comment.
We're closing the TTY in two different places anyway. Closing it here causes the return code to be "skipped".
zmb3
approved these changes
Nov 11, 2022
espadolini
approved these changes
Nov 14, 2022
@zmb3 Are you asking about https://github.com/gravitational/teleport/pull/17687/files#diff-a4192e941574d6233edf26747166f0112babc45504defb4a22dcb211522c605fR272-R273 |
jakule
added a commit
that referenced
this pull request
Nov 17, 2022
This change re-enables the AuditOn system test and fixes the TTY connection between the Teleport parent and child process. It should allow the child to send the error code to the parent, which should fix the test.
This was referenced Nov 17, 2022
jakule
added a commit
that referenced
this pull request
Nov 22, 2022
This change re-enables the AuditOn system test and fixes the TTY connection between the Teleport parent and child process. It should allow the child to send the error code to the parent, which should fix the test. Backport of #17687
rosstimothy
added a commit
that referenced
this pull request
Aug 9, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
rosstimothy
added a commit
that referenced
this pull request
Aug 9, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
rosstimothy
added a commit
that referenced
this pull request
Aug 9, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 12, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-actions bot
pushed a commit
that referenced
this pull request
Aug 12, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-actions bot
pushed a commit
that referenced
this pull request
Aug 12, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-actions bot
pushed a commit
that referenced
this pull request
Aug 12, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 12, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
rosstimothy
added a commit
that referenced
this pull request
Aug 13, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
rosstimothy
added a commit
that referenced
this pull request
Aug 13, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 13, 2024
* Prevent exiting a session prior to output being consumed #17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped. * Fix SSH sessions recorded on proxy not being fully closed (#41434) * fix(srv): SSH remote sessions resources not being closed correctly * refactor(srv): code review suggestions * test(srv): move t.Helper to the correct function * chore(srv): typo * chore(srv): typo --------- Co-authored-by: Gabriel Corado <gabriel.oliveira@goteleport.com>
rosstimothy
added a commit
that referenced
this pull request
Aug 20, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
rosstimothy
added a commit
that referenced
this pull request
Aug 28, 2024
#17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 28, 2024
* Prevent exiting a session prior to output being consumed #17687 attempted to fix flakiness of TestIntegrations/AuditOn by sending an exit-status request _prior_ to consuming all output from the PTY. While this made the test more reliable, it created a scenario that allowed for a session to be completed without all of the data from the PTY being consumed by the client. This condition was hit by running an ansible playbook that output 1MB to stdout. The reason TestIntegrations/AuditOn was flaky is because the exit-status request was not received at times. The mechanism used to send that request requires sending the result over a channel and the request to be sent by another goroutine. That provides an opportunity for the request on the channel to be processed after the underlying ssh connection has been closed. To resolve the issue of missing output, the change in order of operations from #17687 was reverted and the exit-status request is now being sent directly in the same goroutine that waits for the session to end instead. This change now causes the exit-status to be sent later in time, which in the real world should not be noticed, however, some time dependent tests needed to have their timeout for sessions completing bumped. * Fix SSH sessions recorded on proxy not being fully closed (#41434) * fix(srv): SSH remote sessions resources not being closed correctly * refactor(srv): code review suggestions * test(srv): move t.Helper to the correct function * chore(srv): typo * chore(srv): typo --------- Co-authored-by: Gabriel Corado <gabriel.oliveira@goteleport.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change should re-enable and fix the AuditOn test. Read my comment for an explanation of each related change.