CNTRLPLANE-1739: e2e additional tests for pki config - #31491
CNTRLPLANE-1739: e2e additional tests for pki config#31491kaleemsiddiqu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@kaleemsiddiqu: This pull request references CNTRLPLANE-1739 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 story 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. |
|
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 (2)
WalkthroughThe change adds PKI validation helpers and extended tests for kube-controller-manager and machine-config-operator certificates. Tests cover RSA, ECDSA, mixed profiles, reconciliation, secret regeneration, CA status, and key parameters. ChangesOperator PKI certificate coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR adds e2e coverage without changing production behavior, and no concrete correctness, security, availability, or deployment risk is identified; no actionable merge-blocking risk remains after normal verification. Sequence Diagram(s)sequenceDiagram
participant GinkgoTest
participant PKIConfiguration
participant Operator
participant CertificateSecrets
GinkgoTest->>PKIConfiguration: Apply uniform or mixed profile
PKIConfiguration->>Operator: Provide certificate settings
Operator->>CertificateSecrets: Reconcile managed certificates
GinkgoTest->>CertificateSecrets: Delete managed certificate secret
Operator->>CertificateSecrets: Regenerate certificate secret
GinkgoTest->>CertificateSecrets: Validate UID, CA status, and key parameters
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: 6
🧹 Nitpick comments (4)
test/extended/pki/pki_kube_controller_manager.go (2)
92-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mixed configuration sets serving and client profiles that no assertion checks.
applyMixedPKIConfigwritesServingCertificatesas ECDSA P256 andClientCertificatesas ECDSA P521 (seetest/extended/pki/helpers.golines 152-203).testMixedKCMCertificatesonly inspectscsr-signerandcsr-signer-signer, and both expect the signer profile. The test therefore never proves that per-category overrides take effect, which is the purpose of the mixed case.Add a serving or client certificate Secret from the kube-controller-manager operator to the test table, or state in a comment why only signers are observable 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/extended/pki/pki_kube_controller_manager.go` around lines 92 - 102, Update the mixedPKITestConfig entry used by testMixedKCMCertificates to include an observable serving or client certificate Secret whose expected algorithm and curve match the configured ECDSA P256 or ECDSA P521 override, so the test validates category-specific profiles rather than only signer certificates.
178-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth key-size assertions narrow
newCert.KeySizetoint32. Static analysis flags each conversion as a possible truncation. Widen the expected value instead of narrowing the observed value; the assertion result does not change.
test/extended/pki/pki_kube_controller_manager.go#L178-L178: comparenewCert.KeySizeagainstint(tc.rsaSize).test/extended/pki/pki_kube_controller_manager.go#L255-L255: comparenewCert.KeySizeagainstint(testCase.expectedRSASize).🤖 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/pki/pki_kube_controller_manager.go` at line 178, The key-size assertions narrow the observed value and may truncate it. In test/extended/pki/pki_kube_controller_manager.go at lines 178-178 and 255-255, update both assertions to compare newCert.KeySize against widened expected values using int(tc.rsaSize) and int(testCase.expectedRSASize), respectively.Source: Linters/SAST tools
test/extended/pki/pki_machine_config_operator.go (2)
190-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain or remove the trailing sleeps.
time.Sleep(5 * time.Second)runs after each certificate is verified. The purpose is not clear. If the sleep lets the operator settle before the next Secret deletion, add a short comment that states why. Otherwise remove it.Also applies to: 271-271
🤖 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/pki/pki_machine_config_operator.go` at line 190, Remove the 5-second time.Sleep calls following certificate verification in the affected test flow, unless they are required for operator settling; if retained, add a concise comment explaining that purpose.
197-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared regeneration and validation loop.
testMixedMCOCertificatesrepeatstestMCOCertificatesalmost line for line. Only the source of the expected algorithm, RSA size, and curve differs. Extract one helper that accepts a certificate plus its expected key properties, then call it from both functions. The kube-controller-manager suite in this stack looks similar, so the helper can likely serve both files.🤖 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/pki/pki_machine_config_operator.go` around lines 197 - 276, Extract the repeated certificate deletion, regeneration, retrieval, CA validation, and key-property validation logic from testMixedMCOCertificates and testMCOCertificates into a shared helper that accepts an operatorCertificate and expected algorithm, RSA size, and ECDSA curve. Replace both functions’ duplicated loops with calls to this helper while preserving their existing expectations, logging, and verification counts; structure it for reuse by the kube-controller-manager certificate tests where compatible.
🤖 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/pki/pki_kube_controller_manager.go`:
- Around line 176-185: The algorithm verification chains in testKCMCertificates
(test/extended/pki/pki_kube_controller_manager.go:176-185) and
testMixedKCMCertificates
(test/extended/pki/pki_kube_controller_manager.go:253-262) must reject
unsupported algorithm values. Add a final failing branch after the RSA and ECDSA
cases in both locations, so unexpected tc.algorithm or
testCase.expectedAlgorithm values cannot increment verifiedCount or report
success without assertions.
- Around line 73-77: In
test/extended/pki/pki_kube_controller_manager.go#L73-L77, update
testUniformKCMPKIConfigurations to remove the fixed sleep and wait for the
kube-controller-manager operator status to reflect the applied PKI generation
before calling WaitForOperatorProgressingFalse. Apply the same change in
test/extended/pki/pki_kube_controller_manager.go#L112-L116 within
testMixedKCMPKIConfigurations.
- Around line 95-100: Run gofmt on the struct literal containing
signerAlgorithm, servingAlgorithm, and clientAlgorithm so all contiguous
key-value fields, including servingECDSACurve and clientECDSACurve, are aligned
consistently.
In `@test/extended/pki/pki_machine_config_operator.go`:
- Around line 68-78: Replace the fixed time.Sleep after applyPKIConfig with a
generation-based wait that confirms the machine-config operator has observed the
newly applied PKI configuration before calling WaitForOperatorProgressingFalse.
Apply the same change to the corresponding flow around lines 112-116, reusing
the existing operator generation/status wait helpers and preserving the
subsequent reconciliation check.
- Around line 92-102: The mixedConfigs client profile is not covered by
validation. Update testMixedMCOCertificates to include the expected client
certificate Secret in its validation table, using clientAlgorithm and
clientECDSACurve, so the configured client profile is verified alongside the
signer and serving profiles.
- Around line 177-186: Add a terminal else branch to the algorithm validation
blocks at test/extended/pki/pki_machine_config_operator.go lines 177-186 and
258-267, failing the test when tc.algorithm or testCase.expectedAlgorithm is
neither RSA nor ECDSA. Ensure unexpected or empty values cannot increment
verifiedCount without performing a key assertion.
---
Nitpick comments:
In `@test/extended/pki/pki_kube_controller_manager.go`:
- Around line 92-102: Update the mixedPKITestConfig entry used by
testMixedKCMCertificates to include an observable serving or client certificate
Secret whose expected algorithm and curve match the configured ECDSA P256 or
ECDSA P521 override, so the test validates category-specific profiles rather
than only signer certificates.
- Line 178: The key-size assertions narrow the observed value and may truncate
it. In test/extended/pki/pki_kube_controller_manager.go at lines 178-178 and
255-255, update both assertions to compare newCert.KeySize against widened
expected values using int(tc.rsaSize) and int(testCase.expectedRSASize),
respectively.
In `@test/extended/pki/pki_machine_config_operator.go`:
- Line 190: Remove the 5-second time.Sleep calls following certificate
verification in the affected test flow, unless they are required for operator
settling; if retained, add a concise comment explaining that purpose.
- Around line 197-276: Extract the repeated certificate deletion, regeneration,
retrieval, CA validation, and key-property validation logic from
testMixedMCOCertificates and testMCOCertificates into a shared helper that
accepts an operatorCertificate and expected algorithm, RSA size, and ECDSA
curve. Replace both functions’ duplicated loops with calls to this helper while
preserving their existing expectations, logging, and verification counts;
structure it for reuse by the kube-controller-manager certificate tests where
compatible.
🪄 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: e9ea2fdc-8bfa-472e-81d2-bf206dc3b804
📒 Files selected for processing (2)
test/extended/pki/pki_kube_controller_manager.gotest/extended/pki/pki_machine_config_operator.go
|
/test e2e-gcp-ovn-techpreview-pkiconfig |
|
/test verify |
|
/retest |
| expectedAlgorithm: tc.signerAlgorithm, | ||
| expectedRSASize: tc.signerRSASize, | ||
| expectedECDSACurve: tc.signerECDSACurve, | ||
| }, |
There was a problem hiding this comment.
Should the serving and client certs (https://github.com/openshift/origin/pull/31491/changes?diff=unified#diff-8b644802d4f653d8258ac0baf12668bc603f8555f95bdfb7e9f3a31727c84b7cR97-R100) also be verified here as well?
|
|
||
| verifiedCount++ | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Is this sleep necessary? If it is, can we add a comment explaining it's necessary?
|
|
||
| verifiedCount++ | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
|
|
||
| verifiedCount++ | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
|
|
||
| verifiedCount++ | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
additional test for pki config covering kube controller manager and machine config operator Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
48ea73c to
2d1b82a
Compare
|
@kaleemsiddiqu: This pull request references CNTRLPLANE-1739 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 story to target the "5.1.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. |
openshift-ci
Bot
added
the
ready-for-human-review
|
/test e2e-gcp-ovn-techpreview-pkiconfig |
|
Scheduling required tests: |
Verification Analysis — CNTRLPLANE-1739Change under test: New e2e coverage in Evidence
New tests verified
Each test applies a PKI profile, deletes the managed secret, waits for regeneration, and asserts the new cert's CA flag (category-based) and key algorithm/size/curve. Unknown algorithms now hard-fail ( Red checks — both unrelated to this change (infra/flake)
Caveats
Verified by: |
|
/lgtm |
|
@sanchezl: This PR has been marked as verified by 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. |
openshift-ci-robot
added
the
verified
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaleemsiddiqu, sanchezl 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 |
|
/retest |
1 similar comment
|
/retest |
|
@kaleemsiddiqu: 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. |
|
/test e2e-gcp-ovn |
|
Job Failure Risk Analysis for sha: 2d1b82a
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 2d1b82a
New tests seen in this PR at sha: 2d1b82a
|
|
/test e2e-gcp-ovn |
1 similar comment
|
/test e2e-gcp-ovn |
4 participants
additional test for pki config covering kube controller manager and machine config operator
Summary by CodeRabbit