CNTRLPLANE-3237:: pkg/operator/encryption/kms: cleanup by p0lyn0mial · Pull Request #2221 · openshift/library-go

p0lyn0mial · 2026-05-13T14:49:01Z

Summary by CodeRabbit

Bug Fixes
- Strengthened validation of KMS plugin configurations to reject invalid key IDs and improve error handling.
Tests
- Expanded test coverage for KMS plugin configuration validation and secret data key processing.

openshift-ci-robot · 2026-05-13T14:49:06Z

@p0lyn0mial: This pull request references CNTRLPLANE-3237 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 story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

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 · 2026-05-13T14:52:16Z

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: e3f61a46-d138-4f45-bcbe-c8052bf0d823

📥 Commits

Reviewing files that changed from the base of the PR and between 942eeae and b6df00e.

📒 Files selected for processing (5)

pkg/operator/encryption/encryptiondata/secret.go
pkg/operator/encryption/encryptiondata/secret_test.go
pkg/operator/encryption/kms/helpers.go
pkg/operator/encryption/kms/helpers_test.go
pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go

💤 Files with no reviewable changes (2)

pkg/operator/encryption/kms/helpers.go
pkg/operator/encryption/kms/helpers_test.go

Walkthrough

The PR relocates KMS plugin Secret data key derivation and validation logic from the shared kms package to the local encryptiondata package, introducing pluginConfigDataKeyPrefix, toPluginConfigSecretDataKeyFor, and keyIDFromPluginConfigSecretDataKey helpers with enhanced key ID validation.

Changes

KMS Plugin Key Derivation Refactoring

Layer / File(s)	Summary
Local key derivation and validation helpers `pkg/operator/encryption/encryptiondata/secret.go`	Adds `pluginConfigDataKeyPrefix` constant and unexported functions `toPluginConfigSecretDataKeyFor` and `keyIDFromPluginConfigSecretDataKey` with validation that key IDs are non-negative integers; updates `FromSecret` and `ToSecret` to use local helpers instead of `kms.*` equivalents.
Test coverage for key derivation validation `pkg/operator/encryption/encryptiondata/secret_test.go`	Introduces `TestFromSecretPluginDataKeyHandling` and `TestToSecretPluginKeyIDValidation` to verify plugin config key parsing, non-integer/negative suffix rejection, and empty-key handling; adds imports for `corev1` and `configv1` to support test fixtures.
Cleanup of shared kms package `pkg/operator/encryption/kms/helpers.go`, `pkg/operator/encryption/kms/helpers_test.go`	Removes unused `strconv` and `strings` imports from `helpers.go`; removes `TestKeyIDFromPluginConfigSecretDataKey` and `TestToPluginConfigSecretDataKeyFor` test functions from `helpers_test.go` (logic now tested locally in `encryptiondata`).
Test update in dependent modules `pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go`	Removes import of `kms` package; hardcodes expected plugin config secret keys (`"kms-plugin-config-555"`, `"kms-plugin-config-777"`) instead of deriving them via `kms.ToPluginConfigSecretDataKeyFor`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

openshift/library-go#2209: Both PRs involve KMS plugin Secret data key handling; this PR moves key derivation logic into encryptiondata while that PR adds plugin-lifecycle helpers that depend on the same key format.

Suggested reviewers

dgrisonnet

🚥 Pre-merge checks | ✅ 10 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'pkg/operator/encryption/kms: cleanup' is vague and generic, using the non-descriptive term 'cleanup' without conveying what specific changes or improvements are being made in the pull request.	Replace the generic 'cleanup' term with a specific description of the main refactoring, such as 'Move plugin config data key helpers from kms package to encryptiondata package' to clearly communicate the primary purpose of the changes.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
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	This PR contains standard Go tests (testing.T), not Ginkgo tests. All test names are static, descriptive strings with no dynamic information. The Ginkgo-specific check is not applicable.
Test Structure And Quality	✅ Passed	The custom check targets Ginkgo test code quality, but the PR only modifies standard Go tests (using testing.T). No Ginkgo code is present in the modified files, so the check is not applicable.
Microshift Test Compatibility	✅ Passed	No Ginkgo e2e tests added. Only standard Go unit tests, which are outside scope of this check.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests added. New tests in secret_test.go are standard Go unit tests using testing.T, not Ginkgo. Custom check not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	This PR refactors KMS encryption helpers without introducing scheduling constraints, affinity rules, topology constraints, or deployment manifest changes. The check is not applicable.
Ote Binary Stdout Contract	✅ Passed	PR modifies library code and unit tests only. No OTE binary entry points with stdout writes found. No main(), init(), TestMain(), or suite setup functions that could corrupt the test listing.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests are added in this PR. All changes are Go unit tests (using *testing.T) for KMS encryption helpers. The custom check is 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

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

openshift-ci · 2026-05-13T16:20:18Z

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

bertinatto · 2026-05-13T21:02:04Z

+			}
+			secret := &corev1.Secret{Data: data}
+			_, err := encryptiondata.FromSecret(secret)
+			if tt.wantError && err == nil {


should we unify whether we use github.com/stretchr/testify/require in library-go unit tests? Right now it's a mix of require and manual if/t.Fatal checks

bertinatto · 2026-05-13T21:39:02Z

+					tt.keyID: {},
+				},
+			}
+			_, err := encryptiondata.ToSecret("ns", "name", cfg)


IIUC, the idea of this new test is to test toPluginConfigSecretDataKeyFor via ToSecret. In general I think it's better to unit test the function directly because then you can check exactly what it produces for a given input. For example, the old test verified that for a given key 42, the function would produce a data key kms-plugin-config-42, but now it only checks that the function didn't return any errors.

to test toPluginConfigSecretDataKeyFor we'd need to change the package directive in this file to encryptiondata though (it's currently encryptiondata_test)

yes, we could create a new test file which would be in the same pkg as the production code to test the priv methods. then we could just move the prev tests. wdty ?

bertinatto · 2026-05-13T21:58:41Z

+			data := baseSecret(t)
+			for k, v := range tt.extraKeys {
+				data[k] = v
+			}


nit: maps.Copy(data, tt.extraKeys)

ardaguclu · 2026-05-14T06:05:40Z

Changes look good to me. Comments from @bertinatto make sense.

ardaguclu · 2026-05-15T05:56:17Z

Comments are all about test changes. I think, those can be handled separately.
/lgtm

openshift-ci · 2026-05-15T05:56:51Z

[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:

~~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

p0lyn0mial · 2026-05-15T06:52:58Z

/hold

I think I will move the tests to a private file.

pkg/operator/encryption/kms: cleanup

b6df00e

p0lyn0mial requested a review from ardaguclu May 13, 2026 14:49

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

p0lyn0mial requested a review from bertinatto May 13, 2026 14:49

openshift-ci Bot requested review from dgrisonnet and hexfusion May 13, 2026 14:50

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

bertinatto reviewed May 13, 2026

View reviewed changes

openshift-ci Bot assigned ardaguclu May 15, 2026

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNTRLPLANE-3237:: pkg/operator/encryption/kms: cleanup#2221

CNTRLPLANE-3237:: pkg/operator/encryption/kms: cleanup#2221
p0lyn0mial wants to merge 1 commit into
openshift:masterfrom
p0lyn0mial:cleanup_encryption_helpers

p0lyn0mial commented May 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

openshift-ci-robot commented May 13, 2026 •

edited by openshift-ci Bot

Loading

Uh oh!

coderabbitai Bot commented May 13, 2026 •

edited

Loading

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

openshift-ci Bot commented May 13, 2026

Uh oh!

bertinatto May 13, 2026

Uh oh!

bertinatto May 13, 2026

Uh oh!

p0lyn0mial May 14, 2026

Uh oh!

bertinatto May 13, 2026

Uh oh!

ardaguclu commented May 14, 2026

Uh oh!

ardaguclu commented May 15, 2026

Uh oh!

openshift-ci Bot commented May 15, 2026

Uh oh!

p0lyn0mial commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

p0lyn0mial commented May 13, 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 13, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

openshift-ci Bot commented May 13, 2026

Uh oh!

bertinatto May 13, 2026

Choose a reason for hiding this comment

Uh oh!

bertinatto May 13, 2026

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial May 14, 2026

Choose a reason for hiding this comment

Uh oh!

bertinatto May 13, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented May 14, 2026

Uh oh!

ardaguclu commented May 15, 2026

Uh oh!

openshift-ci Bot commented May 15, 2026

Uh oh!

p0lyn0mial commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

p0lyn0mial commented May 13, 2026 •

edited by coderabbitai Bot

Loading

openshift-ci-robot commented May 13, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented May 13, 2026 •

edited

Loading