From 08c3fcf3df1db154f0408103a609d404e398fef8 Mon Sep 17 00:00:00 2001
From: Diamon Wiggins <38189728+diamonwiggins@users.noreply.github.com>
Date: Fri, 27 Oct 2023 09:48:09 -0400
Subject: [PATCH] Gracefully handle unreachable URIs in
 loadSupportBundleSpecsFromURIs (#1383)

* gracefully handle unreachable URIs in loadSupportBundleSpecsFromURIs

* let caller decide how to handle the error

* fix klog import

* Add a test to ensure failing to load uri does not error

---------

Co-authored-by: Evans Mungai <evans@replicated.com>
---
 cmd/troubleshoot/cli/run.go      | 15 +++++++--
 cmd/troubleshoot/cli/run_test.go | 56 +++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go
index bed7c9e35..89f471701 100644
--- a/cmd/troubleshoot/cli/run.go
+++ b/cmd/troubleshoot/cli/run.go
@@ -232,6 +232,7 @@ the %s Admin Console to begin analysis.`
 	return nil
 }
 
+// loadSupportBundleSpecsFromURIs loads support bundle specs from URIs
 func loadSupportBundleSpecsFromURIs(ctx context.Context, kinds *loader.TroubleshootKinds) (*loader.TroubleshootKinds, error) {
 	remoteRawSpecs := []string{}
 	for _, s := range kinds.SupportBundlesV1Beta2 {
@@ -239,14 +240,21 @@ func loadSupportBundleSpecsFromURIs(ctx context.Context, kinds *loader.Troublesh
 			// We are using LoadSupportBundleSpec function here since it handles prompting
 			// users to accept insecure connections
 			// There is an opportunity to refactor this code in favour of the Loader APIs
+			// TODO: Pass ctx to LoadSupportBundleSpec
 			rawSpec, err := supportbundle.LoadSupportBundleSpec(s.Spec.Uri)
 			if err != nil {
-				return nil, errors.Wrapf(err, "failed to load support bundle from URI %q", s.Spec.Uri)
+				// In the event a spec can't be loaded, we'll just skip it and print a warning
+				klog.Warningf("unable to load support bundle from URI: %q: %v", s.Spec.Uri, err)
+				continue
 			}
 			remoteRawSpecs = append(remoteRawSpecs, string(rawSpec))
 		}
 	}
 
+	if len(remoteRawSpecs) == 0 {
+		return kinds, nil
+	}
+
 	return loader.LoadSpecs(ctx, loader.LoadOptions{
 		RawSpecs: remoteRawSpecs,
 	})
@@ -264,9 +272,10 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface)
 	if !viper.GetBool("no-uri") {
 		moreKinds, err := loadSupportBundleSpecsFromURIs(ctx, kinds)
 		if err != nil {
-			return nil, nil, err
+			klog.Warningf("unable to load support bundles from URIs: %v", err)
+		} else {
+			kinds.Add(moreKinds)
 		}
-		kinds.Add(moreKinds)
 	}
 
 	// Check if we have any collectors to run in the troubleshoot specs
diff --git a/cmd/troubleshoot/cli/run_test.go b/cmd/troubleshoot/cli/run_test.go
index a02e12c0d..645e12f2d 100644
--- a/cmd/troubleshoot/cli/run_test.go
+++ b/cmd/troubleshoot/cli/run_test.go
@@ -4,13 +4,29 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
+	"time"
 
+	"github.com/replicatedhq/troubleshoot/pkg/httputil"
 	"github.com/replicatedhq/troubleshoot/pkg/loader"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+var orig = `
+apiVersion: troubleshoot.sh/v1beta2
+kind: SupportBundle
+metadata:
+  name: sb-1
+spec:
+  uri: $MY_URI
+  collectors:
+    - configMap:
+        name: kube-root-ca.crt
+        namespace: default
+`
+
 func Test_loadSupportBundleSpecsFromURIs(t *testing.T) {
 	// Run a webserver to serve the spec
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -25,18 +41,7 @@ spec:
 	}))
 	defer srv.Close()
 
-	orig := `
-apiVersion: troubleshoot.sh/v1beta2
-kind: SupportBundle
-metadata:
-  name: sb-1
-spec:
-  uri: ` + srv.URL + `
-  collectors:
-    - configMap:
-        name: kube-root-ca.crt
-        namespace: default
-`
+	orig := strings.ReplaceAll(orig, "$MY_URI", srv.URL)
 
 	ctx := context.Background()
 	kinds, err := loader.LoadSpecs(ctx, loader.LoadOptions{RawSpec: orig})
@@ -49,3 +54,30 @@ spec:
 	require.Len(t, moreKinds.SupportBundlesV1Beta2, 1)
 	assert.NotNil(t, moreKinds.SupportBundlesV1Beta2[0].Spec.Collectors[0].ClusterInfo)
 }
+
+func Test_loadSupportBundleSpecsFromURIs_TimeoutError(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(2 * time.Second) // this will cause a timeout
+	}))
+	defer srv.Close()
+	ctx := context.Background()
+
+	kinds, err := loader.LoadSpecs(ctx, loader.LoadOptions{
+		RawSpec: strings.ReplaceAll(orig, "$MY_URI", srv.URL),
+	})
+	require.NoError(t, err)
+
+	// Set the timeout on the http client to 10ms
+	// supportbundle.LoadSupportBundleSpec does not yet use the context
+	before := httputil.GetHttpClient().Timeout
+	httputil.GetHttpClient().Timeout = 10 * time.Millisecond
+	defer func() {
+		// Reinstate the original timeout. Its a global var so we need to reset it
+		httputil.GetHttpClient().Timeout = before
+	}()
+
+	kindsAfter, err := loadSupportBundleSpecsFromURIs(ctx, kinds)
+	require.NoError(t, err)
+
+	assert.Equal(t, kinds, kindsAfter)
+}