From 59c73dcd1a1c87cff70450cc8421a236a01dbee5 Mon Sep 17 00:00:00 2001 From: Gerard Nguyen Date: Mon, 15 Jul 2024 19:01:36 +1000 Subject: [PATCH 1/3] * new no-uri flag for preflight * implement load additional spec from URIs --- cmd/preflight/cli/root.go | 1 + internal/specs/specs.go | 28 ++++++++++++++++++++++++++++ pkg/loader/loader.go | 30 ++++++++++++++++++++++++++++++ pkg/preflight/read_specs.go | 6 ++++++ 4 files changed, 65 insertions(+) diff --git a/cmd/preflight/cli/root.go b/cmd/preflight/cli/root.go index 19f8b81d9..012f869aa 100644 --- a/cmd/preflight/cli/root.go +++ b/cmd/preflight/cli/root.go @@ -71,6 +71,7 @@ that a cluster meets the requirements to run an application.`, // Dry run flag should be in cmd.PersistentFlags() flags made available to all subcommands // Adding here to avoid that cmd.Flags().Bool("dry-run", false, "print the preflight spec without running preflight checks") + cmd.Flags().Bool("no-uri", false, "When this flag is used, Preflight does not attempt to retrieve the spec referenced by the uri: field`") k8sutil.AddFlags(cmd.Flags()) diff --git a/internal/specs/specs.go b/internal/specs/specs.go index a63c3fd0d..7ea90069b 100644 --- a/internal/specs/specs.go +++ b/internal/specs/specs.go @@ -361,3 +361,31 @@ func LoadFromCluster(ctx context.Context, client kubernetes.Interface, selectors RawSpecs: rawSpecs, }) } + +// LoadAdditionalSpecFromURIs loads additional specs from the URIs provided in the troubleshoot kinds. +// This function will modify kinds in place. +func LoadAdditionalSpecFromURIs(ctx context.Context, kinds *loader.TroubleshootKinds) { + uris := kinds.GetURIs() + if len(uris) == 0 { + klog.Info("No additional URIs found in all specs") + return + } + for _, uri := range uris { + rawSpec, err := downloadFromHttpURL(ctx, uri, nil) + if err != nil { + klog.Warningf("failed to download spec from URI %q: %v", uri, err) + continue + } + k, err := loader.LoadSpecs(ctx, loader.LoadOptions{RawSpec: string(rawSpec)}) + if err != nil { + klog.Warningf("failed to load spec from URI %q: %v", uri, err) + continue + } + kinds.Add(k) + } + + // dedup these top level specs + kinds.SupportBundlesV1Beta2 = util.Dedup(kinds.SupportBundlesV1Beta2) + kinds.PreflightsV1Beta2 = util.Dedup(kinds.PreflightsV1Beta2) + kinds.HostPreflightsV1Beta2 = util.Dedup(kinds.HostPreflightsV1Beta2) +} diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index e139835df..c0ed92e11 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -131,6 +131,36 @@ func (kinds *TroubleshootKinds) ToYaml() (string, error) { return strings.Join(rawList, "---\n"), nil } +// GetURIs dynamically extracts all the URIs from the troubleshoot kinds +// .Spec.Uri field +func (kinds *TroubleshootKinds) GetURIs() []string { + uris := []string{} + obj := reflect.ValueOf(*kinds) + + for i := 0; i < obj.NumField(); i++ { + field := obj.Field(i) + if field.Kind() != reflect.Slice { + continue + } + + for count := 0; count < field.Len(); count++ { + val := field.Index(count) + specField := val.FieldByName("Spec") + if !specField.IsValid() { + continue + } + uriField := specField.FieldByName("Uri") + if !uriField.IsValid() || uriField.Kind() != reflect.String { + continue + } + + uris = append(uris, uriField.String()) + } + } + + return uris +} + func NewTroubleshootKinds() *TroubleshootKinds { return &TroubleshootKinds{} } diff --git a/pkg/preflight/read_specs.go b/pkg/preflight/read_specs.go index 2a1f66b17..7f324f81b 100644 --- a/pkg/preflight/read_specs.go +++ b/pkg/preflight/read_specs.go @@ -29,6 +29,12 @@ func readSpecs(args []string) (*loader.TroubleshootKinds, error) { return nil, err } + // Load additional specs from URIs + // only when no-uri flag is not set + if !viper.GetBool("no-uri") { + specs.LoadAdditionalSpecFromURIs(ctx, kinds) + } + ret := loader.NewTroubleshootKinds() // Concatenate all preflight inclusterSpecs that don't have an upload destination From b6fda31bde3a9e27ac5cec0181cfc8ed14d37e8d Mon Sep 17 00:00:00 2001 From: Gerard Nguyen Date: Tue, 16 Jul 2024 16:39:51 +1000 Subject: [PATCH 2/3] * use set for unique uri * add unit tests --- internal/specs/specs.go | 2 +- internal/specs/specs_test.go | 36 ++++++++++++++++++++++++++++++++++++ pkg/loader/loader.go | 6 +++--- pkg/loader/loader_test.go | 30 ++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/internal/specs/specs.go b/internal/specs/specs.go index 7ea90069b..d4f82efad 100644 --- a/internal/specs/specs.go +++ b/internal/specs/specs.go @@ -370,7 +370,7 @@ func LoadAdditionalSpecFromURIs(ctx context.Context, kinds *loader.TroubleshootK klog.Info("No additional URIs found in all specs") return } - for _, uri := range uris { + for uri := range uris { rawSpec, err := downloadFromHttpURL(ctx, uri, nil) if err != nil { klog.Warningf("failed to download spec from URI %q: %v", uri, err) diff --git a/internal/specs/specs_test.go b/internal/specs/specs_test.go index b32df9b29..93420eabc 100644 --- a/internal/specs/specs_test.go +++ b/internal/specs/specs_test.go @@ -224,3 +224,39 @@ spec: require.NoError(t, err) require.Len(t, specs.HostCollectorsV1Beta2, 1) } + +func TestLoadAdditionalSpecFromURIs(t *testing.T) { + m := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`apiVersion: troubleshoot.sh/v1beta2 +apiVersion: troubleshoot.sh/v1beta2 +kind: Preflight +metadata: + name: preflight-2 +spec: + collectors: + - ceph: {} +`)) + })) + defer m.Close() + kinds := loader.NewTroubleshootKinds() + kinds.PreflightsV1Beta2 = []troubleshootv1beta2.Preflight{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "preflight-1", + }, + Spec: troubleshootv1beta2.PreflightSpec{ + Uri: m.URL, + Collectors: []*troubleshootv1beta2.Collect{ + { + DNS: &troubleshootv1beta2.DNS{}, + }, + }, + }, + }, + } + + LoadAdditionalSpecFromURIs(context.Background(), kinds) + require.Len(t, kinds.PreflightsV1Beta2, 2) + require.Len(t, kinds.PreflightsV1Beta2[0].Spec.Collectors, 1) // ceph + require.Len(t, kinds.PreflightsV1Beta2[1].Spec.Collectors, 1) // dns +} diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index c0ed92e11..7a6431245 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -133,8 +133,8 @@ func (kinds *TroubleshootKinds) ToYaml() (string, error) { // GetURIs dynamically extracts all the URIs from the troubleshoot kinds // .Spec.Uri field -func (kinds *TroubleshootKinds) GetURIs() []string { - uris := []string{} +func (kinds *TroubleshootKinds) GetURIs() map[string]bool { + uris := map[string]bool{} obj := reflect.ValueOf(*kinds) for i := 0; i < obj.NumField(); i++ { @@ -154,7 +154,7 @@ func (kinds *TroubleshootKinds) GetURIs() []string { continue } - uris = append(uris, uriField.String()) + uris[uriField.String()] = true } } diff --git a/pkg/loader/loader_test.go b/pkg/loader/loader_test.go index 606db12d2..43e6985a2 100644 --- a/pkg/loader/loader_test.go +++ b/pkg/loader/loader_test.go @@ -632,3 +632,33 @@ func TestLoadingEmptySpec(t *testing.T) { }, }, kinds) } + +func TestGetURIs(t *testing.T) { + kinds := NewTroubleshootKinds() + kinds.SupportBundlesV1Beta2 = []troubleshootv1beta2.SupportBundle{ + { + Spec: troubleshootv1beta2.SupportBundleSpec{ + Uri: "https://supportbundle.com", + }, + }, + } + kinds.PreflightsV1Beta2 = []troubleshootv1beta2.Preflight{ + { + Spec: troubleshootv1beta2.PreflightSpec{ + Uri: "https://preflight.com", + }, + }, + } + kinds.CollectorsV1Beta2 = []troubleshootv1beta2.Collector{ + { + Spec: troubleshootv1beta2.CollectorSpec{ + Uri: "https://supportbundle.com", + }, + }, + } + uris := kinds.GetURIs() + assert.Equal(t, map[string]bool{ + "https://supportbundle.com": true, + "https://preflight.com": true, + }, uris) +} From e27b49bfd2a687469d4bca0737d8325c5a1aef1d Mon Sep 17 00:00:00 2001 From: Gerard Nguyen Date: Thu, 18 Jul 2024 13:49:55 +1000 Subject: [PATCH 3/3] updates from code review --- internal/specs/specs.go | 88 ++++++++++++++++++++++++++++-------- internal/specs/specs_test.go | 6 +-- pkg/loader/loader.go | 30 ------------ pkg/loader/loader_test.go | 30 ------------ 4 files changed, 72 insertions(+), 82 deletions(-) diff --git a/internal/specs/specs.go b/internal/specs/specs.go index d4f82efad..2f442e355 100644 --- a/internal/specs/specs.go +++ b/internal/specs/specs.go @@ -362,30 +362,80 @@ func LoadFromCluster(ctx context.Context, client kubernetes.Interface, selectors }) } -// LoadAdditionalSpecFromURIs loads additional specs from the URIs provided in the troubleshoot kinds. +// LoadAdditionalSpecFromURIs loads additional specs from the URIs provided in the kinds. // This function will modify kinds in place. func LoadAdditionalSpecFromURIs(ctx context.Context, kinds *loader.TroubleshootKinds) { - uris := kinds.GetURIs() - if len(uris) == 0 { - klog.Info("No additional URIs found in all specs") - return - } - for uri := range uris { - rawSpec, err := downloadFromHttpURL(ctx, uri, nil) - if err != nil { - klog.Warningf("failed to download spec from URI %q: %v", uri, err) + + obj := reflect.ValueOf(*kinds) + + // iterate over all fields of the TroubleshootKinds + // e.g. SupportBundlesV1Beta2, PreflightsV1Beta2, etc. + for i := 0; i < obj.NumField(); i++ { + field := obj.Field(i) + if field.Kind() != reflect.Slice { continue } - k, err := loader.LoadSpecs(ctx, loader.LoadOptions{RawSpec: string(rawSpec)}) - if err != nil { - klog.Warningf("failed to load spec from URI %q: %v", uri, err) - continue + + // look at each spec in the slice + // e.g. each spec in []PreflightsV1Beta2 + for count := 0; count < field.Len(); count++ { + currentSpec := field.Index(count) + specName := currentSpec.Type().Name() + + // check if .Spec.Uri exists + specField := currentSpec.FieldByName("Spec") + if !specField.IsValid() { + continue + } + uriField := specField.FieldByName("Uri") + if uriField.Kind() != reflect.String { + continue + } + + // download spec from URI + uri := uriField.String() + if uri == "" { + continue + } + rawSpec, err := downloadFromHttpURL(ctx, uri, nil) + if err != nil { + klog.Warningf("failed to download spec from URI %q: %v", uri, err) + continue + } + + // load spec from raw spec + uriKinds, err := loader.LoadSpecs(ctx, loader.LoadOptions{RawSpec: string(rawSpec)}) + if err != nil { + klog.Warningf("failed to load spec from URI %q: %v", uri, err) + continue + } + + // replace original spec with the loaded spec from URI + newSpec := getFirstSpecOf(uriKinds, specName) + if !newSpec.IsValid() { + klog.Warningf("failed to read spec of type %s in URI %s", specName, uri) + continue + } + currentSpec.Set(newSpec) } - kinds.Add(k) } +} - // dedup these top level specs - kinds.SupportBundlesV1Beta2 = util.Dedup(kinds.SupportBundlesV1Beta2) - kinds.PreflightsV1Beta2 = util.Dedup(kinds.PreflightsV1Beta2) - kinds.HostPreflightsV1Beta2 = util.Dedup(kinds.HostPreflightsV1Beta2) +// dynamically get spec from kinds of given name +// return first element of the spec slice +func getFirstSpecOf(kinds *loader.TroubleshootKinds, name string) reflect.Value { + obj := reflect.ValueOf(*kinds) + for i := 0; i < obj.NumField(); i++ { + field := obj.Field(i) + if field.Kind() != reflect.Slice { + continue + } + if field.Len() > 0 { + if field.Index(0).Type().Name() == name { + // return first element + return field.Index(0) + } + } + } + return reflect.Value{} } diff --git a/internal/specs/specs_test.go b/internal/specs/specs_test.go index 93420eabc..e2f3fa2c2 100644 --- a/internal/specs/specs_test.go +++ b/internal/specs/specs_test.go @@ -256,7 +256,7 @@ spec: } LoadAdditionalSpecFromURIs(context.Background(), kinds) - require.Len(t, kinds.PreflightsV1Beta2, 2) - require.Len(t, kinds.PreflightsV1Beta2[0].Spec.Collectors, 1) // ceph - require.Len(t, kinds.PreflightsV1Beta2[1].Spec.Collectors, 1) // dns + require.Len(t, kinds.PreflightsV1Beta2, 1) + require.Len(t, kinds.PreflightsV1Beta2[0].Spec.Collectors, 1) + require.NotNil(t, kinds.PreflightsV1Beta2[0].Spec.Collectors[0].Ceph) } diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 7a6431245..e139835df 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -131,36 +131,6 @@ func (kinds *TroubleshootKinds) ToYaml() (string, error) { return strings.Join(rawList, "---\n"), nil } -// GetURIs dynamically extracts all the URIs from the troubleshoot kinds -// .Spec.Uri field -func (kinds *TroubleshootKinds) GetURIs() map[string]bool { - uris := map[string]bool{} - obj := reflect.ValueOf(*kinds) - - for i := 0; i < obj.NumField(); i++ { - field := obj.Field(i) - if field.Kind() != reflect.Slice { - continue - } - - for count := 0; count < field.Len(); count++ { - val := field.Index(count) - specField := val.FieldByName("Spec") - if !specField.IsValid() { - continue - } - uriField := specField.FieldByName("Uri") - if !uriField.IsValid() || uriField.Kind() != reflect.String { - continue - } - - uris[uriField.String()] = true - } - } - - return uris -} - func NewTroubleshootKinds() *TroubleshootKinds { return &TroubleshootKinds{} } diff --git a/pkg/loader/loader_test.go b/pkg/loader/loader_test.go index 43e6985a2..606db12d2 100644 --- a/pkg/loader/loader_test.go +++ b/pkg/loader/loader_test.go @@ -632,33 +632,3 @@ func TestLoadingEmptySpec(t *testing.T) { }, }, kinds) } - -func TestGetURIs(t *testing.T) { - kinds := NewTroubleshootKinds() - kinds.SupportBundlesV1Beta2 = []troubleshootv1beta2.SupportBundle{ - { - Spec: troubleshootv1beta2.SupportBundleSpec{ - Uri: "https://supportbundle.com", - }, - }, - } - kinds.PreflightsV1Beta2 = []troubleshootv1beta2.Preflight{ - { - Spec: troubleshootv1beta2.PreflightSpec{ - Uri: "https://preflight.com", - }, - }, - } - kinds.CollectorsV1Beta2 = []troubleshootv1beta2.Collector{ - { - Spec: troubleshootv1beta2.CollectorSpec{ - Uri: "https://supportbundle.com", - }, - }, - } - uris := kinds.GetURIs() - assert.Equal(t, map[string]bool{ - "https://supportbundle.com": true, - "https://preflight.com": true, - }, uris) -}