PKI config tests#31341
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds extended e2e coverage for configurable PKI key algorithms and certificate regeneration. The new tests apply PKI profiles, delete certificate secrets, wait for regeneration, and verify RSA/ECDSA details for kube-apiserver and service-ca certificates. ChangesExtended PKI e2e coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
13280d2 to
1820a38
Compare
|
/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
|
Scheduling required tests: |
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
1820a38 to
aa956c0
Compare
|
/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
|
Scheduling required tests: |
Tests for kube-apiserver and service-ca added Signed-off-by: Kaleemullah Siddiqui <ksiddiqu@redhat.com>
kaleemsiddiqu
force-pushed
the
pki-tests-new
branch
from
aa956c0 to
adbcf03
Compare
|
@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. |
|
/test e2e-gcp-ovn-techpreview-serial-1of2 |
|
Scheduling required tests: |
2 participants
Tests for kube-apiserver and service-ca added
Summary by CodeRabbit