CNTRLPLANE-3741: PKI config tests for service-ca and kube-apiserver-operator#31341
Open
kaleemsiddiqu wants to merge 1 commit into
Open
CNTRLPLANE-3741: PKI config tests for service-ca and kube-apiserver-operator#31341kaleemsiddiqu wants to merge 1 commit into
kaleemsiddiqu wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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 extended e2e coverage for configurable PKI profiles, including PKI test registration, shared helpers, and kube-apiserver and service-ca certificate regeneration checks for uniform and mixed key settings. ChangesConfigurable PKI e2e coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
gangwgr
reviewed
Jun 25, 2026
Contributor
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
13280d2 to
1820a38
Compare
Author
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
test/extended/pki/helpers.go (1)
108-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildKeyConfigsilently returns an incomplete config for unrecognized algorithms.If
algorithmis neither RSA nor ECDSA, onlyAlgorithmis set and both key bodies stay zero-valued, which could produce a confusing apply result downstream. Consider asserting/erroring on the default case so misconfigured test cases fail fast.🤖 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/helpers.go` around lines 108 - 125, buildKeyConfig currently falls through for any algorithm other than KeyAlgorithmRSA or KeyAlgorithmECDSA, leaving KeyConfig partially populated and hiding misconfigured test inputs. Update buildKeyConfig to explicitly handle the supported algorithms and add a default branch that fails fast (for example by asserting or returning an error) when an unrecognized KeyAlgorithm is passed, so callers of buildKeyConfig get an immediate signal instead of an incomplete config.
🤖 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/helpers.go`:
- Around line 284-293: waitForSecretRegeneration currently treats any existing
secret as success, so it can return the old secret or a newly created secret
before cert data is ready. Update the helper in helpers.go to wait for a
genuinely regenerated secret by comparing against the deleted secret’s
UID/resourceVersion (or by confirming the secret is gone first), and then
require that the new secret contains certKey in Data before returning. Adjust
the call site in pki_kube_apiserver.go to pass the identifier needed to
distinguish the old secret from the recreated one.
- Around line 168-174: The PKI create-or-update flow in the helper currently
treats every Get failure as “missing” and falls back to Create, which can hide
RBAC, timeout, or server errors. Update the logic in the PKI helper path to only
call Create when configClient.ConfigV1alpha1().PKIs().Get returns
apierrors.IsNotFound(err), and otherwise return the original error; apply the
same NotFound-gated pattern in applyMixedPKIConfig as well.
In `@test/extended/pki/pki_kube_apiserver.go`:
- Around line 378-383: The deleteCertificateSecret failure path in
pki_kube_apiserver.go can mask a false pass because the loop only logs a warning
and continues without verifying anything. Update the certificate regeneration
checks around deleteCertificateSecret in the relevant test loop(s) so that a
missing/un-deletable expected secret causes the test to fail, or track whether
at least one certificate was successfully deleted and verified before allowing
the spec to pass. Apply the same fix to the duplicate verification block that
uses the same pattern elsewhere in the file.
- Around line 184-188: The deferred cleanup in `g.DeferCleanup` is using
`context.Background()`, which drops the test/spec context and leaves
`cleanupPKIConfiguration` unbounded; update the cleanup path to derive from the
existing spec context in `pki_kube_apiserver.go` using a detached context (for
example via `context.WithoutCancel`) and wrap it with a timeout before calling
`cleanupPKIConfiguration` so cleanup still has context values but cannot run
indefinitely.
- Around line 25-171: The package-level integrationTestCertificates slice in
pki_kube_apiserver.go is unused by the kube-apiserver test helpers. Remove the
dead variable, or update the relevant test setup functions to reference
integrationTestCertificates directly so the shared certificate list is actually
consumed. Use the integrationTestCertificates symbol and the kube-apiserver PKI
test helpers in this file to locate the change.
- Around line 173-174: The top-level ginkgo recovery hook in the `Describe` body
is unnecessary here. Remove the `defer g.GinkgoRecover()` from the `PKI
Configuration` `Describe` block in `pki_kube_apiserver.go`, and only add
`GinkgoRecover()` inside any spawned goroutine that may call `Fail` or Gomega in
the future.
In `@test/extended/pki/pki_service_ca.go`:
- Around line 331-333: The cleanup in the deferred namespace deletion is
ignoring a returned error, which can hide failed teardown; update the defer in
the pki service CA test to capture the result of
kubeClient.CoreV1().Namespaces().Delete and explicitly assert or log any error
instead of discarding it. Use the existing cleanup block around the testNS
deletion in the deferred func and keep the failure handling consistent with the
test’s other error checks so namespace leaks are surfaced.
- Around line 33-35: The deferred cleanup in the Ginkgo test uses
context.Background(), which disconnects it from the spec context. Update the
DeferCleanup block in pki_service_ca.go to derive a detached context from the
existing test context using context.WithoutCancel(ctx) and wrap it with a
bounded timeout, then pass that context into cleanupPKIConfiguration so cleanup
still runs independently without losing test-scoped values. Use the existing
test context variable from the surrounding spec and keep the cleanup logic
inside g.DeferCleanup.
---
Nitpick comments:
In `@test/extended/pki/helpers.go`:
- Around line 108-125: buildKeyConfig currently falls through for any algorithm
other than KeyAlgorithmRSA or KeyAlgorithmECDSA, leaving KeyConfig partially
populated and hiding misconfigured test inputs. Update buildKeyConfig to
explicitly handle the supported algorithms and add a default branch that fails
fast (for example by asserting or returning an error) when an unrecognized
KeyAlgorithm is passed, so callers of buildKeyConfig get an immediate signal
instead of an incomplete config.
🪄 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: 0bcf1c75-08b8-4f9c-b678-e0aeb6c80603
📒 Files selected for processing (4)
test/extended/include.gotest/extended/pki/helpers.gotest/extended/pki/pki_kube_apiserver.gotest/extended/pki/pki_service_ca.go
Contributor
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
1820a38 to
aa956c0
Compare
Author
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/extended/pki/pki_kube_apiserver.go (3)
196-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared regeneration/verify loop.
testKubeAPIServerCertificatesandtestMixedKubeAPIServerCertificatesdiffer only in how the expected algorithm/size/curve is sourced; the get-UID → delete → wait → verify body is duplicated. A small helper taking(cert operatorCertificate, expectedAlgorithm, expectedRSASize, expectedECDSACurve)would remove the duplication and keep timeout/verification logic in one place.🤖 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_apiserver.go` around lines 196 - 384, The regeneration/verify flow is duplicated in testKubeAPIServerCertificates and testMixedKubeAPIServerCertificates; extract the shared get-UID → deleteCertificateSecret → waitForSecretRegeneration → getCertificateFromSecret loop into a helper. Make the helper take operatorCertificate plus the expected algorithm/size/curve inputs so both callers can reuse the same timeout and verification logic while only differing in how expectations are populated.
232-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
if err != nilaround the assertion.
o.Expect(err).NotTo(o.HaveOccurred())already fails (and halts via panic) whenerr != nil, so the surrounding conditional adds nothing and only obscures intent. The same pattern repeats at Lines 336-339. Assert unconditionally.♻️ Suggested change
- oldSecret, err := kubeClient.CoreV1().Secrets(cert.Namespace).Get(ctx, cert.SecretName, metav1.GetOptions{}) - if err != nil { - o.Expect(err).NotTo(o.HaveOccurred(), "certificate %s/%s must exist before deletion", cert.Namespace, cert.SecretName) - } + oldSecret, err := kubeClient.CoreV1().Secrets(cert.Namespace).Get(ctx, cert.SecretName, metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred(), "certificate %s/%s must exist before deletion", cert.Namespace, cert.SecretName)🤖 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_apiserver.go` around lines 232 - 236, Remove the redundant `if err != nil` guard around the `o.Expect(err).NotTo(o.HaveOccurred(), ...)` assertion in the certificate secret lookup, and let the expectation run unconditionally before using `oldSecret.UID`. Apply the same cleanup to the repeated pattern later in this test (the `kubeClient.CoreV1().Secrets(...).Get` plus `o.Expect` block), so the intent is clearer and the assertion alone handles failures.
42-48: 🩺 Stability & Availability | 🔵 TrivialAccept the Ginkgo context in these
Itbodies.These
[Slow]specs can wait up to 20 minutes, socontext.TODO()bypasses Ginkgo timeout and cancellation handling. Thread the spec context through instead.♻️ Suggested change
- g.It("should validate uniform PKI configurations and certificate regeneration [apigroup:config.openshift.io][Slow][Skipped:MicroShift]", func() { - testUniformPKIConfigurations(context.TODO(), kubeClient, configClient) - }) + g.It("should validate uniform PKI configurations and certificate regeneration [apigroup:config.openshift.io][Slow][Skipped:MicroShift]", func(ctx context.Context) { + testUniformPKIConfigurations(ctx, kubeClient, configClient) + }) - g.It("should validate mixed PKI configurations and certificate regeneration [apigroup:config.openshift.io][Slow][Skipped:MicroShift]", func() { - testMixedPKIConfigurations(context.TODO(), kubeClient, configClient) - }) + g.It("should validate mixed PKI configurations and certificate regeneration [apigroup:config.openshift.io][Slow][Skipped:MicroShift]", func(ctx context.Context) { + testMixedPKIConfigurations(ctx, kubeClient, configClient) + })🤖 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_apiserver.go` around lines 42 - 48, The two Ginkgo It bodies are using context.TODO(), which bypasses Ginkgo’s timeout and cancellation handling for these long-running [Slow] specs. Update the uniform and mixed PKI specs to accept the Ginkgo context from the It body and pass that spec context through to testUniformPKIConfigurations and testMixedPKIConfigurations instead of creating a TODO context, so cancellation and timeout behavior is preserved.Source: Path instructions
🤖 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/extended/pki/pki_kube_apiserver.go`:
- Around line 196-384: The regeneration/verify flow is duplicated in
testKubeAPIServerCertificates and testMixedKubeAPIServerCertificates; extract
the shared get-UID → deleteCertificateSecret → waitForSecretRegeneration →
getCertificateFromSecret loop into a helper. Make the helper take
operatorCertificate plus the expected algorithm/size/curve inputs so both
callers can reuse the same timeout and verification logic while only differing
in how expectations are populated.
- Around line 232-236: Remove the redundant `if err != nil` guard around the
`o.Expect(err).NotTo(o.HaveOccurred(), ...)` assertion in the certificate secret
lookup, and let the expectation run unconditionally before using
`oldSecret.UID`. Apply the same cleanup to the repeated pattern later in this
test (the `kubeClient.CoreV1().Secrets(...).Get` plus `o.Expect` block), so the
intent is clearer and the assertion alone handles failures.
- Around line 42-48: The two Ginkgo It bodies are using context.TODO(), which
bypasses Ginkgo’s timeout and cancellation handling for these long-running
[Slow] specs. Update the uniform and mixed PKI specs to accept the Ginkgo
context from the It body and pass that spec context through to
testUniformPKIConfigurations and testMixedPKIConfigurations instead of creating
a TODO context, so cancellation and timeout behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 57aa33bc-1f72-4c5b-94ea-09a80b5afb76
📒 Files selected for processing (4)
test/extended/include.gotest/extended/pki/helpers.gotest/extended/pki/pki_kube_apiserver.gotest/extended/pki/pki_service_ca.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/extended/include.go
- test/extended/pki/helpers.go
- test/extended/pki/pki_service_ca.go
openshift-ci
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
aa956c0 to
adbcf03
Compare
Author
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
Contributor
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
adbcf03 to
ba29d50
Compare
Author
|
/test e2e-gcp-ovn-techpreview-pkiconfig |
Author
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
Author
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
1 similar comment
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
Author
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
1 similar comment
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
Author
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
Author
|
/retest |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr, kaleemsiddiqu 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 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f0278a2
New tests seen in this PR at sha: f0278a2
|
…perator Tests for kube-apiserver and service-ca for pki config Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
f0278a2 to
04f645f
Compare
openshift-ci-robot
removed
the
verified
Contributor
|
New changes are detected. LGTM label has been removed. |
Author
|
/test e2e-gcp-ovn-techpreview-pkiconfig |
Contributor
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 04f645f
New tests seen in this PR at sha: 04f645f
|
Author
|
/retest |
Contributor
|
@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: 04f645f
New tests seen in this PR at sha: 04f645f
|
1 similar comment
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 04f645f
New tests seen in this PR at sha: 04f645f
|
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
3 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.
Tests for kube-apiserver and service-ca added
Summary by CodeRabbit
openshift/pkiconfig) to validate configurable PKI certificate regeneration.kube-apiserverPKI regeneration by deleting targeted certificate Secrets and confirming regenerated certificate key properties for uniform and mixed configurations.service-caunder theConfigurablePKIfeature gate, including regeneration and validation of signing and serving certificates (RSA and ECDSA key/curve parameters, with mixed overrides).