Fleet CLI dump command filter for resources#4641
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds filtering capabilities to the Fleet CLI dump command, allowing users to filter resources by namespace, GitRepo, Bundle, or HelmOp. The PR introduces new CLI flags (--all-namespaces, --bundle, --helmop, --gitrepo) and implements intelligent resource filtering based on the Fleet resource relationship model.
Changes:
- Added namespace-based filtering with
--namespace(now functional) and--all-namespaces(-A) flags, making--namespacedefault tofleet-localwhen not specified - this is a breaking change - Added resource-specific filtering options:
--gitrepo,--bundle, and--helmopflags (mutually exclusive) - Implemented label-based filtering for BundleDeployments, Contents, and Secrets to include only resources related to filtered GitRepos/Bundles/HelmOps
- Fixed HelmOp template structure to use
repoinstead ofchartfield
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/cli/dump/dump.go | Core implementation of filtering logic including namespace filtering, resource collection functions, and label selector construction |
| internal/cmd/cli/dump/dump_test.go | Unit tests for new filtering functions including GitRepo, Bundle, and HelmOp filtering |
| internal/cmd/cli/dump.go | CLI interface changes adding new flags and validation logic for mutually exclusive options |
| e2e/single-cluster/cli_dump_test.go | E2E tests verifying namespace, GitRepo, Bundle, and HelmOp filtering behavior |
| e2e/assets/helmop-template.yaml | Fixed HelmOp template to use correct repo field structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e2e/assets/helmop-template.yaml
Outdated
| repo: {{.Chart}} | ||
| chart: "" |
There was a problem hiding this comment.
helmop-template.yaml sets spec.helm.repo to the OCI URL and forces spec.helm.chart to an empty string. In the Fleet API, HelmOptions.Chart is the field intended for go-getter/OCI chart URLs, while HelmOptions.Repo is for HTTPS helm repos (used together with a chart name). This template likely produces invalid HelmOps and may break e2e HelmOp creation. Set spec.helm.chart to {{.Chart}} (and omit/empty repo) for OCI usage.
| repo: {{.Chart}} | |
| chart: "" | |
| repo: "" | |
| chart: {{.Chart}} |
There was a problem hiding this comment.
Not true, the documentation clearly states, that for OCI URLs the repo field is the one to be used. However, using a Chart variable in the repo field is clearly confusing.
| gzr, err := gzip.NewReader(f) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| tr := tar.NewReader(gzr) | ||
|
|
There was a problem hiding this comment.
extractResourcesFromArchive() creates a gzip reader but never closes it. Add a defer gzr.Close() after gzip.NewReader to avoid leaking resources during the e2e run (especially when many tests parse dumps).
| tgz, err := os.Create(path) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create %s: %w", path, err) | ||
| } | ||
|
|
||
| gz := gzip.NewWriter(tgz) | ||
| w := tar.NewWriter(gz) | ||
|
|
There was a problem hiding this comment.
CreateWithClients creates the output file with os.Create but never closes it. Even though tar/gzip writers are closed, the underlying file descriptor can remain open (and buffered data may not be fully flushed on some platforms). Add a deferred close for the file (and consider deferring tar/gzip closes too, so early returns don’t leak resources).
| case opt.Bundle != "": | ||
| cfg.bundleNames, err = validateAndGetBundle(ctx, d, opt.Namespace, opt.Bundle) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| logger.Info("Filtering by Bundle", "namespace", opt.Namespace, "bundle", opt.Bundle) | ||
| case opt.GitRepo != "": | ||
| cfg.bundleNames, err = collectBundleNamesByGitRepo(ctx, d, opt.Namespace, opt.GitRepo, opt.FetchLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to collect bundle names for gitrepo %q: %w", opt.GitRepo, err) | ||
| } | ||
| logger.Info("Filtering by GitRepo", "namespace", opt.Namespace, "gitrepo", opt.GitRepo, "bundles", len(cfg.bundleNames)) | ||
| case opt.HelmOp != "": | ||
| cfg.bundleNames, err = collectBundleNamesByHelmOp(ctx, d, opt.Namespace, opt.HelmOp, opt.FetchLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to collect bundle names for helmop %q: %w", opt.HelmOp, err) | ||
| } | ||
| logger.Info("Filtering by HelmOp", "namespace", opt.Namespace, "helmop", opt.HelmOp, "bundles", len(cfg.bundleNames)) | ||
| default: | ||
| cfg.bundleNames, err = collectBundleNames(ctx, d, opt.Namespace, opt.FetchLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to collect bundle names: %w", err) | ||
| } | ||
| logger.Info("Filtering by namespace", "namespace", opt.Namespace, "bundles", len(cfg.bundleNames)) | ||
| } | ||
|
|
||
| // Collect content IDs if content options are enabled | ||
| if (opt.WithContent || opt.WithContentMetadata) && len(cfg.bundleNames) > 0 { | ||
| cfg.contentIDs, err = collectContentIDs(ctx, d, opt.Namespace, cfg.bundleNames, opt.FetchLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to collect content IDs: %w", err) | ||
| } | ||
| logger.Info("Collected content IDs from bundles", "count", len(cfg.contentIDs)) | ||
| } | ||
|
|
||
| // Collect secret names if secret options are enabled | ||
| if (opt.WithSecrets || opt.WithSecretsMetadata) && len(cfg.bundleNames) > 0 { | ||
| cfg.secretNames, err = collectSecretNames(ctx, d, logger, opt.Namespace, cfg.bundleNames, opt.FetchLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to collect secret names: %w", err) | ||
| } | ||
| logger.Info("Collected secret names from GitRepos/Bundles", "count", len(cfg.secretNames)) | ||
| } | ||
|
|
There was a problem hiding this comment.
When filtering by GitRepo/HelmOp, an empty bundleNames result currently falls through as “no bundle-name restriction”. That causes downstream logic to become overly broad: addBundleDeployments() will match all bundledeployments in the namespace, and WithContent/WithSecrets can end up dumping all Contents (contentIDs stays nil) / all Secrets (secretNames stays nil). Treat “no bundles found” as an empty result set for dependent resources (or return an error if the GitRepo/HelmOp doesn’t exist), and ensure you pass empty (non-nil) slices and/or short-circuit adding BDs/Contents/Secrets in this case.
internal/cmd/cli/dump.go
Outdated
| if d.AllNamespaces && d.Namespace != "fleet-local" { | ||
| // Check if namespace was explicitly set to something other than default | ||
| if cmd.Flags().Changed("namespace") { | ||
| return fmt.Errorf("--namespace and --all-namespaces are mutually exclusive") | ||
| } | ||
| } | ||
| if d.Gitrepo != "" && d.Bundle != "" { | ||
| return fmt.Errorf("--bundle and --gitrepo are mutually exclusive") | ||
| } | ||
| if d.Gitrepo != "" && !cmd.Flags().Changed("namespace") { | ||
| return fmt.Errorf("--gitrepo requires --namespace to be explicitly specified") | ||
| } | ||
| if d.Bundle != "" && !cmd.Flags().Changed("namespace") { | ||
| return fmt.Errorf("--bundle requires --namespace to be explicitly specified") | ||
| } | ||
| if d.Helmop != "" && d.Gitrepo != "" { | ||
| return fmt.Errorf("--helmop and --gitrepo are mutually exclusive") | ||
| } | ||
| if d.Helmop != "" && d.Bundle != "" { | ||
| return fmt.Errorf("--helmop and --bundle are mutually exclusive") | ||
| } | ||
| if d.Helmop != "" && !cmd.Flags().Changed("namespace") { | ||
| return fmt.Errorf("--helmop requires --namespace to be explicitly specified") |
There was a problem hiding this comment.
The current validation allows --all-namespaces with an explicitly provided --namespace=fleet-local (and also allows --all-namespaces together with --gitrepo/--bundle/--helmop). In that case the code passes Namespace="" to dump.Options, so the resource filter is effectively ignored and the dump becomes unfiltered. Consider making --all-namespaces mutually exclusive with any explicit --namespace and with --gitrepo/--bundle/--helmop to avoid surprising behavior.
| for i, name := range names { | ||
| if name != tt.expectedNames[i] { | ||
| t.Errorf("expected bundle name %q at index %d, got %q", tt.expectedNames[i], i, name) |
There was a problem hiding this comment.
Test_collectBundleNamesByGitRepo asserts the exact order of returned bundle names. Kubernetes List ordering isn’t guaranteed, and the implementation doesn’t promise sorting; this can make the test brittle if the client implementation changes. Consider sorting both slices before comparison or asserting as a set (e.g., ElementsMatch).
| for i, name := range names { | |
| if name != tt.expectedNames[i] { | |
| t.Errorf("expected bundle name %q at index %d, got %q", tt.expectedNames[i], i, name) | |
| // Compare bundle names ignoring order, since Kubernetes List ordering is not guaranteed. | |
| sortedNames := append([]string(nil), names...) | |
| sortedExpected := append([]string(nil), tt.expectedNames...) | |
| slices.Sort(sortedNames) | |
| slices.Sort(sortedExpected) | |
| for i, name := range sortedNames { | |
| if name != sortedExpected[i] { | |
| t.Errorf("expected bundle name %q at index %d, got %q", sortedExpected[i], i, name) |
There was a problem hiding this comment.
I don't think the fake client would do that but I implemented it anyway.
internal/cmd/cli/dump/dump.go
Outdated
| func addBundleDeployments(ctx context.Context, d dynamic.Interface, logger logr.Logger, w *tar.Writer, bundleNames []string, opt Options) error { | ||
| // When filtering by namespace, use label selector for bundle-namespace | ||
| if !opt.AllNamespaces && opt.Namespace != "" { | ||
| selector, err := buildBundleNameSelector(opt.Namespace, bundleNames) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build bundle name selector: %w", err) | ||
| } | ||
| return addObjectsWithLabelSelector(ctx, d, logger, "bundledeployments", w, selector, opt.FetchLimit) | ||
| } | ||
| return addObjectsToArchive(ctx, d, logger, "bundledeployments", w, opt) |
There was a problem hiding this comment.
addBundleDeployments(): when namespace filtering is on and bundleNames is empty, buildBundleNameSelector() produces a selector that matches all bundledeployments in the namespace (bundle-namespace only). That’s correct for pure namespace filtering, but it breaks GitRepo/HelmOp filtering when bundleNames is empty (or unexpectedly empty). Consider guarding: if (opt.GitRepo/opt.HelmOp/opt.Bundle is set) and len(bundleNames)==0, return nil (or error) instead of selecting everything.
| matchingLabels := client.MatchingLabels{ | ||
| "app": appLabel, | ||
| shardKey: shardValue, | ||
| } | ||
| var pods corev1.PodList | ||
| for { | ||
| opts := []client.ListOption{ | ||
| client.InNamespace(svc.Namespace), | ||
| matchingLabels, | ||
| client.Limit(fetchLimit), | ||
| client.Limit(opt.FetchLimit), | ||
| client.Continue(pods.Continue), |
There was a problem hiding this comment.
createDialer(): matchingLabels includes shardKey even when shardKey is empty, which can produce an invalid label selector and make the pod List call fail for non-sharded services. Build matchingLabels conditionally (always include "app", and only include the shard label if shardKey != "").
internal/cmd/cli/dump/dump.go
Outdated
| // addObjectsWithNameFilter fetches resources from a namespace and filters by resource names | ||
| func addObjectsWithNameFilter(ctx context.Context, d dynamic.Interface, logger logr.Logger, resource string, w *tar.Writer, names []string, opt Options) error { | ||
| if len(names) == 0 { | ||
| // No names to filter, don't add any resources | ||
| return nil | ||
| } | ||
|
|
||
| // Create a map for efficient name lookup | ||
| nameMap := make(map[string]bool, len(names)) | ||
| for _, name := range names { | ||
| nameMap[name] = true | ||
| } | ||
|
|
||
| rID := schema.GroupVersionResource{ | ||
| Group: "fleet.cattle.io", | ||
| Version: "v1alpha1", | ||
| Resource: resource, | ||
| } | ||
|
|
||
| logger.V(1).Info("Fetching with name filter...", "resource", rID.String(), "names", len(names)) | ||
|
|
||
| lo := metav1.ListOptions{Limit: opt.FetchLimit} | ||
| for { | ||
| var list *unstructured.UnstructuredList | ||
| var err error | ||
|
|
||
| if opt.Namespace != "" && !opt.AllNamespaces { | ||
| list, err = d.Resource(rID).Namespace(opt.Namespace).List(ctx, lo) | ||
| } else { | ||
| list, err = d.Resource(rID).List(ctx, lo) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to list %s: %w", resource, err) | ||
| } |
There was a problem hiding this comment.
addObjectsWithNameFilter() lists all objects in the (possibly large) namespace and then filters client-side by name. For the common case where names is small (e.g. a single GitRepo/HelmOp), it would be much more efficient to fetch each object by name with Get (and write it if found), avoiding a full List and pagination.
| // Apply namespace filtering when opt.Namespace is set and not in all-namespaces mode | ||
| if opt.Namespace != "" && !opt.AllNamespaces { | ||
| list, err = dynamic.Resource(rID).Namespace(opt.Namespace).List(ctx, lo) | ||
| } else { | ||
| list, err = dynamic.Resource(rID).List(ctx, lo) | ||
| } |
There was a problem hiding this comment.
The PR description says that when filtering is enabled, resources from system namespaces are always included. However, addObjectsToArchive() applies Namespace(opt.Namespace) to most resources when opt.Namespace is set, which excludes system-namespace instances for those resource types (events/secrets are handled separately via getNamespaces). Either extend the dump logic to also include system namespaces for these resources, or update the behavior/documentation to match.
There was a problem hiding this comment.
Clarified that only secrets and events from system namespaces are included (as they are in the unfiltered dump)
internal/cmd/cli/dump/dump.go
Outdated
|
|
||
| lo := metav1.ListOptions{Limit: fetchLimit} | ||
| for { | ||
| list, err := d.Resource(gitRepoRID).Namespace(namespace).List(ctx, lo) |
There was a problem hiding this comment.
Question: Is this function is ignoring the --gitrepo filter?
Asking because it lists all the existing GitRepos in the given namespace, isn't it?
internal/cmd/cli/dump/dump.go
Outdated
| un, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&item) | ||
| if err != nil { | ||
| logger.Error( | ||
| fmt.Errorf("resource %v", item), | ||
| "Skipping resource listed as gitrepo but with incompatible format; this should not happen", | ||
| ) | ||
| continue | ||
| } | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(un, &gitRepo); err != nil { |
There was a problem hiding this comment.
I think list, err := d.Resource(gitRepoRID).Namespace(namespace).List(ctx, lo) returns a *unstructured.UnstructuredList so items in the list would be already Unstructured.
I think you can just call directly untime.DefaultUnstructuredConverter.FromUnstructured and skip the
un, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&item) call
There was a problem hiding this comment.
Nice, unstructured.Unstructed has an Object field that returns map[string]interface{} which I can use in ToUnstructured!
| secretNames []string | ||
| namespace string | ||
| useFiltering bool | ||
| } |
There was a problem hiding this comment.
nit (maybe too pedantic so feel free to ignore): filterConfig seems to mix filter parameters (bundleNames, namespace) with contents collected (contentIDs and secretNames).
Wouldn't it be better to split? Or maybe change the name of the struct.
There was a problem hiding this comment.
It was not too pedantic. While bundleNames is collected, namespace was not required to be in filterConfig. Thanks!
Refers to #4419
This PR adds the following CLI arguments to the Fleet CLI
dumpcommand:--all-namespaces(short:-A)--bundle string--helmop string--gitrepo string--bundle string,--helmop string,--gitrepo stringare mutually exclusive.Breaking Change
--namespacehas not been added, as it was defined before. But it was unused, and now it is being used. Since the default for--namespaceisfleet-local, that is the namespace that afleet dumpwithout--namespaceargument will dump, namely resources from thefleet-localnamespace only. This is a breaking change and it has been introduced because it is consistent with how other commands for the fleetcli work and also how kubectl works and because the user needs to specify a namespace to point to a specific GitRepo, Bundle or HelmOp resource. Dumping all namespaces explicitly requires the--all-namespaces(or-A) argument.Restrictions
Bundlenamespacemappings are included in the archive but not resolved. This means that clusters they point to are not (yet) in the archive when filtering for specific a specific namespace or resource.
Additional Information
When filtering is enabled at any level, secrets and events from system namespaces are always included.
Checklist
fleet-docs repository.