NO-JIRA: remove old mock plugin deployment by gangwgr · Pull Request #902 · openshift/cluster-authentication-operator

gangwgr · 2026-05-28T06:38:24Z

Remove old mock plugin deployment

Summary by CodeRabbit

Tests
- Simplified KMS encryption end-to-end tests by removing explicit mock KMS deployment steps.
- Tests now run without those mock deployment calls, relying on existing provider setup during the scenarios.

coderabbitai · 2026-05-28T06:38:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 52537cc8-3e91-414c-83b8-07fa3589aa19

📥 Commits

Reviewing files that changed from the base of the PR and between 119bb0e and ded6d04.

📒 Files selected for processing (1)

test/e2e-encryption-kms/encryption_kms.go

💤 Files with no reviewable changes (1)

test/e2e-encryption-kms/encryption_kms.go

Walkthrough

Removes explicit librarykms.DeployUpstreamMockKMSPlugin calls from two KMS e2e tests; both tests now invoke their corresponding library test helpers directly.

Changes

KMS E2E Test Cleanup

Layer / File(s)	Summary
On/off scenario: remove mock deploy `test/e2e-encryption-kms/encryption_kms.go`	Removed the explicit `librarykms.DeployUpstreamMockKMSPlugin` call from `testKMSEncryptionOnOff`; the test now calls `library.TestEncryptionTurnOnAndOff` directly.
Providers migration: remove mock deploy `test/e2e-encryption-kms/encryption_kms.go`	Removed the explicit `librarykms.DeployUpstreamMockKMSPlugin` call from `testKMSEncryptionProvidersMigration`; the test now calls `library.TestEncryptionProvidersMigration` directly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

lgtm, verified

Suggested reviewers

kaleemsiddiqu

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'NO-JIRA: remove old mock plugin deployment' directly and accurately summarizes the main change: removal of the deprecated librarykms.DeployUpstreamMockKMSPlugin calls from the KMS encryption tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Test names are stable and deterministic: "TestKMSEncryptionOnOff" and "TestKMSEncryptionProvidersMigration" contain only static, descriptive strings with no dynamic values.
Test Structure And Quality	✅ Passed	Tests have proper timeouts ([Timeout:120m]), single responsibility via library helpers, meaningful names/descriptions, and follow codebase patterns. Removal of mock plugin improves test clarity.
Microshift Test Compatibility	✅ Passed	PR removes code, not adds new tests. Check applies only when NEW Ginkgo e2e tests are added; no new tests present in this PR.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	Tests added are KMS encryption feature tests for OAuth tokens that test encryption/decryption logic independent of cluster topology, similar to existing encryption tests already in codebase.
Topology-Aware Scheduling Compatibility	✅ Passed	PR only removes test code (mock KMS plugin deployment calls). No deployment manifests, operator code, or controllers were added/modified, so topology-aware scheduling check preconditions not met.
Ote Binary Stdout Contract	✅ Passed	PR removes mock KMS plugin calls from test functions (within safe It() blocks). No process-level stdout writes detected: TestMain has no prints, var initializers use only Ginkgo framework.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR removes old mock plugin deployment calls from existing tests; no new Ginkgo e2e tests are added. Check applies only to new tests being added, not modifications to existing tests.
No-Weak-Crypto	✅ Passed	No weak crypto algorithms (MD5/SHA1/DES/RC4/3DES/Blowfish/ECB), custom crypto implementations, or unsafe secret comparisons found in PR changes.
Container-Privileges	✅ Passed	PR modifies only Go test code in encryption_kms.go, removing a function call; no Kubernetes manifests or container privilege configurations are affected.
No-Sensitive-Data-In-Logs	✅ Passed	PR removes calls to DeployUpstreamMockKMSPlugin logging only a public container image URL. No new logging added and the image URL is not sensitive data as defined by the check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ardaguclu · 2026-05-28T07:08:14Z

/lgtm
/approve

openshift-ci · 2026-05-28T07:09:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu

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~~ [ardaguclu]

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

tjungblu · 2026-05-28T07:39:17Z

/retitle NO-JIRA: remove old mock plugin deployment

tjungblu · 2026-05-28T07:39:21Z

/verified by ci

openshift-ci-robot · 2026-05-28T07:39:26Z

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

Details

In response to this:

Remove old mock plugin deployment

