Skip to content
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

Add quiet mode #94

Open
wants to merge 2 commits into
base: mainline
Choose a base branch
from
Open

Add quiet mode #94

wants to merge 2 commits into from

Conversation

chancez
Copy link

@chancez chancez commented Apr 4, 2024 •
edited
Loading

Details

Closes #85

Introduces an env-var config option AWS_SSM_QUIET to disable writing information messages to stdout; instead sending them to stderr.

Considerations: I kinda just went with stderr; but we could also just send them to io.Discard instead if that makes more sense to others.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Rationale

There's suggestions to use grep or head/tail to filter these out; however this doesn't work in many cases for quite a few reasons:

  • Filtering the output means we may not see other messages with the same text that isn't coming from SSM. Eg grep -v SessionId will filter out lines with SessionId coming from the remote host.
  • These tools only work on ASCII and passing binary data through them may not work as expected.
  • It assumes the output of these logs never change, making it very fragile.
  • Tools like grep, head and tail are all line buffered, so anything interactive that also needs to filter the output will be a degraded user-experience due to the buffering. Also buffering can break many terminal features entirely.
  • It can be difficult to actually remove the "correct" output. If I'm trying to cat a binary file from the remote host and store that output locally; I have to filter the Starting session output, the Exiting session output, and I need to ensure I remove the empty lines before/after the log messages too. Doing it correctly is quite difficult.

The change is opt-in, meaning it won't effect anyone who doesn't configure AWS_SSM_QUIET, and in general; the output is still produced, but it goes to stderr which users will still see in their terminal, the only impact is if your passing the output of SSM to another program with the env-var set.

@chancez chancez mentioned this pull request Apr 26, 2024
Open
2 tasks
@asharpe asharpe mentioned this pull request Jun 3, 2024
Open
@asharpe
Copy link

asharpe commented Jun 17, 2024

@chancez I'd like to help out with testing this. I built this change against the latest upstream and when I run it I get

1.2.0.0
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0xdc858]

goroutine 1 [running]:
fmt.Fprintf({0x0, 0x0}, {0x5b8ce3, 0x12}, {0x0, 0x0, 0x0})
	/usr/local/go/src/fmt/print.go:205 +0x58
github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session.(*Session).Execute(0x40003fc000, {0x91bde0, 0x4000033b00})
	/session-manager-plugin/build/private/src/github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session/session.go:223 +0x50
github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session.glob..func1(0x40003fc000, {0x91bde0, 0x4000033b00})
	/session-manager-plugin/build/private/src/github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session/session.go:93 +0x38
github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session.ValidateInputAndStartSession({0x4000020070, 0x7, 0x7}, {0x90adc0, 0x4000010020})
	/session-manager-plugin/build/private/src/github.com/aws/session-manager-plugin/src/sessionmanagerplugin/session/session.go:214 +0x948
main.main()
	/session-manager-plugin/src/sessionmanagerplugin-main/main.go:34 +0x6c

Command '['session-manager-plugin',...

without really knowing what I'm doing I made the following change which allows it to run...

diff --git src/sessionmanagerplugin/session/session.go src/sessionmanagerplugin/session/session.go
index be2005e2..053735d1 100644
--- src/sessionmanagerplugin/session/session.go
+++ src/sessionmanagerplugin/session/session.go
@@ -204,7 +204,8 @@ func ValidateInputAndStartSession(args []string, out io.Writer) {
        session.Endpoint = ssmEndpoint
        session.ClientId = clientId
        session.TargetId = target
-       session.DataChannel = &datachannel.DataChannel{Out: out}
+       session.DataChannel = &datachannel.DataChannel{}
+       session.Out = out

    default:
        fmt.Fprint(out, "Invalid Operation")

Does this look right?

@asharpe
Copy link

asharpe commented Jun 17, 2024

Also trying this...

diff --git src/sessionmanagerplugin/session/session.go src/sessionmanagerplugin/session/session.go
index be2005e2..356694bd 100644
--- src/sessionmanagerplugin/session/session.go
+++ src/sessionmanagerplugin/session/session.go
@@ -205,6 +205,7 @@ func ValidateInputAndStartSession(args []string, out io.Writer) {
        session.ClientId = clientId
        session.TargetId = target
        session.DataChannel = &datachannel.DataChannel{Out: out}
+       session.Out = out

    default:
        fmt.Fprint(out, "Invalid Operation")

@chancez
Copy link
Author

chancez commented Jun 17, 2024

Yeah your most recent change looks correct, I guess I missed that. I'm not sure why I didn't hit the panic though 🤔

@chancez chancez force-pushed the pr/chancez/quiet_mode branch from ccc419c to 3d071e6 Compare June 17, 2024 16:03
@chancez
Copy link
Author

chancez commented Jun 17, 2024

Updated and rebased

@asharpe
Copy link

asharpe commented Jun 18, 2024

My test is an open SSH connection with this running

while sleep 5; do date; done

then I shut the node down via AWS API (it's a cloudformation thing for work).

With that second change and AWS_SSM_QUIET=true I get the following output...


                              Exiting session with sessionId: user@domain-rxzmaeptxyl6nsjqnia37hywgy.

                                                                                                           client_loop: send disconnect: Broken pipe

Versus this without it...

Bad packet length 966958809.
ssh_dispatch_run_fatal: Connection to UNKNOWN port 65535: Connection corrupted

I'm going to call this a win! Nice one :D

Is there anything else I can help with here?

@asharpe asharpe mentioned this pull request Jun 18, 2024
Open
@bugfood
Copy link

bugfood commented Jun 25, 2024

Thank you for this. I happened to run across the PR while trying to find a way to suppress these messages. This is making it difficult to do some ad-hoc scripting.

As a user, my preference would be:

  1. By default, do not output status messages when there is no error. Allow enabling status messages via e.g. AWS_SSM_VERBOSE=1.
  2. If outputting status messages, always use stderr, not stdout.

@bbhoss
Copy link

bbhoss commented Oct 2, 2024

Not sure this will ever be merged by AWS, but nice work either way. I feel that it was improper design to ever send these informational messages to stdout in the first place. openssh has been around for a long time and set the standard here. I think ideally the messages would just start going there but it could break someone's existing scripting that assumed stdout would have these messages. I also think naming it "quiet" would be a little bit of a misnomer since it just redirects the output to stderr, which a typical user would not be thinking about when they enable quiet mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a quiet mode
4 participants
@chancez @asharpe @bugfood @bbhoss