Bugfix ingress route settings #2636
Conversation
Signed-off-by: hlts2 <[email protected]>
|
[CHATOPS:HELP] ChatOps commands.
|
WalkthroughWalkthroughThe pull request includes extensive modifications to the ingress configuration in a Kubernetes YAML file. Key changes involve adding trailing slashes to existing service paths and introducing new paths such as Changes
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}
Signed-off-by: hlts2 <[email protected]>
Deploying vald with
|
| Latest commit: |
4031712
|
| Status: | ⚡️ Build in progress... |
|
Note The format check CI is failing, but this CI will be fixed other PR |
hlts2
requested review from
kpango,
a team and
kmrmt
and removed request for
a team
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/fix%2Fmirror%2Fingress/c4f6e32cee621ff1b0b1a671cdfca55dbd9668fe |
Signed-off-by: hlts2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- charts/vald/templates/gateway/ing.yaml (1 hunks)
Additional comments not posted (8)
charts/vald/templates/gateway/ing.yaml (8)
65-66: LGTM!The addition of the trailing slash to the path and the conditions for setting the backend service are correct.
72-78: LGTM!The addition of the trailing slash to the path and the conditions for setting the backend service are correct.
97-102: LGTM!The addition of the trailing slash to the path and the conditions for setting the backend service are correct.
103-108: LGTM!The addition of the trailing slash to the path and the conditions for setting the backend service are correct.
140-145: LGTM!The addition of the trailing slash to the path and the conditions for setting the backend service are correct.
147-206: LGTM!The addition of trailing slashes to the paths and the conditions for setting the backend services are correct. The paths and their corresponding backend services are logically organized.
277-283: LGTM!The catch-all path "/" and the condition for setting the backend service are correct.
284-297: LGTM!The path "/grpc.reflection.v1.ServerReflection/" and the dynamic backend service assignment based on
$filterIngEnabledare correct.
Signed-off-by: hlts2 <[email protected]>
Signed-off-by: hlts2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
charts/vald/templates/gateway/ing.yaml (1)
284-311: LGTM: Addition of gRPC reflection pathsThe addition of paths for gRPC reflection services is beneficial for service discovery and debugging. The conditional backend service assignment based on whether the filter is enabled is correct.
Consider refactoring to reduce duplication
The configuration for the two versions of the gRPC reflection service (v1alpha and v1) is identical. Consider refactoring to reduce duplication, potentially using a loop or a template function.
Here's a potential refactor using a Helm named template:
{{- define "grpcReflectionPath" }} - path: "/grpc.reflection.{{ . }}.ServerReflection/ServerReflectionInfo" {{- if $filterIngEnabled }} backend: service: name: {{ $filter.name }} {{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 6 }} pathType: {{ $filter.ingress.pathType }} {{- else }} backend: service: name: {{ $lb.name }} {{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 6 }} pathType: {{ $lb.ingress.pathType }} {{- end }} {{- end }} # Usage: {{ include "grpcReflectionPath" "v1alpha" }} {{ include "grpcReflectionPath" "v1" }}This refactor would reduce duplication and make it easier to maintain if changes are needed in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- charts/vald/templates/gateway/ing.yaml (1 hunks)
Additional comments not posted (5)
charts/vald/templates/gateway/ing.yaml (5)
97-120: LGTM: New paths and backend service assignmentsThe addition of new paths ("/vald.v1.Remove/", "/vald.v1.Object/", and "/vald.v1.Flush/") with appropriate backend service assignments is correct and consistent with the overall structure. The use of trailing slashes for all paths maintains consistency.
147-206: LGTM: Consistent path updates and correct backend service assignmentsThe changes in this section maintain consistency with the trailing slash pattern for all paths. The backend service assignments are correct based on the enabled services (filter and lb). The addition of new paths ("/vald.v1.Remove/", "/vald.v1.Flush/", and "/vald.v1.Filter/") is appropriate and consistent with the overall structure.
277-278: LGTM: Correct path for lb-only scenarioThe use of the root path "/" for routing all traffic to the lb service when only lb is enabled is correct and appropriate.
Line range hint
1-311: Overall assessment: Approve with minor suggestionsThe changes to the ingress configuration improve consistency and add support for new services. The use of trailing slashes for paths and conditional backend service assignments based on enabled services are well-implemented.
Key points:
- Trailing slashes added to most paths for consistency.
- New paths added for additional services (Remove, Object, Flush, Filter).
- Conditional backend service assignments based on enabled services.
- gRPC reflection paths added for service discovery and debugging.
Suggestions for improvement:
- Add trailing slash to the UpdateTimestamp path for full consistency.
- Consider refactoring the duplicate gRPC reflection configuration to reduce redundancy.
- Ensure the TODOs for changing backend services (UpdateTimestamp and Flush) are tracked and implemented in the future.
Overall, these changes enhance the routing logic and flexibility of the ingress configuration. The suggestions provided are minor and can be addressed in a follow-up commit if desired.
127-145: LGTM: New paths and backend service assignmentsThe addition of new paths ("/mirror.v1.Mirror/" and "/vald.v1.Filter/") with appropriate backend service assignments is correct and consistent with the overall structure.
Reminder: Update backend service for Flush
The comment on line 127 indicates a future task to change the backend service for Flush. Ensure this is tracked and implemented when the functionality is available in the mirror service.
To verify the current implementation and track future changes, run:
Verification successful
Verification Successful: Paths and TODOs Confirmed
The paths "/vald.v1.Flush/" and "/vald.v1.Update/UpdateTimestamp" are correctly present, and the TODO comments for updating the backend service after implementing Flush and UpdateTimestamp are properly documented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current implementation and track TODO rg --type yaml 'path: "/vald.v1.Flush/' charts/vald/templates/gateway/ing.yaml rg --type yaml '# NOTE: Change backend service to mirror after Flush' charts/vald/templates/gateway/ing.yamlLength of output: 834
Signed-off-by: hlts2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
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 (4)
charts/vald/templates/gateway/ing.yaml (4)
70-106: LGTM: Consistent path formatting and new functionalityThe addition of trailing slashes to paths and the introduction of new paths for Remove, Object, and Filter operations improve consistency and expand functionality.
82-88: Add trailing slash to UpdateTimestamp path for consistencyThe UpdateTimestamp path is inconsistent with the other paths in this block.
Add a trailing slash to the UpdateTimestamp path:
- - path: "/vald.v1.Update/UpdateTimestamp" + - path: "/vald.v1.Update/UpdateTimestamp/"Also, don't forget to address the TODO comment about changing the backend service for UpdateTimestamp once it's implemented in the mirror service.
151-286: LGTM: Consistent changes across different gateway configurationsThe changes in these blocks are consistent with the improvements made in the first block, ensuring proper path formatting and backend service assignments across different gateway configurations.
230-236: Add trailing slash to UpdateTimestamp path for consistencySimilar to the previous block, the UpdateTimestamp path is inconsistent with the other paths in this block.
Add a trailing slash to the UpdateTimestamp path:
- - path: "/vald.v1.Update/UpdateTimestamp" + - path: "/vald.v1.Update/UpdateTimestamp/"Also, don't forget to address the TODO comment about changing the backend service for UpdateTimestamp once it's implemented in the mirror service.
Signed-off-by: hlts2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
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 (9)
charts/vald/templates/gateway/ing.yaml (9)
82-88: Consider adding a trailing slash to the UpdateTimestamp pathFor consistency with other paths, consider adding a trailing slash to the
/vald.v1.Update/UpdateTimestamppath.- - path: "/vald.v1.Update/UpdateTimestamp" + - path: "/vald.v1.Update/UpdateTimestamp/"Also, note the TODO comment about changing the backend service to mirror after UpdateTimestamp is implemented in mirror. Ensure this is tracked for future updates.
89-106: New path configurations addedThe addition of path configurations for
/vald.v1.Update/,/vald.v1.Upsert/, and/vald.v1.Remove/is consistent with the overall changes. Note that the/vald.v1.Remove/path is now directed to the mirror service.These changes improve the routing logic and provide more granular control over service routing.
119-124: New Object path configuration addedA new path configuration for
/vald.v1.Object/has been added and is directed to the filter service. This is consistent with the overall changes to improve routing granularity.This addition provides more specific routing for general Object operations.
131-137: New Flush path configuration addedA new path configuration for
/vald.v1.Flush/has been added and is currently directed to the lb service. This is consistent with the overall changes to improve routing granularity.Note the TODO comment about changing the backend service to mirror after Flush is implemented in mirror. Ensure this is tracked for future updates.
138-149: New Mirror Register and Filter path configurations addedNew path configurations have been added for
/mirror.v1.Mirror/Register(directed to the mirror service) and/vald.v1.Filter/(directed to the filter service). These additions are consistent with the overall changes to improve routing granularity.These new paths provide more specific routing for Mirror and Filter operations, enhancing the flexibility of the ingress configuration.
151-186: New path configurations for filter and lb enabled scenarioNew path configurations have been added for various operations when only filter and lb are enabled. The routing logic directs Remove and Index operations to the lb service, while other operations are directed to the filter service.
This configuration provides a clear routing strategy for scenarios where only filter and lb are enabled, enhancing the flexibility of the ingress configuration.
187-204: Updated Object operations routing for filter and lb enabled scenarioThe routing logic for Object operations has been updated when only filter and lb are enabled. While Exists and GetTimestamp operations are still directed to the lb service, a new general
/vald.v1.Object/path has been added and is directed to the filter service.This configuration provides more granular control over Object operations routing, improving the flexibility of the ingress configuration.
205-217: New Flush and Filter path configurations for filter and lb enabled scenarioNew path configurations have been added for
/vald.v1.Flush/(directed to the lb service) and/vald.v1.Filter/(directed to the filter service) when only filter and lb are enabled. These additions are consistent with the overall changes to improve routing granularity.These new paths provide more specific routing for Flush and Filter operations, enhancing the flexibility of the ingress configuration in this specific scenario.
218-286: New path configurations for mirror and lb enabled scenarioNew path configurations have been added for various operations when only mirror and lb are enabled. The routing logic is as follows:
- Search, Object, Index, and Flush operations are directed to the lb service.
- Insert, Update, Upsert, Remove, and Mirror Register operations are directed to the mirror service.
- UpdateTimestamp is currently directed to the lb service, but there's a note to change this in the future.
This configuration provides a clear routing strategy for scenarios where only mirror and lb are enabled, enhancing the flexibility of the ingress configuration.
Note the TODO comment about changing the backend service for UpdateTimestamp to mirror after it's implemented. Ensure this is tracked for future updates.
Signed-off-by: hlts2 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
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 (5)
charts/vald/templates/gateway/ing.yaml (5)
150-215: LGTM! Conditional configurations look goodThe conditional configurations for different combinations of enabled services provide good flexibility. The consistent use of trailing slashes and appropriate backend service assignments are well implemented.
Line range hint
1-315: Overall, the changes improve routing configuration and flexibilityThe modifications to this Ingress configuration file enhance the routing setup for the Vald project in several ways:
- Consistent use of trailing slashes in paths improves routing robustness.
- Addition of new paths (e.g., Remove, Object, Flush, Filter) expands the API surface.
- Conditional configurations provide flexibility for different deployment scenarios.
- Integration of gRPC reflection improves service discoverability.
To ensure all paths are consistently using trailing slashes, run:
#!/bin/bash # Check for paths without trailing slashes rg --type yaml 'path: ".*[^/]"' charts/vald/templates/gateway/ing.yamlThis script should return no results if all paths are correctly formatted with trailing slashes.
88-148: LGTM! Remember to update the backend service for FlushThe addition of new paths and consistent use of trailing slashes improve the routing configuration.
Don't forget to implement the TODO mentioned in the comment:
# NOTE: Change backend service to mirror after Flush is implemented in mirror.Run this script to track the implementation status:
#!/bin/bash # Check if Flush is implemented in mirror rg --type go 'func.*Flush' $(fd -t f -e go . | grep mirror)
68-87: LGTM! Remember to update the backend service for UpdateTimestampThe addition of trailing slashes to the paths and the split of the Update path improve consistency and flexibility.
Don't forget to implement the TODO mentioned in the comment:
# NOTE: Change backend service to mirror after UpdateTimestamp is implemented in mirror.Run this script to track the implementation status:
217-285: LGTM! Remember to update backend services for UpdateTimestamp and FlushThe conditional configurations for different service combinations are well implemented and consistent.
Don't forget to implement the TODOs mentioned in the comments:
- For UpdateTimestamp:
# NOTE: Change backend service to mirror after UpdateTimestamp is implemented in mirror.
- For Flush:
# NOTE: Change backend service to mirror after Flush is implemented in mirror.Run these scripts to track the implementation status:
* 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]>
* feat: correct ingress route * fix: helm template parse error * fix: bugfix ingress route for filter * fix: route order * fix: updatetimestamp route * fix: route settings * fix: server reflection path * fix: server reflection handling * fix: tweak * fix: deleted unnecessary variable --------- Signed-off-by: hlts2 <[email protected]> Co-authored-by: Hiroto Funakoshi <[email protected]> Co-authored-by: Yusuke Kato <[email protected]>
* 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]>
* 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]>
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
/vald.v1.Remove/,/vald.v1.Object/,/vald.v1.Flush/, and/vald.v1.Filter/.Improvements