-
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
flare shipping #1352
flare shipping #1352
Conversation
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 reasonable. Two caveats:
- I'm not sure what's up with the
SetDebugUploadRequestURL
- I think I want to get
1.1.1
cut first, aim to merge on thurs or something? (Though maybe this is safe to merge with the command line options as they are. Thoughts?)
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.
Nice work! I think this is good to merge. A small pile of nits, but I think we can iterate on them in a future PR. I'm sure we'll need to as we get the k2 side moving with it.
@@ -250,46 +251,53 @@ func RunDoctor(ctx context.Context, k types.Knapsack, w io.Writer) { | |||
} | |||
} | |||
|
|||
type runtimeEnvironmentTyp string | |||
type RuntimeEnvironmentType string |
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.
Does this need to be exported?
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.
yes, this is how the consumer and cmd line paths define standalone / insitu
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.
They shouldn't need the type exported, just the values for it. (Unless it's in a method signature)
pkg/debug/shipper/shipper.go
Outdated
currentUser := "unknown" | ||
consoleUsers, err := consoleuser.CurrentUsers(ctx) | ||
|
||
switch { | ||
case err != nil: | ||
currentUser = fmt.Sprintf("error getting current users: %s", err) | ||
case len(consoleUsers) > 0: | ||
currentUser = consoleUsers[0].Username | ||
default: // no console users | ||
currentUser = "no console users" | ||
} |
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.
If it's run from the command line, it should be in the ENV as USER
or SUDO_USER
, and if it's insitu, we probably want every consuleUser. not just the first.
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 don't quite understand what this means, we should look in the env or set in the env? Also, what should we do with the ENV?
I updated it to include all console users.
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.
The variable was called currentUser
, but it was grabbing the first console user. The console users are valuable, but I'm not sure they're the user taking this action. Consider someone remotely ssh'ed into a machine...
AFAIK the only way to tell the user who is taking the action is to either look at something like your process uid, or if it's from a shell examine os.GetEnv("USER")
. sudo will also set os.Getenv("SUDO_USER")
.
I'm not sure what windows does.
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.
Let's try!
this PR adds 2 options for flare shipping
sudo /usr/local/kolide-2/bin/launcher flare note=some_note save=upload
here is also a PR @directionless is working to support the gcp uploads