Summary by CodeRabbit

Tests

Simplified KMS encryption end-to-end tests by removing redundant setup operations.

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 · 2026-05-28T07:39:33Z

@tjungblu: This PR has been marked as verified by ci.

Details

In response to this:

/verified by ci

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-05-28T08:14:13Z

/hold
To see kms jobs green

gangwgr · 2026-05-28T09:40:57Z

/testwith openshift/cluster-authentication-operator/master/e2e-aws-operator-encryption-kms-ote openshift/origin#31228

openshift-ci · 2026-05-28T09:41:04Z

@gangwgr, testwith: could not generate prow job. ERROR:

BUG: test 'e2e-aws-operator-encryption-kms-ote' not found in injected config

gangwgr · 2026-05-28T09:41:22Z

/test e2e-operator

ardaguclu · 2026-05-28T09:42:13Z

/test ?

ardaguclu · 2026-05-28T09:42:55Z

/test e2e-aws-operator-encryption-kms

ardaguclu · 2026-05-28T09:43:49Z

/test e2e-gcp-operator-encryption-kms

gangwgr · 2026-05-28T10:13:52Z

/test e2e-aws-operator-encryption-kms-ote

ardaguclu · 2026-05-28T12:21:06Z

/test e2e-aws-operator-encryption-kms

gangwgr · 2026-05-28T12:36:38Z

/test e2e-aws-operator-encryption-kms-ote

tjungblu · 2026-05-28T12:48:27Z

don't you have to use the DefaultVaultEncryptionProvider instead?

not sure about the I see one @ardaguclu pr not merged
@ardaguclu @bertinatto can we use the DefaultVaultEncryptionProvider?

which PR do we need from Arda?

openshift/library-go#2241

There is no prerequisite pr for using original vault instance. But it has no relation with the purpose of this pr

I think we can begin the process of switching to original vault

I would like to get off the mock as soon as possible, but we can have this as a separate PR.

@bertinatto just mentioned that we have to wait for a new nightly/CI build for the changes to be effective across all operators. Then the tests should also clear.

@gangwgr ~~you can open a pr to test original vault image~~ more changes are needed, see openshift/cluster-kube-apiserver-operator#2169

openshift-ci-robot · 2026-05-28T13:42:26Z

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

Details

In response to this:

Remove old mock plugin deployment

Summary by CodeRabbit

Tests

Simplified KMS encryption end-to-end tests by removing mock KMS deployment steps.

Switched tests to use the non-mocked encryption provider and updated migration scenarios to exercise real-provider behavior.

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.

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/e2e-encryption-kms/encryption_kms.go (1)

59-67: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale migration test steps comment

Line 61 says the test deploys the mock KMS plugin, but that setup was removed in this PR. Please update the numbered steps so they match current behavior and avoid confusion during failures/triage.

Suggested doc-only patch

 // testKMSEncryptionProvidersMigration tests migration between KMS and AES encryption providers.
 // This test:
-// 1. Deploys the mock KMS plugin
-// 2. Creates a test OAuth access token (TokenOfLife)
-// 3. Randomly picks one AES encryption provider (AESGCM or AESCBC)
-// 4. Shuffles the selected AES provider with KMS to create a randomized migration order
-// 5. Migrates between the providers in the shuffled order
-// 6. Verifies token is correctly encrypted after each migration
+// 1. Creates a test OAuth access token (TokenOfLife)
+// 2. Randomly picks one AES encryption provider (AESGCM or AESCBC)
+// 3. Shuffles the selected AES provider with KMS to create a randomized migration order
+// 4. Migrates between the providers in the shuffled order
+// 5. Verifies token is correctly encrypted after each migration

