Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport PR #2636 to release/v1.7 for Bugfix ingress route settings #2642

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Backport PR #2636 to release/v1.7 for Bugfix ingress route settings #2642

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 162 additions & 31 deletions charts/vald/templates/gateway/ing.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
{{- $filter := .Values.gateway.filter -}}
{{- $filterIngEnabled := and $filter.enabled $filter.ingress.enabled -}}
{{- $mirror := .Values.gateway.mirror -}}
{{- $mirrorIngEnabled := and $mirror.enabled $mirror.ingress.enabled -}}
{{- $lb := .Values.gateway.lb -}}
{{- $lbIngEnabled := and $lb.enabled $lb.ingress.enabled -}}
{{- $gateway := "" -}}
{{- $gatewayName := "" -}}
{{- $reflectionEnabled := .Values.defaults.server_config.servers.grpc.server.grpc.enable_reflection -}}
{{- $filter := .Values.gateway.filter -}}
{{- $filterIngEnabled := and $filter.enabled $filter.ingress.enabled -}}
{{- $filterReflectionEnabled := and $filterIngEnabled (default $reflectionEnabled $filter.server_config.servers.grpc.enable_reflection) -}}
{{- $mirror := .Values.gateway.mirror -}}
{{- $mirrorIngEnabled := and $mirror.enabled $mirror.ingress.enabled -}}
{{- $lb := .Values.gateway.lb -}}
{{- $lbIngEnabled := and $lb.enabled $lb.ingress.enabled -}}
{{- $lbReflectionEnabled := and $lbIngEnabled (default $reflectionEnabled $lb.server_config.servers.grpc.enable_reflection) -}}
{{- $gateway := "" -}}
{{- $gatewayName := "" -}}
{{- if or $filterIngEnabled $mirrorIngEnabled $lbIngEnabled }}
{{- if $filterIngEnabled }}
{{- $gateway = $filter -}}
Expand Down Expand Up @@ -62,125 +65,253 @@ spec:
- host: {{ $gateway.ingress.host }}
http:
paths:
{{- if and $mirrorIngEnabled $filterIngEnabled $lb.enabled }}
- path: "/vald.v1.Search"
{{- 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 }}
Comment on lines +68 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Standardize Trailing Slashes: Decide whether to include or omit trailing slashes in all path definitions to ensure consistent routing behavior.
  2. Update Affected Paths: Modify the paths to adhere to the chosen standard across all affected lines.
  3. 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:

  1. 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.

  2. 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

{{- else if and $filterIngEnabled $lb.enabled }}
- path: "/vald.v1.Index"
- 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/"
backend:
service:
name: {{ $filter.name }}
{{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }}
pathType: {{ $filter.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/"
backend:
service:
name: {{ $filter.name }}
{{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }}
pathType: {{ $filter.ingress.pathType }}
- path: "/vald.v1.Remove/"
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.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/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.Flush/"
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
- backend:
- path: "/vald.v1.Filter/"
backend:
service:
name: {{ $filter.name }}
{{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }}
pathType: {{ $filter.ingress.pathType }}
{{- else if and $mirrorIngEnabled $lb.enabled }}
- path: "/vald.v1.Search"
- path: "/vald.v1.Search/"
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
- path: "/vald.v1.Index"
- path: "/vald.v1.Insert/"
backend:
service:
name: {{ $mirror.name }}
{{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }}
pathType: {{ $mirror.ingress.pathType }}
# 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.Object.Exists"
- path: "/vald.v1.Update/"
backend:
service:
name: {{ $mirror.name }}
{{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }}
pathType: {{ $mirror.ingress.pathType }}
- path: "/vald.v1.Upsert/"
backend:
service:
name: {{ $mirror.name }}
{{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }}
pathType: {{ $mirror.ingress.pathType }}
- 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/"
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.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: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
- backend:
- path: "/mirror.v1.Mirror/Register"
backend:
service:
name: {{ $mirror.name }}
{{- include "vald.ingressPort" (dict "Values" $mirror.ingress) | nindent 12 }}
pathType: {{ $mirror.ingress.pathType }}
{{- else if $lbIngEnabled }}
- backend:
- path: "/"
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
{{- end }}
{{- if or $filterReflectionEnabled $lbReflectionEnabled }}
- path: "/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo"
{{- if $filterReflectionEnabled }}
backend:
service:
name: {{ $filter.name }}
{{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }}
pathType: {{ $filter.ingress.pathType }}
{{- else }}
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
{{- end }}
- path: "/grpc.reflection.v1.ServerReflection/ServerReflectionInfo"
{{- if $filterReflectionEnabled }}
backend:
service:
name: {{ $filter.name }}
{{- include "vald.ingressPort" (dict "Values" $filter.ingress) | nindent 12 }}
pathType: {{ $filter.ingress.pathType }}
{{- else }}
backend:
service:
name: {{ $lb.name }}
{{- include "vald.ingressPort" (dict "Values" $lb.ingress) | nindent 12 }}
pathType: {{ $lb.ingress.pathType }}
{{- end }}
{{- end }}
{{- end }}
Loading