hubcio commented on code in PR #3386:
URL: https://github.com/apache/iggy/pull/3386#discussion_r3333279846
##########
scripts/ci/test-helm.sh:
##########
@@ -266,6 +267,58 @@ validate() {
validate_helm_docs
}
+# PID of the kubectl port-forward process started by get_gateway_base_url.
+# Stored at script scope so smoke() can kill it regardless of exit path.
+HELM_SMOKE_GW_PF_PID=""
+
+get_gateway_base_url() {
+ local svc_selector
+ local svc_name
+ local pf_port=8080
+
+
svc_selector="gateway.envoyproxy.io/owning-gateway-name=${HELM_SMOKE_GATEWAY_NAME},gateway.envoyproxy.io/owning-gateway-namespace=${HELM_SMOKE_GATEWAY_NAMESPACE}"
+
+ svc_name="$(kubectl get svc \
+ --namespace "$HELM_SMOKE_GATEWAY_NAMESPACE" \
+ --selector "$svc_selector" \
+ -o jsonpath='{.items[0].metadata.name}')"
+
+ if [ -z "$svc_name" ]; then
+ echo "Error: could not find Envoy Gateway proxy service" >&2
+ return 1
+ fi
+
+ kubectl port-forward "svc/${svc_name}" "${pf_port}:80" \
+ --namespace "$HELM_SMOKE_GATEWAY_NAMESPACE" \
+ >/dev/null 2>&1 &
+ HELM_SMOKE_GW_PF_PID=$!
Review Comment:
`get_gateway_base_url` is invoked as `gateway_url="$(get_gateway_base_url)"`
at line 441, so it runs in a command-substitution subshell. that means this
`HELM_SMOKE_GW_PF_PID=$!` sets the var only inside the subshell - the parent's
copy stays empty, so `kill "${HELM_SMOKE_GW_PF_PID:-}"` at line 477 runs `kill
""` and does nothing, and the `kubectl port-forward` orphans holding
127.0.0.1:8080. the comment at 270-271 about killing it regardless of exit path
isn't true. on the ephemeral ci runner the orphan dies with the vm so the
workflow still passes, but running this locally as a dev tool leaves a stale
port-forward that breaks the next run's bind on :8080. separately there's no
`trap`, so even with correct scoping the kill only fires on the success path -
a failed curl/test aborts under set -e before reaching line 477. cleanest fix
is to start the port-forward directly in `smoke()` so the pid lands in the
parent, plus one `trap ... EXIT` that kills it and removes the temp values
file.
##########
scripts/ci/setup-helm-smoke-cluster.sh:
##########
@@ -223,13 +122,67 @@ else
fi
kubectl config use-context "$kind_context" >/dev/null
-kubectl apply -f
"https://raw.githubusercontent.com/kubernetes/ingress-nginx/${HELM_SMOKE_INGRESS_NGINX_VERSION}/deploy/static/provider/kind/deploy.yaml";
-kubectl -n ingress-nginx rollout status deployment/ingress-nginx-controller
--timeout="$HELM_SMOKE_INGRESS_NGINX_TIMEOUT"
-kubectl -n ingress-nginx wait \
- --for=condition=ready \
- pod \
- --selector=app.kubernetes.io/component=controller \
- --timeout="$HELM_SMOKE_INGRESS_NGINX_TIMEOUT"
-wait_for_completed_job ingress-nginx-admission-create
-wait_for_completed_job ingress-nginx-admission-patch
-wait_for_ingress_validation
+
+# Install EG first so its bundled GW API CRDs land before the v1.5
safe-upgrade VAP.
+# We then upgrade those CRDs to the pinned version via server-side apply.
+echo "Installing Envoy Gateway ${HELM_SMOKE_ENVOY_GATEWAY_VERSION}..."
+helm upgrade --install eg \
Review Comment:
envoy gateway v1.4.1 is pinned here while the gateway api crds get upgraded
to v1.5.0 (line 138) on a v1.35.0 kind node (line 54). per the envoy gateway
compat matrix, eg v1.4 targets gateway api v1.3.0 and k8s v1.30-v1.33, so all
three are out of the supported matrix (no eg release through v1.6 lists k8s
v1.35 or gw-api v1.5). it probably still reconciles since the core
Gateway/HTTPRoute types are v1 GA and the v1.5 fields are additive, but it's
unsupported, and if the controller can't satisfy a v1.5 default or policy the
Gateway never reaches Programmed and the wait at line 184 burns the full 5m
then fails. worth bumping eg to a release that lists these versions, or pinning
gw-api/kind back to the v1.4 matrix. (the k8s v1.35 pin is carried over from
the old nginx path; only eg + gw-api are new here.)
##########
scripts/ci/setup-helm-smoke-cluster.sh:
##########
@@ -223,13 +122,67 @@ else
fi
kubectl config use-context "$kind_context" >/dev/null
-kubectl apply -f
"https://raw.githubusercontent.com/kubernetes/ingress-nginx/${HELM_SMOKE_INGRESS_NGINX_VERSION}/deploy/static/provider/kind/deploy.yaml";
-kubectl -n ingress-nginx rollout status deployment/ingress-nginx-controller
--timeout="$HELM_SMOKE_INGRESS_NGINX_TIMEOUT"
-kubectl -n ingress-nginx wait \
- --for=condition=ready \
- pod \
- --selector=app.kubernetes.io/component=controller \
- --timeout="$HELM_SMOKE_INGRESS_NGINX_TIMEOUT"
-wait_for_completed_job ingress-nginx-admission-create
-wait_for_completed_job ingress-nginx-admission-patch
-wait_for_ingress_validation
+
+# Install EG first so its bundled GW API CRDs land before the v1.5
safe-upgrade VAP.
+# We then upgrade those CRDs to the pinned version via server-side apply.
+echo "Installing Envoy Gateway ${HELM_SMOKE_ENVOY_GATEWAY_VERSION}..."
+helm upgrade --install eg \
+ oci://docker.io/envoyproxy/gateway-helm \
+ --version "$HELM_SMOKE_ENVOY_GATEWAY_VERSION" \
+ --namespace "$HELM_SMOKE_GATEWAY_NAMESPACE" \
+ --create-namespace \
+ --wait \
+ --timeout "$HELM_SMOKE_GATEWAY_TIMEOUT"
+
+echo "Upgrading Gateway API CRDs to ${HELM_SMOKE_GATEWAY_API_VERSION}..."
+kubectl apply --server-side --force-conflicts \
+ -f
"https://github.com/kubernetes-sigs/gateway-api/releases/download/${HELM_SMOKE_GATEWAY_API_VERSION}/standard-install.yaml";
+
+echo "Creating EnvoyProxy, GatewayClass, and Gateway..."
+kubectl apply -f - <<EOF
+apiVersion: gateway.envoyproxy.io/v1alpha1
+kind: EnvoyProxy
+metadata:
+ name: iggy-smoke-proxy
+ namespace: ${HELM_SMOKE_GATEWAY_NAMESPACE}
+spec:
+ provider:
+ type: Kubernetes
+ kubernetes:
+ envoyService:
+ type: NodePort
Review Comment:
`type: NodePort` here is effectively unused. the new kind config (lines
82-87) has no extraPortMappings, and the smoke test reaches envoy only via
`kubectl port-forward svc/...:80`, which tunnels through the apiserver to the
ClusterIP regardless of service type - the allocated node port is never dialed.
default ClusterIP is enough, so the provider override can be dropped.
##########
scripts/ci/test-helm.sh:
##########
@@ -266,6 +267,58 @@ validate() {
validate_helm_docs
}
+# PID of the kubectl port-forward process started by get_gateway_base_url.
+# Stored at script scope so smoke() can kill it regardless of exit path.
+HELM_SMOKE_GW_PF_PID=""
+
+get_gateway_base_url() {
+ local svc_selector
+ local svc_name
+ local pf_port=8080
+
+
svc_selector="gateway.envoyproxy.io/owning-gateway-name=${HELM_SMOKE_GATEWAY_NAME},gateway.envoyproxy.io/owning-gateway-namespace=${HELM_SMOKE_GATEWAY_NAMESPACE}"
+
+ svc_name="$(kubectl get svc \
+ --namespace "$HELM_SMOKE_GATEWAY_NAMESPACE" \
+ --selector "$svc_selector" \
+ -o jsonpath='{.items[0].metadata.name}')"
+
+ if [ -z "$svc_name" ]; then
+ echo "Error: could not find Envoy Gateway proxy service" >&2
+ return 1
+ fi
+
+ kubectl port-forward "svc/${svc_name}" "${pf_port}:80" \
+ --namespace "$HELM_SMOKE_GATEWAY_NAMESPACE" \
+ >/dev/null 2>&1 &
+ HELM_SMOKE_GW_PF_PID=$!
+
+ # Wait up to 15 s for the tunnel to accept connections.
Review Comment:
the comment says up to 15 s, but the loop is 15 iterations of curl
`--max-time 2` plus `sleep 1`, so worst case is closer to 45s if the connection
hangs to max-time each iter (~30s on the connect-timeout path). just the
comment is off, the loop itself is fine.
##########
scripts/ci/test-helm.sh:
##########
@@ -266,6 +267,58 @@ validate() {
validate_helm_docs
}
+# PID of the kubectl port-forward process started by get_gateway_base_url.
+# Stored at script scope so smoke() can kill it regardless of exit path.
+HELM_SMOKE_GW_PF_PID=""
+
+get_gateway_base_url() {
+ local svc_selector
+ local svc_name
+ local pf_port=8080
+
+
svc_selector="gateway.envoyproxy.io/owning-gateway-name=${HELM_SMOKE_GATEWAY_NAME},gateway.envoyproxy.io/owning-gateway-namespace=${HELM_SMOKE_GATEWAY_NAMESPACE}"
+
+ svc_name="$(kubectl get svc \
Review Comment:
single-shot lookup with no retry. the Gateway reaching `Programmed` doesn't
guarantee the owning-gateway Service object exists yet, so a transient miss
here fails the run. also on an empty result `kubectl ...
jsonpath='{.items[0]...}'` exits non-zero and prints a raw `array index out of
bounds` to stderr - the `[ -z ]` guard below does still catch it (the
assignment doesn't abort under set -e in this command-sub form), but both the
raw error and the friendly one end up printed. add `2>/dev/null` to the lookup
and wrap it in a short retry.
##########
scripts/ci/test-helm.sh:
##########
@@ -266,6 +267,58 @@ validate() {
validate_helm_docs
}
+# PID of the kubectl port-forward process started by get_gateway_base_url.
+# Stored at script scope so smoke() can kill it regardless of exit path.
+HELM_SMOKE_GW_PF_PID=""
+
+get_gateway_base_url() {
+ local svc_selector
+ local svc_name
+ local pf_port=8080
Review Comment:
`pf_port=8080` is the one knob in this script without a `${VAR:-default}`
override - every other tunable is env-overridable. minor consistency thing; an
env override like `HELM_SMOKE_GATEWAY_PF_PORT` would let someone work around a
busy host loopback.
##########
scripts/ci/test-helm.sh:
##########
@@ -358,16 +396,60 @@ EOF
return "$helm_status"
fi
+ kubectl apply -f - <<EOF
+apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+ name: iggy-server
+ namespace: ${HELM_SMOKE_NAMESPACE}
+spec:
+ parentRefs:
+ - name: ${HELM_SMOKE_GATEWAY_NAME}
+ namespace: ${HELM_SMOKE_GATEWAY_NAMESPACE}
+ hostnames:
+ - "${HELM_SMOKE_SERVER_HOST}"
+ rules:
+ - matches:
+ - path:
+ type: PathPrefix
+ value: /
+ backendRefs:
+ - name: ${HELM_SMOKE_RELEASE}
+ port: 3000
+---
+apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+ name: iggy-ui
+ namespace: ${HELM_SMOKE_NAMESPACE}
+spec:
+ parentRefs:
+ - name: ${HELM_SMOKE_GATEWAY_NAME}
+ namespace: ${HELM_SMOKE_GATEWAY_NAMESPACE}
+ hostnames:
+ - "${HELM_SMOKE_UI_HOST}"
+ rules:
+ - matches:
+ - path:
+ type: PathPrefix
+ value: /
+ backendRefs:
+ - name: ${HELM_SMOKE_RELEASE}-ui
+ port: 3050
+EOF
+
+ gateway_url="$(get_gateway_base_url)"
+
kubectl version --client=true > "$HELM_SMOKE_REPORT_DIR/kubectl-version.txt"
kubectl -n "$HELM_SMOKE_NAMESPACE" rollout status
"deployment/$HELM_SMOKE_RELEASE" --timeout="$HELM_SMOKE_TIMEOUT"
Review Comment:
these two `rollout status` calls are redundant - the `helm upgrade --install
--wait` above already blocks until both deployments are available, and you
return early if helm fails, so by here they're already rolled out and these
return immediately. can drop them (they do still emit a line to the ci log if
that's worth keeping).
--
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]
