USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730
Draft
agullon wants to merge 3 commits into
Draft
USHIFT-6746: migrate 23 OTP router tests to Robot Framework#6730agullon wants to merge 3 commits into
agullon wants to merge 3 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
WalkthroughThis PR adds comprehensive end-to-end testing for MicroShift's router (OpenShift Ingress Controller), covering route validation, advanced configurations, and logging. New test infrastructure includes Kubernetes manifests for workloads, a shared Robot Framework library with 40+ keywords for deployment/probing/verification, two test suites with 13 test cases total, and scenario scripts orchestrating execution across multiple RHEL versions. ChangesRouter End-to-End Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ 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
Comment on lines
+50
to
+58
| containers: | ||
| - name: nginx | ||
| image: quay.io/openshifttest/nginx-alpine@sha256:cee6930776b92dc1e93b73f9e5965925d49cff3d2e91e1d071c2f0ff72cbca29 | ||
| volumeMounts: | ||
| - name: service-secret | ||
| mountPath: /etc/nginx/certs/ | ||
| - name: nginx-config | ||
| mountPath: /etc/nginx/ | ||
| volumes: |
Contributor
There was a problem hiding this comment.
Add an explicit container securityContext.
Line 52 currently allows default runtime behavior, which can permit root execution and privilege escalation. Please harden this container explicitly.
Suggested patch
containers:
- name: nginx
image: quay.io/openshifttest/nginx-alpine@sha256:cee6930776b92dc1e93b73f9e5965925d49cff3d2e91e1d071c2f0ff72cbca29
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
+ capabilities:
+ drop:
+ - ALL
volumeMounts:
- name: service-secret
mountPath: /etc/nginx/certs/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| containers: | |
| - name: nginx | |
| image: quay.io/openshifttest/nginx-alpine@sha256:cee6930776b92dc1e93b73f9e5965925d49cff3d2e91e1d071c2f0ff72cbca29 | |
| volumeMounts: | |
| - name: service-secret | |
| mountPath: /etc/nginx/certs/ | |
| - name: nginx-config | |
| mountPath: /etc/nginx/ | |
| volumes: | |
| containers: | |
| - name: nginx | |
| image: quay.io/openshifttest/nginx-alpine@sha256:cee6930776b92dc1e93b73f9e5965925d49cff3d2e91e1d071c2f0ff72cbca29 | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| capabilities: | |
| drop: | |
| - ALL | |
| volumeMounts: | |
| - name: service-secret | |
| mountPath: /etc/nginx/certs/ | |
| - name: nginx-config | |
| mountPath: /etc/nginx/ | |
| volumes: |
🤖 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/assets/router/web-server-signed-deploy.yaml` around lines 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.
Comment on lines
+131
to
+137
| Router Pod Env Should Have Value | ||
| [Documentation] Check an env var value in the router pod. | ||
| [Arguments] ${env_name} ${expected_value} | ||
| ${actual}= Oc Get JsonPath | ||
| ... pod ${ROUTER_NS} ${EMPTY} | ||
| ... .items[*].spec.containers[*].env[?(@.name=="${env_name}")].value | ||
| Should Be Equal As Strings ${actual} ${expected_value} |
Contributor
There was a problem hiding this comment.
Scope env lookup to one router pod to avoid flakiness.
Line 135 uses .items[*]... across all pods. During router rollouts this can return multiple values, so Line 137 may fail nondeterministically.
Suggested patch
Router Pod Env Should Have Value
[Documentation] Check an env var value in the router pod.
[Arguments] ${env_name} ${expected_value}
+ ${pod_name}= Get Router Pod Name
${actual}= Oc Get JsonPath
- ... pod ${ROUTER_NS} ${EMPTY}
- ... .items[*].spec.containers[*].env[?(@.name=="${env_name}")].value
+ ... pod ${ROUTER_NS} ${pod_name}
+ ... .spec.containers[*].env[?(@.name=="${env_name}")].value
Should Be Equal As Strings ${actual} ${expected_value}🤖 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/resources/router.resource` around lines 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.
Comment on lines
+117
to
+120
| ${env}= Oc Get JsonPath | ||
| ... pod ${ROUTER_NS} ${EMPTY} | ||
| ... .items[*].spec.containers[*].env[?(@.name=="ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK")].value | ||
| Should Be Equal As Strings ${env} true |
Contributor
There was a problem hiding this comment.
Avoid single-pod assumption in the namespace-ownership env assertion.
Line 120 compares the aggregated JsonPath result to a single true. If multiple router pods are returned, this can become true true and fail despite valid config.
Suggested fix
- ${env}= Oc Get JsonPath
+ ${env}= Oc Get JsonPath
... pod ${ROUTER_NS} ${EMPTY}
... .items[*].spec.containers[*].env[?(@.name=="ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK")].value
- Should Be Equal As Strings ${env} true
+ @{env_values}= Split String ${env}
+ FOR ${v} IN @{env_values}
+ Should Be Equal As Strings ${v} true
+ END📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ${env}= Oc Get JsonPath | |
| ... pod ${ROUTER_NS} ${EMPTY} | |
| ... .items[*].spec.containers[*].env[?(@.name=="ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK")].value | |
| Should Be Equal As Strings ${env} true | |
| ${env}= Oc Get JsonPath | |
| ... pod ${ROUTER_NS} ${EMPTY} | |
| ... .items[*].spec.containers[*].env[?(@.name=="ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK")].value | |
| @{env_values}= Split String ${env} | |
| FOR ${v} IN @{env_values} | |
| Should Be Equal As Strings ${v} true | |
| END |
🤖 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/suites/router/router-routes.robot` around lines 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.
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
Comment on lines
+15
to
+16
| exit_if_image_not_found "${start_image}" | ||
|
|
Contributor
There was a problem hiding this comment.
Remove image-availability guard from teardown/test phases.
exit_if_image_not_found exits 0 when missing. In scenario_remove_vms and scenario_run_tests, that can silently skip cleanup/tests if the registry/image is unavailable later in the job. Keep this guard only in scenario_create_vms.
Suggested patch
scenario_remove_vms() {
- exit_if_image_not_found "${start_image}"
-
remove_vm host1
}
scenario_run_tests() {
- exit_if_image_not_found "${start_image}"
-
run_tests host1 \
suites/router/router-routes.robot \
suites/router/router-config.robot
}Also applies to: 21-22
🤖 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/scenarios-bootc/el9/releases/el98-lrel`@router-extended.sh around lines
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.
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
Comment on lines
+102
to
+105
| [Arguments] ${pod_name} ${ns} ${url} ${resolve} ${flags}=${EMPTY} | ||
| ${output}= Run With Kubeconfig | ||
| ... oc exec -n ${ns} ${pod_name} -- curl ${url} -sI --resolve ${resolve} --connect-timeout 10 ${flags} | ||
| RETURN ${output} |
Contributor
There was a problem hiding this comment.
Don't hardcode HEAD requests in the shared curl helper.
-I strips the response body. That makes downstream checks like the custom 503 error-page assertion unreachable through this keyword, because Custom:Application Unavailable will never be returned here.
🤖 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/resources/router.resource` around lines 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.
Comment on lines
+338
to
+348
| Generate Client Cert File In Dir | ||
| [Documentation] Generate a CSR, key, and signed cert in the given directory using a name prefix. | ||
| [Arguments] ${tmpdir} ${host} ${cn} ${cert_prefix} | ||
| Generate CSR And Key ${tmpdir}/${cert_prefix}.key ${tmpdir}/${cert_prefix}.csr /CN=${cn} | ||
| VAR ${san}= subjectAltName = DNS.1:*.${BASE_DOMAIN},DNS.2:${host} | ||
| Sign CSR With CA | ||
| ... ${tmpdir}/${cert_prefix}.csr | ||
| ... ${tmpdir}/ca.crt | ||
| ... ${tmpdir}/ca.key | ||
| ... ${tmpdir}/${cert_prefix}.crt | ||
| ... ${san} |
Contributor
There was a problem hiding this comment.
Pass the subject through unchanged.
Generate CSR And Key already takes a full subject string. The current wrapper prepends /CN= again, so the new callers that pass /CN=example-test.com end up generating /CN=/CN=example-test.com, which will break the mTLS subject-filter case.
Suggested patch
Generate Client Cert File In Dir
[Documentation] Generate a CSR, key, and signed cert in the given directory using a name prefix.
[Arguments] ${tmpdir} ${host} ${cn} ${cert_prefix}
- Generate CSR And Key ${tmpdir}/${cert_prefix}.key ${tmpdir}/${cert_prefix}.csr /CN=${cn}
+ Generate CSR And Key ${tmpdir}/${cert_prefix}.key ${tmpdir}/${cert_prefix}.csr ${cn}
VAR ${san}= subjectAltName = DNS.1:*.${BASE_DOMAIN},DNS.2:${host}
Sign CSR With CA
... ${tmpdir}/${cert_prefix}.csr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Generate Client Cert File In Dir | |
| [Documentation] Generate a CSR, key, and signed cert in the given directory using a name prefix. | |
| [Arguments] ${tmpdir} ${host} ${cn} ${cert_prefix} | |
| Generate CSR And Key ${tmpdir}/${cert_prefix}.key ${tmpdir}/${cert_prefix}.csr /CN=${cn} | |
| VAR ${san}= subjectAltName = DNS.1:*.${BASE_DOMAIN},DNS.2:${host} | |
| Sign CSR With CA | |
| ... ${tmpdir}/${cert_prefix}.csr | |
| ... ${tmpdir}/ca.crt | |
| ... ${tmpdir}/ca.key | |
| ... ${tmpdir}/${cert_prefix}.crt | |
| ... ${san} | |
| Generate Client Cert File In Dir | |
| [Documentation] Generate a CSR, key, and signed cert in the given directory using a name prefix. | |
| [Arguments] ${tmpdir} ${host} ${cn} ${cert_prefix} | |
| Generate CSR And Key ${tmpdir}/${cert_prefix}.key ${tmpdir}/${cert_prefix}.csr ${cn} | |
| VAR ${san}= subjectAltName = DNS.1:*.${BASE_DOMAIN},DNS.2:${host} | |
| Sign CSR With CA | |
| ... ${tmpdir}/${cert_prefix}.csr | |
| ... ${tmpdir}/ca.crt | |
| ... ${tmpdir}/ca.key | |
| ... ${tmpdir}/${cert_prefix}.crt | |
| ... ${san} |
🤖 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/resources/router.resource` around lines 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.
Comment on lines
+702
to
+710
| Verify Custom LB Ports And IP | ||
| [Documentation] Check the router-default LB has the expected IP and custom port numbers. | ||
| [Arguments] ${host_ip} | ||
| ${lb_ips}= Get LB IPs | ||
| Should Contain ${lb_ips} ${host_ip} | ||
| ${http_port}= Get LB Port http | ||
| Should Be Equal As Strings ${http_port} ${ALT_HTTP_PORT} | ||
| ${https_port}= Get LB Port https | ||
| Should Be Equal As Strings ${https_port} ${ALT_HTTPS_PORT} |
Contributor
There was a problem hiding this comment.
Check the custom LB IP set exactly.
Should Contain still passes if router-default exposes extra LB IPs. This test is meant to prove the custom listenAddress limits exposure to the selected host IP, so the assertion should fail when additional IPs are present.
🤖 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/suites/router/router-config.robot` around lines 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}.
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.
Summary
openshift-tests-private/test/extended/router/microshift.goto Robot Framework in the MicroShift reporouter-routes.robotrouter-config.robottest/resources/router.resourcewith reusable keywords for workload deployment, route creation, curl-from-pod, haproxy inspection, and OpenSSL cert generationel98-src@router-extended.shCI scenarios (presubmits + releases) for the new test files; updates existingrouter.shscenarios to explicitly referencerouter.robotinstead of the directory wildcard to avoid picking up the new (slower) testsnetworking-smoke.robot) and OCP-73621 (namespace ownership toggle, already covered byrouter.robot)Test plan
make verify-rflocally to confirm lint and format passel98-src@router-extendedpresubmit to validate new testsel98-src@routerpresubmit still passes (only runsrouter.robot)router-routes.robotandrouter-config.robotfor correctness🤖 Generated with Claude Code
Summary by CodeRabbit