WIP: Avoid TLS port-forward to stale endpoints on degraded TNF clusters. - #31502
WIP: Avoid TLS port-forward to stale endpoints on degraded TNF clusters.#31502eggfoobar wants to merge 1 commit into
Conversation
When DEGRADED_NODE is set, ForwardPortAndExecute targets a Running pod on a Ready node so TestTLSMinimumVersions/TestTLSDefaults do not follow fenced-node endpoints that still appear Ready. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: ehila <ehila@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Walkthrough
ChangesDegraded-cluster port forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ForwardPortAndExecute
participant portForwardTargetOnReadyNode
participant KubernetesAPI
participant PortForwardRetryFlow
ForwardPortAndExecute->>portForwardTargetOnReadyNode: Resolve pod and target port
portForwardTargetOnReadyNode->>KubernetesAPI: Query Service, Nodes, and Pods
KubernetesAPI-->>portForwardTargetOnReadyNode: Return resolution data
portForwardTargetOnReadyNode-->>ForwardPortAndExecute: Return pod resource and port
ForwardPortAndExecute->>PortForwardRetryFlow: Forward to resolved target
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-degraded |
|
@eggfoobar: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-aggregate periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-degraded 2 |
|
@eggfoobar: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/257e8f40-958b-11f1-95e4-356eb7bec005-0 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended/util/tls.go`:
- Around line 124-131: Update the pod selection loop to require a true PodReady
condition in addition to PodRunning before returning the pod and targetPort. Use
the pod’s status conditions to reject non-ready pods, ensuring the retry loop
does not repeatedly select the first non-ready pod.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 32fecd0b-6097-42e7-9198-607254a77718
📒 Files selected for processing (1)
test/extended/util/tls.go
| for _, pod := range pods.Items { | ||
| if pod.DeletionTimestamp != nil || pod.Status.Phase != corev1.PodRunning { | ||
| continue | ||
| } | ||
| if _, ok := readyNodes[pod.Spec.NodeName]; !ok { | ||
| continue | ||
| } | ||
| return "pod/" + pod.Name, targetPort, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/extended/util/tls.go --items all
rg -n -C 4 --type go 'portForwardTargetOnReadyNode|PodReady|NodeReady|Status\.Conditions' test/extendedRepository: openshift/origin
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n test/extended/util/tls.go | sed -n '35,170p'
printf '\nForwardPortAndExecute call sites:\n'
rg -n -C 5 --type go 'ForwardPortAndExecute\(' test/extended
printf '\nRetry and port-forward helpers:\n'
rg -n -C 6 --type go 'Eventually|Consistently|Retry|port-forward|PortForward' test/extended/util/tls.goRepository: openshift/origin
Length of output: 10354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
source = Path("test/extended/util/tls.go").read_text()
selection = source[source.index("func portForwardTargetOnReadyNode"):source.index("func servicePortToTargetPort")]
retry = source[source.index("var err error"):source.index("return err", source.index("var err error"))]
assert "pod.Status.Phase != corev1.PodRunning" in selection
assert "PodReady" not in selection
assert 'resource, port = podResource, podPort' in source
assert retry.count("resource") == 1, retry
pods = [
{"name": "not-ready", "phase": "Running", "node": "ready-node", "ready": False},
{"name": "ready", "phase": "Running", "node": "ready-node", "ready": True},
]
ready_nodes = {"ready-node"}
selected = next(
pod["name"]
for pod in pods
if pod["phase"] == "Running"
and pod["node"] in ready_nodes
)
targets = [selected for _ in range(3)]
print({
"running_condition_present": True,
"pod_ready_condition_present": "PodReady" in selection,
"selected_pod": selected,
"retry_targets": targets,
})
PYRepository: openshift/origin
Length of output: 315
Select a Ready pod before port-forwarding.
PodRunning does not imply PodReady. Because target selection occurs before the retry loop, all three retries can use the first non-ready pod. Check the PodReady condition before returning the pod.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended/util/tls.go` around lines 124 - 131, Update the pod selection
loop to require a true PodReady condition in addition to PodRunning before
returning the pod and targetPort. Use the pod’s status conditions to reject
non-ready pods, ensuring the retry loop does not repeatedly select the first
non-ready pod.
|
Scheduling required tests: |
|
@eggfoobar: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
When DEGRADED_NODE is set, ForwardPortAndExecute targets a Running pod on a Ready node so TestTLSMinimumVersions/TestTLSDefaults do not follow fenced-node endpoints that still appear Ready.
Summary by CodeRabbit