Add new subcommand to dump Agent Policies

[mrodm]

Fixes #671

Add a new subcommand under dump named agent-policies that is in charge of getting and writing the required agent policies.
Example of the usage output for this new subcommand:

 $ ./elastic-package dump agent-policies -h
elastic-package has been installed.
Use this command to dump agent policies created by Fleet as part of a package installation.

Use this command as a exploratory tool to dump agent policies as they are created by Fleet when installing a package. Dumped agent policies are stored in files as they are returned by APIs of the stack, without any processing.

Usage:
  elastic-package dump agent-policies [flags]

Flags:
      --agent-policy string   name of the agent policy
  -h, --help                  help for agent-policies

Global Flags:
  -o, --output string    path to directory where exported assets will be stored (default "package-dump")
  -P, --package string   name of the package
  -v, --verbose          verbose mode

Examples of usages of this new subcommand:

• Dump a specific agent policy:

  elastic-package dump agent-policies --agent-policy <agent_policy_id>
  

• Dump all agent policies set in Fleet:

  elastic-package dump agent-policies
  

• Dump the agent policies where a package:

  elastic-package dump agent-policies --package <package_name>
  

Currently, these agent policies are obtained directly from Fleet API using this endpoint: <fleet_api>/api/fleet/agent_policies

These agent policies are being stored as they are returned by the APIs without any processing. If there any secrets that must be redacted, probably it would be better to wait until this elastic/package-spec#339

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-06-28T14:28:48.954+0000

• Duration: 30 min 55 sec

Test stats 🧪

Test	Results
Failed	0
Passed	770
Skipped	0
Total	770

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[elasticmachine]

🌐 Coverage report

Name	Metrics % (covered/total)	Diff
Packages	100.0% (31/31)	💚
Files	66.379% (77/116)	👍 0.008
Classes	61.963% (101/163)	👍 0.571
Methods	49.923% (326/653)	👍 0.238
Lines	33.208% (2919/8790)	👎 -0.102
Conditionals	100.0% (0/0)	💚

[jsoriano]

This is looking good, I also tried it and works well. Added some suggestions.

It'd be also good to add tests, also to be able to see what kind of documents are dumped. You can follow the same strategy used in the dump installed-objects command, where an elasticsearch client is mocked with pre-recorded responses, see

elastic-package/internal/dump/installedobjects_test.go

Line 81 in dec6298

client := estest.ElasticsearchClient(s.T(), s.RecordDir)

.

[jsoriano]

Mention somewhere that if no flag is provided, all policies are dumped.

[jsoriano]

Nit. Log a different message if no policies are dumped. It prints now:

Dumped 0 agent policies filtering by package name apache to package-dump

[jsoriano]

Nit. This API is using different approaches to dump per agent policy or per package name. For agent policy the parameter is passed on the constructor, and for package name in the dump method.

I would suggest to use a consistent approach for both ways of duping policies.

For example it could be like this:

dumper := dump.NewAgentPoliciesDumper(kibanaClient)
err = dumper.DumpAgentPolicy(cmd.Context(), outputPath, dump.AgentPolicyOptions{Name: agentPolicy})
...
err = dumper.DumpAgentPolicy(cmd.Context(), outputPath, dump.AgentPolicyOptions{PackageName: packageName})

Or with explicit methods:

dumper := dump.NewAgentPoliciesDumper(kibanaClient)
err = dumper.DumpByName(cmd.Context(), outputPath, agentPolicy)
...
err = dumper.DumpByPackage(cmd.Context(), outputPath, packageName)
...
err = dumper.DumpAll(cmd.Context(), outputPath)

Or initialize the dumper for the given configuration:

dumper := dump.NewAgentPoliciesDumper(kibanaClient, agentPolicy, packageName)
err = dumper.Dump(cmd.Context())

[mrodm]

You're right, there was a mix of different approaches in these calls. Updated using explicit methods.

[jsoriano]

When possible, it'd be better to use the zero value to indicate that this is not set, de-referencing pointers is always a risk of panics.

Suggested change

	name *string
	name string

And then you can check if this is set by checking if name != "".

[mrodm]

Ok, I'll change to be a regular string.

[mrodm]

This is looking good, I also tried it and works well. Added some suggestions.

It'd be also good to add tests, also to be able to see what kind of documents are dumped. You can follow the same strategy used in the dump installed-objects command, where an elasticsearch client is mocked with pre-recorded responses, see

elastic-package/internal/dump/installedobjects_test.go

Line 81 in dec6298

client := estest.ElasticsearchClient(s.T(), s.RecordDir)

.

Thanks for all your comments.
Regarding testing, if there is no inconvenience, I'll add those tests in a following branch

[jsoriano]

Looks good, added some suggestions but nothing really blocking.

