SREP-1220: Add E2E test for revoking and deleting CertificateRequests with certman label #377

amitupadh · 2025-08-11T19:50:10Z

This PR adds an end-to-end test to validate that when a CertificateRequest contains the label certificaterequests.certman.managed.openshift.io, the following workflow is performed successfully:

Revoke the certificate associated with the CertificateRequest.

Delete the CertificateRequest after removing its finalizers.

Verify reconciliation creates a new CertificateRequest .

Ensure the total number of issued certificates remains unchanged before and after the process.

To run the test case, you need to export the below environment variable:
DISABLE_JUNIT_REPORT=false

To run the test:
go test -tags=osde2e ./test/e2e -v

openshift-ci-robot · 2025-08-11T19:50:14Z

@amitupadh: This pull request references SREP-1220 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 task to target the "4.20.0" version, but no target version was set.

Details

In response to this:

This PR adds an end-to-end test to validate that when a CertificateRequest contains the label certificaterequests.certman.managed.openshift.io, the following workflow is performed successfully:
Revoke the certificate associated with the CertificateRequest.
Delete the CertificateRequest after removing its finalizers.
Verify reconciliation creates a new CertificateRequest .
Ensure the total number of issued certificates remains unchanged before and after the process.
To run the test case, you need to export the below environment variable:
DISABLE_JUNIT_REPORT=false

To run the test:
go test -tags=osde2e ./test/e2e -v

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 · 2025-08-11T19:50:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-08-11T19:50:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amitupadh
Once this PR has been reviewed and has the lgtm label, please assign clcollins for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2025-11-17T17:26:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.15%. Comparing base (1cc6dff) to head (4fd5fa2).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   57.15%   57.15%           
=======================================
  Files          29       29           
  Lines        2138     2138           
=======================================
  Hits         1222     1222           
  Misses        802      802           
  Partials      114      114

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

openshift-ci · 2025-12-02T07:16:20Z

@amitupadh: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

amitupadh · 2025-12-02T07:23:08Z

/label tide/merge-method-squash

tnierman · 2025-12-03T17:42:10Z

test/e2e/certman_operator_tests.go

