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
| time.Sleep(10 * time.Second) | ||
|
|
||
| e2e.Logf("Waiting for kube-controller-manager operator to reconcile PKI config...") | ||
| err = exutil.WaitForOperatorProgressingFalse(ctx, configClient, "kube-controller-manager") | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "kube-controller-manager operator did not reconcile PKI config %s", tc.name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both suites sleep for a fixed time before they wait for the operator, so the wait can observe a stale condition. time.Sleep(10 * time.Second) does not guarantee that the kube-controller-manager operator has reacted to the new PKI spec. If the operator has not yet set Progressing=True, WaitForOperatorProgressingFalse returns at once on the pre-change condition, the test deletes the signer Secrets under the old profile, and the algorithm assertions fail intermittently.
test/extended/pki/pki_kube_controller_manager.go#L73-L77: remove the sleep intestUniformKCMPKIConfigurationsand wait until the operator status reflects the appliedPKIgeneration before you wait forProgressing=False.test/extended/pki/pki_kube_controller_manager.go#L112-L116: apply the same wait intestMixedKCMPKIConfigurations.
📍 Affects 1 file
test/extended/pki/pki_kube_controller_manager.go#L73-L77(this comment)test/extended/pki/pki_kube_controller_manager.go#L112-L116
🤖 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 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.
| signerAlgorithm: configv1alpha1.KeyAlgorithmRSA, | ||
| signerRSASize: 4096, | ||
| servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | ||
| servingECDSACurve: configv1alpha1.ECDSACurveP256, | ||
| clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | ||
| clientECDSACurve: configv1alpha1.ECDSACurveP521, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run gofmt on this struct literal.
The keys in this literal are not aligned consistently. servingECDSACurve and clientECDSACurve use a different column than the neighboring keys. gofmt aligns all values in a contiguous key-value run, so make verify will report a diff.
♻️ Proposed formatting
{
- name: "RSA4096-signers",
- signerAlgorithm: configv1alpha1.KeyAlgorithmRSA,
- signerRSASize: 4096,
- servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
- servingECDSACurve: configv1alpha1.ECDSACurveP256,
- clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
- clientECDSACurve: configv1alpha1.ECDSACurveP521,
+ name: "RSA4096-signers",
+ signerAlgorithm: configv1alpha1.KeyAlgorithmRSA,
+ signerRSASize: 4096,
+ servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
+ servingECDSACurve: configv1alpha1.ECDSACurveP256,
+ clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
+ clientECDSACurve: configv1alpha1.ECDSACurveP521,
},As per coding guidelines: "Run make verify for lint and generated-file checks".
📝 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.
| signerAlgorithm: configv1alpha1.KeyAlgorithmRSA, | |
| signerRSASize: 4096, | |
| servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | |
| servingECDSACurve: configv1alpha1.ECDSACurveP256, | |
| clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | |
| clientECDSACurve: configv1alpha1.ECDSACurveP521, | |
| { | |
| name: "RSA4096-signers", | |
| signerAlgorithm: configv1alpha1.KeyAlgorithmRSA, | |
| signerRSASize: 4096, | |
| servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | |
| servingECDSACurve: configv1alpha1.ECDSACurveP256, | |
| clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | |
| clientECDSACurve: configv1alpha1.ECDSACurveP521, | |
| }, |
🤖 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 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.
Source: Coding guidelines
| if tc.algorithm == configv1alpha1.KeyAlgorithmRSA { | ||
| o.Expect(newCert.Algorithm).To(o.Equal("RSA"), "expected RSA algorithm for %s/%s", cert.Namespace, cert.SecretName) | ||
| o.Expect(int32(newCert.KeySize)).To(o.Equal(tc.rsaSize), "expected RSA key size %d for %s/%s", tc.rsaSize, cert.Namespace, cert.SecretName) | ||
| e2e.Logf(" Certificate verified: RSA-%d", newCert.KeySize) | ||
| } else if tc.algorithm == configv1alpha1.KeyAlgorithmECDSA { | ||
| o.Expect(newCert.Algorithm).To(o.Equal("ECDSA"), "expected ECDSA algorithm for %s/%s", cert.Namespace, cert.SecretName) | ||
| expectedCurve := string(tc.ecdsaCurve) | ||
| o.Expect(newCert.Curve).To(o.Equal(expectedCurve), "expected ECDSA curve %s for %s/%s", expectedCurve, cert.Namespace, cert.SecretName) | ||
| e2e.Logf(" Certificate verified: ECDSA-%s", newCert.Curve) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Both assertion chains lack a final branch, so an unexpected algorithm passes with no verification. Each if/else if chain covers only KeyAlgorithmRSA and KeyAlgorithmECDSA. For any other value, no property assertion runs, verifiedCount still increments, and the spec reports success.
test/extended/pki/pki_kube_controller_manager.go#L176-L185: add a failing final branch fortc.algorithmintestKCMCertificates.test/extended/pki/pki_kube_controller_manager.go#L253-L262: add the same failing final branch fortestCase.expectedAlgorithmintestMixedKCMCertificates.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 177-177: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(newCert.KeySize)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
📍 Affects 1 file
test/extended/pki/pki_kube_controller_manager.go#L176-L185(this comment)test/extended/pki/pki_kube_controller_manager.go#L253-L262
🤖 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 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.
| err := applyPKIConfig(ctx, configClient, tc) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "error applying PKI config %s", tc.name) | ||
|
|
||
| e2e.Logf("PKI configuration %s applied successfully", tc.name) | ||
|
|
||
| time.Sleep(10 * time.Second) | ||
|
|
||
| e2e.Logf("Waiting for machine-config operator to reconcile PKI config...") | ||
| err = exutil.WaitForOperatorProgressingFalse(ctx, configClient, "machine-config") | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "machine-config operator did not reconcile PKI config %s", tc.name) | ||
| e2e.Logf("Operator has reconciled PKI configuration") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the fixed sleep with a generation-based wait.
time.Sleep(10 * time.Second) at Line 73 assumes the machine-config operator starts to report Progressing=True within 10 seconds. If the operator is slower, WaitForOperatorProgressingFalse observes the stale Progressing=False state from before the config change and returns immediately. The test then deletes Secrets before the new PKI profile is in effect, which produces flakes.
Wait for the operator status to observe the new config generation, then wait for Progressing=False. The same pattern exists at Lines 112-116.
🤖 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 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.
| mixedConfigs := []mixedPKITestConfig{ | ||
| { | ||
| name: "RSA4096-signer-P256-serving", | ||
| signerAlgorithm: configv1alpha1.KeyAlgorithmRSA, | ||
| signerRSASize: 4096, | ||
| servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | ||
| servingECDSACurve: configv1alpha1.ECDSACurveP256, | ||
| clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA, | ||
| clientECDSACurve: configv1alpha1.ECDSACurveP521, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The client profile in the mixed config is never validated.
clientAlgorithm and clientECDSACurve are set, but testMixedMCOCertificates only validates the serving and signer Secrets. The mixed test therefore does not prove that the client profile is applied. Add a client certificate Secret to the validation table, or remove the unused fields to avoid a false impression of coverage.
🤖 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 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.
| if tc.algorithm == configv1alpha1.KeyAlgorithmRSA { | ||
| o.Expect(newCert.Algorithm).To(o.Equal("RSA"), "expected RSA algorithm for %s/%s", cert.Namespace, cert.SecretName) | ||
| o.Expect(int32(newCert.KeySize)).To(o.Equal(tc.rsaSize), "expected RSA key size %d for %s/%s", tc.rsaSize, cert.Namespace, cert.SecretName) | ||
| e2e.Logf(" Certificate verified: RSA-%d", newCert.KeySize) | ||
| } else if tc.algorithm == configv1alpha1.KeyAlgorithmECDSA { | ||
| o.Expect(newCert.Algorithm).To(o.Equal("ECDSA"), "expected ECDSA algorithm for %s/%s", cert.Namespace, cert.SecretName) | ||
| expectedCurve := string(tc.ecdsaCurve) | ||
| o.Expect(newCert.Curve).To(o.Equal(expectedCurve), "expected ECDSA curve %s for %s/%s", expectedCurve, cert.Namespace, cert.SecretName) | ||
| e2e.Logf(" Certificate verified: ECDSA-%s", newCert.Curve) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a terminal else to the algorithm assertions. Both validation blocks branch on RSA and then ECDSA with no final else. If the expected algorithm is empty or a new value, no key assertion runs, verifiedCount still increments, and the test passes without validating anything.
test/extended/pki/pki_machine_config_operator.go#L177-L186: add anelsethat fails the test for an unexpectedtc.algorithmvalue.test/extended/pki/pki_machine_config_operator.go#L258-L267: add the sameelsefor an unexpectedtestCase.expectedAlgorithmvalue.
🛡️ Proposed fix for the uniform case
} else if tc.algorithm == configv1alpha1.KeyAlgorithmECDSA {
o.Expect(newCert.Algorithm).To(o.Equal("ECDSA"), "expected ECDSA algorithm for %s/%s", cert.Namespace, cert.SecretName)
expectedCurve := string(tc.ecdsaCurve)
o.Expect(newCert.Curve).To(o.Equal(expectedCurve), "expected ECDSA curve %s for %s/%s", expectedCurve, cert.Namespace, cert.SecretName)
e2e.Logf(" Certificate verified: ECDSA-%s", newCert.Curve)
+ } else {
+ g.Fail(fmt.Sprintf("unexpected key algorithm %q in test config %s", tc.algorithm, tc.name))
}🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 178-178: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(newCert.KeySize)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
📍 Affects 1 file
test/extended/pki/pki_machine_config_operator.go#L177-L186(this comment)test/extended/pki/pki_machine_config_operator.go#L258-L267
🤖 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 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.
|
/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 |
|
@kaleemsiddiqu: 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. |
|
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
|
4 participants
additional test for pki config covering kube controller manager and machine config operator
Summary by CodeRabbit