OCPBUGS-99535: Use public IP on DCM tests#31423
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/severity-important
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-99535, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe router ingress tests now separate router and external execution pods, target the router pod IP, clean up the external pod, update request helpers, and remove the allowlist-allowed case while retaining allowlist-denied coverage. ChangesRouter execution model
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant ExternalExecPod
participant RouterPod
Test->>ExternalExecPod: execute curl or metric request
ExternalExecPod->>RouterPod: connect using router pod IP
RouterPod-->>ExternalExecPod: return response
ExternalExecPod-->>Test: return command output
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-99535, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcmoraisjr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/router/config_manager_ingress.go`:
- Around line 160-166: Update the router pod selection flow before constructing
execPodRef so it polls the selected pod until router.Status.PodIP is non-empty,
refreshing the pod status as needed. Assign execPod only after a valid PodIP is
available, preserving the existing namespace/name and curl --resolve target
behavior.
- Around line 160-166: Separate the router target address from the client IP
used for the allowlist in the execPod setup. Keep execPod.ipAddress for curl’s
--resolve target, and introduce or reuse a distinct value for the observed
proxy/client address when setting haproxy.router.openshift.io/ip_allowlist;
alternatively ensure this allowlist request bypasses the HTTP proxy so the
expected client IP is observed.
🪄 Autofix (Beta)
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: Pro Plus
Run ID: 052e837c-287b-4a95-9449-30ee9e0d6de6
📒 Files selected for processing (1)
test/extended/router/config_manager_ingress.go
f39141f to
ee4efa8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/router/config_manager_ingress.go (1)
387-388: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftKeep positive allowlist coverage.
Deleting the allowed case leaves the suite unable to detect regressions that reject legitimate clients. Restore a positive assertion using a deterministic client/source address (or a proxy-bypassing request path), while retaining the router pod IP only as the request destination.
🤖 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/router/config_manager_ingress.go` around lines 387 - 388, Restore a positive allowlist assertion in the “check route after adding annotation - allowlist denied” test case, using a deterministic client/source address or proxy-bypassing request path that is allowed; keep the router pod IP only as the request destination and preserve the denied-case coverage.
🤖 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.
Nitpick comments:
In `@test/extended/router/config_manager_ingress.go`:
- Around line 387-388: Restore a positive allowlist assertion in the “check
route after adding annotation - allowlist denied” test case, using a
deterministic client/source address or proxy-bypassing request path that is
allowed; keep the router pod IP only as the request destination and preserve the
denied-case coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e6b117-8b83-46f2-85e7-e32d282c1724
📒 Files selected for processing (1)
test/extended/router/config_manager_ingress.go
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
/test ? |
|
Here ya go @jcmoraisjr - I think this confirms your fix: /test e2e-aws-proxy |
ee4efa8 to
9065689
Compare
|
/test e2e-aws-proxy |
|
Scheduling required tests: |
DCM tests make requests to the router instance from the router container itself, and uses the loopback interface to reach the proxy. This is failing on environments that configures Squid Proxy, since Squid runs on another pod, does not use the curl resolution, and does not know the local hostnames created in the test. This update is using the public IP address of the router pod and making requests from an external pod which does not have Squid configured. Also, execPodRef has now external and router pod references: the former is used to make requests, the later is used to inspect internal state of the router. https://redhat.atlassian.net/browse/OCPBUGS-99535
9065689 to
867705e
Compare
|
/test e2e-aws-proxy |
|
/test e2e-metal-ipi-ovn |
|
Scheduling required tests: |
|
Number of flakes, mainly infra related, giving another chance. /retest |
|
@jcmoraisjr: 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. |
3 participants
DCM tests make requests to the router instance from the router container itself, and uses the loopback interface to reach the proxy. This is failing on environments that configures Squid Proxy, since Squid runs on another pod and cannot reach HAProxy using the loopback interface.
This update is using the public IP address of the router pod, so it works from its own container and also when passing through Squid.
https://redhat.atlassian.net/browse/OCPBUGS-99535
Summary by CodeRabbit