NO-JIRA: Enable test-e2e-encryption by ardaguclu · Pull Request #2087 · openshift/library-go

ardaguclu · 2026-01-23T10:50:03Z

It appears that integration tests for encryption controllers are never being executed (example job). This PR updates the Makefile target to enable them.

openshift-ci-robot · 2026-01-23T10:50:07Z

@ardaguclu: This pull request explicitly references no jira issue.

Details

In response to this:

SSIA

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.

ardaguclu · 2026-01-26T10:41:42Z

test/e2e-encryption/encryption_test.go

-		"kubeapiservers.operator.openshift.io=identity,aescbc:7;kubeschedulers.operator.openshift.io=identity,aescbc:7",
-		// 7 is migrated, plus one backed key (5). 6 is deleted, and therefore is not preserved (would be if the operand config was not deleted)
+		// 7 is migrated, backed key (5) is immediately included when rotating through identity. 6 is deleted, not preserved (would be if operand config was not deleted)
+		"kubeapiservers.operator.openshift.io=identity,aescbc:7,aescbc:5;kubeschedulers.operator.openshift.io=identity,aescbc:7,aescbc:5",


This one is missed in #615

yeah, i think you are right.

ardaguclu · 2026-01-26T10:47:41Z

/cc @p0lyn0mial

p0lyn0mial · 2026-01-27T09:35:29Z

test/e2e-encryption/encryption_test.go

