From 1ff429c906694362307f4891b668769fabd328ee Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 2 Oct 2024 16:28:49 +0100 Subject: [PATCH 1/9] fix: Allow using load-cluster-specs=false Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/root.go | 2 +- cmd/troubleshoot/cli/run.go | 25 +++++++++---------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/cmd/troubleshoot/cli/root.go b/cmd/troubleshoot/cli/root.go index 78b3cc2b9..0231260f0 100644 --- a/cmd/troubleshoot/cli/root.go +++ b/cmd/troubleshoot/cli/root.go @@ -77,7 +77,7 @@ If no arguments are provided, specs are automatically loaded from the cluster by cmd.Flags().Bool("interactive", true, "enable/disable interactive mode") cmd.Flags().Bool("collect-without-permissions", true, "always generate a support bundle, even if it some require additional permissions") cmd.Flags().StringSliceP("selector", "l", []string{"troubleshoot.sh/kind=support-bundle"}, "selector to filter on for loading additional support bundle specs found in secrets within the cluster") - cmd.Flags().Bool("load-cluster-specs", false, "enable/disable loading additional troubleshoot specs found within the cluster. This is the default behavior if no spec is provided as an argument") + cmd.Flags().Bool("load-cluster-specs", true, "enable/disable loading additional troubleshoot specs found within the cluster. This is the default behavior if no spec is provided as an argument") cmd.Flags().String("since-time", "", "force pod logs collectors to return logs after a specific date (RFC3339)") cmd.Flags().String("since", "", "force pod logs collectors to return logs newer than a relative duration like 5s, 2m, or 3h.") cmd.Flags().StringP("output", "o", "", "specify the output file path for the support bundle") diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go index f141ed87d..5167922d6 100644 --- a/cmd/troubleshoot/cli/run.go +++ b/cmd/troubleshoot/cli/run.go @@ -292,24 +292,17 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface) err error ) - if len(args) < 1 { - fmt.Println("\r\033[36mNo specs provided, attempting to load from cluster...\033[m") - kinds, err = specs.LoadFromCluster(ctx, client, vp.GetStringSlice("selector"), vp.GetString("namespace")) - if err != nil { - return nil, nil, errors.Wrap(err, "failed to load specs from cluster, and no specs were provided as arguments") - } - if len(redactors) > 0 { - additionalKinds, err := specs.LoadFromCLIArgs(ctx, client, allArgs, vp) - if err != nil { - return nil, nil, errors.Wrap(err, "failed to load redactors from CLI args") - } - kinds.RedactorsV1Beta2 = append(kinds.RedactorsV1Beta2, additionalKinds.RedactorsV1Beta2...) - } - } else { - kinds, err = specs.LoadFromCLIArgs(ctx, client, allArgs, vp) + kinds, err = specs.LoadFromCLIArgs(ctx, client, allArgs, vp) + if err != nil { + return nil, nil, errors.Wrap(err, "failed to load specs from CLI args") + } + + if len(redactors) > 0 { + additionalKinds, err := specs.LoadFromCLIArgs(ctx, client, allArgs, vp) if err != nil { - return nil, nil, errors.Wrap(err, "failed to load specs from CLI args") + return nil, nil, errors.Wrap(err, "failed to load redactors from CLI args") } + kinds.RedactorsV1Beta2 = append(kinds.RedactorsV1Beta2, additionalKinds.RedactorsV1Beta2...) } // Load additional specs from support bundle URIs From d72cb2d230e35dab4049a0162d408761c26d56f4 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 7 Oct 2024 13:40:23 +0100 Subject: [PATCH 2/9] Some more simplification Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/run.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go index 5167922d6..72ea864ae 100644 --- a/cmd/troubleshoot/cli/run.go +++ b/cmd/troubleshoot/cli/run.go @@ -297,14 +297,6 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface) return nil, nil, errors.Wrap(err, "failed to load specs from CLI args") } - if len(redactors) > 0 { - additionalKinds, err := specs.LoadFromCLIArgs(ctx, client, allArgs, vp) - if err != nil { - return nil, nil, errors.Wrap(err, "failed to load redactors from CLI args") - } - kinds.RedactorsV1Beta2 = append(kinds.RedactorsV1Beta2, additionalKinds.RedactorsV1Beta2...) - } - // Load additional specs from support bundle URIs // only when no-uri flag is not set and no URLs are provided in the args if !viper.GetBool("no-uri") { From a240ddb0bd6e65d4a59c23623b513ec328d0fd70 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 7 Oct 2024 20:24:58 +0100 Subject: [PATCH 3/9] Ensure error in loading specs is printed in CLI Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/run.go | 5 ++++- pkg/types/types.go | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go index 72ea864ae..52748f1a6 100644 --- a/cmd/troubleshoot/cli/run.go +++ b/cmd/troubleshoot/cli/run.go @@ -311,7 +311,10 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface) if len(kinds.CollectorsV1Beta2) == 0 && len(kinds.HostCollectorsV1Beta2) == 0 && len(kinds.SupportBundlesV1Beta2) == 0 { - return nil, nil, types.NewExitCodeError(constants.EXIT_CODE_CATCH_ALL, errors.Wrap(err, "no collectors specified to run. Use --debug and/or -v=2 to see more information")) + return nil, nil, types.NewExitCodeError( + constants.EXIT_CODE_CATCH_ALL, + fmt.Errorf("no collectors specified to run. Use --debug and/or -v=2 to see more information"), + ) } // Merge specs diff --git a/pkg/types/types.go b/pkg/types/types.go index d710362a7..def76e996 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -20,6 +20,7 @@ type ExitError interface { type ExitCodeError struct { Msg string Code int + Err error } type ExitCodeWarning struct { @@ -30,6 +31,10 @@ func (e *ExitCodeError) Error() string { return e.Msg } +func (e *ExitCodeError) Unwrap() error { + return e.Err +} + func (e *ExitCodeError) ExitStatus() int { return e.Code } @@ -39,7 +44,7 @@ func NewExitCodeError(exitCode int, theErr error) *ExitCodeError { if theErr != nil { useErr = theErr.Error() } - return &ExitCodeError{Msg: useErr, Code: exitCode} + return &ExitCodeError{Msg: useErr, Code: exitCode, Err: theErr} } func NewExitCodeWarning(theErrMsg string) *ExitCodeWarning { From 72ec21f358041128e98ed002b603f0fd6c4b192e Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 8 Oct 2024 15:13:43 +0100 Subject: [PATCH 4/9] Run linter Signed-off-by: Evans Mungai --- pkg/types/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/types/types.go b/pkg/types/types.go index def76e996..47fa1b5ce 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -20,7 +20,7 @@ type ExitError interface { type ExitCodeError struct { Msg string Code int - Err error + Err error } type ExitCodeWarning struct { From 01c16d88f9c77e6aa01ad4dd23e91f0d51936095 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 8 Oct 2024 16:10:54 +0100 Subject: [PATCH 5/9] Fix failing tests Signed-off-by: Evans Mungai --- test/validate-support-bundle-e2e.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/validate-support-bundle-e2e.sh b/test/validate-support-bundle-e2e.sh index 51f7bfcb0..7b6c3d47a 100755 --- a/test/validate-support-bundle-e2e.sh +++ b/test/validate-support-bundle-e2e.sh @@ -23,6 +23,7 @@ recreate_tmpdir ./bin/support-bundle --debug \ --interactive=false \ examples/support-bundle/e2e.yaml \ + --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" @@ -77,7 +78,7 @@ if ! grep "\*\*\*HIDDEN\*\*\*" "$tmpdir/$bundle_directory_name/static-hi.log"; t exit 1 fi -echo "======= Generating support bundle from k8s cluster using --load-cluster-specs ======" +echo "======= Generating support bundle from k8s cluster using default --load-cluster-specs ======" recreate_tmpdir kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 --interactive=false --load-cluster-specs --output=$tmpdir/$bundle_archive_name @@ -193,6 +194,7 @@ recreate_tmpdir kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 --interactive=false secret/default/labelled-support-bundle-1/custom-spec-key \ --redactors configmap/default/labelled-redactor-spec-1/customer-redactor-spec \ + --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" @@ -217,6 +219,7 @@ kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 \ --interactive=false \ configmap/labelled-specs/labelled-support-bundle-2 \ + --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" From 94be6b90b2d39f99ed572e4a8c2efa9c33596529 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 8 Oct 2024 16:15:09 +0100 Subject: [PATCH 6/9] Remove unnecessary test case rename Signed-off-by: Evans Mungai --- test/validate-support-bundle-e2e.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/validate-support-bundle-e2e.sh b/test/validate-support-bundle-e2e.sh index 7b6c3d47a..a8008312f 100755 --- a/test/validate-support-bundle-e2e.sh +++ b/test/validate-support-bundle-e2e.sh @@ -78,7 +78,7 @@ if ! grep "\*\*\*HIDDEN\*\*\*" "$tmpdir/$bundle_directory_name/static-hi.log"; t exit 1 fi -echo "======= Generating support bundle from k8s cluster using default --load-cluster-specs ======" +echo "======= Generating support bundle from k8s cluster using --load-cluster-specs ======" recreate_tmpdir kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 --interactive=false --load-cluster-specs --output=$tmpdir/$bundle_archive_name From 6ba6eb80c88808978ffa7c2909d3d589fd82ad07 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Tue, 8 Oct 2024 16:40:14 +0100 Subject: [PATCH 7/9] Fix error wrapping Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/run.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/troubleshoot/cli/run.go b/cmd/troubleshoot/cli/run.go index 52748f1a6..75a252124 100644 --- a/cmd/troubleshoot/cli/run.go +++ b/cmd/troubleshoot/cli/run.go @@ -313,7 +313,7 @@ func loadSpecs(ctx context.Context, args []string, client kubernetes.Interface) len(kinds.SupportBundlesV1Beta2) == 0 { return nil, nil, types.NewExitCodeError( constants.EXIT_CODE_CATCH_ALL, - fmt.Errorf("no collectors specified to run. Use --debug and/or -v=2 to see more information"), + errors.New("no collectors specified to run. Use --debug and/or -v=2 to see more information"), ) } From 8b72e5a636b376fd7bb1d2642ca7fdd5ef33b3af Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 10 Oct 2024 12:04:22 +0100 Subject: [PATCH 8/9] Check if load-cluster-specs was provided in cli Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/root.go | 15 ++++++++++++++- internal/specs/specs.go | 2 ++ pkg/loader/loader.go | 28 ++++++++++++++-------------- test/validate-support-bundle-e2e.sh | 3 --- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/cmd/troubleshoot/cli/root.go b/cmd/troubleshoot/cli/root.go index 0231260f0..d33197f11 100644 --- a/cmd/troubleshoot/cli/root.go +++ b/cmd/troubleshoot/cli/root.go @@ -44,6 +44,19 @@ If no arguments are provided, specs are automatically loaded from the cluster by RunE: func(cmd *cobra.Command, args []string) error { v := viper.GetViper() + // If there are not locations to load specs passed in the cli args, we should + // load them from the cluster by setting "load-cluster-specs=true". If the caller + // provided "--load-cluster-specs" cli option, we should respect that. + if len(args) == 0 { + // Check if --load-cluster-specs was set by the cli caller by + // checking if the flag was not changed from the default value + flg := cmd.Flags().Lookup("load-cluster-specs") + if flg != nil && !flg.Changed { + // Load specs from the cluster if no spec(s) is(are) provided in the cli args + v.Set("load-cluster-specs", true) + } + } + closer, err := traces.ConfigureTracing("support-bundle") if err != nil { // Do not fail running support-bundle if tracing fails @@ -77,7 +90,7 @@ If no arguments are provided, specs are automatically loaded from the cluster by cmd.Flags().Bool("interactive", true, "enable/disable interactive mode") cmd.Flags().Bool("collect-without-permissions", true, "always generate a support bundle, even if it some require additional permissions") cmd.Flags().StringSliceP("selector", "l", []string{"troubleshoot.sh/kind=support-bundle"}, "selector to filter on for loading additional support bundle specs found in secrets within the cluster") - cmd.Flags().Bool("load-cluster-specs", true, "enable/disable loading additional troubleshoot specs found within the cluster. This is the default behavior if no spec is provided as an argument") + cmd.Flags().Bool("load-cluster-specs", false, "enable/disable loading additional troubleshoot specs found within the cluster. Do not load by default unless no specs are provided in the cli args") cmd.Flags().String("since-time", "", "force pod logs collectors to return logs after a specific date (RFC3339)") cmd.Flags().String("since", "", "force pod logs collectors to return logs newer than a relative duration like 5s, 2m, or 3h.") cmd.Flags().StringP("output", "o", "", "specify the output file path for the support bundle") diff --git a/internal/specs/specs.go b/internal/specs/specs.go index 2f442e355..cfb0060f0 100644 --- a/internal/specs/specs.go +++ b/internal/specs/specs.go @@ -268,6 +268,8 @@ func downloadFromHttpURL(ctx context.Context, url string, headers map[string]str // to list & read secrets and configmaps from all namespaces, we will fallback to trying each // namespace individually, and eventually default to the configured kubeconfig namespace. func LoadFromCluster(ctx context.Context, client kubernetes.Interface, selectors []string, ns string) (*loader.TroubleshootKinds, error) { + klog.V(1).Infof("Load troubleshoot specs from the cluster using selectors: %v", selectors) + if reflect.DeepEqual(selectors, []string{"troubleshoot.sh/kind=support-bundle"}) { // Its the default selector so we append troubleshoot.io/kind=support-bundle to it due to backwards compatibility selectors = append(selectors, "troubleshoot.io/kind=support-bundle") diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index e139835df..872b78278 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -83,14 +83,18 @@ type TroubleshootKinds struct { } func (kinds *TroubleshootKinds) IsEmpty() bool { - return len(kinds.AnalyzersV1Beta2) == 0 && - len(kinds.CollectorsV1Beta2) == 0 && - len(kinds.HostCollectorsV1Beta2) == 0 && - len(kinds.HostPreflightsV1Beta2) == 0 && - len(kinds.PreflightsV1Beta2) == 0 && - len(kinds.RedactorsV1Beta2) == 0 && - len(kinds.RemoteCollectorsV1Beta2) == 0 && - len(kinds.SupportBundlesV1Beta2) == 0 + return kinds.Len() == 0 +} + +func (kinds *TroubleshootKinds) Len() int { + return len(kinds.AnalyzersV1Beta2) + + len(kinds.CollectorsV1Beta2) + + len(kinds.HostCollectorsV1Beta2) + + len(kinds.HostPreflightsV1Beta2) + + len(kinds.PreflightsV1Beta2) + + len(kinds.RedactorsV1Beta2) + + len(kinds.RemoteCollectorsV1Beta2) + + len(kinds.SupportBundlesV1Beta2) } func (kinds *TroubleshootKinds) Add(other *TroubleshootKinds) { @@ -200,7 +204,7 @@ func (l *specLoader) loadFromStrings(rawSpecs ...string) (*TroubleshootKinds, er // If it's not a configmap or secret, just append it to the splitdocs splitdocs = append(splitdocs, rawDoc) } else { - klog.V(1).Infof("Skip loading %q kind", parsed.Kind) + klog.V(2).Infof("Skip loading %q kind", parsed.Kind) } } @@ -254,11 +258,7 @@ func (l *specLoader) loadFromSplitDocs(splitdocs []string) (*TroubleshootKinds, } } - if kinds.IsEmpty() { - klog.V(1).Info("No troubleshoot specs were loaded") - } else { - klog.V(1).Info("Loaded troubleshoot specs successfully") - } + klog.V(2).Infof("Loaded %d troubleshoot specs successfully", kinds.Len()) return kinds, nil } diff --git a/test/validate-support-bundle-e2e.sh b/test/validate-support-bundle-e2e.sh index a8008312f..51f7bfcb0 100755 --- a/test/validate-support-bundle-e2e.sh +++ b/test/validate-support-bundle-e2e.sh @@ -23,7 +23,6 @@ recreate_tmpdir ./bin/support-bundle --debug \ --interactive=false \ examples/support-bundle/e2e.yaml \ - --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" @@ -194,7 +193,6 @@ recreate_tmpdir kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 --interactive=false secret/default/labelled-support-bundle-1/custom-spec-key \ --redactors configmap/default/labelled-redactor-spec-1/customer-redactor-spec \ - --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" @@ -219,7 +217,6 @@ kubectl apply -f "$PRJ_ROOT/testdata/supportbundle/labelled-specs" ./bin/support-bundle -v1 \ --interactive=false \ configmap/labelled-specs/labelled-support-bundle-2 \ - --load-cluster-specs=false \ --output=$tmpdir/$bundle_archive_name if [ $? -ne 0 ]; then echo "support-bundle command failed" From 0cc8c3eb2684cb6a44302b94d53435de6148db00 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Thu, 10 Oct 2024 12:19:25 +0100 Subject: [PATCH 9/9] Better wording in comments Signed-off-by: Evans Mungai --- cmd/troubleshoot/cli/root.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cmd/troubleshoot/cli/root.go b/cmd/troubleshoot/cli/root.go index d33197f11..f2cd03901 100644 --- a/cmd/troubleshoot/cli/root.go +++ b/cmd/troubleshoot/cli/root.go @@ -44,12 +44,11 @@ If no arguments are provided, specs are automatically loaded from the cluster by RunE: func(cmd *cobra.Command, args []string) error { v := viper.GetViper() - // If there are not locations to load specs passed in the cli args, we should + // If there are no locations to load specs from passed in the cli args, we should // load them from the cluster by setting "load-cluster-specs=true". If the caller - // provided "--load-cluster-specs" cli option, we should respect that. + // provided "--load-cluster-specs" cli option, we should respect it. if len(args) == 0 { - // Check if --load-cluster-specs was set by the cli caller by - // checking if the flag was not changed from the default value + // Check if --load-cluster-specs was set by the cli caller to avoid overriding it flg := cmd.Flags().Lookup("load-cluster-specs") if flg != nil && !flg.Changed { // Load specs from the cluster if no spec(s) is(are) provided in the cli args