Backport PR #2636 to release/v1.7 for Bugfix ingress route settings #2642
Conversation
* feat: correct ingress route Signed-off-by: hlts2 <[email protected]> * fix: helm template parse error Signed-off-by: hlts2 <[email protected]> * fix: bugfix ingress route for filter Signed-off-by: hlts2 <[email protected]> * fix: route order Signed-off-by: hlts2 <[email protected]> * fix: updatetimestamp route Signed-off-by: hlts2 <[email protected]> * fix: route settings Signed-off-by: hlts2 <[email protected]> * fix: server reflection path Signed-off-by: hlts2 <[email protected]> * fix: server reflection handling Signed-off-by: hlts2 <[email protected]> * fix: tweak Signed-off-by: hlts2 <[email protected]> * fix: deleted unnecessary variable Signed-off-by: hlts2 <[email protected]> --------- Signed-off-by: hlts2 <[email protected]> Co-authored-by: Yusuke Kato <[email protected]>
Deploying vald with
|
| Latest commit: |
f023003
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84fe25c8.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-fix-mi-pp9g.vald.pages.dev |
WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Outside diff range and nitpick comments (1)
charts/vald/templates/gateway/ing.yaml (1)
216-279: LGTM: Consistent routing configuration for mirror and LBThe routing configuration for the scenario where mirror and LB are enabled has been updated consistently with the previous segments. The changes provide more specific routing based on the enabled components, which aligns well with the PR objectives.
Points to note:
The use of trailing slashes in paths remains consistent, maintaining a uniform routing pattern across different scenarios.
There are still TODO comments indicating future changes (e.g., lines 229-230 and 266-267). It would be beneficial to create follow-up tasks to implement these changes.
Would you like me to create GitHub issues to track the implementation of the features mentioned in the TODO comments?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- charts/vald/templates/gateway/ing.yaml (2 hunks)
Additional context used
yamllint
charts/vald/templates/gateway/ing.yaml
[error] 16-16: syntax error: expected the node content, but found '-'
(syntax)
Additional comments not posted (6)
charts/vald/templates/gateway/ing.yaml (6)
16-24: LGTM: Enhanced gRPC reflection controlThe introduction of
reflectionEnabled,filterReflectionEnabled, andlbReflectionEnabledvariables provides more granular control over gRPC reflection settings for different gateway components. This change aligns well with the PR objective of enhancing routing capabilities and fixing ingress settings.Tools
yamllint
[error] 16-16: syntax error: expected the node content, but found '-'
(syntax)
149-215: LGTM: Consistent routing configuration for filter and LBThe routing configuration for the scenario where filter and LB are enabled has been updated consistently with the previous segment. The changes provide more specific routing based on the enabled components, which aligns well with the PR objectives.
The use of trailing slashes in paths remains consistent, which is good for maintaining a uniform routing pattern across different scenarios.
280-285: LGTM: Simplified routing for LB-only configurationThe routing configuration for the scenario where only LB is enabled has been simplified to route all traffic to the LB service. This straightforward approach aligns well with the PR objectives for this specific scenario.
287-315: LGTM: Enhanced gRPC reflection routingThe addition of gRPC reflection routes is a valuable enhancement to the system. These routes are conditionally added based on the reflection settings of the filter and LB components, ensuring that reflection is only enabled for the appropriate services.
This change aligns well with the PR objectives of enhancing routing capabilities and provides better support for gRPC clients that rely on server reflection.
16-16: False positive: Syntax is correct for Helm templatesThe static analysis tool reported a syntax error on this line, but this is a false positive. The syntax
{{- $reflectionEnabled := .Values.defaults.server_config.servers.grpc.server.grpc.enable_reflection -}}is correct for Helm template variable declaration.Static analysis tools sometimes struggle with template languages like Helm's. In this case, the syntax is valid and doesn't require any changes.
Tools
yamllint
[error] 16-16: syntax error: expected the node content, but found '-'
(syntax)
Line range hint
1-315: Overall: Significant improvements to gateway routing and reflection settingsThis PR introduces substantial enhancements to the Vald gateway's Ingress configuration:
- More granular control over gRPC reflection settings with new variables.
- Improved routing configurations for various scenarios (mirror, filter, and LB combinations).
- Consistent use of trailing slashes in paths across different routing configurations.
- Addition of gRPC reflection routes with conditional enabling.
These changes align well with the PR objectives of enhancing routing capabilities and fixing ingress settings. The modifications provide better flexibility and control over the gateway's behavior in different scenarios.
Suggestions for follow-up:
- Address the TODOs and comments indicating future changes (e.g., UpdateTimestamp and Flush implementations in mirror).
- Consider creating a task to review and validate the trailing slash usage in paths to ensure it meets the intended API design.
Overall, these changes significantly improve the gateway's routing capabilities and reflect a thoughtful approach to handling various deployment scenarios.
| {{- if and $mirrorIngEnabled $filterIngEnabled $lb.enabled }} | ||
| - path: "/vald.v1.Search/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - path: "/vald.v1.Insert" | ||
| - path: "/vald.v1.Insert/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - path: "/vald.v1.Update" | ||
| # NOTE: Change backend service to mirror after UpdateTimestamp is implemented in mirror. | ||
| - path: "/vald.v1.Update/UpdateTimestamp" | ||
| backend: | ||
| service: | ||
| name: {{ $lb.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }} | ||
| pathType: {{ $lb.ingress.pathType }} | ||
| - path: "/vald.v1.Update/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - path: "/vald.v1.Upsert" | ||
| - path: "/vald.v1.Upsert/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - path: "/vald.v1.Index" | ||
| - path: "/vald.v1.Remove/" | ||
| backend: | ||
| service: | ||
| name: {{ $mirror.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }} | ||
| pathType: {{ $mirror.ingress.pathType }} | ||
| - path: "/vald.v1.Object/Exists" | ||
| backend: | ||
| service: | ||
| name: {{ $lb.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }} | ||
| pathType: {{ $lb.ingress.pathType }} | ||
| - path: "/vald.v1.Object.Exists" | ||
| - path: "/vald.v1.Object/GetTimestamp" | ||
| backend: | ||
| service: | ||
| name: {{ $lb.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }} | ||
| pathType: {{ $lb.ingress.pathType }} | ||
| - path: "/vald.v1.Object.GetTimestamp" | ||
| - path: "/vald.v1.Object/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - path: "/vald.v1.Index/" | ||
| backend: | ||
| service: | ||
| name: {{ $lb.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }} | ||
| pathType: {{ $lb.ingress.pathType }} | ||
| - path: "/vald.v1.Object" | ||
| # NOTE: Change backend service to mirror after Flush is implemented in mirror. | ||
| - path: "/vald.v1.Flush/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| name: {{ $lb.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} | ||
| - backend: | ||
| pathType: {{ $lb.ingress.pathType }} | ||
| - path: "/mirror.v1.Mirror/Register" | ||
| backend: | ||
| service: | ||
| name: {{ $mirror.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }} | ||
| pathType: {{ $mirror.ingress.pathType }} | ||
| - path: "/vald.v1.Filter/" | ||
| backend: | ||
| service: | ||
| name: {{ $filter.name }} | ||
| {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }} | ||
| pathType: {{ $filter.ingress.pathType }} |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Inconsistent Use of Trailing Slashes in Paths
The analysis of the routing configuration reveals inconsistencies in the use of trailing slashes in path definitions. This may lead to unexpected routing behaviors and should be standardized to align with your API design.
Affected Lines:
-
Lines with trailing slashes:
/vald.v1.Search/(Lines: 69, 150, 217)/vald.v1.Insert/(Lines: 75, 156, 223)/vald.v1.Update/(Lines: 88, 162, 236)/vald.v1.Upsert/(Lines: 94, 168, 242)/vald.v1.Remove/(Lines: 100, 174, 248)/vald.v1.Index/(Lines: 124, 180, 260)/vald.v1.Flush/(Lines: 131, 267, 273)/vald.v1.Filter/(Lines: 143, 210, 280)
-
Paths without trailing slashes:
/vald.v1.Update/UpdateTimestamp(Lines: 82, 230)/vald.v1.Object/Exists(Lines: 106, 186)/vald.v1.Object/GetTimestamp(Lines: 112, 192)/vald.v1.Object/(Lines: 118, 198, 254)/mirror.v1.Mirror/Register(Lines: 137, 273)/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo(Line: 288)/grpc.reflection.v1.ServerReflection/ServerReflectionInfo(Line: 302)/(Line: 280)
Recommendations:
- Standardize Trailing Slashes: Decide whether to include or omit trailing slashes in all path definitions to ensure consistent routing behavior.
- Update Affected Paths: Modify the paths to adhere to the chosen standard across all affected lines.
- Review Routing Logic: Ensure that the routing logic correctly interprets the standardized paths to prevent any mismatches or routing issues.
Analysis chain
LGTM: Improved routing configuration
The routing configuration has been significantly enhanced to provide more specific routing based on the enabled components (mirror, filter, and LB). This aligns well with the PR objectives.
However, there are a couple of points to consider:
-
Some routes are marked for future changes (e.g., lines 81-82 and 130-131). It would be beneficial to create follow-up tasks to implement these changes.
-
The use of trailing slashes in paths (e.g., "/vald.v1.Search/") might affect routing behavior. Ensure that this is the intended configuration and is consistent with your API design.
To verify the consistency of trailing slashes in paths, you can run the following command:
This will list all path definitions, allowing you to review the consistency of trailing slash usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency in the use of trailing slashes in paths
grep -n 'path:' charts/vald/templates/gateway/ing.yaml | sed 's/^ *//g'
Length of output: 1587
|
[CHATOPS:HELP] ChatOps commands.
|
Description
Fixed a problem with proper routing if the PATH setting was not made correctly.
The following are generated ingress manifest
when lb are enabled
when mirror and lb are enabled
when mirror and lb and filter are enabled
when filter and lb are enabled
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements