Skip to content

CNTRLPLANE-1739: e2e additional tests for pki config #31491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kaleemsiddiqu wants to merge 1 commit into
base: main
Choose a base branch
from
+547 −0
Open

CNTRLPLANE-1739: e2e additional tests for pki config #31491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
271 changes: 271 additions & 0 deletions test/extended/pki/pki_kube_controller_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,271 @@
package pki

import (
"context"
"time"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
e2e "k8s.io/kubernetes/test/e2e/framework"

configv1alpha1 "github.com/openshift/api/config/v1alpha1"
configclient "github.com/openshift/client-go/config/clientset/versioned"
exutil "github.com/openshift/origin/test/extended/util"
)

const (
kubeControllerManagerOperatorNamespace = "openshift-kube-controller-manager-operator"
)

var _ = g.Describe("[sig-cluster-lifecycle][OCPFeatureGate:ConfigurablePKI][Serial][Disruptive][Suite:openshift/pkiconfig] PKI Configuration", g.Ordered, func() {
oc := exutil.NewCLIWithoutNamespace("kube-controller-manager-pki")

var kubeClient kubernetes.Interface
var configClient configclient.Interface

g.BeforeAll(func(ctx context.Context) {
kubeClient = oc.AdminKubeClient()
configClient = oc.AdminConfigClient()

g.DeferCleanup(func() {
cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Minute)
defer cancel()
cleanupPKIConfiguration(cleanupCtx, configClient)
})
})

g.It("should validate uniform PKI configurations and certificate regeneration for kube-controller-manager [apigroup:config.openshift.io][Skipped:MicroShift]", func(ctx context.Context) {
testUniformKCMPKIConfigurations(ctx, kubeClient, configClient)
})

g.It("should validate mixed PKI configurations and certificate regeneration for kube-controller-manager [apigroup:config.openshift.io][Skipped:MicroShift]", func(ctx context.Context) {
testMixedKCMPKIConfigurations(ctx, kubeClient, configClient)
})
})

func testUniformKCMPKIConfigurations(ctx context.Context, kubeClient kubernetes.Interface, configClient configclient.Interface) {
e2e.Logf("Testing uniform PKI configurations for kube-controller-manager...")

testConfigs := []pkiTestConfig{
{
name: "RSA-4096",
algorithm: configv1alpha1.KeyAlgorithmRSA,
rsaSize: 4096,
},
{
name: "ECDSA-P384",
algorithm: configv1alpha1.KeyAlgorithmECDSA,
ecdsaCurve: configv1alpha1.ECDSACurveP384,
},
}

for _, tc := range testConfigs {
e2e.Logf("\n=== Testing configuration: %s ===", tc.name)

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 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)
Comment on lines +73 to +77

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 in testUniformKCMPKIConfigurations and wait until the operator status reflects the applied PKI generation before you wait for Progressing=False.
  • test/extended/pki/pki_kube_controller_manager.go#L112-L116: apply the same wait in testMixedKCMPKIConfigurations.
📍 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.

e2e.Logf("Operator has reconciled PKI configuration")

e2e.Logf("Testing kube-controller-manager certificate regeneration with %s...", tc.name)
testKCMCertificates(ctx, kubeClient, tc)

e2e.Logf("Configuration %s tested successfully", tc.name)
}

e2e.Logf("\nAll uniform PKI configuration tests passed successfully")
}

func testMixedKCMPKIConfigurations(ctx context.Context, kubeClient kubernetes.Interface, configClient configclient.Interface) {
e2e.Logf("Testing mixed PKI configurations for kube-controller-manager...")

mixedConfigs := []mixedPKITestConfig{
{
name: "RSA4096-signers",
signerAlgorithm: configv1alpha1.KeyAlgorithmRSA,
signerRSASize: 4096,
servingAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
servingECDSACurve: configv1alpha1.ECDSACurveP256,
clientAlgorithm: configv1alpha1.KeyAlgorithmECDSA,
clientECDSACurve: configv1alpha1.ECDSACurveP521,
Comment on lines +95 to +100

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

},
}

for _, tc := range mixedConfigs {
e2e.Logf("\n=== Testing mixed configuration: %s ===", tc.name)

err := applyMixedPKIConfig(ctx, configClient, tc)
o.Expect(err).NotTo(o.HaveOccurred(), "error applying mixed PKI config %s", tc.name)

e2e.Logf("Mixed PKI configuration %s applied successfully", tc.name)

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)
e2e.Logf("Operator has reconciled PKI configuration")

e2e.Logf("Testing kube-controller-manager certificate regeneration with mixed config %s...", tc.name)
testMixedKCMCertificates(ctx, kubeClient, tc)

e2e.Logf("Mixed configuration %s tested successfully", tc.name)
}

e2e.Logf("\nAll mixed PKI configuration tests passed successfully")
}

