From fbbcf874055520b3f79112a8046b8dc6b848d9cd Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 21 Nov 2022 18:10:34 +0000 Subject: [PATCH] feat(collectors): Store all pod logs in cluster-resources directory (#821) * feat(collectors): Store all pod logs in cluster-resources directory All pod logs collected by the logs collector will now be stored in /cluster-resources/pods/logs/[namespace]/[pod]/[container].log. This will provide consistency and allow sbctl to find the logs when we run `kubectl logs `. To allow backwards compatibility, symlinks of the log files will be created in the current expected locations. Closes: #744 --- .gitignore | 3 + examples/preflight/e2e.yaml | 2 +- examples/support-bundle/e2e.yaml | 4 +- .../support-bundle/sample-supportbundle.yaml | 19 +++ pkg/collect/cluster_resources.go | 2 +- pkg/collect/logs.go | 66 +++++++-- pkg/collect/logs_test.go | 73 ++++++++++ pkg/collect/redact.go | 34 ++++- pkg/collect/remote_collector.go | 2 +- pkg/collect/result.go | 130 +++++++++++++++--- pkg/collect/run_pod.go | 2 +- pkg/supportbundle/collect.go | 5 +- pkg/supportbundle/supportbundle.go | 18 ++- test/validate-preflight-e2e.sh | 7 +- test/validate-support-bundle-e2e.sh | 6 +- 15 files changed, 318 insertions(+), 55 deletions(-) diff --git a/.gitignore b/.gitignore index 8e72e0006..df59e9325 100644 --- a/.gitignore +++ b/.gitignore @@ -38,3 +38,6 @@ sbom/ # Ignore local pre-commit config .pre-commit-config.yaml + +# Ignore generated support bundles +*.tar.gz diff --git a/examples/preflight/e2e.yaml b/examples/preflight/e2e.yaml index c21f22ffc..b8889f4a1 100644 --- a/examples/preflight/e2e.yaml +++ b/examples/preflight/e2e.yaml @@ -9,7 +9,7 @@ spec: data: "5" - run: collectorName: "static-hi" - image: 'alpine:3.5' + image: 'alpine:3' command: ["echo", "hi static!"] analyzers: - clusterVersion: diff --git a/examples/support-bundle/e2e.yaml b/examples/support-bundle/e2e.yaml index 388dc07e1..ddd54d77e 100644 --- a/examples/support-bundle/e2e.yaml +++ b/examples/support-bundle/e2e.yaml @@ -11,10 +11,10 @@ spec: data: "5" - runPod: collectorName: "static-hi" - podSpec: + podSpec: containers: - name: static-hi - image: alpine:3.5 + image: alpine:3 command: ["echo", "hi static!"] analyzers: - clusterVersion: diff --git a/examples/support-bundle/sample-supportbundle.yaml b/examples/support-bundle/sample-supportbundle.yaml index eb94b6697..42ecbf9a1 100644 --- a/examples/support-bundle/sample-supportbundle.yaml +++ b/examples/support-bundle/sample-supportbundle.yaml @@ -12,6 +12,16 @@ spec: limits: maxAge: 720h # 30*24 maxLines: 10000 + - logs: + collectorName: all-logs + name: all-logs + - runPod: + collectorName: "static-hi" + podSpec: + containers: + - name: static-hi + image: alpine:3 + command: ["echo", "hi static!"] analyzers: - clusterVersion: outcomes: @@ -100,3 +110,12 @@ spec: message: The API deployment has only a single ready replica. - pass: message: There are multiple replicas of the API deployment ready. + - textAnalyze: + checkName: Said hi! + fileName: /static-hi.log + regex: 'hi static' + outcomes: + - fail: + message: Didn't say hi. + - pass: + message: Said hi! diff --git a/pkg/collect/cluster_resources.go b/pkg/collect/cluster_resources.go index 16a9dd692..af7502c1d 100644 --- a/pkg/collect/cluster_resources.go +++ b/pkg/collect/cluster_resources.go @@ -131,7 +131,7 @@ func (c *CollectClusterResources) Collect(progressChan chan<- interface{}) (Coll limits := &troubleshootv1beta2.LogLimits{ MaxLines: 500, } - podLogs, err := savePodLogs(ctx, logsRoot, client, pod, "", container.Name, limits, false) + podLogs, err := savePodLogs(ctx, logsRoot, client, &pod, "", container.Name, limits, false) if err != nil { errPath := filepath.Join("cluster-resources", "pods", "logs", pod.Namespace, pod.Name, fmt.Sprintf("%s-logs-errors.log", container.Name)) output.SaveResult(c.BundlePath, errPath, bytes.NewBuffer([]byte(err.Error()))) diff --git a/pkg/collect/logs.go b/pkg/collect/logs.go index 58504616e..9138991c8 100644 --- a/pkg/collect/logs.go +++ b/pkg/collect/logs.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "strings" "time" @@ -70,10 +71,7 @@ func (c *CollectLogs) Collect(progressChan chan<- interface{}) (CollectorResult, } for _, containerName := range containerNames { - if len(containerNames) == 1 { - containerName = "" // if there was only one container, use the old behavior of not including the container name in the path - } - podLogs, err := savePodLogs(ctx, c.BundlePath, client, pod, c.Collector.Name, containerName, c.Collector.Limits, false) + podLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, containerName, c.Collector.Limits, false) if err != nil { key := fmt.Sprintf("%s/%s-errors.json", c.Collector.Name, pod.Name) if containerName != "" { @@ -91,7 +89,7 @@ func (c *CollectLogs) Collect(progressChan chan<- interface{}) (CollectorResult, } } else { for _, container := range c.Collector.ContainerNames { - containerLogs, err := savePodLogs(ctx, c.BundlePath, client, pod, c.Collector.Name, container, c.Collector.Limits, false) + containerLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, container, c.Collector.Limits, false) if err != nil { key := fmt.Sprintf("%s/%s/%s-errors.json", c.Collector.Name, pod.Name, container) err := output.SaveResult(c.BundlePath, key, marshalErrors([]string{err.Error()})) @@ -111,7 +109,7 @@ func (c *CollectLogs) Collect(progressChan chan<- interface{}) (CollectorResult, return output, nil } -func listPodsInSelectors(ctx context.Context, client *kubernetes.Clientset, namespace string, selector []string) ([]corev1.Pod, []string) { +func listPodsInSelectors(ctx context.Context, client kubernetes.Interface, namespace string, selector []string) ([]corev1.Pod, []string) { serializedLabelSelector := strings.Join(selector, ",") listOptions := metav1.ListOptions{ @@ -126,20 +124,54 @@ func listPodsInSelectors(ctx context.Context, client *kubernetes.Clientset, name return pods.Items, nil } -func savePodLogs(ctx context.Context, bundlePath string, client *kubernetes.Clientset, pod corev1.Pod, name, container string, limits *troubleshootv1beta2.LogLimits, follow bool) (CollectorResult, error) { +func savePodLogs( + ctx context.Context, + bundlePath string, + client *kubernetes.Clientset, + pod *corev1.Pod, + collectorName, container string, + limits *troubleshootv1beta2.LogLimits, + follow bool, +) (CollectorResult, error) { + return savePodLogsWithInterface(ctx, bundlePath, client, pod, collectorName, container, limits, follow) +} + +func savePodLogsWithInterface( + ctx context.Context, + bundlePath string, + client kubernetes.Interface, + pod *corev1.Pod, + collectorName, container string, + limits *troubleshootv1beta2.LogLimits, + follow bool, +) (CollectorResult, error) { podLogOpts := corev1.PodLogOptions{ Follow: follow, Container: container, } - setLogLimits(&podLogOpts, limits, convertMaxAgeToTime) + result := NewResult() - fileKey := fmt.Sprintf("%s/%s", name, pod.Name) + // TODO: Abstract away hard coded directory structure paths + // Maybe create a FS provider or something similar + filePathPrefix := filepath.Join( + "cluster-resources", "pods", "logs", pod.Namespace, pod.Name, pod.Spec.Containers[0].Name, + ) + + // TODO: If collectorName is empty, the path is stored with a leading slash + // Retain this behavior otherwise analysers in the wild may break + // Analysers that need to find a file in the root of the bundle should + // prefix the path with a slash e.g /file.txt. This behavior should be + // properly deprecated in the future. + linkRelPathPrefix := fmt.Sprintf("%s/%s", collectorName, pod.Name) if container != "" { - fileKey = fmt.Sprintf("%s/%s/%s", name, pod.Name, container) + linkRelPathPrefix = fmt.Sprintf("%s/%s/%s", collectorName, pod.Name, container) + filePathPrefix = filepath.Join( + "cluster-resources", "pods", "logs", pod.Namespace, pod.Name, container, + ) } - result := NewResult() + setLogLimits(&podLogOpts, limits, convertMaxAgeToTime) req := client.CoreV1().Pods(pod.Namespace).GetLogs(pod.Name, &podLogOpts) podLogs, err := req.Stream(ctx) @@ -148,11 +180,13 @@ func savePodLogs(ctx context.Context, bundlePath string, client *kubernetes.Clie } defer podLogs.Close() - logWriter, err := result.GetWriter(bundlePath, fileKey+".log") + logWriter, err := result.GetWriter(bundlePath, filePathPrefix+".log") if err != nil { return nil, errors.Wrap(err, "failed to get log writer") } - defer result.CloseWriter(bundlePath, fileKey+".log", logWriter) + // NOTE: deferred calls are executed in LIFO order i.e called in reverse order + defer result.SymLinkResult(bundlePath, linkRelPathPrefix+".log", filePathPrefix+".log") + defer result.CloseWriter(bundlePath, filePathPrefix+".log", logWriter) _, err = io.Copy(logWriter, podLogs) if err != nil { @@ -168,11 +202,13 @@ func savePodLogs(ctx context.Context, bundlePath string, client *kubernetes.Clie } defer podLogs.Close() - prevLogWriter, err := result.GetWriter(bundlePath, fileKey+"-previous.log") + prevLogWriter, err := result.GetWriter(bundlePath, filePathPrefix+"-previous.log") if err != nil { return nil, errors.Wrap(err, "failed to get previous log writer") } - defer result.CloseWriter(bundlePath, fileKey+"-previous.log", logWriter) + // NOTE: deferred calls are executed in LIFO order i.e called in reverse order + defer result.SymLinkResult(bundlePath, linkRelPathPrefix+"-previous.log", filePathPrefix+"-previous.log") + defer result.CloseWriter(bundlePath, filePathPrefix+"-previous.log", logWriter) _, err = io.Copy(prevLogWriter, podLogs) if err != nil { diff --git a/pkg/collect/logs_test.go b/pkg/collect/logs_test.go index 98a022935..e23572c02 100644 --- a/pkg/collect/logs_test.go +++ b/pkg/collect/logs_test.go @@ -1,6 +1,7 @@ package collect import ( + "context" "testing" "time" @@ -9,6 +10,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + testclient "k8s.io/client-go/kubernetes/fake" ) func Test_setLogLimits(t *testing.T) { @@ -77,3 +79,74 @@ func Test_setLogLimits(t *testing.T) { }) } } + +func Test_savePodLogs(t *testing.T) { + tests := []struct { + name string + withContainerName bool + collectorName string + want CollectorResult + }{ + { + name: "with container name", + withContainerName: true, + collectorName: "all-logs", + want: CollectorResult{ + "all-logs/test-pod/nginx.log": []byte("fake logs"), + "all-logs/test-pod/nginx-previous.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx-previous.log": []byte("fake logs"), + }, + }, + { + name: "without container name", + withContainerName: false, + collectorName: "all-logs", + want: CollectorResult{ + "all-logs/test-pod.log": []byte("fake logs"), + "all-logs/test-pod-previous.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx-previous.log": []byte("fake logs"), + }, + }, + { + name: "without container or collector names", + withContainerName: false, + want: CollectorResult{ + "/test-pod.log": []byte("fake logs"), + "/test-pod-previous.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx.log": []byte("fake logs"), + "cluster-resources/pods/logs/my-namespace/test-pod/nginx-previous.log": []byte("fake logs"), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + containerName := "nginx" + client := testclient.NewSimpleClientset() + limits := &troubleshootv1beta2.LogLimits{ + MaxLines: 500, + } + pod, err := client.CoreV1().Pods("my-namespace").Create(ctx, &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: containerName, + }, + }, + }, + }, metav1.CreateOptions{}) + assert.NoError(t, err) + if !tt.withContainerName { + containerName = "" + } + got, err := savePodLogsWithInterface(ctx, "", client, pod, tt.collectorName, containerName, limits, false) + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/collect/redact.go b/pkg/collect/redact.go index 00dbb697e..376d94e4b 100644 --- a/pkg/collect/redact.go +++ b/pkg/collect/redact.go @@ -17,9 +17,28 @@ import ( func RedactResult(bundlePath string, input CollectorResult, additionalRedactors []*troubleshootv1beta2.Redact) error { for k, v := range input { + file := k + var reader io.Reader if v == nil { - r, err := input.GetReader(bundlePath, k) + // Collected contents are in a file. Get a reader to the file. + info, err := os.Lstat(filepath.Join(bundlePath, file)) + if err != nil { + if os.IsNotExist(errors.Cause(err)) { + // File not found, moving on. + continue + } + return errors.Wrap(err, "failed to stat file") + } + + // Redact the target file of a symlink + if info.Mode().Type() == os.ModeSymlink { + file, err = os.Readlink(filepath.Join(bundlePath, file)) + if err != nil { + return errors.Wrap(err, "failed to read symlink") + } + } + r, err := input.GetReader(bundlePath, file) if err != nil { if os.IsNotExist(errors.Cause(err)) { continue @@ -30,19 +49,20 @@ func RedactResult(bundlePath string, input CollectorResult, additionalRedactors reader = r } else { + // Collected contents are in memory. Get a reader to the memory buffer. reader = bytes.NewBuffer(v) } //If the file is .tar, .tgz or .tar.gz, it must not be redacted. Instead it is decompressed and each file inside the //tar is decompressed, redacted and compressed back into the tar. - if filepath.Ext(k) == ".tar" || filepath.Ext(k) == ".tgz" || strings.HasSuffix(k, ".tar.gz") { + if filepath.Ext(file) == ".tar" || filepath.Ext(file) == ".tgz" || strings.HasSuffix(file, ".tar.gz") { tmpDir, err := ioutil.TempDir("", "troubleshoot-subresult-") if err != nil { return errors.Wrap(err, "failed to create temp dir") } defer os.RemoveAll(tmpDir) - subResult, tarHeaders, err := decompressFile(tmpDir, reader, k) + subResult, tarHeaders, err := decompressFile(tmpDir, reader, file) if err != nil { return errors.Wrap(err, "failed to decompress file") } @@ -51,7 +71,7 @@ func RedactResult(bundlePath string, input CollectorResult, additionalRedactors return errors.Wrap(err, "failed to redact file") } - dstFilename := filepath.Join(bundlePath, k) + dstFilename := filepath.Join(bundlePath, file) err = compressFiles(tmpDir, subResult, tarHeaders, dstFilename) if err != nil { return errors.Wrap(err, "failed to re-compress file") @@ -63,12 +83,12 @@ func RedactResult(bundlePath string, input CollectorResult, additionalRedactors continue } - redacted, err := redact.Redact(reader, k, additionalRedactors) + redacted, err := redact.Redact(reader, file, additionalRedactors) if err != nil { - return errors.Wrap(err, "failed to redact") + return errors.Wrap(err, "failed to redact io stream") } - err = input.ReplaceResult(bundlePath, k, redacted) + err = input.ReplaceResult(bundlePath, file, redacted) if err != nil { return errors.Wrap(err, "failed to create redacted result") } diff --git a/pkg/collect/remote_collector.go b/pkg/collect/remote_collector.go index fa2502606..e2481acf1 100644 --- a/pkg/collect/remote_collector.go +++ b/pkg/collect/remote_collector.go @@ -109,7 +109,7 @@ func (c *RemoteCollector) RunCollectorSync(globalRedactors []*troubleshootv1beta if err = RedactResult("", result, globalRedactors); err != nil { // Returning result on error to be consistent with local collector. - return result, errors.Wrap(err, "failed to redact") + return result, errors.Wrap(err, "failed to redact remote collector results") } return result, nil } diff --git a/pkg/collect/result.go b/pkg/collect/result.go index b66769aa1..52f801563 100644 --- a/pkg/collect/result.go +++ b/pkg/collect/result.go @@ -5,11 +5,11 @@ import ( "bytes" "compress/gzip" "io" - "io/ioutil" "os" "path/filepath" "github.com/pkg/errors" + "k8s.io/klog/v2" ) type CollectorResult map[string][]byte @@ -18,16 +18,80 @@ func NewResult() CollectorResult { return map[string][]byte{} } +// SymLinkResult creates a symlink (relativeLinkPath) of relativeFilePath in the bundle. If bundlePath +// is empty, no symlink is created. The relativeLinkPath is always saved in the result map. +func (r CollectorResult) SymLinkResult(bundlePath, relativeLinkPath, relativeFilePath string) error { + // We should have saved the result this symlink is pointing to prior to creating it + klog.Info("Creating symlink ", relativeLinkPath, " -> ", relativeFilePath) + data, ok := r[relativeFilePath] + if !ok { + return errors.Errorf("cannot create symlink, result in %q not found", relativeFilePath) + } + + if bundlePath == "" { + // Memory only bundle + r[relativeLinkPath] = data + return nil + } + + linkPath := filepath.Join(bundlePath, relativeLinkPath) + filePath := filepath.Join(bundlePath, relativeFilePath) + + // If both paths are the same, don't create a symlink + if linkPath == filePath { + return nil + } + + linkDirPath := filepath.Dir(linkPath) + + // Create the directory for the symlink if it doesn't exist + err := os.MkdirAll(linkDirPath, 0777) + if err != nil { + return errors.Wrap(err, "failed to create directory") + } + + // Ensure the file exists + _, err = os.Stat(filePath) + if err != nil { + return errors.Wrap(err, "failed to stat. File may not exist") + } + + // Do nothing if the symlink already exists + _, err = os.Lstat(linkPath) + if err == nil { + return nil + } + + // Create the symlink + // NOTE: When creating an archive, relative paths are used + // to make the bundle more portable. That implementation + // lives in TarSupportBundleDir function. This path needs to + // remain as-is to support memory only bundles e.g preflight + err = os.Symlink(filePath, linkPath) + if err != nil { + return errors.Wrap(err, "failed to create symlink") + } + + klog.V(2).Infof("Created '%s' symlink of '%s'", relativeLinkPath, relativeFilePath) + // store the file name referencing the symlink to have archived + r[relativeLinkPath] = nil + + return nil +} + +// SaveResult saves the collector result to relativePath file on disk. If bundlePath is +// empty, no file is created on disk. The relativePath is always saved in the result map. func (r CollectorResult) SaveResult(bundlePath string, relativePath string, reader io.Reader) error { if reader == nil { return nil } if bundlePath == "" { - data, err := ioutil.ReadAll(reader) + data, err := io.ReadAll(reader) if err != nil { return errors.Wrap(err, "failed to read data") } + // Memory only bundle r[relativePath] = data return nil } @@ -38,7 +102,7 @@ func (r CollectorResult) SaveResult(bundlePath string, relativePath string, read outPath := filepath.Join(bundlePath, fileDir) if err := os.MkdirAll(outPath, 0777); err != nil { - return errors.Wrap(err, "create output file") + return errors.Wrap(err, "failed to create output file directory") } f, err := os.Create(filepath.Join(outPath, fileName)) @@ -57,15 +121,16 @@ func (r CollectorResult) SaveResult(bundlePath string, relativePath string, read func (r CollectorResult) ReplaceResult(bundlePath string, relativePath string, reader io.Reader) error { if bundlePath == "" { - data, err := ioutil.ReadAll(reader) + data, err := io.ReadAll(reader) if err != nil { return errors.Wrap(err, "failed to read data") } + // Memory only bundle r[relativePath] = data return nil } - tmpFile, err := ioutil.TempFile("", "replace-") + tmpFile, err := os.CreateTemp("", "replace-") if err != nil { return errors.Wrap(err, "failed to create temp file") } @@ -87,7 +152,8 @@ func (r CollectorResult) ReplaceResult(bundlePath string, relativePath string, r func (r CollectorResult) GetReader(bundlePath string, relativePath string) (io.ReadCloser, error) { if r[relativePath] != nil { - return ioutil.NopCloser(bytes.NewReader(r[relativePath])), nil + // Memory only bundle + return io.NopCloser(bytes.NewReader(r[relativePath])), nil } if bundlePath == "" { @@ -105,6 +171,7 @@ func (r CollectorResult) GetReader(bundlePath string, relativePath string) (io.R func (r CollectorResult) GetWriter(bundlePath string, relativePath string) (io.Writer, error) { if bundlePath == "" { + // Memory only bundle var b bytes.Buffer return &b, nil } @@ -112,7 +179,7 @@ func (r CollectorResult) GetWriter(bundlePath string, relativePath string) (io.W fileDir, _ := filepath.Split(relativePath) outPath := filepath.Join(bundlePath, fileDir) if err := os.MkdirAll(outPath, 0777); err != nil { - return nil, errors.Wrap(err, "create output file") + return nil, errors.Wrap(err, "failed to create output directory") } filename := filepath.Join(bundlePath, relativePath) @@ -132,6 +199,7 @@ func (r CollectorResult) CloseWriter(bundlePath string, relativePath string, wri } if buff, ok := writer.(*bytes.Buffer); ok { + // Memory only bundle b := buff.Bytes() if b == nil { // nil means data is on disk, so make it an empty array @@ -159,30 +227,50 @@ func TarSupportBundleDir(bundlePath string, input CollectorResult, outputFilenam for relativeName := range input { filename := filepath.Join(bundlePath, relativeName) - info, err := os.Stat(filename) + info, err := os.Lstat(filename) if err != nil { return errors.Wrap(err, "failed to stat file") } fileMode := info.Mode() - if !fileMode.IsRegular() { // support bundle can have only files + if !(fileMode.IsRegular() || fileMode.Type() == os.ModeSymlink) { + // support bundle can have only files or symlinks continue } + hdr, err := tar.FileInfoHeader(info, "") + if err != nil { + return errors.Wrap(err, "failed to tar file info header") + } + parentDirName := filepath.Dir(bundlePath) // this is to have the files inside a subdirectory nameInArchive, err := filepath.Rel(parentDirName, filename) if err != nil { return errors.Wrap(err, "failed to create relative file name") } + // Use the relative path of the file so as to retain directory hierachy + hdr.Name = nameInArchive + + if fileMode.Type() == os.ModeSymlink { + linkTarget, err := os.Readlink(filename) + if err != nil { + return errors.Wrap(err, "failed to get symlink target") + } + + linkTargetInArchive, err := filepath.Rel(parentDirName, linkTarget) + if err != nil { + return errors.Wrap(err, "failed to create relative file name") + } - // tar.FileInfoHeader call causes a crash in static builds - // https://github.com/golang/go/issues/24787 - hdr := &tar.Header{ - Name: nameInArchive, - ModTime: info.ModTime(), - Mode: int64(fileMode.Perm()), - Typeflag: tar.TypeReg, - Size: info.Size(), + // Use the relative path of the link target so as to retain directory hierachy + // i.e link -> ../../../../target.log. When untarred, the link will point to the + // relative path of the target file on the machine where it is untarred. + relLinkPath, err := filepath.Rel(filepath.Dir(nameInArchive), linkTargetInArchive) + if err != nil { + return errors.Wrap(err, "failed to create relative path of symlink target file") + } + + hdr.Linkname = relLinkPath } err = tarWriter.WriteHeader(hdr) @@ -190,7 +278,13 @@ func TarSupportBundleDir(bundlePath string, input CollectorResult, outputFilenam return errors.Wrap(err, "failed to write tar header") } - err = func() error { + func() error { + if fileMode.Type() == os.ModeSymlink { + // Don't copy the symlink, just write the header which + // will create a symlink in the tarball + return nil + } + fileReader, err := os.Open(filename) if err != nil { return errors.Wrap(err, "failed to open source file") diff --git a/pkg/collect/run_pod.go b/pkg/collect/run_pod.go index 20a0f1a89..765c905fb 100644 --- a/pkg/collect/run_pod.go +++ b/pkg/collect/run_pod.go @@ -179,7 +179,7 @@ func runWithoutTimeout(ctx context.Context, bundlePath string, clientConfig *res limits := troubleshootv1beta2.LogLimits{ MaxLines: 10000, } - podLogs, err := savePodLogs(ctx, bundlePath, client, *pod, collectorName, "", &limits, true) + podLogs, err := savePodLogs(ctx, bundlePath, client, pod, collectorName, "", &limits, true) if err != nil { return nil, errors.Wrap(err, "failed to get pod logs") } diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go index 0b834cdb8..cea22be7c 100644 --- a/pkg/supportbundle/collect.go +++ b/pkg/supportbundle/collect.go @@ -58,7 +58,7 @@ func runHostCollectors(hostCollectors []*troubleshootv1beta2.HostCollect, additi if opts.Redact { err := collect.RedactResult(bundlePath, collectResult, globalRedactors) if err != nil { - err = errors.Wrap(err, "failed to redact") + err = errors.Wrap(err, "failed to redact host collector results") return collectResult, err } } @@ -150,8 +150,7 @@ func runCollectors(collectors []*troubleshootv1beta2.Collect, additionalRedactor if opts.Redact { err := collect.RedactResult(bundlePath, collectResult, globalRedactors) if err != nil { - err = errors.Wrap(err, "failed to redact") - return collectResult, err + return collectResult, errors.Wrap(err, "failed to redact in cluster collector results") } } diff --git a/pkg/supportbundle/supportbundle.go b/pkg/supportbundle/supportbundle.go index 11b025078..aca3b380b 100644 --- a/pkg/supportbundle/supportbundle.go +++ b/pkg/supportbundle/supportbundle.go @@ -2,7 +2,6 @@ package supportbundle import ( "fmt" - "io/ioutil" "net/http" "os" "path/filepath" @@ -17,6 +16,7 @@ import ( "github.com/replicatedhq/troubleshoot/pkg/collect" "github.com/replicatedhq/troubleshoot/pkg/convert" "k8s.io/client-go/rest" + "k8s.io/klog/v2" ) type SupportBundleCreateOpts struct { @@ -53,11 +53,12 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a return nil, errors.New("did not receive collector progress chan") } - tmpDir, err := ioutil.TempDir("", "supportbundle") + tmpDir, err := os.MkdirTemp("", "supportbundle") if err != nil { return nil, errors.Wrap(err, "create temp dir") } defer os.RemoveAll(tmpDir) + klog.V(2).Infof("Support bundle created in temporary directory: %s", tmpDir) basename := "" if opts.OutputPath != "" { @@ -88,11 +89,15 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a var result, files, hostFiles collect.CollectorResult + // Cache error returned by collectors and return it at the end of the function + // so as to have a chance to run analyzers and archive the support bundle after. + // If both host and in cluster collectors fail, the errors will be wrapped + collectorsErrs := []string{} if spec.HostCollectors != nil { // Run host collectors hostFiles, err = runHostCollectors(spec.HostCollectors, additionalRedactors, bundlePath, opts) if err != nil { - fmt.Println(errors.Wrap(err, "failed to run host collectors")) + collectorsErrs = append(collectorsErrs, fmt.Sprintf("failed to run host collectors: %s", err)) } } @@ -100,7 +105,7 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a // Run collectors files, err = runCollectors(spec.Collectors, additionalRedactors, bundlePath, opts) if err != nil { - fmt.Println(errors.Wrap(err, "failed to run collectors")) + collectorsErrs = append(collectorsErrs, fmt.Sprintf("failed to run collectors: %s", err)) } } @@ -166,6 +171,11 @@ func CollectSupportBundleFromSpec(spec *troubleshootv1beta2.SupportBundleSpec, a } resultsResponse.FileUploaded = fileUploaded + if len(collectorsErrs) > 0 { + // TODO: Consider a collectors error type + return &resultsResponse, fmt.Errorf(strings.Join(collectorsErrs, "\n")) + } + return &resultsResponse, nil } diff --git a/test/validate-preflight-e2e.sh b/test/validate-preflight-e2e.sh index 2d38dadda..90502d864 100755 --- a/test/validate-preflight-e2e.sh +++ b/test/validate-preflight-e2e.sh @@ -4,7 +4,12 @@ set -euo pipefail tmpdir="$(mktemp -d)" -./bin/preflight --interactive=false --format=json examples/preflight/e2e.yaml > "$tmpdir/result.json" +./bin/preflight --debug --interactive=false --format=json examples/preflight/e2e.yaml > "$tmpdir/result.json" +if [ $? -ne 0 ]; then + echo "preflight command failed" + exit $EXIT_STATUS +fi + cat "$tmpdir/result.json" EXIT_STATUS=0 diff --git a/test/validate-support-bundle-e2e.sh b/test/validate-support-bundle-e2e.sh index d812453ad..f1a7941c6 100755 --- a/test/validate-support-bundle-e2e.sh +++ b/test/validate-support-bundle-e2e.sh @@ -6,7 +6,11 @@ tmpdir="$(mktemp -d)" bundle_archive_name="support-bundle.tar.gz" bundle_directory_name="support-bundle" -./bin/support-bundle --interactive=false examples/support-bundle/e2e.yaml --output=$tmpdir/$bundle_archive_name +./bin/support-bundle --debug --interactive=false examples/support-bundle/e2e.yaml --output=$tmpdir/$bundle_archive_name +if [ $? -ne 0 ]; then + echo "support-bundle command failed" + exit $EXIT_STATUS +fi EXIT_STATUS=0 if ! tar -xvzf $tmpdir/$bundle_archive_name --directory $tmpdir; then