+
+	// wait for CRD to be ready by polling the resource
+	t.Logf("Waiting for CRD to be ready")
+	err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, EncryptionTestTimeout, true, func(ctx context.Context) (bool, error) {


why not to use wait.ForeverTestTimeout ?

Good point. Updated.

p0lyn0mial · 2026-01-27T09:35:41Z

test/e2e-encryption/encryption_test.go

+	// wait for CRD to be ready by polling the resource
+	t.Logf("Waiting for CRD to be ready")
+	err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, EncryptionTestTimeout, true, func(ctx context.Context) (bool, error) {
+		_, err := dynamicClient.Resource(operatorGVR).List(ctx, metav1.ListOptions{})


how does it ensure the CRD is ready ?

should we get the CRD and check some condition ?

Yes, it is better to check the established condition of the CRD. Updated.

p0lyn0mial · 2026-01-27T09:36:24Z

test/e2e-encryption/encryption_test.go

-	dynamicClient, err := dynamic.NewForConfig(kubeConfig)
-	require.NoError(t, err)
-	err = wait.PollImmediate(time.Second, wait.ForeverTestTimeout, func() (bool, error) {
+	err = wait.PollUntilContextTimeout(ctx, time.Second, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) {


here we use wait.ForeverTestTimeout :)

p0lyn0mial · 2026-01-27T09:39:35Z

test/e2e-encryption/encryption_test.go

 	go controllers.Run(ctx, 1)

+	// wait for operator client to be able to read the resource
+	t.Logf("Waiting for operator client to sync")


is this waiting for an informer to sync ?

if yes, should we wait until the informer is synced ?

p0lyn0mial · 2026-01-27T09:41:00Z

test/e2e-encryption/encryption_test.go

 )

+// Encryption controllers run every minute. 2 minutes is sufficient for the required condition to be met.
+var EncryptionTestTimeout = 2 * time.Minute


why not to use wait.ForeverTestTimeout ?

I defined this timeout due to the fact that controllers run per minute

library-go/pkg/operator/encryption/controllers/key_controller.go

Line 110 in d07df3e

ResyncEvery(time.Minute).

to give sufficient time for the expected conditions to be met. However, I believe that if we wait for the sync'ed signal of informers, we won't need this. So that we can use wait.ForeverTestTimeout (30 seconds). I've updated the PR with that.

p0lyn0mial · 2026-01-27T09:42:16Z

test/e2e-encryption/encryption_test.go

+			WithUnsupportedConfigOverrides(runtime.RawExtension{
+				Raw: []byte(fmt.Sprintf(`{"encryption":{"reason":%q}}`, reason)),
+			})
+		err = operatorClient.ApplyOperatorSpec(context.TODO(), "encryption-test", applyConfig)


don't we have a ctx already defined ?

p0lyn0mial · 2026-01-27T09:49:33Z

test/e2e-encryption/encryption_test.go

-		"kubeapiservers.operator.openshift.io=identity,aescbc:7;kubeschedulers.operator.openshift.io=identity,aescbc:7",
-		// 7 is migrated, plus one backed key (5). 6 is deleted, and therefore is not preserved (would be if the operand config was not deleted)
+		// 7 is migrated, backed key (5) is immediately included when rotating through identity. 6 is deleted, not preserved (would be if operand config was not deleted)
+		"kubeapiservers.operator.openshift.io=identity,aescbc:7,aescbc:5;kubeschedulers.operator.openshift.io=identity,aescbc:7,aescbc:5",


yeah, i think you are right.

p0lyn0mial · 2026-01-29T08:40:46Z

test/e2e-encryption/encryption_test.go

+			return false, crdErr
+		}
+
+		for _, condition := range oCRD.Status.Conditions {


could we use apiextensions.IsCRDConditionTrue(oCRD, apiextensions.Established) ?

It appears that IsCRDConditionTrue accepts internal CustomResourceDefinition but in our case, we have external CustomResourceDefinition.

p0lyn0mial · 2026-01-29T08:41:26Z

test/e2e-encryption/encryption_test.go

+	err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) {
+		oCRD, crdErr := apiextensionsClient.CustomResourceDefinitions().Get(ctx, operatorCRD.Name, metav1.GetOptions{})
+		if crdErr != nil {
+			if errors.IsNotFound(crdErr) {


A CRD after creation must be persisted in etcd. I think we can fail on any error.

p0lyn0mial · 2026-01-29T08:50:41Z

test/e2e-encryption/encryption_test.go

 	go controllers.Run(ctx, 1)

+	t.Logf("Waiting for operator informer to sync")
+	if !cache.WaitForCacheSync(stopCh, operatorClient.Informer().HasSynced) {


should we infSynced := operatorInformer.WaitForCacheSync(stopCh) ?

Updated by using operatorInformer.WaitForCacheSync(stopCh). Please let me know your opinions about this. Should I use returned value (infSynced) in somewhere?

p0lyn0mial · 2026-01-29T08:50:56Z

test/e2e-encryption/encryption_test.go

+	if !cache.WaitForCacheSync(stopCh, operatorClient.Informer().HasSynced) {
+		t.Fatalf("failed to sync operator informer")
+	}
+	kubeInformers.WaitForCacheSync(stopCh)


kubeInformers := operatorInformer.WaitForCacheSync(stopCh) ?

Shouldn't we wait kubeInformers sync'ed rather thanoperatorInformer?

I think we should wait for all factories.

p0lyn0mial · 2026-01-29T08:51:21Z

test/e2e-encryption/encryption_test.go

 	operatorInformer.Start(stopCh)
 	go controllers.Run(ctx, 1)

+	t.Logf("Waiting for operator informer to sync")


should we also wait for fakeConfigInformer ?

p0lyn0mial · 2026-01-29T08:51:28Z

test/e2e-encryption/encryption_test.go

+	}
+	kubeInformers.WaitForCacheSync(stopCh)
+	err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) {
+		_, _, _, err := operatorClient.GetOperatorState()


is this required ?

Yeah, I think it becomes redundant. Removed

ardaguclu · 2026-01-29T09:52:31Z

unrelated
/retest

p0lyn0mial · 2026-01-29T09:52:49Z

test/e2e-encryption/encryption_test.go

 	go controllers.Run(ctx, 1)

+	t.Logf("Waiting for informers to sync")
+	operatorInformer.WaitForCacheSync(stopCh)


i think that we should check the response from WaitForCacheSync otherwise we don't know if an informer synced :)

Good point :). I've updated the PR accordingly.

p0lyn0mial · 2026-01-29T10:58:50Z

test/e2e-encryption/encryption_test.go

 	go controllers.Run(ctx, 1)

+	t.Logf("Waiting for informers to sync")
+	if !operatorInformer.WaitForCacheSync(stopCh)[operatorGVR] {


could we just simply check all returned gvrs ?
effectively these are the ones used by the test.

p0lyn0mial · 2026-01-29T10:58:55Z

test/e2e-encryption/encryption_test.go

+			t.Fatalf("informer for %v not synced", typ)
+		}
+	}
+	for typ, synced := range kubeInformers.WaitForCacheSync(stopCh)["openshift-config-managed"] {


could we just simply check all returned gvrs ?
effectively these are the ones used by the test.

p0lyn0mial · 2026-01-29T10:59:41Z

test/e2e-encryption/encryption_test.go

+	if !operatorInformer.WaitForCacheSync(stopCh)[operatorGVR] {
+		t.Fatalf("informer for %v not synced", operatorGVR)
+	}
+	for typ, synced := range fakeConfigInformer.WaitForCacheSync(stopCh) {


nit: could we rename typ to grv ?

p0lyn0mial · 2026-01-29T11:16:46Z

/lgtm

openshift-ci · 2026-01-29T11:17:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, p0lyn0mial

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [p0lyn0mial]

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

ardaguclu · 2026-01-29T11:17:57Z

/hold

p0lyn0mial · 2026-01-29T11:18:00Z

/hold

for ci/prow/e2e-aws-encryption

ardaguclu · 2026-01-29T12:13:11Z

https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_library-go/2087/pull-ci-openshift-library-go-master-e2e-aws-encryption/2016831439449886720 passed
/hold cancel

openshift-ci · 2026-01-29T12:22:51Z

@ardaguclu: 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.

ardaguclu · 2026-02-24T04:28:25Z

/cherrypick release-4.21

openshift-cherrypick-robot · 2026-02-24T04:29:15Z

@ardaguclu: new pull request created: #2130

Details

In response to this:

/cherrypick release-4.21

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.

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

openshift-ci bot requested review from bertinatto and deads2k January 23, 2026 10:50

ardaguclu force-pushed the test-integration branch 4 times, most recently from 36e17a1 to e976b88 Compare January 26, 2026 09:38

ardaguclu commented Jan 26, 2026

View reviewed changes

openshift-ci bot requested a review from p0lyn0mial January 26, 2026 10:47

ardaguclu mentioned this pull request Jan 27, 2026

CNTRLPLANE-2241: Add KMS encryption mode into library-go encryption controllers #2086

Merged

p0lyn0mial reviewed Jan 27, 2026

View reviewed changes

ardaguclu force-pushed the test-integration branch 5 times, most recently from b05eded to dffb2b8 Compare January 28, 2026 10:17

p0lyn0mial reviewed Jan 29, 2026

View reviewed changes

ardaguclu force-pushed the test-integration branch from dffb2b8 to 2159aa7 Compare January 29, 2026 09:46

p0lyn0mial reviewed Jan 29, 2026

View reviewed changes

ardaguclu force-pushed the test-integration branch from 2159aa7 to 403a926 Compare January 29, 2026 10:21

p0lyn0mial reviewed Jan 29, 2026

View reviewed changes

Enable test-e2e-encryption

b402217

ardaguclu force-pushed the test-integration branch from 403a926 to b402217 Compare January 29, 2026 11:10

openshift-ci bot assigned p0lyn0mial Jan 29, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2026

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2026

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

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2026

openshift-merge-bot bot merged commit 60005ae into openshift:master Jan 29, 2026
5 checks passed

ardaguclu deleted the test-integration branch January 29, 2026 12:23

openshift-cherrypick-robot mentioned this pull request Feb 24, 2026

[release-4.21] OCPBUGS-74418: Enable test-e2e-encryption #2130

Merged

Conversation

ardaguclu commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jan 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Jan 26, 2026

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

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

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!

ardaguclu commented Jan 29, 2026

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!

p0lyn0mial commented Jan 29, 2026

Uh oh!

openshift-ci bot commented Jan 29, 2026

Uh oh!

ardaguclu commented Jan 23, 2026 •

edited

Loading