USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730
Draft
agullon wants to merge 10 commits into
Draft
USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730agullon wants to merge 10 commits into
agullon wants to merge 10 commits into
Conversation
Migrate 23 tests from openshift-tests-private/test/extended/router/microshift.go into the MicroShift repo as Robot Framework tests: Non-disruptive route tests (router-routes.robot): - OCP-60136: reencrypt route via Ingress with destination CA certificate - OCP-60266: edge and passthrough route creation and connectivity - OCP-60283: HTTP route via oc expose and reencrypt route - OCP-72802: namespace ownership default (InterNamespaceAllowed) config - OCP-73152: router-default service as LoadBalancer with route connectivity - OCP-73202: default listening IPs and ports with route connectivity Disruptive config tests (router-config.robot): - OCP-73203: custom listening IPs and ports with route connectivity - OCP-73209: enable/disable router (Removed → Managed cycle) - OCP-77349: tuning options default + custom (env vars + haproxy config) - OCP-80508: custom default certificate with real TLS connectivity - OCP-80510: Old and Intermediate TLS security profiles - OCP-80514: Modern and Custom TLS security profiles - OCP-80517: mTLS Required and Optional client certificate policies - OCP-80518: mTLS allowedSubjectPatterns filter - OCP-80520: wildcard routeAdmissionPolicy - OCP-81996: httpCaptureCookies Prefix match in access logs - OCP-81997: httpCaptureCookies Exact match and maxLength truncation - OCP-82000: httpCaptureHeaders request/response in access logs - OCP-82003: httpCaptureHeaders maxLength adherence - OCP-82004: custom 503/404 error pages - OCP-82014: httpLogFormat with real HAProxy format directives - OCP-82015: syslog logging destination with live rsyslogd pod - OCP-84260: negative logging config validation Skipped OCP-60149 (HTTP Ingress, covered by networking-smoke.robot) and OCP-73621 (namespace ownership toggle, covered by router.robot). Also adds: - test/assets/router/: 8 fixture files from OTP testdata/router/ - test/resources/router.resource: shared keywords for workload deployment, route creation, curl-from-pod, haproxy inspection, cert generation - el98-src@router-extended.sh scenarios (presubmits + releases, bootc el9/el10) - Updates existing router scenarios to reference router.robot explicitly instead of the suites/router/ directory wildcard - Adjusts robocop.toml limits for complex integration tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-6746 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds router E2E test assets/manifests, a Robot Framework shared resource library, two Robot suites (routes and router-config), and scenario/CI scripts to run them. ChangesRouter End-to-End Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/assets/router/web-server-signed-deploy.yaml`:
- Around line 50-58: The nginx container currently lacks an explicit
securityContext; update the container spec for the container named "nginx" to
add a securityContext that enforces non-root execution and reduces privileges
(e.g., set runAsNonRoot: true, runAsUser to a non-root UID such as 1000, set
allowPrivilegeEscalation: false, set readOnlyRootFilesystem: true, and drop
capabilities like ["ALL"]); place this securityContext block under the nginx
container entry so the pod explicitly hardens runtime privileges.
In `@test/resources/router.resource`:
- Around line 131-137: The JsonPath currently used in the "Router Pod Env Should
Have Value" keyword queries all router pods via
.items[*].spec.containers[*].env[...] which can return multiple values during
rollouts; change the Oc Get JsonPath call to scope to a single pod (e.g., use
.items[0].spec.containers[*].env[?(@.name=="${env_name}")].value) so only one
value is returned, keeping the rest of the keyword (Should Be Equal As Strings
and variables like ${ROUTER_NS} and ${env_name}) unchanged.
In `@test/suites/router/router-config.robot`:
- Around line 722-723: The assertion is checking the wrong truncated hostname
and the comment is wrong; change the test that uses Wait For Router Logs To
Contain from waiting for "route-unsec82003.ap" to the correctly truncated
16-character hostname "route-unsec82003" and update the preceding comment to
state that the full hostname should be truncated to "route-unsec82003" (16
chars) when maxLength: 16; locate the assertion by the call to Wait For Router
Logs To Contain and the nearby comment in the router-config.robot test and
replace both the expected string and the comment to reflect the correct
16-character truncation.
In `@test/suites/router/router-routes.robot`:
- Around line 171-179: Add admission assertions after each route creation: after
the Create OC Route calls for route-http, route-edge, and route-passth call the
Route Should Be Admitted keyword (same as for route-reen) so every created route
is verified before curl checks; also mirror this change in the other identical
route-creation block that creates route-http, route-edge and route-passth so all
created routes are gated on admission.
- Around line 117-120: The test currently assigns ${env} from Oc Get JsonPath
and then asserts it equals a single "true", which fails when multiple router
pods produce values like "true true"; update the assertion so it verifies every
returned env value for ROuTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK is "true" (e.g.,
split ${env} into items or iterate over the list and assert each item == "true",
or use a collection-level assertion) instead of comparing the aggregated string
with a single "true"; adjust the steps surrounding Oc Get JsonPath / ${env} and
replace the single-string check (Should Be Equal As Strings ${env} true) with a
per-item check so multi-pod results pass.
🪄 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: Enterprise
Run ID: ebedf450-e7d9-43c3-85aa-30293f17a3da
📒 Files selected for processing (22)
robocop.tomltest/assets/router/error-page-404.httptest/assets/router/error-page-503.httptest/assets/router/microshift-ingress-destca.yamltest/assets/router/microshift-ingress-http.yamltest/assets/router/rsyslogd-pod.yamltest/assets/router/test-client-pod.yamltest/assets/router/web-server-deploy.yamltest/assets/router/web-server-signed-deploy.yamltest/resources/router.resourcetest/scenarios-bootc/el10/presubmits/el102-src@router-extended.shtest/scenarios-bootc/el10/presubmits/el102-src@router.shtest/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.shtest/scenarios-bootc/el9/presubmits/el98-src@router-extended.shtest/scenarios-bootc/el9/presubmits/el98-src@router.shtest/scenarios-bootc/el9/releases/el98-lrel@osconfig-router.shtest/scenarios/presubmits/el98-src@router-extended.shtest/scenarios/presubmits/el98-src@router.shtest/scenarios/releases/el98-lrel@router-extended.shtest/scenarios/releases/el98-lrel@router.shtest/suites/router/router-config.robottest/suites/router/router-routes.robot
Remove router-extended scenarios from presubmits and add them under releases for bootc el9 and el10. Non-disruptive smoke tests belong in presubmits; the full router-extended suite (with many disruptive restarts) is better suited for release validation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
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/scenarios-bootc/el9/releases/el98-lrel`@router-extended.sh:
- Around line 15-16: The test script incorrectly calls exit_if_image_not_found
in teardown/test phases which can silently skip cleanup or tests; remove the
exit_if_image_not_found invocations from scenario_remove_vms and
scenario_run_tests and leave the guard only in scenario_create_vms so image
availability only blocks VM creation; locate and delete the
exit_if_image_not_found "${start_image}" calls in the scenario_remove_vms and
scenario_run_tests blocks (also check the duplicate occurrences noted around the
other invocation) and ensure no other teardown/test functions call
exit_if_image_not_found.
🪄 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: Enterprise
Run ID: ee327aa5-6071-49cd-bfcc-37af5609b3c0
📒 Files selected for processing (2)
test/scenarios-bootc/el10/releases/el102-lrel@router-extended.shtest/scenarios-bootc/el9/releases/el98-lrel@router-extended.sh
✅ Files skipped from review due to trivial changes (1)
- test/scenarios-bootc/el10/releases/el102-lrel@router-extended.sh
Revert robocop.toml to original limits and refactor the new robot tests to comply without configuration changes: - Fix LEN07: reduce Wait Until Curl Succeeds From Pod and Curl From Pod Should Contain to 5 args; add specialized HTTPS/cookie/client-cert curl keyword variants in router.resource - Fix LEN04/LEN06: extract long test bodies into helper keywords and move YAML config strings to the Variables section - Fix LEN03: split Prepare Two MTLS Certs, Curl All Cookie Routes, Verify Header Capture, and Verify Syslog Logging into sub-keywords - Fix VAR10: uppercase non-local TEST-scoped variables consistently Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/resources/router.resource`:
- Around line 102-105: The helper currently forces a HEAD request by passing -I
to curl in the oc exec call (the line using "oc exec ... -- curl ${url} -sI
--resolve ${resolve} --connect-timeout 10 ${flags}"), which strips the body;
remove the hardcoded -I so the command becomes "-s --resolve ${resolve}
--connect-timeout 10 ${flags}" (or otherwise omit -I) and rely on the existing
${flags} argument to opt into HEAD when callers need it; update the Robot
keyword that wraps Run With Kubeconfig to use the modified curl invocation so
downstream checks (e.g. custom 503 error-page assertions) can see response
bodies.
- Around line 338-348: The wrapper keyword "Generate Client Cert File In Dir" is
incorrectly prepending "/CN=" before the ${cn} when calling "Generate CSR And
Key", causing double "/CN=/CN=..." for callers that already provide a full
subject; modify the call to Generate CSR And Key to pass ${cn} unchanged (remove
the hardcoded "/CN=" prefix) so the subject provided by callers is forwarded
verbatim.
In `@test/suites/router/router-config.robot`:
- Around line 702-710: The test "Verify Custom LB Ports And IP" currently uses
"Get LB IPs" + "Should Contain", which allows extra IPs; change it to assert
exclusivity by checking the list length and membership: after calling ${lb_ips}=
Get LB IPs add "Length Should Be ${lb_ips} 1" and then keep or add "Should
Contain ${lb_ips} ${host_ip}" (or replace both with creating @{expected}
with ${host_ip} and use "Lists Should Be Equal ${lb_ips} ${expected}").
This ensures the router-default LB exposes exactly the single custom ${host_ip}.
🪄 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: Enterprise
Run ID: 924210dd-c568-4210-9330-ea0caa00cd47
📒 Files selected for processing (3)
test/resources/router.resourcetest/suites/router/router-config.robottest/suites/router/router-routes.robot
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
- Fix mTLS subject filter (OCP-80518): remove double /CN= prefix from Generate Client Cert File In Dir callers. The keyword already prefixes /CN=, so callers should pass the bare CN value (e.g. example-test.com). The malformed subject /CN=/CN=example-test.com would never match the allowedSubjectPatterns filter, causing usr1 cert to return 403 instead of 200 and inverting the test assertions. - Fix custom error page 503 check (OCP-82004): replace the 503 assertion using Wait Until Curl Succeeds From Pod (which uses curl -sI / HEAD method, headers only) with a direct GET curl via a new Custom 503 Body Should Contain keyword. The HTML error body never appears in HEAD response output, so the assertion would always fail. - Fix tuning timeout normalization (OCP-77349): MicroShift normalizes clientTimeout/serverTimeout "60s" to "1m" before propagating to the deployment env vars and haproxy config. Update expected values from "60s" to "1m" to match actual runtime behavior (confirmed from the original Go source which uses 1m as the expected value). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
- Fix header capture maxLength assertion (OCP-82003): the truncated
hostname is exactly 16 chars ("route-unsec82003"), not 19; waiting
for "route-unsec82003.ap" would always time out. Fix the assertion
and the accompanying comment.
- Assert admission for all four route types in Create And Admit Four
Route Types: previously only route-reen was gated; adding checks for
route-http, route-edge, and route-passth gives clearer failure messages
when a specific route type fails to be admitted.
- Check LB IP exclusivity in Verify Custom LB Ports And IP: use
Should Be Equal As Strings instead of Should Contain so the test
fails if the router-default service exposes additional IPs beyond
the configured listenAddress interface.
Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
Contributor
Author
|
Addressed CodeRabbit inline suggestions:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
The syslog_ip argument was declared but never referenced in the keyword body. The haproxy syslog config check is handled separately by Verify Syslog Haproxy Config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
… each) Replace the single router-extended release scenario (~2 hours) with 5 smaller scenarios that each target ~30 minutes of execution time: - router-routes: 6 non-disruptive route type tests (~15 min) - router-config-infra: custom ports, enable/disable, syslog, validation (~32 min) - router-config-tls: custom cert, Old/Intermediate/Modern/Custom TLS, mTLS (~31 min) - router-config-policies: tuning, mTLS subject filter, wildcard, log format (~30 min) - router-config-logging: cookie/header capture, error pages (~32 min) Implementation uses Robot Framework's --include tag filter rather than splitting the .robot file. Each test case in router-config.robot gets a group tag (router-infra, router-tls, router-policies, router-logging) and each scenario script passes --include to select its subset. This works because run_tests passes all args directly to the robot command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Rename the existing router test suite to router-basic.robot for consistency with the new router-routes.robot and router-config.robot files. Update all scenario script references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Replace the single router-config.robot (17 tests, ~2 hours) with 4 files that each match one CI scenario (~30 min each): - router-config-infra.robot: OCP-73203, 73209, 82015, 84260 (~32 min) Custom ports, enable/disable, syslog, negative validation - router-config-tls.robot: OCP-80508, 80510, 80514, 80517 (~31 min) Custom cert, Old/Intermediate/Modern/Custom TLS profiles, mTLS policies - router-config-policies.robot: OCP-77349, 80518, 80520, 82014 (~30 min) Tuning options, mTLS subject filter, wildcard policy, log format - router-config-logging.robot: OCP-81996, 81997, 82000, 82003, 82004 (~32 min) Cookie capture, header capture, maxLength, custom error pages Each file is self-contained with its own Settings, Variables, Test Cases, and Keywords sections. Scenario scripts now reference the specific .robot files directly instead of using --include tags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
…downs - Fix Curl Without Cert Should Return SSL Error: use Run With Kubeconfig with allow_fail=True instead of Curl From Pod. The curl command exits non-zero on SSL handshake failure, which causes Run With Kubeconfig to fail before the Should Contain assertion runs. - Add workload cleanup (web-server-deploy, test-client-pod, rsyslogd-pod) to all 13 disruptive test teardowns that deploy workloads. CI runs with TEST_RANDOMIZATION="all", and all tests in each suite share a single namespace. Without cleanup, the second test to run fails with "already exists" when creating the same-named deployment or pod. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
Author
|
/retest |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
/hold until dynamic scheduling is implemented because this PR adds 5 new scenarios targetting latest release version |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
@agullon: The following test 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
This PR adds 5 new scenarios under release dir, it must be on hold until dynamic scheduling is implemented
Summary
Migrates 23 end-to-end router tests from
openshift-tests-private(Go/Ginkgo) into the MicroShift repo as Robot Framework tests. These tests validate MicroShift's ingress controller configuration pipeline — the path fromconfig.yamlto the router deployment, HAProxy config, and actual route connectivity.Two tests were intentionally skipped because they already have equivalent coverage:
networking-smoke.robotrouter-basic.robotWhat's tested
router-routes.robot@router-routesrouter-config-infra.robot@router-config-infrarouter-config-tls.robot@router-config-tlsrouter-config-policies.robot@router-config-policiesrouter-config-logging.robot@router-config-loggingNew infrastructure
test/resources/router.resource— ~50 shared keywords for workload deployment, route creation/admission, curl-from-pod (HTTP/HTTPS/cookie/mTLS variants), HAProxy config inspection, router log checking, and OpenSSL certificate generationtest/assets/router/— 8 fixture files: nginx deployments (plain + TLS), test client pod, HTTP/reencrypt ingress manifests, custom error pages, rsyslogd podtest/bin/run-single-test.sh— helper to run individual tests or full suites against a remote MicroShift hostOther changes
router.robot→router-basic.robotfor consistencyrouter-basic.robotexplicitly (prevents the new files from being picked up by presubmit scenarios)Test plan
make verify-rfpasses (robocop check + format)@router-routesscenario on CI@router-config-infrascenario on CI@router-config-tlsscenario on CI@router-config-policiesscenario on CI@router-config-loggingscenario on CI@routerpresubmit still passes (only runsrouter-basic.robot)🤖 Generated with Claude Code