NO-JIRA: add two more encryption rotation scenarios by tjungblu · Pull Request #2218 · openshift/library-go

tjungblu · 2026-05-12T11:51:34Z

This adds two new encryption tests:

Forced rotation during first migration
Forced rotation during a rotation

Summary by CodeRabbit

Tests
- Added helpers to apply APIServer encryption, wait for encryption stability, detect migration-in-progress windows, and assert key-rotation counts.
- Added a teardown helper to clear forced key-rotation reasons after tests.
- Introduced rotation test scenarios covering rotations during initial migration and during ongoing migrations, with optional operator-condition hooks to detect in-progress windows.
- Consolidated operator-condition type usage across performance and rotation tests.

openshift-ci-robot · 2026-05-12T11:51:37Z

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

Details

In response to this:

This adds two new encryption tests:

Forced rotation during first migration

Forced rotation during a rotation

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 · 2026-05-12T11:51:39Z

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

tjungblu · 2026-05-12T11:52:20Z

+		// TODO(thomas): wire up a hook to define this from a provider configuration
+		// TODO(thomas): when we're on KMS provider we can grab the keyId from an annotation
+		// TODO(thomas): a new rotation controller can update the annotation for KMS
+
+		// TODO(thomas): alternative, when we detect a new KeyId, we can also remove the migrated-* annotations,
+		// that way the controller will definitely try to reconcile it again.


I'll remove that, sorry

openshift-ci · 2026-05-12T11:53:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tjungblu
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial 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:

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

coderabbitai · 2026-05-12T11:54:01Z

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: 06e73bab-1ecd-49ed-b4d6-d4bbd83f4699

📥 Commits

Reviewing files that changed from the base of the PR and between 7e6ba8f and 9f815c4.

📒 Files selected for processing (4)

pkg/operator/encryption/controllers/migration_controller.go
test/library/encryption/helpers.go
test/library/encryption/perf_scenarios.go
test/library/encryption/scenarios.go

💤 Files with no reviewable changes (1)

test/library/encryption/perf_scenarios.go

✅ Files skipped from review due to trivial changes (1)

pkg/operator/encryption/controllers/migration_controller.go

🚧 Files skipped from review as they are similar to previous changes (2)

test/library/encryption/scenarios.go
test/library/encryption/helpers.go

Walkthrough

Adds test helpers and a shared operator-conditions hook to detect encryption migration progress, refactors tests to apply APIServer encryption type separately, implements polling utilities for stability/rotations and an "in-progress" window, adds two rotation test scenarios using these utilities, removes a duplicate type, and inserts a TODO comment in the migration controller. No production runtime behavior changed.

Changes

Encryption Migration Testing Infrastructure

Layer / File(s)	Summary
Shared operator-condition hook & APIServer change `test/library/encryption/helpers.go`	Adds exported `GetOperatorConditionsFuncType` and `ApplyAPIServerEncryptionType` so tests can set APIServer encryption type immediately without waiting for storage migration.
Refactor: apply-then-wait flow `test/library/encryption/helpers.go`	Refactors `SetAndWaitForEncryptionType` to call `ApplyAPIServerEncryptionType` first, separating the act of applying the config from waiting for migration/rotation outcomes.
Stability & rotation polling helpers `test/library/encryption/helpers.go`	Adds `WaitUntilEncryptionStable`, `WaitForNRotations`, `EncryptionWriteKeySecretID`, `WaitForEncryptionMigrationInProgressWindow`, and internal helpers to poll secret metadata and operator conditions. Also treats `errors.IsUnauthorized` as retriable in `GetLastKeyMeta`.
Forced-rotation cleanup helper `test/library/encryption/helpers.go`	Adds `ClearForcedKeyRotationReason` to clear `encryption.reason` via the unsupported-config update path for test cleanup.
Perf test type relocation `test/library/encryption/perf_scenarios.go`	Removes the local `GetOperatorConditionsFuncType` and the `operator/v1` import so the shared type from `helpers.go` is reused.

Rotation Test Scenarios

Layer / File(s)	Summary
Scenario struct extension `test/library/encryption/scenarios.go`	Adds optional `GetOperatorConditionsFunc GetOperatorConditionsFuncType` to `RotationScenario` so rotation tests can observe operator progress conditions.
Rotation-overlap tests `test/library/encryption/scenarios.go`	Adds `TestEncryptionRotationDuringFirstMigration` and `TestEncryptionRotationDuringOngoingRotation`. Each test computes expected next write-key names, uses `WaitForEncryptionMigrationInProgressWindow` to detect an in-progress migration snapshot (skips if the window ends too fast), forces rotations via `ForceKeyRotation`, waits for expected write-key revisions with `WaitForNRotations`, and asserts final encryption provider convergence. `TestEncryptionRotation` also defers `ClearForcedKeyRotationReason` for cleanup.

Migration controller note

Layer / File(s)	Summary
Controller TODO `pkg/operator/encryption/controllers/migration_controller.go`	Inserts a TODO in `migrateKeysIfNeededAndRevisionStable` documenting planned approaches to determine key IDs from provider config/KMS annotations or to force reconciliation by removing `migrated-*` annotations. No executable logic changed.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Sequence Diagram

