Copilot commented on code in PR #11308:
URL: https://github.com/apache/gravitino/pull/11308#discussion_r3425350246
##########
dev/charts/gravitino/values.yaml:
##########
@@ -504,6 +504,10 @@ extraExposePorts:
ingress:
enabled: false
+ # Resource type to render when ingress is enabled.
+ # "ingress" renders a networking.k8s.io/v1 Ingress (default, backward
compatible).
+ # "gateway" renders a Gateway API gateway.networking.k8s.io HTTPRoute
instead.
+ type: ingress
Review Comment:
The new comment says ingress.type="ingress" renders a networking.k8s.io/v1
Ingress, but this chart conditionally renders networking.k8s.io/v1beta1 or
extensions/v1beta1 on older Kubernetes versions (see templates/ingress.yaml).
Please adjust the values.yaml comment to avoid implying v1-only behavior.
##########
dev/charts/gravitino-iceberg-rest-server/values.yaml:
##########
@@ -323,6 +323,10 @@ extraVolumeMounts:
ingress:
enabled: false
+ # Resource type to render when ingress is enabled.
+ # "ingress" renders a networking.k8s.io/v1 Ingress (default, backward
compatible).
+ # "gateway" renders a Gateway API gateway.networking.k8s.io HTTPRoute
instead.
+ type: ingress
Review Comment:
The new comment says ingress.type="ingress" renders a networking.k8s.io/v1
Ingress, but this chart conditionally renders networking.k8s.io/v1beta1 or
extensions/v1beta1 on older Kubernetes versions (see templates/ingress.yaml).
Please adjust the values.yaml comment to avoid implying v1-only behavior.
##########
dev/charts/gravitino-lance-rest-server/values.yaml:
##########
@@ -140,6 +140,10 @@ readinessProbe:
ingress:
enabled: false
+ # Resource type to render when ingress is enabled.
+ # "ingress" renders a networking.k8s.io/v1 Ingress (default, backward
compatible).
+ # "gateway" renders a Gateway API gateway.networking.k8s.io HTTPRoute
instead.
+ type: ingress
Review Comment:
The new comment says ingress.type="ingress" renders a networking.k8s.io/v1
Ingress, but this chart conditionally renders networking.k8s.io/v1beta1 or
extensions/v1beta1 on older Kubernetes versions (see templates/ingress.yaml).
Please adjust the values.yaml comment to avoid implying v1-only behavior.
##########
dev/charts/gravitino/templates/httproute.yaml:
##########
@@ -0,0 +1,65 @@
+{{- /*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */}}
+
+{{- if and .Values.ingress.enabled (eq (.Values.ingress.type | default
"ingress") "gateway") -}}
+{{- $fullName := include "gravitino.fullname" . -}}
+{{- $svcPort := .Values.service.port -}}
+{{- $gateway := .Values.ingress.gateway | default dict -}}
+apiVersion: {{ $gateway.apiVersion | default "gateway.networking.k8s.io/v1" }}
+kind: HTTPRoute
+metadata:
+ name: {{ $fullName }}
+ labels:
+ {{- include "gravitino.labels" . | nindent 4 }}
+ {{- with .Values.ingress.annotations }}
+ annotations:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ {{- with $gateway.parentRefs }}
+ parentRefs:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+ {{- if .Values.ingress.hosts }}
+ hostnames:
+ {{- range .Values.ingress.hosts }}
+ {{- if .host }}
+ - {{ .host | quote }}
+ {{- end }}
+ {{- end }}
+ {{- end }}
+ rules:
+ {{- range .Values.ingress.hosts }}
+ {{- range .paths }}
+ {{- $pathType := .pathType | default "Prefix" }}
+ {{- if or (eq $pathType "Prefix") (eq $pathType "PathPrefix") (eq
$pathType "ImplementationSpecific") }}
+ {{- $pathType = "PathPrefix" }}
+ {{- else if ne $pathType "Exact" }}
+ {{- fail (printf "ingress.hosts[].paths[].pathType %q is not supported
when ingress.type is \"gateway\"; use Exact, Prefix, or ImplementationSpecific"
$pathType) }}
+ {{- end }}
Review Comment:
In Gateway mode, this template merges all configured hosts into
spec.hostnames, but then emits rules for every path across all hosts. In
HTTPRoute, hostnames apply to the entire resource (not per-rule), so this
changes the Ingress semantics: paths configured under one host will also match
for the other hostnames. To preserve behavior, render one HTTPRoute per host
entry (separate YAML documents) or explicitly fail/validate when multiple hosts
with differing paths are configured.
##########
dev/charts/gravitino-iceberg-rest-server/templates/httproute.yaml:
##########
@@ -0,0 +1,65 @@
+{{- /*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */}}
+
+{{- if and .Values.ingress.enabled (eq (.Values.ingress.type | default
"ingress") "gateway") -}}
+{{- $fullName := include "gravitino-iceberg-rest-server.fullname" . -}}
+{{- $svcPort := .Values.service.port -}}
+{{- $gateway := .Values.ingress.gateway | default dict -}}
+apiVersion: {{ $gateway.apiVersion | default "gateway.networking.k8s.io/v1" }}
+kind: HTTPRoute
+metadata:
+ name: {{ $fullName }}
+ labels:
+ {{- include "gravitino-iceberg-rest-server.labels" . | nindent 4 }}
+ {{- with .Values.ingress.annotations }}
+ annotations:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ {{- with $gateway.parentRefs }}
+ parentRefs:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+ {{- if .Values.ingress.hosts }}
+ hostnames:
+ {{- range .Values.ingress.hosts }}
+ {{- if .host }}
+ - {{ .host | quote }}
+ {{- end }}
+ {{- end }}
+ {{- end }}
+ rules:
+ {{- range .Values.ingress.hosts }}
+ {{- range .paths }}
+ {{- $pathType := .pathType | default "Prefix" }}
+ {{- if or (eq $pathType "Prefix") (eq $pathType "PathPrefix") (eq
$pathType "ImplementationSpecific") }}
+ {{- $pathType = "PathPrefix" }}
+ {{- else if ne $pathType "Exact" }}
+ {{- fail (printf "ingress.hosts[].paths[].pathType %q is not supported
when ingress.type is \"gateway\"; use Exact, Prefix, or ImplementationSpecific"
$pathType) }}
+ {{- end }}
Review Comment:
In Gateway mode, this template merges all configured hosts into
spec.hostnames, but then emits rules for every path across all hosts. In
HTTPRoute, hostnames apply to the entire resource (not per-rule), so this
changes the Ingress semantics: paths configured under one host will also match
for the other hostnames. To preserve behavior, render one HTTPRoute per host
entry (separate YAML documents) or explicitly fail/validate when multiple hosts
with differing paths are configured.
##########
dev/charts/gravitino-lance-rest-server/templates/ingress.yaml:
##########
@@ -17,7 +17,7 @@
under the License.
*/}}
-{{- if .Values.ingress.enabled -}}
+{{- if and .Values.ingress.enabled (ne (.Values.ingress.type | default
"ingress") "gateway") -}}
{{- $fullName := include "gravitino-lance-rest-server.fullname" . -}}
{{- $svcPort := .Values.service.port -}}
{{- if and .Values.ingress.className (not (semverCompare ">=1.18-0"
.Capabilities.KubeVersion.GitVersion)) }}
Review Comment:
The ingress template behavior now changes based on ingress.type (Ingress
should not be rendered when type="gateway"), but this chart's helm-unittest
suites don't assert that Ingress is suppressed in gateway mode (unlike the
iceberg chart). Please add a unit test case that sets ingress.enabled=true and
ingress.type=gateway and asserts hasDocuments: 0 for ingress.yaml.
##########
dev/charts/gravitino/templates/ingress.yaml:
##########
@@ -17,7 +17,7 @@
under the License.
*/}}
- {{- if .Values.ingress.enabled -}}
+ {{- if and .Values.ingress.enabled (ne (.Values.ingress.type | default
"ingress") "gateway") -}}
{{- $fullName := include "gravitino.fullname" . -}}
{{- $svcPort := .Values.service.port -}}
Review Comment:
There are stray leading spaces before the opening template actions in this
file (e.g., the top-level `if` and variable declarations). Because these use
`{{- ... -}}` it likely won’t change rendered output, but it’s inconsistent
with the other charts’ templates and makes diffs/noise harder to read.
##########
dev/charts/gravitino/templates/ingress.yaml:
##########
@@ -17,7 +17,7 @@
under the License.
*/}}
- {{- if .Values.ingress.enabled -}}
+ {{- if and .Values.ingress.enabled (ne (.Values.ingress.type | default
"ingress") "gateway") -}}
{{- $fullName := include "gravitino.fullname" . -}}
{{- $svcPort := .Values.service.port -}}
{{- if and .Values.ingress.className (not (semverCompare ">=1.18-0"
.Capabilities.KubeVersion.GitVersion)) }}
Review Comment:
The ingress template behavior now changes based on ingress.type (Ingress
should not be rendered when type="gateway"), but this chart's helm-unittest
suites don't assert that Ingress is suppressed in gateway mode (unlike the
iceberg chart). Please add a unit test case that sets ingress.enabled=true and
ingress.type=gateway and asserts hasDocuments: 0 for ingress.yaml.
##########
dev/charts/gravitino-lance-rest-server/templates/httproute.yaml:
##########
@@ -0,0 +1,65 @@
+{{- /*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */}}
+
+{{- if and .Values.ingress.enabled (eq (.Values.ingress.type | default
"ingress") "gateway") -}}
+{{- $fullName := include "gravitino-lance-rest-server.fullname" . -}}
+{{- $svcPort := .Values.service.port -}}
+{{- $gateway := .Values.ingress.gateway | default dict -}}
+apiVersion: {{ $gateway.apiVersion | default "gateway.networking.k8s.io/v1" }}
+kind: HTTPRoute
+metadata:
+ name: {{ $fullName }}
+ labels:
+ {{- include "gravitino-lance-rest-server.labels" . | nindent 4 }}
+ {{- with .Values.ingress.annotations }}
+ annotations:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+spec:
+ {{- with $gateway.parentRefs }}
+ parentRefs:
+ {{- toYaml . | nindent 4 }}
+ {{- end }}
+ {{- if .Values.ingress.hosts }}
+ hostnames:
+ {{- range .Values.ingress.hosts }}
+ {{- if .host }}
+ - {{ .host | quote }}
+ {{- end }}
+ {{- end }}
+ {{- end }}
+ rules:
+ {{- range .Values.ingress.hosts }}
+ {{- range .paths }}
+ {{- $pathType := .pathType | default "Prefix" }}
+ {{- if or (eq $pathType "Prefix") (eq $pathType "PathPrefix") (eq
$pathType "ImplementationSpecific") }}
+ {{- $pathType = "PathPrefix" }}
+ {{- else if ne $pathType "Exact" }}
+ {{- fail (printf "ingress.hosts[].paths[].pathType %q is not supported
when ingress.type is \"gateway\"; use Exact, Prefix, or ImplementationSpecific"
$pathType) }}
+ {{- end }}
Review Comment:
In Gateway mode, this template merges all configured hosts into
spec.hostnames, but then emits rules for every path across all hosts. In
HTTPRoute, hostnames apply to the entire resource (not per-rule), so this
changes the Ingress semantics: paths configured under one host will also match
for the other hostnames. To preserve behavior, render one HTTPRoute per host
entry (separate YAML documents) or explicitly fail/validate when multiple hosts
with differing paths are configured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