🤖 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/e2e-encryption-kms/encryption_kms.go` around lines 59 - 67, The comment
above the testKMSEncryptionProvidersMigration function is outdated because the
mock KMS plugin deployment was removed; update the numbered steps in that block
(the comment starting "// testKMSEncryptionProvidersMigration tests migration
between KMS and AES encryption providers.") to reflect current behavior—remove
or modify step 1 about deploying the mock KMS plugin and renumber/clarify
remaining steps (creating TokenOfLife, selecting/shuffling AES provider,
performing migrations, and verifying token encryption) so the comment matches
what testKMSEncryptionProvidersMigration actually does.

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

Outside diff comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 59-67: The comment above the testKMSEncryptionProvidersMigration
function is outdated because the mock KMS plugin deployment was removed; update
the numbered steps in that block (the comment starting "//
testKMSEncryptionProvidersMigration tests migration between KMS and AES
encryption providers.") to reflect current behavior—remove or modify step 1
about deploying the mock KMS plugin and renumber/clarify remaining steps
(creating TokenOfLife, selecting/shuffling AES provider, performing migrations,
and verifying token encryption) so the comment matches what
testKMSEncryptionProvidersMigration actually does.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c424b5ac-799a-4f06-b5a2-108630fc1e6a

📥 Commits

Reviewing files that changed from the base of the PR and between 5d8baed and 119bb0e.

📒 Files selected for processing (1)

test/e2e-encryption-kms/encryption_kms.go

ployment

coderabbitai · 2026-05-28T14:13:30Z

Actionable comments posted: 0

ardaguclu · 2026-05-28T14:13:54Z

/lgtm
/hold
we should see KMS green

ardaguclu · 2026-05-28T17:11:15Z

/retest

openshift-ci · 2026-05-28T19:35:55Z

@gangwgr: 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/e2e-aws-operator-encryption-kms-ote	`5d8baed`	link	false	`/test e2e-aws-operator-encryption-kms-ote`

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-05-28T19:39:49Z

/hold cancel

bertinatto · 2026-05-29T00:37:26Z

/verified by ci

kms job passed: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/902/pull-ci-openshift-cluster-authentication-operator-master-e2e-aws-operator-encryption-kms/2060046296021995520

openshift-ci-robot · 2026-05-29T00:37:36Z

@bertinatto: This PR has been marked as verified by ci.

Details

In response to this:

/verified by ci

kms job passed: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/902/pull-ci-openshift-cluster-authentication-operator-master-e2e-aws-operator-encryption-kms/2060046296021995520

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 Bot requested review from kaleemsiddiqu and tjungblu May 28, 2026 06:39

gangwgr force-pushed the rm-old-plugin branch from 7d4753f to 5d8baed Compare May 28, 2026 06:41

openshift-ci Bot assigned ardaguclu May 28, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2026

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2026

openshift-ci Bot changed the title ~~Remove old mock plugin deployment~~ NO-JIRA: remove old mock plugin deployment May 28, 2026

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

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label May 28, 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 May 28, 2026

tjungblu reviewed May 28, 2026

View reviewed changes

gangwgr force-pushed the rm-old-plugin branch from 5d8baed to 119bb0e Compare May 28, 2026 13:38

openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label May 28, 2026

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2026

coderabbitai Bot reviewed May 28, 2026

View reviewed changes

Remove old mock plugin de

ded6d04

ployment

gangwgr force-pushed the rm-old-plugin branch from 119bb0e to ded6d04 Compare May 28, 2026 14:12

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 28, 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 May 28, 2026

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label May 29, 2026

openshift-merge-bot Bot merged commit 7eccc16 into openshift:master May 29, 2026
17 checks passed

Uh oh!

Conversation

gangwgr commented May 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

openshift-ci Bot commented May 28, 2026

Uh oh!

tjungblu commented May 28, 2026

Uh oh!

tjungblu commented May 28, 2026

Uh oh!

openshift-ci-robot commented May 28, 2026

Summary by CodeRabbit

Uh oh!

openshift-ci-robot commented May 28, 2026

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

gangwgr commented May 28, 2026

Uh oh!

openshift-ci Bot commented May 28, 2026

Uh oh!

gangwgr commented May 28, 2026

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

gangwgr commented May 28, 2026

Uh oh!

ardaguclu commented May 28, 2026

Uh oh!

gangwgr commented May 28, 2026

Uh oh!

tjungblu May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gangwgr May 28, 2026

Choose a reason for hiding this comment

Uh oh!

tjungblu May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gangwgr May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu May 28, 2026 • 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!

ardaguclu May 28, 2026

Choose a reason for hiding this comment

Uh oh!

tjungblu May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu May 29, 2026 • 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!

openshift-ci-robot commented May 28, 2026

Summary by CodeRabbit

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented May 28, 2026

gangwgr commented May 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

ardaguclu May 28, 2026 •

edited

Loading

ardaguclu May 29, 2026 •

edited

Loading