Encryption refactor fn injection by p0lyn0mial · Pull Request #2402 · openshift/library-go

p0lyn0mial · 2026-08-04T08:13:18Z

Summary by CodeRabbit

New Features
- Added support for previewing encryption keys and configuration changes before they are applied.
- Improved handling of encryption configuration and key-secret retrieval across management workflows.
Refactor
- Streamlined key discovery, generation, migration, state evaluation, and cleanup.
- Preserved existing validation, error handling, and encryption-state behavior.
Tests
- Expanded coverage for new encryption configuration calculations, including fresh KMS setup and migration scenarios.
- Updated tests for revised configuration retrieval behavior.

…instead of clients Replace the deployer interface and secretClient/encryptionSecretSelector parameters with two injected functions, making the inputs explicit and decoupling the statemachine from specific client types. All five callers are updated with inline closures binding their existing clients. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Move the secret creation side effect out of checkAndCreateKeys into sync, making the function's output explicit. sync now handles Create, AlreadyExists validation, and event recording. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Refactor getCurrentModeReasonAndEncryptionConfig and the GetEncryptionConfigAndState call site to accept their client reads as explicit function parameters rather than using receiver fields directly. The three bound functions are stored on the struct and wired in the constructor, making all external dependencies of checkAndCreateKeys visible in its signature. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Refactor generateKeySecret to accept getKMSPluginSecret and getKMSPluginConfigMap as explicit function parameters instead of using the receiver's secretClient and configMapClient directly. Both functions are stored on the struct, wired in the constructor, and threaded through checkAndCreateKeys following the same pattern as the other resource fetchers. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…puts to checkAndCreateKeys Pass instanceName and unsupportedConfigPrefix as explicit parameters through checkAndCreateKeys into generateKeySecret and getCurrentModeReasonAndEncryptionConfig respectively, removing the last remaining receiver accesses from those helper methods. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…unctions Convert checkAndCreateKeys, getCurrentModeReasonAndEncryptionConfig and generateKeySecret from methods to package-level functions. The absence of a receiver proves all inputs are fully mapped out as explicit parameters. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…e functions Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-08-04T08:14:12Z

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: ef7fef09-5edb-488e-b5c7-66b3bc5f2309

📥 Commits

Reviewing files that changed from the base of the PR and between 7b5b5d8 and 9e385c9.

📒 Files selected for processing (1)

pkg/operator/encryption/controllers/encryption_computer_test.go

Walkthrough

This change replaces direct encryption client dependencies with injected callbacks across state retrieval, key generation, and controller synchronization. Key operations are extracted into standalone functions. New computer types provide side-effect-free key and encryption-config Secret computation.

Changes

Encryption computation decoupling

Layer / File(s)	Summary
Injected encryption state retrieval `pkg/operator/encryption/statemachine/transition.go`, `pkg/operator/encryption/controllers/condition_controller.go`, `pkg/operator/encryption/controllers/migration_controller.go`, `pkg/operator/encryption/controllers/prune_controller.go`	`GetEncryptionConfigAndState` now accepts callbacks for the deployed encryption-config Secret and key Secret listing. Controllers provide these callbacks.
Key computation and synchronization `pkg/operator/encryption/controllers/key_controller.go`, `pkg/operator/encryption/controllers/key_computer.go`, `pkg/operator/encryption/controllers/key_controller_test.go`	Key generation uses injected lookups and returns a desired Secret. Synchronization creates the Secret, validates conflicts, and records events. `KeyComputer` exposes side-effect-free key computation.
Encryption-config computation and application `pkg/operator/encryption/controllers/state_controller.go`	Encryption-config generation is split into reusable helpers. The controller computes and applies the desired Secret, then records pending change events.
Combined computation and validation `pkg/operator/encryption/controllers/encryption_computer.go`, `pkg/operator/encryption/controllers/encryption_computer_test.go`	`EncryptionComputer` combines key and encryption-config computation. Tests cover fresh KMS setup and AESCBC-to-KMS migration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

openshift/library-go#2399: Both refactor encryption-config and key Secret computation in the same controller and state-machine areas.
openshift/library-go#2401: Both update encryption controller interfaces and encryption Secret retrieval.

