Adds Flare Command by gmmeyer · Pull Request #282 · DataDog/datadog-agent

gmmeyer · 2017-06-07T20:49:31Z

What does this PR do?

This creates a flare and sends it up to Datadog.

Additional Notes

Not everything is implemented. This is a first stab at the flare command. The info command is not yet implemented, so that will not yet work. The goexpvar is not output into the flare. The autoconf is not output into the flare. These will all need to be implemented in the final product.
I have some tests, we'll likely want to write more. My tests just check that the files are properly stripped and that an archive is created. I think we'll likely want to test the archive that's created as well (that's a more involved process than can be accomplished in a unit test).
We will need to do a lot of resiliancy testing to ensure that it will always work.
It needs to be tested on Windows.
I added something new as well. In addition to zipping up all the files, it will dump the in memory config out into its own file. (Viper has a wonderful ability to do this.)
One other deviation is that I run the full cleaner against every file, rather than having cleaners for config files and the root config file be different. Since we've unified a lot of the structure of the settings, this makes sense to do.
I do not use the logger for the flare. I use regular print statements, as they are more natural for this kind of output than logging output. There is very little output for the flare.

However, it works and zips up all we have right now!

masci

Left few comments, some of them might be because this is somehow still WIP so feel free to ignore.

masci · 2017-06-13T08:16:54Z

is reading/decoding the request body needed?

It is not since I am no longer sending the flare from the agent side but rather from the client side.

masci · 2017-06-13T08:23:09Z

In this case we should return the complete error string to the client I think - a client might not have access to the agent logs and they would only see "The flare failed to be created".
Also, http.Error returns plain text in the response body, so we should set the Content-Type header last thing, or not setting at all and return the file path as plain text.

That's a good idea. I will do that.

masci · 2017-06-13T08:24:06Z

nit: can you wrap all the declarations in a var() block?

masci · 2017-06-13T08:24:43Z

So long, my shrug friend! 😄

We can keep it if you'd like ¯_(ツ)_/¯

masci · 2017-06-13T08:29:21Z

We could print the error contained in the response body here

masci · 2017-06-13T08:32:15Z

SendFlare could be invoked by the IPC api as well, through a new endpoint like agent/flare/send or similar

Yes it could, do you think we should do this? I am not adverse to this idea.

Don't have a strong opinion and don't want to add too much to this PR, we can just keep this in mind

I made some adjustments that will allow it to do that!

I didn't write that into it yet, but I abstracted a few things out that will make it easier to do

masci · 2017-06-14T08:36:33Z

nit: no need to export this function I guess

You're right!

masci · 2017-06-15T11:38:36Z

+		} else {
+			fmt.Println("The agent was unable to make the flare.")
+		}
+		fmt.Println("Initiating flare locally.")


Do you think would it be useful adding to the flare whether it's been created locally or by a running agent? We can derive this info from the agent logs - they would contain a line about flare creation.

Sounds like a good idea to me.

masci · 2017-06-15T11:43:29Z

@@ -0,0 +1,140 @@
+package flare


I think the package is specific enough to live in pkg/flare, same level as (for example) version and pidfile.

I'll move it out there

masci · 2017-06-15T11:52:23Z

