-
Notifications
You must be signed in to change notification settings - Fork 103
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.
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
set up windows event logger first in main #1672
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a560791
set up windows event logger first in main
James-Pickett be5adb4
remove debug line
James-Pickett 8696490
return logger instead of new one
James-Pickett 74fc823
move sys log to multislogger package, use default logger on error ins…
James-Pickett 5af6df5
missed commit
James-Pickett File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
//go:build !windows | ||
// +build !windows | ||
|
||
package multislogger | ||
|
||
import ( | ||
"io" | ||
) | ||
|
||
func SystemSlogger() (*MultiSlogger, io.Closer, error) { | ||
return defaultSystemSlogger(), io.NopCloser(nil), nil | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
//go:build windows | ||
// +build windows | ||
|
||
package multislogger | ||
|
||
import ( | ||
"context" | ||
"io" | ||
"log/slog" | ||
|
||
"github.com/kolide/launcher/pkg/log/eventlog" | ||
"golang.org/x/sys/windows" | ||
) | ||
|
||
const serviceName = "launcher" | ||
|
||
func SystemSlogger() (*MultiSlogger, io.Closer, error) { | ||
if !windows.GetCurrentProcessToken().IsElevated() { | ||
syslogger := defaultSystemSlogger() | ||
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting idea. Is the intent to try to capture people running in a terminal? Im game for trying this. I don't know if we have better rope to know what our caller is |
||
|
||
syslogger.Log(context.TODO(), slog.LevelDebug, | ||
"launcher running on windows without elevated permissions, using default stderr instead of eventlog", | ||
) | ||
|
||
return syslogger, io.NopCloser(nil), nil | ||
} | ||
|
||
eventLogWriter, err := eventlog.NewWriter(serviceName) | ||
if err != nil { | ||
syslogger := defaultSystemSlogger() | ||
|
||
syslogger.Log(context.TODO(), slog.LevelError, | ||
"could not create eventlog writer, using default stderr instead of eventlog", | ||
"err", err, | ||
) | ||
|
||
return syslogger, io.NopCloser(nil), nil | ||
} | ||
|
||
systemSlogger := New(slog.NewJSONHandler(eventLogWriter, &slog.HandlerOptions{ | ||
Level: slog.LevelInfo, | ||
})) | ||
|
||
return systemSlogger, eventLogWriter, nil | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we close this just before starting the windows svc? it have two windows event writers does not seem to be an issue 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, yeah? We might not want to do the
defer
anyway since defers won't execute when we exit withos.Exit
, which we do a lot in this file.I think it'd be ideal if it were easy to pass this system slogger into
runWindowsSvc
, but that looks like it would be kind of an annoying refactor to have to do...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it would be an annoying refactor, also weird passing around a closer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure we want the refactor. I'm less sure we want it today (I'm not sure how hard it would be, it's just adding slogger as a parameter to the subcommands, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we would also need to pass around the closer within main.go if we don't want to use defer .... I'm torn between wanting clean code and wanting to make sure we close the handler before we exit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet we could refactor out the os.Exit calls and use defer as expected. Or at least consolidate them and be clever. I wouldn't want to pass closers around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could do something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's not bad. I was thinking of something much more complicated:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both of these are good options 👍 since we already have 2 other defer funcs in here that we'd probably want to move around too, I'm fine w punting this refactor to a subsequent PR in order to merge this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding system multislogger to subcommands: #1674