+
+		log.Log.Info("STEP 1: Fetching existing CertificateRequest with owned=true label")
+		crList, err := dynamicClient.Resource(crGVR).Namespace(namespace).List(ctx, metav1.ListOptions{
+			LabelSelector: "certificaterequests.certman.managed.openshift.io",


This label does not exist on any certificaterequest that I've seen: which component do we expect adds this?

tnierman · 2025-12-03T17:47:01Z

test/e2e/certman_operator_tests.go

+
+		// Step 2: Delete the CertificateRequest
+		log.Log.Info("STEP 2: Deleting the original CertificateRequest")
+		err = dynamicClient.Resource(crGVR).Namespace(namespace).Delete(ctx, originalCRName, metav1.DeleteOptions{})


Which certificaterequests are we deleting here? As far as I can tell, nothing was created in the BeforeAll() above, so it seems like we're expecting a certificaterequest to already exist in the test evironment.

In general, I would caution against us blindly deleting objects that we retrieved with a list operation unless we're using a specific Selector

tnierman · 2025-12-03T17:47:47Z

test/e2e/certman_operator_tests.go

+		gomega.Eventually(func() bool {
+			cr, err := dynamicClient.Resource(crGVR).Namespace(namespace).Get(ctx, originalCRName, metav1.GetOptions{})
+			if err != nil {
+				log.Log.Info("CR appears to be deleted already", "name", originalCRName)
+				return true
+			}
+			if cr.GetDeletionTimestamp() == nil {
+				log.Log.Info("CR not marked for deletion yet", "name", cr.GetName())
+				return false
+			}
+
+			finalizers, found, err := unstructured.NestedStringSlice(cr.Object, "metadata", "finalizers")
+			if err != nil {
+				log.Log.Error(err, "Error retrieving finalizers")
+				return false
+			}
+			if !found || len(finalizers) == 0 {
+				log.Log.Info("No finalizers present", "name", cr.GetName())
+				return false
+			}
+
+			crCopy := cr.DeepCopy()
+			_ = unstructured.SetNestedStringSlice(crCopy.Object, []string{}, "metadata", "finalizers")
+
+			_, err = dynamicClient.Resource(crGVR).Namespace(namespace).Update(ctx, crCopy, metav1.UpdateOptions{})
+			if err != nil {
+				log.Log.Error(err, "Failed to remove finalizer")
+				return false
+			}
+			return true
+		}, 1*time.Minute, 5*time.Second).Should(gomega.BeTrue(), "Finalizer should be removed")


Why remove the finalizer? Shouldn't this test include coverage for certman-operator's own finalizing logic?

tnierman · 2025-12-03T17:49:59Z

test/e2e/certman_operator_tests.go

+
+		// Step 4: Wait for new CertificateRequest with new UID
+		var newCRName string
+		gomega.Eventually(func() bool {


Unless we're creating a dnszone.hive.openshift.io object as a prerequisite to this test and I've missed it (which could very well be the case!), a new certificaterequest would never get recreated just because one was deleted

amitupadh · 2025-12-08T07:27:13Z

/hold

Delete a labeled CertificateRequest and ensures it is recreated

f77305f

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2025

amitupadh added 2 commits August 12, 2025 10:38

Test lable changed

9f1aade

Conflict resolved

82df677

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2025

amitupadh marked this pull request as ready for review August 29, 2025 09:40

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2025

openshift-ci bot requested review from Mhodesty and tnierman August 29, 2025 09:40

Merge branch 'master' into SREP-1220

0364f62

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2025

amitupadh added 2 commits November 18, 2025 11:18

ran make boilerplate-update for ci validate check

7cabfdd

ran make boilerplate-update for ci validate check

4fd5fa2

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 2, 2025

tnierman reviewed Dec 3, 2025

View reviewed changes

amitupadh marked this pull request as draft December 8, 2025 04:44

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2025

Merge branch 'master' into SREP-1220

0714b76

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SREP-1220: Add E2E test for revoking and deleting CertificateRequests with certman label #377

SREP-1220: Add E2E test for revoking and deleting CertificateRequests with certman label #377

Uh oh!

amitupadh commented Aug 11, 2025

Uh oh!

openshift-ci-robot commented Aug 11, 2025 •

edited by openshift-ci bot

Loading

Uh oh!

openshift-ci bot commented Aug 11, 2025

Uh oh!

openshift-ci bot commented Aug 11, 2025

Uh oh!

codecov bot commented Nov 17, 2025 •

edited

Loading

Uh oh!

openshift-ci bot commented Dec 2, 2025

Uh oh!

amitupadh commented Dec 2, 2025

Uh oh!

tnierman Dec 3, 2025

Uh oh!

tnierman Dec 3, 2025

Uh oh!

tnierman Dec 3, 2025

Uh oh!

tnierman Dec 3, 2025 •

edited

Loading

Uh oh!

amitupadh commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SREP-1220: Add E2E test for revoking and deleting CertificateRequests with certman label #377

Are you sure you want to change the base?

SREP-1220: Add E2E test for revoking and deleting CertificateRequests with certman label #377

Uh oh!

Conversation

amitupadh commented Aug 11, 2025

Uh oh!

openshift-ci-robot commented Aug 11, 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 bot commented Aug 11, 2025

Uh oh!

openshift-ci bot commented Aug 11, 2025

Uh oh!

codecov bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

openshift-ci bot commented Dec 2, 2025

Uh oh!

amitupadh commented Dec 2, 2025

Uh oh!

tnierman Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

tnierman Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

tnierman Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

tnierman Dec 3, 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!

amitupadh commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Aug 11, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Nov 17, 2025 •

edited

Loading

tnierman Dec 3, 2025 •

edited

Loading