OPRUN-3941: [OTE] Add webhook tests (without should be tolerant to openshift-service-ca certificate rotation) #442

camilamacedo86 · 2025-08-15T17:40:32Z

PR Description

It reintroduces the reverted PR #434,
but without the failing test (as agreed in Slack thread).

The base reference for this file is #442, which was reverted [#434].

Delta Summary (New vs Old)

Removed test
❌ It("should be tolerant to openshift-service-ca certificate rotation", …)
‣ Failed → reason for the revert

Added (requested in #424 review
✅ Diagnostics & dumps

AfterEach failure hook → pod logs, resource describes, webhook config dumps
DeferCleanup in TLS secret test → dump cert details on failure
printTLSCertInfo() util → parse & log Subject/Issuer/Validity/SANs

✅ Const

webhookServiceCert = "webhook-operator-webhook-service-cert" (instead of fixed string literal)

Renamed / Changed
🔄 webhookTestGVRV1 → webhookTestV1 (requested in #424 review
🔄 webhookTestGVRV2 → webhookTestV2 (requested in #424 review
🔄 newWebhookTestV1() → newWebhookTest() (requested in #424 review
🔄 Namespace deletion → wait.PollUntilContextTimeout → Eventually (5m timeout, avoid flakes)
🔄 TLS secret recreation wait → 2m → 5m (to avoid flakes)

📌 To check the delta, compare the webhook.go file from the old merged+reverted PR and the new version in this PR. All the other files are utils.

Tests

Tested locally
Passing in e2e / pre-merge
Verified with /payload-aggregate See details on https://pr-payload-tests.ci.openshift.org/runs/ci/af81eff0-7b94-11f0-93ec-12968849859f-0

openshift-ci-robot · 2025-08-15T17:40:35Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

Increase the timeout for "should be tolerant to openshift-service-ca certificate rotation" from 2 minutes to 5 minutes to reduce flakes.

Add dumping of container logs and kubectl describe pods output for better diagnostics.

Include targeted certificate details dump (tls.crt parse) when failures occur.

Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor of code from openshift/origin#30059.

Assisted-by: Gemini

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 · 2025-08-15T17:41:02Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

Skip "should be tolerant to openshift-service-ca certificate rotation". More info: https://issues.redhat.com/browse/OCPBUGS-60564

Add dumping of container logs and kubectl describe pods output for better diagnostics.

Include targeted certificate details dump (tls.crt parse) when failures occur.

Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor of code from openshift/origin#30059.

Assisted-by: Gemini

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 · 2025-08-15T17:41:15Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

Skip "should be tolerant to openshift-service-ca certificate rotation". More info: https://issues.redhat.com/browse/OCPBUGS-60564

Add dumping of container logs and kubectl describe pods output for better diagnostics.

Include targeted certificate details dump (tls.crt parse) when failures occur.

Add additional check to verify webhook responsiveness after certificate rotation.

This change is a refactor of code from openshift/origin#30059.

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.

camilamacedo86 · 2025-08-15T17:42:46Z

openshift/tests-extension/test/webhooks.go

+
+		It("should be tolerant to openshift-service-ca certificate rotation", func(ctx SpecContext) {
+			// FIXME: https://issues.redhat.com/browse/OCPBUGS-60564
+			Skip("Skipping this test until we ensure that it can work reliably")


This test must be skipped for now.
We will work on it in a follow up

camilamacedo86 · 2025-08-15T17:43:46Z

/payload-aggregate-with-prs periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 5

camilamacedo86 · 2025-08-15T18:58:40Z

/payload-aggregate-with-prs periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 5

camilamacedo86 · 2025-08-15T18:59:39Z

/payload-aggregate-with-prs periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

camilamacedo86 · 2025-08-15T21:14:32Z

/payload-aggregate-with-prs periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

camilamacedo86 · 2025-08-15T21:35:10Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

openshift-ci · 2025-08-15T21:35:14Z

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b96934e0-7a1f-11f0-91bf-2cddbeabd8dd-0

camilamacedo86 · 2025-08-16T08:04:04Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 10

openshift-ci · 2025-08-16T08:04:40Z

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/94147170-7a77-11f0-9e36-38ae811e5825-0

camilamacedo86 · 2025-08-16T13:08:14Z

/restest-required

openshift-ci-robot · 2025-08-17T17:58:30Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

(Improvements)

Add dumping of container logs and kubectl describe pods output for better diagnostics.

Include targeted certificate details dump (tls.crt parse) when failures occur.

Add additional check to verify webhook responsiveness after certificate rotation.

Add tests without problematic:
"name": "[sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA][Skipped:Disconnected][Serial] OLMv1 operator with webhooks should be tolerant to openshift-service-ca certificate rotation",
This change is a refactor of code from openshift/origin#30059.

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 · 2025-08-17T17:59:47Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

(Improvements)

Add dumping of container logs and kubectl describe pods output for better diagnostics.

Include targeted certificate details dump (tls.crt parse) when failures occur.

Add additional check to verify webhook responsiveness after certificate rotation.

Add tests without problematic:
"name": "[sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA][Skipped:Disconnected][Serial] OLMv1 operator with webhooks should be tolerant to openshift-service-ca certificate rotation",
Reason: It did not pass in the latest try on the sippy, and we created an OCPBug to analyse the scenario.

This change is a refactor of code from openshift/origin#30059.

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 · 2025-08-17T18:00:53Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

This change is a refactor of code from openshift/origin#30059 with improvements and logs for better diagnostics.

Add tests without that one which faced issues, which is:
"name": "[sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA][Skipped:Disconnected][Serial] OLMv1 operator with webhooks should be tolerant to openshift-service-ca certificate rotation",
We created an OCPBug to analyse the scenario.

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.

camilamacedo86 · 2025-08-17T18:04:56Z

/payload-aggregate periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial 5

openshift-ci · 2025-08-17T18:04:59Z

@camilamacedo86: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.20-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/af81eff0-7b94-11f0-93ec-12968849859f-0

openshift-ci-robot · 2025-08-18T05:56:17Z

@camilamacedo86: This pull request references OPRUN-3941 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 epic to target the "4.20.0" version, but no target version was set.

In response to this:

This change is a refactor of code from openshift/origin#30059 with improvements and logs for better diagnostics.

Add tests without that one which faced issues, which is:
"name": "[sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA][Skipped:Disconnected][Serial] OLMv1 operator with webhooks should be tolerant to openshift-service-ca certificate rotation",
We created an OCPBug to analyse the scenario.

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.

anik120 · 2025-08-18T13:00:28Z

openshift/tests-extension/test/olmv1-incompatible.go

@@ -85,7 +81,7 @@ var _ = Describe("[sig-olmv1][OCPFeatureGate:NewOLM][Skipped:Disconnected] OLMv1
 		DeferCleanup(fileCleanup)
 		By(fmt.Sprintf("created operator tarball %q", fileOperator))

-		By(fmt.Sprintf("starting the operator build with %q via RAW URL", cmdLine))
+		By("starting the operator build via RAW URL")


what is RAW URL?

A RAW URL usually means a direct link to the unprocessed (raw) content of a file, without any extra formatting, preview, or surrounding interface.

Example:

Regular URL: https://github.com/user/repo/blob/main/file.txt

Raw URL:https://raw.githubusercontent.com/user/repo/main/file.txt

The change here is just because we are using the utils, so we don't need or have cmdLine anymore.

Requested in the PR to add the follow ups (which is in hold because we cannot add the test that fails): #430 (comment)

In this particular case, it is invoking a k8s API via a URL (using --raw) rather than allowing the client (kubectl or oc) to determine the URL to use. This is needed because building an image is a specific Openshift operation not supported by kubectl. Although it is supported by oc, I wanted to make it generic enough to allow kubectl to be used. Because of this raw URL use, we can't directly use the k8sClient API.

anik120 · 2025-08-18T13:06:16Z

openshift/tests-extension/test/webhooks.go

+			if CurrentSpecReport().Failed() {
+				By("dumping pod logs for debugging")
+				helpers.GetAllPodLogs(ctx, webhookOperatorInstallNamespace)
+				helpers.DescribePods(ctx, webhookOperatorInstallNamespace)
+				helpers.DescribeClusterCatalogs(ctx)
+				helpers.DescribeAllClusterExtensions(ctx, webhookOperatorInstallNamespace)
+				By("dumping webhook diagnostics")
+				// Additional diagnostics specific for this test
+				helpers.RunAndPrint(ctx, "get", "secret", "-n", webhookOperatorInstallNamespace, webhookServiceCert, "-oyaml")
+				helpers.RunAndPrint(ctx, "get", "mutatingwebhookconfigurations.admissionregistration.k8s.io", "-oyaml")
+				helpers.RunAndPrint(ctx, "get", "validatingwebhookconfigurations.admissionregistration.k8s.io", "-oyaml")
+			}


Rethinking this: do we need all these? Fyi the original PR has the executil.DumpPodsLogs etc etc which I pulled in without any changes...but rethinking this now: this is over explaining things. We'll get all of these from the must gather of the test cluster.

Not exactly — the must-gather isn’t collected before we run all the cleanups.
Yes, we need this, but it will only appear if/when a test fails, and only for that specific test (similar to how it works in ocp/origin) if we develop it this way.

There are definitely circumstances where we need to gather this data before the must-gather, e.g. in the case he pod is deleted during the test.

Also note that secrets are not part of the must gather.

Makes sense.

anik120 · 2025-08-18T13:14:19Z

openshift/tests-extension/test/webhooks.go

+		It("should be tolerant to tls secret deletion", func(ctx SpecContext) {
+			certificateSecretName := webhookServiceCert
+			By("ensuring secret exists before deletion attempt")
+			Eventually(func(g Gomega) {
+				secret := &corev1.Secret{}
+				err := k8sClient.Get(ctx, client.ObjectKey{Name: certificateSecretName, Namespace: webhookOperatorInstallNamespace}, secret)
+				g.Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to get secret %s/%s", webhookOperatorInstallNamespace, certificateSecretName))
+			}).WithTimeout(1 * time.Minute).WithPolling(5 * time.Second).Should(Succeed())
+
+			By("checking webhook is responsive through secret recreation after manual deletion")
+			tlsSecret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      certificateSecretName,
+					Namespace: webhookOperatorInstallNamespace,
+				},
+			}
+			err := k8sClient.Delete(ctx, tlsSecret, client.PropagationPolicy(metav1.DeletePropagationBackground))
+			Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
+
+			DeferCleanup(func() {
+				// Specific check for this test
+				if CurrentSpecReport().Failed() {
+					By("dumping certificate details for debugging")
+					secret := &corev1.Secret{}
+					if err := k8sClient.Get(ctx, client.ObjectKey{
+						Name:      webhookServiceCert,
+						Namespace: webhookOperatorInstallNamespace,
+					}, secret); err == nil {
+						if crt, ok := secret.Data["tls.crt"]; ok && len(crt) > 0 {
+							printTLSCertInfo(crt)
+						} else {
+							_, _ = GinkgoWriter.Write([]byte("[diag] tls.crt key not found or empty in secret\n"))
+						}
+					} else {
+						fmt.Fprintf(GinkgoWriter, "[diag] failed to get secret for cert dump: %v\n", err)
+					}
+				}
+			})
+
+			By("waiting for the webhook operator's service certificate secret to be recreated and populated")
+			Eventually(func(g Gomega) {
+				secret := &corev1.Secret{}
+				err := k8sClient.Get(ctx, client.ObjectKey{Name: certificateSecretName, Namespace: webhookOperatorInstallNamespace}, secret)
+				if apierrors.IsNotFound(err) {
+					GinkgoLogr.Info(fmt.Sprintf("Secret %s/%s not found yet (still polling for recreation)", webhookOperatorInstallNamespace, certificateSecretName))
+					return
+				}
+				g.Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to get webhook service certificate secret %s/%s: %v", webhookOperatorInstallNamespace, certificateSecretName, err))
+				g.Expect(secret.Data).ToNot(BeEmpty(), "expected webhook service certificate secret data to not be empty after recreation")
+			}).WithTimeout(5*time.Minute).WithPolling(10*time.Second).Should(Succeed(), "webhook service certificate secret did not get recreated and populated within timeout")
+
+			Eventually(func(g Gomega) {
+				resourceName := fmt.Sprintf("tls-deletion-test-%s", rand.String(5))
+				resource := newWebhookTest(resourceName, webhookOperatorInstallNamespace, true)
+
+				_, err := dynamicClient.Resource(webhookTestV1).Namespace(webhookOperatorInstallNamespace).Create(ctx, resource, metav1.CreateOptions{})
+				g.Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to create test resource %s: %v", resourceName, err))
+
+				err = dynamicClient.Resource(webhookTestV1).Namespace(webhookOperatorInstallNamespace).Delete(ctx, resource.GetName(), metav1.DeleteOptions{})
+				g.Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred(), fmt.Sprintf("failed to delete test resource %s: %v", resourceName, err))
+			}).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed())
+		})
+	})


Can we stick to whatever was here only please: https://github.com/openshift/origin/pull/30059/files#diff-6dd6fa78ac85235012d3c9910f8510bc1640c830a83888681bd5922cec0dffcbR166-R188

All of these print stuff is okay when debugging but feels unclean in a prod environment to run every time. And is also a lot of code to review, which means a lot of surface area for bugs.

The print is required when errors occur, otherwise we don’t get the information needed for debugging.
The output will only appear if the test fails — that’s the key point to keep in mind.

See the snippet below and note the guard: if CurrentSpecReport().Failed().
The only change here compared to the previously approved PR is the code inside that if block.

By the way, you were one of the folks who originally requested additional dumps and logs 🙂

As the information is available when the test fails, and is silent otherwise, I think we are fine.

Makes sense.

tmshort · 2025-08-19T14:30:50Z

openshift/tests-extension/test/olmv1-incompatible.go

The changes to this file are not strictly related to the subject of this PR. It is mostly some cleanup using other APIs (i.e. to find a command-line client), to avoid duplication. The changes are fine. So, while it may not belong as part of this PR, I'm not going to push for it to be separated.

Also, this could've used some of the new helpers that were written in this PR (e.g. NewClusterCatalog), but things can be combined later as needed.

(Admittedly, I did write this fairly independently of the rest of the code, where there wasn't that much infrastructure in-place.)

tmshort · 2025-08-19T14:35:36Z

openshift/tests-extension/pkg/helpers/commands.go

+		// Verify that the tool is working by checking its version.
+		if err := exec.Command(t, "version", "--client").Run(); err == nil {
+			return t, nil
+		}


Not sure this is strictly necessary, but it does ensure that the client can be executed.

I think is good right to ensure that all is fine mainly because we can either run locally

tmshort · 2025-08-19T14:39:34Z

openshift/tests-extension/pkg/helpers/commands.go

+// RunAndPrint runs a `kubectl/oc` command via RunK8sCommand and writes both stdout and stderr
+// to the GinkgoWriter. It also prints the exact command being run.
+func RunAndPrint(ctx context.Context, args ...string) {
+	fmt.Fprintf(GinkgoWriter, "\n[diag] running: oc %s\n", strings.Join(args, " "))


Technically, it's not running oc, but could be running kubectl, so the statement "It also prints the exact command being run" is in correct.
A nit, some arguments might contain spaces, that might accidentally be interpreted as separate commands when printed in this manner. I wonder if it's worthwhile to add quotes around each of args?

Here should be

fmt.Fprintf(GinkgoWriter, "\n[diag] running: %s\n", strings.Join(args, " "))

Only.

I think we can add that I wonder if it's worthwhile to add quotes around each of args?

tmshort · 2025-08-19T14:41:31Z

openshift/tests-extension/pkg/helpers/dump.go

+	"strings"
+
+	//nolint:staticcheck // ST1001: dot-imports for readability
+	. "github.com/onsi/ginkgo/v2"


The alternative would be to use g as the import reference. That might improve readability more than using . (and the consequences surrounding that).

IHMO the g is hard to understand because one we need to add g other o. But we are doing it in all files (not only on those), while I agree that can be debatable can we agree taht if we change that would be in a follow up so we can change all places?

tmshort · 2025-08-19T14:42:33Z

openshift/tests-extension/pkg/helpers/dump.go

+}
+
+func subHeader(format string, a ...any) {
+	fmt.Fprintf(GinkgoWriter, "\n--- %s ---\n", fmt.Sprintf(format, a...))


Nit: I almost want the --- and === to be the same length.

tmshort · 2025-08-19T14:47:11Z

openshift/tests-extension/pkg/helpers/dump.go

+}
+
+// DescribeClusterCatalogs lists all ClusterCatalogs and runs `describe` on each.
+func DescribeClusterCatalogs(ctx context.Context) {


Nit, below "All" is used in the function name for ClusterExtensions, but not ClusterCatalogs.

We could rename for DescribeAllClusterCatalogs

tmshort · 2025-08-19T14:53:06Z

openshift/tests-extension/test/webhooks.go

+// printTLSCertInfo parses a PEM-encoded TLS certificate and prints useful debug info.
+// It shows validity period and SANs (DNS/IP) to help debug webhook cert issues.


This function feels as though it should be in the helpers package.

It seems valid only for this specific case scenario that is why it is here

tmshort · 2025-08-19T14:56:18Z

openshift/tests-extension/test/webhooks.go

+	fmt.Fprintf(GinkgoWriter, "[diag] Subject: %s\n", cert.Subject.String())
+	fmt.Fprintf(GinkgoWriter, "[diag] Issuer: %s\n", cert.Issuer.String())
+	fmt.Fprintf(GinkgoWriter, "[diag] Valid From: %s\n", cert.NotBefore.Format(time.RFC3339))
+	fmt.Fprintf(GinkgoWriter, "[diag] Valid Until: %s\n", cert.NotAfter.Format(time.RFC3339))
+	fmt.Fprintf(GinkgoWriter, "[diag] IsCA: %t\n", cert.IsCA)


nit: serial number might be useful here.

Interest yep we might could either add as well

tmshort · 2025-08-19T15:13:31Z

openshift/tests-extension/test/webhooks.go

+	webhookServiceCert         = "webhook-operator-webhook-service-cert"
+)
+
+var _ = Describe("[sig-olmv1][OCPFeatureGate:NewOLMWebhookProviderOpenshiftServiceCA][Skipped:Disconnected][Serial] OLMv1 operator with webhooks",


+1 [Serial]

tmshort · 2025-08-19T15:17:42Z

openshift/tests-extension/test/webhooks.go

+				helpers.DescribeAllClusterExtensions(ctx, webhookOperatorInstallNamespace)
+				By("dumping webhook diagnostics")
+				// Additional diagnostics specific for this test
+				helpers.RunAndPrint(ctx, "get", "secret", "-n", webhookOperatorInstallNamespace, webhookServiceCert, "-oyaml")


Should this be using the printTLSCertInfo function below? I'm a bit concerned about dumping out secret contents (although admittedly, it's only a temporary secret for the lifetime of the service in question).

I think is is fine for this reason, and it could help us diagnostic issues

tmshort · 2025-08-19T15:25:33Z

openshift/tests-extension/test/webhooks.go

+			k8sClient = env.Get().K8sClient
+			restCfg := env.Get().RestCfg
+			var err error
+			dynamicClient, err = dynamic.NewForConfig(restCfg)


Why is this being used rather than k8sClient? We should just have to add the scheme and we'd be good to go?

It's only used for 2 types of resources.

Controller-runtime client.Client (k8sClient) -> Only works with structured, typed client. (e.g., corev1.Secret, olmv1.ClusterCatalog).

dynamic.Interface (dynamicClient) -> This is a generic, unstructured client. -> We need it because we are constructing the webhookTestV1

We are testing webhook behavior for a CRD (WebhookTest) defined by an operator. That is why we used it. To do:

https://github.com/openshift/operator-framework-operator-controller/pull/442/files/697e82c8242753c96b004573a908a9fd116e1cc7#diff-27e72954a06af3e527121bb5416f87b6b262bba32643109274b9d7a54fc464d9R235-R263

The dynamic client provides a way to create, get, and delete arbitrary resources using only their Group/Version/Resource identifiers

Hmmm... so we can't add that API to our scheme? These are coreos APIs... But I see your point.

For our APIs, we add the schema once in the test env and then use typed objects.

Example: usage in helpers/cluster_extension.go works because we register the API in env/cluster.go.

For the operator under test, it doesn’t seems to make sense to import and register its schema for all tests. We don’t know where its Go types are, and we don’t want to depend on them. Instead, we just use the dynamic client with unstructured.

I think we can might say

Our APIs (same repo) + K8s : add schema + typed k8sClient is fine.

Other operator’s CRDs/thridy-parties : it is easier use dynamic client.

tmshort · 2025-08-19T15:32:35Z

Most of the nits, can be fixed up later, but I do question the use of dynamicClient, when the k8sClient ought to be usable with the correct scheme.
Also, since we're dumping out a secret, specifically a certificate, it would be better presented as a certificate than some random secret, which might be flagged as a security/data leak.

tmshort · 2025-08-19T15:50:30Z

Dumping out the secret as a secret is not all that useful, having it printed out as a certificate will be much more useful, and since the key is not included in the dump, we won’t be potentially tagged with data leakage. If you can fix that, I’d be OK with the PR, and the nits fixed later

- Add dumping of container logs and `kubectl describe pods` output for better diagnostics. - Include targeted certificate details dump (`tls.crt` parse) when failures occur. - Add additional check to verify webhook responsiveness after certificate rotation. This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini

camilamacedo86 · 2025-08-19T15:59:35Z

openshift/tests-extension/test/webhooks.go

+
+	fmt.Fprintf(GinkgoWriter, "[diag] Subject: %s\n", cert.Subject.String())
+	fmt.Fprintf(GinkgoWriter, "[diag] Issuer: %s\n", cert.Issuer.String())
+	fmt.Fprintf(GinkgoWriter, "[diag] Serial Number: %X\n", cert.SerialNumber)


@tmshort ^ added :-)

camilamacedo86 · 2025-08-19T16:07:10Z

@tmshort

Thank you a lot for your help !!!
All nits and comments is addressed we remove the dump of the secret!

tmshort · 2025-08-19T17:09:46Z

openshift/tests-extension/pkg/helpers/commands.go

+		// Add quotes only if whitespace or special chars are present
+		if strings.ContainsAny(a, " \t") {
+			quoted[i] = fmt.Sprintf("%q", a)
+		} else {
+			quoted[i] = a
+		}


tmshort · 2025-08-19T17:14:05Z

/approve
/lgtm

openshift-ci · 2025-08-19T17:14:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, camilamacedo86, tmshort

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~DOWNSTREAM_OWNERS~~ [tmshort]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86 · 2025-08-19T19:08:18Z

/test openshift-e2e-aws

openshift-ci · 2025-08-19T21:29:54Z

@camilamacedo86: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`1f2debf`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2025-08-19T23:44:17Z

[ART PR BUILD NOTIFIER]

Distgit: ose-olm-operator-controller
This PR has been included in build ose-olm-operator-controller-container-v4.20.0-202508192316.p0.gfbdba7b.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-08-20T00:11:15Z

[ART PR BUILD NOTIFIER]

Distgit: ose-olm-catalogd
This PR has been included in build ose-olm-catalogd-container-v4.20.0-202508192316.p0.gfbdba7b.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 15, 2025

openshift-ci bot requested review from gallettilance and oceanc80 August 15, 2025 17:41

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch from 112467e to 07d5c5a Compare August 15, 2025 17:42

camilamacedo86 commented Aug 15, 2025

View reviewed changes

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch 2 times, most recently from baa450a to 0415e67 Compare August 15, 2025 20:57

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch from 0415e67 to 3a22145 Compare August 17, 2025 17:46

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch from 3a22145 to d778da1 Compare August 17, 2025 17:58

camilamacedo86 changed the title ~~OPRUN-3941: [OTE] Add webhook tests skipping flake test~~ OPRUN-3941: [OTE] Add webhook tests (without should be tolerant to openshift-service-ca certificate rotation) Aug 17, 2025

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch from d778da1 to 697e82c Compare August 17, 2025 19:25

anik120 suggested changes Aug 18, 2025

View reviewed changes

anik120 removed their assignment Aug 19, 2025

tmshort reviewed Aug 19, 2025

View reviewed changes

camilamacedo86 force-pushed the add-webhook-tests-with-skip-certca branch from 697e82c to 1f2debf Compare August 19, 2025 15:58

camilamacedo86 commented Aug 19, 2025

View reviewed changes

tmshort reviewed Aug 19, 2025

View reviewed changes

openshift-ci bot assigned tmshort Aug 19, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2025

openshift-merge-bot bot merged commit fbdba7b into openshift:main Aug 19, 2025
12 of 13 checks passed

camilamacedo86 deleted the add-webhook-tests-with-skip-certca branch August 19, 2025 23:46

		// printTLSCertInfo parses a PEM-encoded TLS certificate and prints useful debug info.
		// It shows validity period and SANs (DNS/IP) to help debug webhook cert issues.

OPRUN-3941: [OTE] Add webhook tests (without should be tolerant to openshift-service-ca certificate rotation) #442

OPRUN-3941: [OTE] Add webhook tests (without should be tolerant to openshift-service-ca certificate rotation) #442

Uh oh!

Conversation

camilamacedo86 commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Delta Summary (New vs Old)

Tests

Uh oh!

openshift-ci-robot commented Aug 15, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 15, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 15, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Aug 15, 2025

Uh oh!

camilamacedo86 commented Aug 15, 2025

Uh oh!

camilamacedo86 commented Aug 15, 2025

Uh oh!

camilamacedo86 commented Aug 15, 2025

Uh oh!

camilamacedo86 commented Aug 15, 2025

Uh oh!

openshift-ci bot commented Aug 15, 2025

Uh oh!

camilamacedo86 commented Aug 16, 2025

Uh oh!

openshift-ci bot commented Aug 16, 2025

Uh oh!

camilamacedo86 commented Aug 16, 2025

Uh oh!

openshift-ci-robot commented Aug 17, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 17, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 17, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 commented Aug 17, 2025

Uh oh!

openshift-ci bot commented Aug 17, 2025

Uh oh!

openshift-ci-robot commented Aug 18, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

camilamacedo86 commented Aug 15, 2025 •

edited

Loading

openshift-ci-robot commented Aug 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 17, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 18, 2025 •

edited by openshift-ci bot

Loading

camilamacedo86 Aug 18, 2025 •

edited

Loading