func testKCMCertificates(ctx context.Context, kubeClient kubernetes.Interface, tc pkiTestConfig) {
// Both secrets are managed by the cert rotation controller in the
// operator namespace. The csr-signer in the operand namespace
// (openshift-kube-controller-manager) is a resource-sync copy.
// Test csr-signer (child) before csr-signer-signer (parent CA) to avoid
// cascade: deleting the parent CA triggers automatic re-signing of the
// child, which may reuse the existing key pair rather than generating a
// new one from the PKI profile.
testCerts := []operatorCertificate{
{
Namespace: kubeControllerManagerOperatorNamespace,
SecretName: "csr-signer",
CertKey: "tls.crt",
Category: "signer",
},
{
Namespace: kubeControllerManagerOperatorNamespace,
SecretName: "csr-signer-signer",
CertKey: "tls.crt",
Category: "signer",
},
}

verifiedCount := 0
for _, cert := range testCerts {
e2e.Logf(" Testing %s certificate: %s/%s", cert.Category, 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)
oldUID := string(oldSecret.UID)

err = deleteCertificateSecret(ctx, kubeClient, cert.Namespace, cert.SecretName)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to delete certificate %s/%s", cert.Namespace, cert.SecretName)
e2e.Logf(" Certificate deleted")

e2e.Logf(" Waiting for certificate regeneration...")

regenCtx, regenCancel := context.WithTimeout(ctx, 3*time.Minute)
err = waitForSecretRegeneration(regenCtx, kubeClient, cert.Namespace, cert.SecretName, cert.CertKey, oldUID)
regenCancel()
o.Expect(err).NotTo(o.HaveOccurred(), "error waiting for certificate %s/%s regeneration", cert.Namespace, cert.SecretName)
e2e.Logf(" Certificate regenerated")

newCert, err := getCertificateFromSecret(ctx, kubeClient, cert.Namespace, cert.SecretName, cert.CertKey)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting regenerated certificate %s/%s", cert.Namespace, cert.SecretName)

o.Expect(newCert.IsCA).To(o.BeTrue(), "signer certificate %s/%s should be a CA", cert.Namespace, cert.SecretName)

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)
}
Comment on lines +176 to +185

@coderabbitai coderabbitai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 for tc.algorithm in testKCMCertificates.
  • test/extended/pki/pki_kube_controller_manager.go#L253-L262: add the same failing final branch for testCase.expectedAlgorithm in testMixedKCMCertificates.
🧰 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.


verifiedCount++

time.Sleep(5 * time.Second)
}

o.Expect(verifiedCount).To(o.BeNumerically(">", 0), "at least one certificate must be verified")
e2e.Logf(" Configuration test completed: %d certificates verified", verifiedCount)
}

func testMixedKCMCertificates(ctx context.Context, kubeClient kubernetes.Interface, tc mixedPKITestConfig) {
testCerts := []struct {
cert operatorCertificate
expectedAlgorithm configv1alpha1.KeyAlgorithm
expectedRSASize int32
expectedECDSACurve configv1alpha1.ECDSACurve
}{
{
cert: operatorCertificate{
Namespace: kubeControllerManagerOperatorNamespace,
SecretName: "csr-signer",
CertKey: "tls.crt",
Category: "signer",
},
expectedAlgorithm: tc.signerAlgorithm,
expectedRSASize: tc.signerRSASize,
expectedECDSACurve: tc.signerECDSACurve,
},
{
cert: operatorCertificate{
Namespace: kubeControllerManagerOperatorNamespace,
SecretName: "csr-signer-signer",
CertKey: "tls.crt",
Category: "signer",
},
expectedAlgorithm: tc.signerAlgorithm,
expectedRSASize: tc.signerRSASize,
expectedECDSACurve: tc.signerECDSACurve,
},
}

verifiedCount := 0
for _, testCase := range testCerts {
cert := testCase.cert
e2e.Logf(" Testing %s certificate: %s/%s", cert.Category, 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)
oldUID := string(oldSecret.UID)

err = deleteCertificateSecret(ctx, kubeClient, cert.Namespace, cert.SecretName)
o.Expect(err).NotTo(o.HaveOccurred(), "failed to delete certificate %s/%s", cert.Namespace, cert.SecretName)
e2e.Logf(" Certificate deleted")

e2e.Logf(" Waiting for certificate regeneration...")

regenCtx, regenCancel := context.WithTimeout(ctx, 3*time.Minute)
err = waitForSecretRegeneration(regenCtx, kubeClient, cert.Namespace, cert.SecretName, cert.CertKey, oldUID)
regenCancel()
o.Expect(err).NotTo(o.HaveOccurred(), "error waiting for certificate %s/%s regeneration", cert.Namespace, cert.SecretName)
e2e.Logf(" Certificate regenerated")

newCert, err := getCertificateFromSecret(ctx, kubeClient, cert.Namespace, cert.SecretName, cert.CertKey)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting regenerated certificate %s/%s", cert.Namespace, cert.SecretName)

o.Expect(newCert.IsCA).To(o.BeTrue(), "signer certificate %s/%s should be a CA", cert.Namespace, cert.SecretName)

if testCase.expectedAlgorithm == configv1alpha1.KeyAlgorithmRSA {
o.Expect(newCert.Algorithm).To(o.Equal("RSA"), "expected RSA algorithm for %s certificate %s/%s", cert.Category, cert.Namespace, cert.SecretName)
o.Expect(int32(newCert.KeySize)).To(o.Equal(testCase.expectedRSASize), "expected RSA key size %d for %s certificate %s/%s", testCase.expectedRSASize, cert.Category, cert.Namespace, cert.SecretName)
e2e.Logf(" %s certificate verified: RSA-%d", cert.Category, newCert.KeySize)
} else if testCase.expectedAlgorithm == configv1alpha1.KeyAlgorithmECDSA {
o.Expect(newCert.Algorithm).To(o.Equal("ECDSA"), "expected ECDSA algorithm for %s certificate %s/%s", cert.Category, cert.Namespace, cert.SecretName)
expectedCurve := string(testCase.expectedECDSACurve)
o.Expect(newCert.Curve).To(o.Equal(expectedCurve), "expected ECDSA curve %s for %s certificate %s/%s", expectedCurve, cert.Category, cert.Namespace, cert.SecretName)
e2e.Logf(" %s certificate verified: ECDSA-%s", cert.Category, newCert.Curve)
}

verifiedCount++

time.Sleep(5 * time.Second)
}

o.Expect(verifiedCount).To(o.BeNumerically(">", 0), "at least one certificate must be verified")
e2e.Logf(" Configuration test completed: %d certificates verified", verifiedCount)
}
Loading