[jsoriano]

Nit. To avoid having to store all policies in memory, we could pass a visitor method here, so policies can be directly processed.
The signature of the function would be something like this:

func (c *Client) ForEachRawPolicy(visitFn func(json.RawMessage) error) error {

And then here:

for _, item := range resp.Items {
    if err := visitFn(item); err != nil {
        return err
    }
}

But no need to do it now, we can make this change later if this is a problem at some point.

[jsoriano]

Nit. Maybe we should rename dumpInstalledObject now 🙂

[mrodm]

I've just renamed it to dumpJSONResource.
Do you think it is worthy to move it to different file in the same package dump? @jsoriano

Something like internal/dump/utils.go ? I would move that function and formatJSON

[jsoriano]

Yeah, this could be moved to some common file, maybe called json.go, as you prefer.

[jsoriano]

🚀

[jsoriano]

Failure in CI is related to this change.

[mtojek]

Do you think that there is an option to cover the implementation with tests? I'm thinking about some end-to-end tests like bash scripts.
You could boot up the stack, dump default policies, and take it down.

EDIT:

I guess that you can also modify the test-stack-command and run the dump command before taking down the stack.

[mtojek]

nit: Maybe we can add more guidance to the dump description. agent-policies would be another supported feature :)

[mrodm]

Right, I've updated the dump description as:

const dumpLongDescription = `Use this command as an exploratory tool to dump resources from Elastic Stack (objects installed as part of package and agent policies).`

Would you add anything else here?

[mtojek]

side question to @jsoriano:

With your change enforcing SSL mode, will such options (like tlsSkipVerify) work? I'm thinking about the use case when a developer wants to dump agent policies from some random Kibana, not booted with elastic-package.

[jsoriano]

Yes, this flag should continue working after the changes in #847.

[mtojek]

Oh, now the Ingest Manager is called the Fleet :)

[mrodm]

Updating comments here. I was not totally sure about whether or not they were referring to the same.
Thanks!

[mrodm]

Do you think that there is an option to cover the implementation with tests? I'm thinking about some end-to-end tests like bash scripts. You could boot up the stack, dump default policies, and take it down.

EDIT:

I guess that you can also modify the test-stack-command and run the dump command before taking down the stack.

At first, I was planning to add the tests in a following branch, but I'll give it a try to add everything (code and tests) in this branch.

I will check for this testing those scripts and the test-stack-command command, and I also wanted to give it a chance to what @jsoriano mentioned in this comment

[mrodm]

In order to be used in internal/kibana/test/http_test.go I had to set get method as public. The others I've changed it to ensure consistency.

[mtojek]

nit: is such filename safe on Windows :)? Seriously, I don't know...

[mrodm]

Ouch... According to this doc the question mark is not a valid char in filenames on Windows. I will add that char to be replaced by ".". I don't think there will be no issue to replace both "&" and "?" chars by "." because they are used just in the query params.

[mtojek]

I see that you had to modify methods in many places. If this is only because the test is in a different package, I would rather keep it in the same one.

[mrodm]

I'll check this option. I prefer to keep those methods as private if possible.

[mtojek]

nit: root.json?

[mrodm]

Right, it must be json
Thanks!

[mtojek]

👍

[mtojek]

There is formatter package in elastic-package, maybe you can use it here.

[mrodm]

Oh, ok! Better reuse the formatter package.
I'll use it and update all the dumped files.
Thanks!

[mrodm]

According to this #862 (comment) , I'll keep using this method (formatJSON) to write the files.

[mtojek]

Yes, please do :)

[jsoriano]

Take into account that the JSONformatter is replacing invalid HTML characters with the UTF-8 encoding, this is done by default for HTML purpouses, but not really needed here. Previous formatter was just indenting the files for readability, without Marshalling and Unmarshalling them.

In any case I guess it doesn't mind here if we are comparing documents formatted in the same way.

[mtojek]

I would keep it as is for consistency reasons - the same formatter used everywhere, unless HTML entities are considered to be a bug in this case.

[jsoriano]

Well, thinking a second time about this, this is not only used for testing. We have to take into account that these are dumps from Elasticsearch/Kibana, we are modifying them here, what could be unexpected, a user dumping these objects with elastic-package is going to see something different to what is seen in Kibana or using the APIs.

Also, if we modify them, we should also check that they continue being valid and generate the same result if imported back in Elasticsearch/Kibana.

Thinking about this, and the purpouse of the dump command I would suggest to go back to the previous formatting.

[mrodm]

Ok, I see now the reason there was an explicit formatJSON method previously.
As you mentioned, it should be equivalent. However, as these dump commands are intended to be used as an exploratory tool for users, I think it would be better to just revert this change/commit and use the previous one, so these files can be inspected/debugged easier.
I'll revert this change