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

@@ -232,21 +232,29 @@ 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 {
  if s.Spec.Uri != "" && util.IsURL(s.Spec.Uri) {
  // 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

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)
+}