sequenceDiagram
    participant Test as rgba(0,128,255,0.5)
    participant APIServer as rgba(0,200,100,0.5)
    participant Kubernetes as rgba(200,100,0,0.5)
    participant Operator as rgba(180,0,180,0.5)

    Test->>APIServer: ApplyAPIServerEncryptionType (immediate)
    APIServer->>Operator: Operator sees desired encryption config
    Operator->>Kubernetes: migrate keys / annotate resources
    Kubernetes-->>Operator: secret metadata updates (write-key)
    Operator-->>Test: Operator condition (Progressing) via GetOperatorConditionsFunc
    Test->>Kubernetes: Poll secret metadata (WriteKey ID) / WaitForNRotations
    Test->>Operator: ForceKeyRotation (unsupported config)
    Operator->>Kubernetes: new write-key secret created
    Kubernetes-->>Test: Poll detects expected rotations / stability

Suggested reviewers

dgrisonnet

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding two new encryption rotation test scenarios.
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	Check not applicable. PR modifies traditional Go test helpers, not Ginkgo tests. No Ginkgo It(), Describe(), etc. constructs found in modified files.
Test Structure And Quality	✅ Passed	Test functions use defer cleanup, appropriate timeouts (25min window, 69min+ migration), and follow established patterns. Assertions match codebase conventions with meaningful messages where needed.
Microshift Test Compatibility	✅ Passed	The new functions are test library scenario helpers, not Ginkgo e2e tests. The check applies only to Ginkgo e2e tests (It(), Describe(), etc.), so these are not in scope.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	These are test helper library functions (not Ginkgo tests). They handle encryption key rotation at the API level without multi-node assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	PR contains only Go source code (operator logic and test helpers). No scheduling constraints, manifests, affinity rules, node selectors, or topology-specific assumptions are introduced.
Ote Binary Stdout Contract	✅ Passed	This is a library repository with no binary entry points. The OTE Binary Stdout Contract only applies to binaries with main(), init(), TestMain(), or suite setup code. Not applicable here.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests were added. The new functions are test library helpers (accepting testing.TB) in test/library/encryption, not Ginkgo test definitions. Check not applicable.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

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

This adds two new encryption tests: * Forced rotation during first migration * Forced rotation during a rotation Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

from library-go openshift/library-go#2218 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

p0lyn0mial

I did a first quick pass. I’d like to clarify the scope. Are we creating new cases that will support all providers, or just KMS?

p0lyn0mial · 2026-05-13T11:23:01Z

+		t.Skipf("initial migration finished before an in-progress window was observed; set GetOperatorConditionsFunc or use a cluster where migration stays visible longer")
+	}
+
+	require.NoError(e, ForceKeyRotation(e, scenario.UnsupportedConfigFunc, fmt.Sprintf("test-rotation-during-first-migration-%s", rand.String(4))))


ForceKeyRotation has no effect on KMS. What is out plan here ? Do we want to run the new scenarios for all encryption providers ?

ForceKeyRotation will be implemented differently later on

p0lyn0mial · 2026-05-13T11:24:33Z

+// ClearForcedKeyRotationReason clears encryption.reason under UnsupportedConfigOverrides (same merge path as
+// ForceKeyRotation). Call when a test finishes so the next test in sequence does not inherit a non-empty
+// reason and the key controller does not keep seeing an external rotation request.
+func ClearForcedKeyRotationReason(t testing.TB, updateUnsupportedConfig UpdateUnsupportedConfigFunc) error {


instead of a new fn why no to change teh ForceKeyRotation to register a cleanup routine via t.Cleanup ?

why would I couple those two functionalities together? what if I just want to force a key rotation and not cleanup?

p0lyn0mial · 2026-05-13T11:25:30Z

+
+	require.NoError(e, ForceKeyRotation(e, scenario.UnsupportedConfigFunc, fmt.Sprintf("test-rotation-during-first-migration-%s", rand.String(4))))
+	// n=2: one write-key revision from turning encryption on, one from ForceKeyRotation.
+	WaitForNRotations(e, clientSet.Kube, scenario.EncryptionProvider, scenario.TargetGRs, ns, labelSelector, prevMeta, 2)


also for KMS, rotation will not create a new key.

yep, so this would be implemented differently in KMS - I assume this would use this API that you and Ben cobbled together so far

openshift-ci · 2026-05-14T09:01:00Z

PR needs rebase.

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.

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

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 May 12, 2026

tjungblu mentioned this pull request May 12, 2026

NO-JIRA: add more rotation tests scenarios openshift/cluster-kube-apiserver-operator#2149

Open

tjungblu commented May 12, 2026

View reviewed changes

tjungblu force-pushed the rotation_tests branch from 2e4a77c to 7e6ba8f Compare May 12, 2026 13:47

NO-JIRA: add two more encryption rotation scenarios

9f815c4

This adds two new encryption tests: * Forced rotation during first migration * Forced rotation during a rotation Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

tjungblu force-pushed the rotation_tests branch from 7e6ba8f to 9f815c4 Compare May 13, 2026 08:13

tjungblu added a commit to tjungblu/cluster-kube-apiserver-operator that referenced this pull request May 13, 2026

NO-JIRA: add more rotation tests scenarios

070c3a4

from library-go openshift/library-go#2218 Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

p0lyn0mial reviewed May 13, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2026

Conversation

tjungblu commented May 12, 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!

openshift-ci-robot commented May 12, 2026

Uh oh!

openshift-ci Bot commented May 12, 2026

Uh oh!

tjungblu May 12, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented May 12, 2026

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

p0lyn0mial left a comment

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial May 13, 2026

Choose a reason for hiding this comment

Uh oh!

tjungblu May 13, 2026

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial May 13, 2026

Choose a reason for hiding this comment

Uh oh!

tjungblu May 13, 2026

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial May 13, 2026

Choose a reason for hiding this comment

Uh oh!

tjungblu May 13, 2026

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tjungblu commented May 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 12, 2026 •

edited

Loading