🚥 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 clearly summarizes the main change: refactoring encryption controllers to use function injection.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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	Changed tests use standard Go subtests with fixed literal names; no Ginkgo titles or dynamic pod, namespace, timestamp, UUID, node, or IP data appear in titles.
Test Structure And Quality	✅ Passed	The changed tests use standard testing.T and t.Run, not Ginkgo; they use fake objects, perform no cluster waits, and create no resources that require cleanup.
Microshift Test Compatibility	✅ Passed	The only added test is a standard Go `testing.T` unit test; it adds no Ginkgo e2e test and therefore triggers no MicroShift compatibility check.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	This PR contains only Go unit tests (testing.T), not Ginkgo e2e tests. The custom check for SNO-incompatible Ginkgo tests does not apply to unit test files.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR changes only encryption controllers, state-machine callbacks, and tests; the 10-file diff adds no manifests or scheduling constraints such as affinity, topology spread, selectors, toleration...
Ote Binary Stdout Contract	✅ Passed	Changed files add no main/init or Ginkgo suite setup and no stdout writes; existing klog calls remain inside controller/helper methods, while top-level initializers are literals.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds standard Go unit tests only; no Ginkgo declarations, IP assumptions, network calls, or external connectivity requirements are present.
No-Weak-Crypto	✅ Passed	The PR adds no MD5, SHA1, DES, RC4, Blowfish, ECB, or custom crypto code; it only refactors existing key generation and adds Secret handling without secret/token comparisons.
Container-Privileges	✅ Passed	The PR changes only encryption-controller Go code and Secret test fixtures; the PR diff adds no privileged, host namespace, SYS_ADMIN, root, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging statements that expose sensitive data were added in this PR. The refactoring uses function injection but does not introduce any new log calls.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

openshift-ci · 2026-08-04T08:14:13Z

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 · 2026-08-04T08:18:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~pkg/operator/encryption/OWNERS~~ [p0lyn0mial]

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

…mputation Add a private computeKeySecret method on keyController that binds all stored functions and calls checkAndCreateKeys. sync now calls through this method. Add KeyComputer, a thin wrapper that accepts a keyController and exposes ComputeKey so future components can compute what key would be needed without creating it. Also fix double Fn suffix on struct field names introduced by the previous rename. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…applying it Move resourceapply.ApplySecret out of generateEncryptionConfigSecret into sync, following the same pattern as checkAndCreateKeys. The function now returns the secret to apply and pre-computed pending events; sync applies the secret and emits events only if ApplySecret reports a change. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…nConfigSecret Store deployedEncryptionConfigSecretFn and listKeySecretsFn on the stateController struct, wire them in the constructor, and pass them as explicit parameters to generateEncryptionConfigSecret, following the same pattern as the key controller. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…unction and add EncryptionComputer Inject instanceName as an explicit parameter into generateEncryptionConfigSecret and applyEncryptionConfigSecret, removing the last receiver accesses and promoting both to package-level functions. Add computeEncryptionConfigSecret as a private wrapper method on stateController (analogous to computeKeySecret on keyController). Add EncryptionComputer, a combined component that accepts both controllers and exposes read-only computation of both the key secret and the encryption config secret. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…roller Add computeEncryptionConfigSecretWithCustomListKeySecretFn on stateController to allow injecting a custom listKeySecretsFn into the encryption config computation. Add EncryptionComputer combining both controllers, exposing ComputeEncryptionConfigSecretWithNewKey which chains the key controller output into the state controller's computation by prepending the would-be key secret to the list of existing secrets. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Two table-driven scenarios with full YAML representations of both expected outputs — the newly computed KMS key secret and the resulting encryption config secret: 1. Fresh KMS setup (no existing keys): new key gets ID 1, encryption config places it as a read key in the first transitional pass. 2. Migrating from AESCBC to KMS (one fully-migrated AESCBC key exists): new KMS key gets ID 8, encryption config carries both keys as read keys alongside an identity write key until convergence. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

openshift-ci · 2026-08-07T09:47:45Z

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.

p0lyn0mial and others added 7 commits August 4, 2026 09:34

encryption: add Fn suffix to function parameters to highlight they ar…

184c886

…e functions Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

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 4, 2026

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

p0lyn0mial and others added 6 commits August 4, 2026 10:29

coderabbitai Bot mentioned this pull request Aug 4, 2026

CNTRLPLANE-3237: key controller schedules a preflight check #2392

Merged

ardaguclu mentioned this pull request Aug 5, 2026

NO-JIRA: Dynamic namespace instead of hard coded openshift-config-managed #2401

Closed

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

Navigation Menu

Uh oh!

Encryption refactor fn injection - #2402

Encryption refactor fn injection#2402
p0lyn0mial wants to merge 13 commits into
openshift:masterfrom
p0lyn0mial:encryption-refactor-fn-injection

p0lyn0mial commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

openshift-ci Bot commented Aug 4, 2026

Uh oh!

openshift-ci Bot commented Aug 4, 2026

Uh oh!

openshift-ci Bot commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

p0lyn0mial commented Aug 4, 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 Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

openshift-ci Bot commented Aug 4, 2026

Uh oh!

openshift-ci Bot commented Aug 4, 2026

Uh oh!

openshift-ci Bot commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

p0lyn0mial commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading