Add dump and diagnose command - #121
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
- Coverage 54.57% 0.00% -54.58%
==========================================
Files 17 6 -11
Lines 885 685 -200
==========================================
- Hits 483 0 -483
- Misses 330 685 +355
+ Partials 72 0 -72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fd08916 to
3f31098
Compare
|
Add some doc? |
Boomatang
left a comment
Boomatang
left a comment
There was a problem hiding this comment.
There is some changes I would like to see.
- flag validation
- namespace required explicitly, not defaulting to all, if anything the default should be from the kubeconfig. Like what the kubectl does.
- Created directories only when ready to write data to location.
| if err := os.MkdirAll(dumpOutputDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create output directory: %w", err) |
There was a problem hiding this comment.
In dumpResourceType you call os.MkdirAll for the resource. The path used is for build from the dumpOutputDir and the resource name. Why do we need to make this call here? There is a number of possible errors that can happen after this which could cause blank directories to be created.
There was a problem hiding this comment.
We don't need to make it here. I'll refactor the code to move the directory creation to just before the file write, ensuring we only create directories when we actually have data to write.
|
|
||
| cmd.Flags().StringVarP(&dumpNamespace, "namespace", "n", "", "Namespace to dump resources from (default: all namespaces)") | ||
| cmd.Flags().BoolVarP(&dumpAllNamespaces, "all-namespaces", "A", false, "Dump resources from all namespaces") | ||
| cmd.Flags().StringVarP(&dumpOutputDir, "output", "o", "", "Output directory (default: ./kuadrant-dump-<timestamp>)") |
There was a problem hiding this comment.
On allowing the user to set the output directory. What happens if the directory is not empty? Are we overriding existing data in that directory?
There was a problem hiding this comment.
Yep, it's overwriting the existing data in that directory. Ill add a --force flag and check if the directory is non-empty before proceeding
| listObj client.ObjectList, | ||
| itemsFunc func(client.ObjectList) []runtime.Object, |
There was a problem hiding this comment.
Why not pass in the struct is being looped over?
There was a problem hiding this comment.
I'll refactor the code to use a generic approach.
| obj.SetResourceVersion("") | ||
| obj.SetUID("") | ||
| obj.SetGeneration(0) |
There was a problem hiding this comment.
Why are you clearing these? If I am dumping resources, I would want to be able to check the resource version and generation against other points of information. The UID maybe I don't want that, but by and large I would prefer to have it.
| if dumpFormat == "json" { | ||
| data, err = yaml.Marshal(item) | ||
| // Note: Using yaml.Marshal for both since we'd need proper JSON encoder setup | ||
| // This still produces valid JSON-compatible output | ||
| } else { | ||
| // Default to YAML | ||
| data, err = yaml.Marshal(item) | ||
| } |
There was a problem hiding this comment.
Why have this check if there is no difference?
There was a problem hiding this comment.
some stale code, that shouldve been removed
| cmd.Flags().StringVarP(&dumpNamespace, "namespace", "n", "", "Namespace to dump resources from (default: all namespaces)") | ||
| cmd.Flags().BoolVarP(&dumpAllNamespaces, "all-namespaces", "A", false, "Dump resources from all namespaces") |
There was a problem hiding this comment.
Can there be some input validation on the inputs. The user should not be able to set the namespace and all-namespaces at the same time.
I also don't think the default should be to dump all the namespaces if not set.
There was a problem hiding this comment.
Ill add some validation , however the namesapce will be defaulted to all as we cant assume that all users will install their resources in a specific namespace and dumping the required resources from all namespaces is more convenient.
There was a problem hiding this comment.
If we can't set the default namespace from the kubeconfig, we should force the setting of the field. I understand it maybe more convenient to have it set to all namespaces, but that is not predictable. Take for example, the user runs kubectl get kuadrant, what output would you expect.
There was a problem hiding this comment.
I would expect the kuadrant resource from whatever namespace the kubeconfig is currently set to. And that makes sense when you're only retrieving one resource that is in a specific namespace.
When retrieving multiple resources that could be in different namespaces, I think it makes sense to dump resources from all namespaces by default, as this is more than likely what most users will want to do.
@eguzki wdyt ? default to specific namespace or all-namespaces?
There was a problem hiding this comment.
As long as the desired behavior is documented, both approaches LGTM. I guess I am trying to say pick the easiest to implement and make it clear in doc.
| cmd.Flags().StringVarP(&diagnoseNamespace, "namespace", "n", "", "Namespace to diagnose (default: all namespaces)") | ||
| cmd.Flags().BoolVarP(&diagnoseAllNamespaces, "all-namespaces", "A", false, "Diagnose resources from all namespaces") |
There was a problem hiding this comment.
Same comment on validation and default as before.
| // Setup Kubernetes client | ||
| cfg, err := config.GetConfig() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get kubeconfig: %w", err) | ||
| } | ||
|
|
||
| // Register Gateway API and Kuadrant schemes | ||
| if err := gatewayapiv1.AddToScheme(scheme.Scheme); err != nil { | ||
| return fmt.Errorf("failed to add Gateway API to scheme: %w", err) | ||
| } | ||
| if err := kuadrantv1alpha1.AddToScheme(scheme.Scheme); err != nil { | ||
| return fmt.Errorf("failed to add Kuadrant v1alpha1 API to scheme: %w", err) | ||
| } | ||
| if err := kuadrantv1beta1.AddToScheme(scheme.Scheme); err != nil { | ||
| return fmt.Errorf("failed to add Kuadrant v1beta1 API to scheme: %w", err) | ||
| } | ||
| if err := kuadrantv1beta2.AddToScheme(scheme.Scheme); err != nil { | ||
| return fmt.Errorf("failed to add Kuadrant v1beta2 API to scheme: %w", err) | ||
| } | ||
|
|
||
| k8sClient, err := client.New(cfg, client.Options{Scheme: scheme.Scheme}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create Kubernetes client: %w", err) | ||
| } |
There was a problem hiding this comment.
This looks to be the same in the other command. Can this be refactored to be a shared command.
| // Check status conditions | ||
| for _, condition := range policy.Status.Conditions { | ||
| switch condition.Type { | ||
| case "Accepted": | ||
| if string(condition.Status) != string(metav1.ConditionTrue) { | ||
| issues = append(issues, fmt.Sprintf("Not Accepted: %s - %s", condition.Reason, condition.Message)) | ||
| } | ||
| case "Enforced": | ||
| if string(condition.Status) != string(metav1.ConditionTrue) { | ||
| issues = append(issues, fmt.Sprintf("Not Enforced: %s - %s", condition.Reason, condition.Message)) | ||
| report.Summary.UnenforcedRateLimitPolicies++ | ||
| } else { | ||
| isEnforced = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These blocks seem to be repeated in all the diagnose functions, can it be refactored to be more general?
| // Print Gateway issues | ||
| if len(report.GatewayIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("GATEWAY ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.GatewayIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print HTTPRoute issues | ||
| if len(report.HTTPRouteIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("HTTPROUTE ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.HTTPRouteIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print AuthPolicy issues | ||
| if len(report.AuthPolicyIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("AUTHPOLICY ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.AuthPolicyIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print RateLimitPolicy issues | ||
| if len(report.RateLimitPolicyIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("RATELIMITPOLICY ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.RateLimitPolicyIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print DNSPolicy issues | ||
| if len(report.DNSPolicyIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("DNSPOLICY ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.DNSPolicyIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print TLSPolicy issues | ||
| if len(report.TLSPolicyIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("TLSPOLICY ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.TLSPolicyIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| // Print Kuadrant issues | ||
| if len(report.KuadrantIssues) > 0 { | ||
| fmt.Println("\n" + strings.Repeat("-", 80)) | ||
| fmt.Println("KUADRANT ISSUES:") | ||
| fmt.Println(strings.Repeat("-", 80)) | ||
| for _, issue := range report.KuadrantIssues { | ||
| printResourceIssue(issue) | ||
| } | ||
| } | ||
|
|
||
| fmt.Println("\n" + strings.Repeat("=", 80)) | ||
| } |
There was a problem hiding this comment.
Can this be converted in to some loop?
Signed-off-by: Patryk Stefanski <pstefans@redhat.com>
|
@Boomatang fyi I'm putting this PR on hold. I want to chat with Chris and the wider team if it's worth keeping and maintaining this repo, or at the very least removing some commands if we're not planning on supporting them. |
Can you share links to where these conversations are going on? In the dns-operator we are making design for kuadrant-dns plugin based on this repo existing. |
|
It will be raised and discussed at the next kudarant community call |
a7df22c to
e10a5ff
Compare
|
@Boomatang this is ready for review again, I addressed your comments and removed the generate commands as the outcome of the discussion on this weeks community call was that no one had hard opinions/thoughts on keeping those commands in. |
Signed-off-by: Patryk Stefanski <pstefans@redhat.com>
e10a5ff to
113ec0c
Compare
|
Was there any feedback from Chris on the future of those commands? During the community call it was mentioned that you would be speaking with them during the week. |
|
Also can you split this in to two PRs. The adding of one function has nothing to do with the removal of the others. |
|
@Boomatang was not planning on speaking with Chris about this, when you say those commands do you mean the new ones Im adding in or the generate commands Im removing ? |
New commands require the most recent kuadrant api. The generate commands rely on the outdated api, since this PR is moving to use the most up to date api. I think it makes sense to remove the commands in this PR as otherwise there will exist a version in which those commands are broken. |
|
Closing this for now, as it's gotten stale, and with similar results can be achieved with the help of Claude |
4 participants
Add
diagnoseanddumpcommands for Kuadrant troubleshootingOverview
This PR introduces two new diagnostic commands to
kuadrantctlthat help users troubleshoot and investigate issues with their Kuadrant and Gateway API resources.This could also be paired up with a claude / command to help debug/fix the environment.
Use Cases
What's New
1.
kuadrantctl diagnose- Health Check CommandA comprehensive diagnostic tool that analyzes Kuadrant and Gateway API resources to identify configuration issues and unhealthy states.
Features:
Usage:
Exports all Kuadrant and Gateway API resources to files for offline investigation, debugging, or sharing with support teams.
Features:
Usage:
How It Works Under the Hood
Both commands leverage the controller-runtime client to interact with the Kubernetes API:
- Inspects status conditions on each resource (e.g., Programmed, Accepted, Enforced)
- Identifies resources where condition status is not True
- Aggregates issues and generates summary statistics
- Outputs formatted report to stdout
- Retrieves full resource manifests from the cluster
- Strips cluster-specific metadata (UID, resourceVersion, managedFields)
- Marshals to YAML/JSON
- Writes each resource to a separate file organized by type