Add RF cases for trust manager - #7129
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kasturinarra 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
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Robot tests for Pebble ACME root CA distribution through trust-manager and for Bundle target persistence across a MicroShift restart. ChangesTrust-manager integration tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RobotTest
participant Pebble
participant CertManager
participant TrustManager
participant TargetConfigMap
participant MicroShift
RobotTest->>Pebble: Retrieve and validate root CA
RobotTest->>CertManager: Create issuer and CA resources
RobotTest->>TrustManager: Create source Secret and Bundle
TrustManager->>TargetConfigMap: Synchronize PEM data
RobotTest->>TargetConfigMap: Verify Bundle output
RobotTest->>MicroShift: Restart
RobotTest->>TrustManager: Wait for recovery
RobotTest->>TargetConfigMap: Compare persisted data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/suites/optional/cert-manager.robot`:
- Around line 185-190: Update the assertion following the Oc Get JsonPath call
to verify that ${cm_data} contains ${pebble_ca}, replacing the generic BEGIN
CERTIFICATE check while preserving the existing failure message context.
- Around line 506-512: Replace the fixed Sleep delay in the Pebble root CA
retrieval flow with readiness polling that repeatedly runs curl and validates
both a successful return code and PEM content. Retry until the endpoint is
ready, then preserve the existing RETURN of ${result.stdout} and failure
diagnostics.
🪄 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: 51e94dec-a7bb-4655-a8a4-1dd7400736bf
📒 Files selected for processing (1)
test/suites/optional/cert-manager.robot
2acf344 to
6a3ddfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/suites/optional/cert-manager.robot (1)
477-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing this keyword in the existing
Trust Manager Bundle With Cert Manager CAtest.
Setup Trust Bundle With Cert Manager CAduplicates the ClusterIssuer/CA-certificate/Bundle flow already inline at lines 133-143. Extracting it here is good, but the older test wasn't updated to call it, so the two copies can now drift.♻️ Proposed consolidation
Trust Manager Bundle With Cert Manager CA [Documentation] Verify trust-manager Bundle can use a cert-manager CA secret as a source [Tags] trust-manager [Setup] Enable Trust Manager - Apply Template ${CLUSTER_ISSUER_TMPL} - Oc Wait -n ${NAMESPACE} clusterissuer ${ISSUER_NAME} - ... --for="condition=Ready" --timeout=${DEFAULT_WAIT_TIMEOUT} - - Apply Template ${CA_CERTIFICATE_TMPL} - Oc Wait -n ${TRUST_MANAGER_NS} certificate ca-certificate - ... --for="condition=Ready" --timeout=${DEFAULT_WAIT_TIMEOUT} - - Apply Template ${TRUST_BUNDLE_SECRET_TMPL} - Oc Wait bundle ${TRUST_MANAGER_BUNDLE_NAME} - ... --for=jsonpath='{.status.conditions[?(@.type=="Synced")].status}'=True --timeout=${DEFAULT_WAIT_TIMEOUT} + Setup Trust Bundle With Cert Manager CA🤖 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/optional/cert-manager.robot` around lines 477 - 489, Update the existing “Trust Manager Bundle With Cert Manager CA” test to call the shared “Setup Trust Bundle With Cert Manager CA” keyword instead of duplicating the ClusterIssuer, CA-certificate, and Bundle setup steps. Remove the inline setup sequence while preserving the test’s remaining behavior.
🤖 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/suites/optional/cert-manager.robot`:
- Around line 499-521: Update Fetch Pebble Root CA and Pebble Root CA Should Be
Available so the readiness keyword returns the already-validated curl output,
then have Fetch Pebble Root CA return that value after Wait Until Keyword
Succeeds. Remove the redundant post-poll Process.Run Process curl and preserve
the existing rc and certificate-content checks.
---
Nitpick comments:
In `@test/suites/optional/cert-manager.robot`:
- Around line 477-489: Update the existing “Trust Manager Bundle With Cert
Manager CA” test to call the shared “Setup Trust Bundle With Cert Manager CA”
keyword instead of duplicating the ClusterIssuer, CA-certificate, and Bundle
setup steps. Remove the inline setup sequence while preserving the test’s
remaining behavior.
🪄 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: afd7ef52-7ce0-4bbe-b7f7-45d02710379e
📒 Files selected for processing (1)
test/suites/optional/cert-manager.robot
6a3ddfd to
6ce78e0
Compare
|
/test e2e-aws-tests-bootc-el9 |
| [Documentation] Fetch the root CA certificate from Pebble management API via port-forward | ||
| [Arguments] ${namespace} | ||
| ${port_fwd}= Process.Start Process | ||
| ... oc port-forward deployment/pebble 15000:15000 |
There was a problem hiding this comment.
Fetch Pebble Root CA starts oc port-forward asynchronously but never checks if it actually started
|
|
||
| Pebble Root CA Should Be Available | ||
| [Documentation] Check that the Pebble management API returns a valid CA certificate | ||
| ${result}= Process.Run Process curl -sk https://localhost:15000/roots/0 |
There was a problem hiding this comment.
curl does not use --fail — curl returns rc=0 even on HTTP 4xx/5xx responses. If Pebble returns an error page, the rc check passes and the response body (potentially invalid) gets stored as the CA certificate. Fix: Add --fail to the curl flags so HTTP errors produce a non-zero exit code.
| [Teardown] Run Keywords | ||
| ... Cleanup Trust Bundle | ||
| ... AND Oc Delete secret ca-source-secret -n ${TRUST_MANAGER_NS} --ignore-not-found | ||
| ... AND Cleanup HTTP01 Resources |
There was a problem hiding this comment.
Cleanup HTTP01 Resources deletes the Pebble deployment and service but not the pebble ConfigMap created by pebble-server.yaml. Pre-existing issue, but both new tests inherit it.
Fix: Add Oc Delete configmap/pebble -n ${NAMESPACE} --ignore-not-found.
6ce78e0 to
9f21baa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/optional/cert-manager.robot (1)
479-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the new helper in the existing test to remove duplication.
Setup Trust Bundle With Cert Manager CAduplicates theApply Template/Oc Waitsequence already present in the pre-existing testTrust Manager Bundle With Cert Manager CA(lines 133-143). Call the new keyword from that test body instead of keeping two copies of the same sequence. Two copies increase the risk that future changes to one location miss the other.Apply this change at lines 133-143 (outside the segment shown here):
Trust Manager Bundle With Cert Manager CA [Documentation] Verify trust-manager Bundle can use a cert-manager CA secret as a source [Tags] trust-manager [Setup] Enable Trust Manager Setup Trust Bundle With Cert Manager CA ${cm_data}= Oc Get JsonPath ... configmap ... ${NAMESPACE} ... ${TRUST_MANAGER_BUNDLE_NAME} ... .data.ca-bundle\\.crt Should Contain ${cm_data} BEGIN CERTIFICATE msg=ConfigMap does not contain CA certificate data🤖 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/optional/cert-manager.robot` around lines 479 - 489, Update the existing Trust Manager Bundle With Cert Manager CA test to call Setup Trust Bundle With Cert Manager CA for the shared setup sequence, removing the duplicated Apply Template and Oc Wait steps while preserving the subsequent certificate-data assertions.
🤖 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/suites/optional/cert-manager.robot`:
- Around line 479-489: Update the existing Trust Manager Bundle With Cert
Manager CA test to call Setup Trust Bundle With Cert Manager CA for the shared
setup sequence, removing the duplicated Apply Template and Oc Wait steps while
preserving the subsequent certificate-data assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5e88c314-01d1-4e93-bffc-a28271b2d7a7
📒 Files selected for processing (1)
test/suites/optional/cert-manager.robot
9f21baa to
5e835e9
Compare
|
/test release-5.0-periodics-e2e-aws-tests-nightly |
|
/test e2e-aws-tests-periodic |
|
/test e2e-aws-tests |
|
@kasturinarra: all tests passed! 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. |
2 participants
Summary by CodeRabbit