From f81c5bef6603f132aa109519e43efc25d402307e Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 1 Sep 2023 16:06:18 +0200 Subject: [PATCH] diagnostics creates the necessary folder for -f argumen (#3340) (#3343) - makes diagnostics command to create the necessary folders when a custom path is provided by the '-f' flag. - refactors the diagnosticCmd splitting it into smaller functions - add test createFile - rename logs_test.go createFile function to createFileEmpty so its name wont collide with the newly created 'createFile' function. add license headaer (cherry picked from commit 4cd83bf2d4fdc5ca0f54e8e314af73be2eff0ab5) Co-authored-by: Anderson Queiroz --- ...creates-necessary-folders-for--f-flag.yaml | 32 ++++++++++ internal/pkg/agent/cmd/diagnostics.go | 63 +++++++++++++------ internal/pkg/agent/cmd/diagnostics_test.go | 51 +++++++++++++++ internal/pkg/agent/cmd/logs_test.go | 30 ++++----- 4 files changed, 143 insertions(+), 33 deletions(-) create mode 100644 changelog/fragments/1693544911-Diagnostics-command-creates-necessary-folders-for--f-flag.yaml create mode 100644 internal/pkg/agent/cmd/diagnostics_test.go diff --git a/changelog/fragments/1693544911-Diagnostics-command-creates-necessary-folders-for--f-flag.yaml b/changelog/fragments/1693544911-Diagnostics-command-creates-necessary-folders-for--f-flag.yaml new file mode 100644 index 00000000000..feefb19812e --- /dev/null +++ b/changelog/fragments/1693544911-Diagnostics-command-creates-necessary-folders-for--f-flag.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: bug + +# Change summary; a 80ish characters long description of the change. +summary: Diagnostics command creates necessary folders for -f flag + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +#description: + +# Affected component; a word indicating the component this changeset affects. +component: diagnostics command + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +#pr: https://github.com/owner/repo/1234 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +#issue: https://github.com/owner/repo/1234 diff --git a/internal/pkg/agent/cmd/diagnostics.go b/internal/pkg/agent/cmd/diagnostics.go index bcd3ff339e4..b084366750d 100644 --- a/internal/pkg/agent/cmd/diagnostics.go +++ b/internal/pkg/agent/cmd/diagnostics.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "os" + "path" "time" "github.com/elastic/elastic-agent/pkg/control/v2/client" @@ -39,25 +40,46 @@ func newDiagnosticsCommand(_ []string, streams *cli.IOStreams) *cobra.Command { } func diagnosticCmd(streams *cli.IOStreams, cmd *cobra.Command) error { - fileName, _ := cmd.Flags().GetString("file") - if fileName == "" { + filepath, _ := cmd.Flags().GetString("file") + if filepath == "" { ts := time.Now().UTC() - fileName = "elastic-agent-diagnostics-" + ts.Format("2006-01-02T15-04-05Z07-00") + ".zip" // RFC3339 format that replaces : with -, so it will work on Windows + filepath = "elastic-agent-diagnostics-" + ts.Format("2006-01-02T15-04-05Z07-00") + ".zip" // RFC3339 format that replaces : with -, so it will work on Windows } ctx := handleSignal(context.Background()) + // 1st create the file to store the diagnostics, if it fails, anything else + // is pointless. + f, err := createFile(filepath) + if err != nil { + return fmt.Errorf("could not create diagnostics file %q: %w", filepath, err) + } + defer f.Close() + + cpuProfile, _ := cmd.Flags().GetBool("cpu-profile") + agentDiag, unitDiags, compDiags, err := collectDiagnostics(ctx, streams, cpuProfile) + if err != nil { + return fmt.Errorf("failed collecting diagnostics: %w", err) + } + + if err := diagnostics.ZipArchive(streams.Err, f, agentDiag, unitDiags, compDiags); err != nil { + return fmt.Errorf("unable to create archive %q: %w", filepath, err) + } + fmt.Fprintf(streams.Out, "Created diagnostics archive %q\n", filepath) + fmt.Fprintln(streams.Out, "***** WARNING *****\nCreated archive may contain plain text credentials.\nEnsure that files in archive are redacted before sharing.\n*******************") + return nil +} + +func collectDiagnostics(ctx context.Context, streams *cli.IOStreams, cpuProfile bool) ([]client.DiagnosticFileResult, []client.DiagnosticUnitResult, []client.DiagnosticComponentResult, error) { daemon := client.New() err := daemon.Connect(ctx) if err != nil { - return fmt.Errorf("failed to connect to daemon: %w", err) + return nil, nil, nil, fmt.Errorf("failed to connect to daemon: %w", err) } defer daemon.Disconnect() - fetchCPU, _ := cmd.Flags().GetBool("cpu-profile") - - additionalDiags := []cproto.AdditionalDiagnosticRequest{} - if fetchCPU { + var additionalDiags []cproto.AdditionalDiagnosticRequest + if cpuProfile { // console will just hang while we wait for the CPU profile; print something so user doesn't get confused fmt.Fprintf(streams.Out, "Creating diagnostics archive, waiting for CPU profile...\n") additionalDiags = []cproto.AdditionalDiagnosticRequest{cproto.AdditionalDiagnosticRequest_CPU} @@ -79,19 +101,24 @@ func diagnosticCmd(streams *cli.IOStreams, cmd *cobra.Command) error { } if len(compDiags) == 0 && len(unitDiags) == 0 && len(agentDiag) == 0 { - return fmt.Errorf("no diags could be fetched") + return nil, nil, nil, fmt.Errorf("no diags could be fetched") } - f, err := os.Create(fileName) - if err != nil { - return fmt.Errorf("error creating .zip file: %w", err) + return agentDiag, unitDiags, compDiags, nil +} + +func createFile(filepath string) (*os.File, error) { + // Ensure all the folders on filepath exist as os.Create does not do so. + // 0777 is the same permission, before unmask, os.Create uses. + dir := path.Dir(filepath) + if err := os.MkdirAll(dir, 0777); err != nil { + return nil, fmt.Errorf("could not create folders to save diagnostics on %q: %w", + dir, err) } - defer f.Close() - if err := diagnostics.ZipArchive(streams.Err, f, agentDiag, unitDiags, compDiags); err != nil { - return fmt.Errorf("unable to create archive %q: %w", fileName, err) + f, err := os.Create(filepath) + if err != nil { + return nil, fmt.Errorf("error creating .zip file: %w", err) } - fmt.Fprintf(streams.Out, "Created diagnostics archive %q\n", fileName) - fmt.Fprintln(streams.Out, "***** WARNING *****\nCreated archive may contain plain text credentials.\nEnsure that files in archive are redacted before sharing.\n*******************") - return nil + return f, nil } diff --git a/internal/pkg/agent/cmd/diagnostics_test.go b/internal/pkg/agent/cmd/diagnostics_test.go new file mode 100644 index 00000000000..1e265f97aba --- /dev/null +++ b/internal/pkg/agent/cmd/diagnostics_test.go @@ -0,0 +1,51 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +package cmd + +import ( + "os" + "path" + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_createFile(t *testing.T) { + dir := t.TempDir() + existingFile := "existingfile.zip" + f, err := os.Create(path.Join(dir, "existingFile")) + require.NoErrorf(t, err, "could not create file %q", path.Join(dir, "existingFile")) + err = f.Close() + require.NoError(t, err, "could not close file") + + testCases := []struct { + name string + filePath string + }{ + { + name: "ExistingFile", + filePath: path.Join(dir, existingFile), + }, + { + name: "NewFile", + filePath: path.Join(dir, "newfile.zip"), + }, + { + name: "NonexistentFolders", + filePath: path.Join(dir, "nonexistent", "folders", "file.zip"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + file, err := createFile(tc.filePath) + require.NoError(t, err, "failed creating diagnostics file %q", + tc.filePath) + defer func() { + file.Close() + }() + }) + } +} diff --git a/internal/pkg/agent/cmd/logs_test.go b/internal/pkg/agent/cmd/logs_test.go index 1ff3cc6170e..620ed100b4c 100644 --- a/internal/pkg/agent/cmd/logs_test.go +++ b/internal/pkg/agent/cmd/logs_test.go @@ -38,10 +38,10 @@ func TestGetLogFilenames(t *testing.T) { t.Run("returns the correct sorted filelist", func(t *testing.T) { dir := t.TempDir() - createFile(t, dir, file2) - createFile(t, dir, file) - createFile(t, dir, file1) - createFile(t, dir, file3) + createFileEmpty(t, dir, file2) + createFileEmpty(t, dir, file) + createFileEmpty(t, dir, file1) + createFileEmpty(t, dir, file3) names, err := getLogFilenames(dir) require.NoError(t, err) @@ -62,14 +62,14 @@ func TestGetLogFilenames(t *testing.T) { prevDayFile2 := "elastic-agent-20230529-2.ndjson" prevDayFile3 := "elastic-agent-20230529-3.ndjson" - createFile(t, dir, file2) - createFile(t, dir, file) - createFile(t, dir, prevDayFile1) - createFile(t, dir, file1) - createFile(t, dir, prevDayFile) - createFile(t, dir, prevDayFile2) - createFile(t, dir, file3) - createFile(t, dir, prevDayFile3) + createFileEmpty(t, dir, file2) + createFileEmpty(t, dir, file) + createFileEmpty(t, dir, prevDayFile1) + createFileEmpty(t, dir, file1) + createFileEmpty(t, dir, prevDayFile) + createFileEmpty(t, dir, prevDayFile2) + createFileEmpty(t, dir, file3) + createFileEmpty(t, dir, prevDayFile3) names, err := getLogFilenames(dir) require.NoError(t, err) @@ -99,7 +99,7 @@ func TestGetLogFilenames(t *testing.T) { t.Run("does not return non-log entries", func(t *testing.T) { dir := t.TempDir() - createFile(t, dir, "excluded") + createFileEmpty(t, dir, "excluded") names, err := getLogFilenames(dir) require.NoError(t, err) @@ -109,7 +109,7 @@ func TestGetLogFilenames(t *testing.T) { t.Run("returns a list of one", func(t *testing.T) { dir := t.TempDir() - createFile(t, dir, file1) + createFileEmpty(t, dir, file1) names, err := getLogFilenames(dir) require.NoError(t, err) @@ -539,7 +539,7 @@ func generateLines(prefix string, start, end int) string { return b.String() } -func createFile(t *testing.T, dir, name string) { +func createFileEmpty(t *testing.T, dir, name string) { createFileContent(t, dir, name, nil) }