+
+	err = filepath.Walk(config.Datadog.GetString("confd_path"), func(path string, f os.FileInfo, err error) error {
+		if f.IsDir() {
+			return nil


not sure this plays well with the new check.d option, where you can have config files within folders, like http_check.d/123.yaml

Ah I didn't know that was a thing now. We'll need to configure it to handle that. How do we detect that elsewhere?

The file Configuration Provider does this: https://github.com/DataDog/datadog-agent/blob/master/pkg/collector/providers/file.go#L113

masci · 2017-06-15T11:53:47Z

+		return err
+	}
+
+	if config.Datadog.ConfigFileUsed() != "" {


we should keep in mind env vars can override the contents of the config file

That's why I add the parsed config file above: https://github.com/DataDog/datadog-agent/pull/282/files/4001c64157dab3f0c7d532c1f2741afd7d88fb6a#diff-9e878f3fd97d7fc945eb0efd548b865bR68

masci · 2017-06-15T12:05:10Z

+// 'Are you sure you want to continue [y/N]? '
+
+func askForInput(before string, after string) (string, error) {
+	reader := bufio.NewReader(os.Stdin)


What about using a scanner instead (https://golang.org/pkg/bufio/#Scanner)? This way we could set a reasonable buffer and avoid overflow attacks (I know the threat is very little but this way we show we care about security).

Sure, we can do that

masci

took another pass trying to identify critical paths, I still think the PR is good but left few comments on something I didn't notice on the first pass

gmmeyer · 2017-06-15T16:52:43Z

I created a second test and made the command more robust. However, in making it more robust, I also ensured that it will create a flare even if there is nothing to put into the flare.

Is this how it should work?

masci

LGTM! Feel free to add here the support to checks.d config folders, pre-approving the review in case you want to do it in a separate PR

masci · 2017-06-16T14:25:33Z

+
+	err = filepath.Walk(config.Datadog.GetString("confd_path"), func(path string, f os.FileInfo, err error) error {
+		if f.IsDir() {
+			return nil


The file Configuration Provider does this: https://github.com/DataDog/datadog-agent/blob/master/pkg/collector/providers/file.go#L113

masci · 2017-06-16T17:26:30Z

!

…s a config option or an env var (#282) * allowing for implicit auth to be set as a config option or an env var * addressed comments

gmmeyer force-pushed the greg/flare branch from d89389a to 47d1d26 Compare June 8, 2017 01:35

masci added the do-not-merge/WIP label Jun 8, 2017

gmmeyer force-pushed the greg/flare branch from 21e5c20 to c970445 Compare June 11, 2017 19:56

gmmeyer changed the title ~~[WIP] Flare~~ Adds Flare Command Jun 11, 2017

gmmeyer force-pushed the greg/flare branch 2 times, most recently from 1e127e8 to d509c84 Compare June 12, 2017 20:00

masci removed the do-not-merge/WIP label Jun 13, 2017

masci suggested changes Jun 13, 2017

View reviewed changes

masci reviewed Jun 14, 2017

View reviewed changes

gmmeyer and others added 21 commits June 14, 2017 18:37

saving my place

92d4d49

implements more of zipfile functionality, adds http posting

f698d31

adjusts the order, adds the actual config file

3166636

saving my place

426e0da

Added IPC endpoints for the flare command

8e5077b

adds most of the options, creates flare

cd649f5

sends a preliminary flare

602a790

finishes the first stage of the flare command for Unix

185db67

remove declaration of type from left hand side

374a511

adds strip test

7dc7848

adds simple test for archive

941f7b6

dump the settings to yaml, rather than the whole struct

37a249a

report error from all additions to zipfile

d969a6b

handle errors

8e910b9

flare errors are printed in util, no need to print again

c4b943e

everything should be contained in a folder named after the host

a0457a2

responses to massi's critiques, small refactor

a706eba

removes last references to fmt in the util flare pkg

94081e5

explicitly turn off logging

d8eec87

fix nil pointer error on archive test

f7fc0f9

do not export AnalyzeFlare

d214e6b

gmmeyer force-pushed the greg/flare branch from b364242 to ccfd02c Compare June 15, 2017 01:41

use new get hostname syntax

faffd34

gmmeyer force-pushed the greg/flare branch from ccfd02c to faffd34 Compare June 15, 2017 02:57

gmmeyer added 2 commits June 14, 2017 20:08

adds commit hash for archivex

8ee23de

make it more fault tolerant

4001c64

masci reviewed Jun 15, 2017

View reviewed changes

masci suggested changes Jun 15, 2017

View reviewed changes

gmmeyer added 3 commits June 15, 2017 12:07

fixes NPE, moves flare to own pkg

466151e

clear up vetshadow

fe77fb0

check error on email input

4079e16

masci approved these changes Jun 16, 2017

View reviewed changes

gmmeyer added 2 commits June 16, 2017 10:06

adds dist folder to config paths

3573d14

adds checksd path as well

c7a1d93

gmmeyer merged commit bb9a051 into master Jun 16, 2017

gmmeyer deleted the greg/flare branch June 16, 2017 18:39

s-alad pushed a commit that referenced this pull request Jan 6, 2026

[AGENTCFG-401] Allowing for implicit Vault authentication to be set a…

4375ca2

…s a config option or an env var (#282) * allowing for implicit auth to be set as a config option or an env var * addressed comments

Conversation

gmmeyer commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Additional Notes

Uh oh!

masci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci left a comment

Choose a reason for hiding this comment

Uh oh!

gmmeyer commented Jun 15, 2017

Uh oh!

masci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

gmmeyer commented Jun 7, 2017 •

edited

Loading