Repository navigation
[dashboard] Show service LB IP - #1524
Conversation
Fix an incorrect JSON path that prevented Service LoadBalancer IPs from rendering in the table view. Signed-off-by: Timofei Larkin <[email protected]>
Summary of ChangesHello Timofei Larkin (@lllamnyp), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue in the dashboard where Service LoadBalancer IPs were not being rendered correctly. The change involves updating a specific JSON path used to fetch these IPs, ensuring that the dashboard now accurately displays the LoadBalancer IP addresses for services, thereby improving the visibility and usability of service information. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughLoadBalancerIP field source for two CustomColumnsOverride entries in a dashboard configuration file was modified from the spec field to the status load balancer ingress IP field. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request corrects the JSON path for retrieving the Service LoadBalancer IP in the dashboard's table view. The change from .spec.loadBalancerIP to .status.loadBalancer.ingress[0].ip is a good fix, as the assigned IP is in the service's status. However, I've noted a potential issue where a load balancer might provide a hostname instead of an IP, which the current path does not account for. I've added a comment with a suggestion to make this more robust.
| createCustomColumnWithJsonPath("Name", ".metadata.name", "S", "service", getColorForType("service"), "/openapi-ui/{2}/{reqsJsonPath[0]['.metadata.namespace']['-']}/factory/kube-service-details/{reqsJsonPath[0]['.metadata.name']['-']}"), | ||
| createStringColumn("ClusterIP", ".spec.clusterIP"), | ||
| createStringColumn("LoadbalancerIP", ".spec.loadBalancerIP"), | ||
| createStringColumn("LoadbalancerIP", ".status.loadBalancer.ingress[0].ip"), |
There was a problem hiding this comment.
This change correctly points to the service status to get the load balancer ingress information. However, a LoadBalancerIngress can have either an ip or a hostname. This JSON path only considers the ip field. If the load balancer provides a hostname (e.g., on AWS), this field will be empty.
Consider using a more robust way to display the ingress point, which could be either an IP or a hostname. Perhaps a custom column component could handle this logic by checking for .ip first, and then falling back to .hostname if ip is not present.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/dashboard/static_refactored.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| createCustomColumnWithJsonPath("Name", ".metadata.name", "S", "service", getColorForType("service"), "/openapi-ui/{2}/{reqsJsonPath[0]['.metadata.namespace']['-']}/factory/kube-service-details/{reqsJsonPath[0]['.metadata.name']['-']}"), | ||
| createStringColumn("ClusterIP", ".spec.clusterIP"), | ||
| createStringColumn("LoadbalancerIP", ".spec.loadBalancerIP"), | ||
| createStringColumn("LoadbalancerIP", ".status.loadBalancer.ingress[0].ip"), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify consistency with line 137.
The change correctly uses .status.loadBalancer.ingress[0].ip instead of the deprecated .spec.loadBalancerIP field. However, line 137 in the "factory-details-v1.services" CustomColumnsOverride still uses .spec.loadBalancerIP. For consistency, both table views should reference the same field.
Run the following script to verify if line 137 should also be updated:
🏁 Script executed:
#!/bin/bash
# Description: Check if line 137 needs the same fix as line 145
# Display the inconsistent usage
echo "=== Line 137 (unchanged) ==="
rg -n "factory-details-v1.services" -A 5 internal/controller/dashboard/static_refactored.go | grep -A 1 "LoadbalancerIP"
echo ""
echo "=== Line 145 (changed) ==="
rg -n "stock-namespace-/v1/services" -A 5 internal/controller/dashboard/static_refactored.go | grep -A 1 "LoadbalancerIP"
echo ""
echo "=== Other LoadBalancer IP usages ==="
rg -n "loadBalancer.ingress\[0\]" internal/controller/dashboard/static_refactored.goLength of output: 1014
Replace .spec.loadBalancerIP with .status.loadBalancer.ingress[0].ip at line 137
- Update
createStringColumn("LoadbalancerIP", ".spec.loadBalancerIP")→
createStringColumn("LoadbalancerIP", ".status.loadBalancer.ingress[0].ip") - (Optional) Add null-safety for
ingress[0]and consider a fallback to
.status.loadBalancer.ingress[0].hostnamefor hostname-only LoadBalancers
🤖 Prompt for AI Agents
In internal/controller/dashboard/static_refactored.go around line 145, replace
the existing LoadbalancerIP column path `.spec.loadBalancerIP` with the status
path `.status.loadBalancer.ingress[0].ip`; update
createStringColumn("LoadbalancerIP", ".spec.loadBalancerIP") to use
".status.loadBalancer.ingress[0].ip", add null-safety checks to avoid panics
when ingress or ingress[0] is missing, and implement a fallback to
`.status.loadBalancer.ingress[0].hostname` when ip is empty or absent so
hostname-only LoadBalancers still show a value.
Timofei Larkin (lllamnyp)
added
the
kind/backport
Fix an incorrect JSON path that prevented Service LoadBalancer IPs from rendering in the table view. (cherry picked from commit 466f0fe) Signed-off-by: Timofei Larkin <[email protected]>
2 participants
Fix an incorrect JSON path that prevented Service LoadBalancer IPs from rendering in the table view.
Summary by CodeRabbit