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 for KMS plugin secret keys to reject invalid or negative key IDs and improve error handling.
Refactor
- Internal handling of plugin secret key formatting/parsing consolidated and localized to reduce external coupling.
Tests
- Expanded and reorganized tests for plugin secret key construction/parsing and plugin configuration handling, including new negative cases.

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: 7e98a0ce-c43c-4bcb-9cee-82a3604ab6e2

📥 Commits

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

📒 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_test.go
pkg/operator/encryption/kms/helpers.go

Walkthrough

Derives and parses KMS plugin Secret data keys locally in package encryptiondata via new unexported helpers; updates encryptiondata to use them, moves related tests into the package and adds unit tests; removes now-unused imports from kms helpers and updates dependent tests to use hardcoded key strings.

Changes

KMS Plugin Key Derivation Refactoring

Layer / File(s)	Summary
Local key derivation and validation helpers `pkg/operator/encryption/encryptiondata/secret.go`	Adds `pluginConfigDataKeyPrefix` 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 and package move `pkg/operator/encryption/encryptiondata/secret_test.go`	Moves tests into package `encryptiondata`, updates `TestExtractUniqueAndSortedKMSConfigurations` to call local `ExtractUniqueAndSortedKMSConfigurations`, and adds `TestKeyIDFromPluginConfigSecretDataKey` and `TestToPluginConfigSecretDataKeyFor` to validate parsing and key construction/validation.
KMS helpers import cleanup `pkg/operator/encryption/kms/helpers.go`	Removes now-unused `strconv`/`strings` stdlib imports from the import block.
Dependent test updates `pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go`	Removes `kms` import usage and hardcodes expected plugin config secret data key strings (`"kms-plugin-config-555"`, `"kms-plugin-config-777"`) in sidecar tests.

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 labels

lgtm

Suggested reviewers

dgrisonnet

🚥 Pre-merge checks | ✅ 9 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	Tests use minimal assertion messages without adequate context. Assertions like "expected error but got nil" lack information about what operation failed, making debugging difficult when tests fail.	Add descriptive context to all assertions (e.g., t.Fatalf("failed to parse valid key ID: %v", err)) or use require package for more flexible messaging patterns consistent with sidecar_test.go.
Title check	❓ Inconclusive	The PR title references a Jira ticket and mentions 'cleanup' in the kms package, which partially aligns with the refactoring work shown, but 'cleanup' is vague and doesn't clearly convey the main change of extracting key derivation helpers from the kms package to the encryptiondata package.	Consider a more specific title that describes the core refactoring, such as 'Move KMS key derivation helpers to encryptiondata package' to better communicate the intent and scope of the changes.

✅ Passed checks (9 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	The check is not applicable: all test files use standard Go testing (func Test*), not Ginkgo. No Ginkgo imports or syntax found in the repository.
Microshift Test Compatibility	✅ Passed	No Ginkgo e2e tests added in this PR. All changes are regular Go unit tests using the standard testing package, not Ginkgo. Check does not apply.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	PR contains no Ginkgo e2e tests. The changes add/modify only standard Go unit tests in pkg/ using testing.T interface, not Ginkgo e2e tests, so SNO compatibility check does not apply.
Topology-Aware Scheduling Compatibility	✅ Passed	PR reorganizes KMS encryption helper functions with no scheduling constraints introduced. Changes are limited to key generation utilities and test reorganization.
Ote Binary Stdout Contract	✅ Passed	PR contains no stdout writes in process-level code (main, init, TestMain, suite setup, or var initializers) that would violate OTE Binary Stdout Contract.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	This PR modifies unit tests using standard Go testing patterns, not Ginkgo e2e tests. The check targeting Ginkgo e2e tests 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.}

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

replaced require with standard assertions.

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

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

/hold

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

p0lyn0mial · 2026-05-21T08:13:25Z

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

done.

/hold cancel

…ing assertions

bertinatto

/lgtm

openshift-ci · 2026-05-21T10:43:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, bertinatto, 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~~ [bertinatto,p0lyn0mial]

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

bertinatto · 2026-05-21T10:46:10Z

@@ -1,4 +1,4 @@
-package encryptiondata_test
+package encryptiondata


ardaguclu · 2026-05-21T11:42:18Z

/retest

openshift-ci · 2026-05-21T12:13:29Z

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

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 do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 15, 2026

p0lyn0mial force-pushed the cleanup_encryption_helpers branch from b6df00e to f5bd097 Compare May 21, 2026 08:09

openshift-ci Bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2026

pkg/operator/encryption/kms: cleanup

7388263

p0lyn0mial force-pushed the cleanup_encryption_helpers branch from f5bd097 to 7388263 Compare May 21, 2026 08:12

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

pkg/operator/encryption/kms: replace require calls with standard test…

d617f82

…ing assertions

bertinatto reviewed May 21, 2026

View reviewed changes

openshift-ci Bot assigned bertinatto May 21, 2026

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

bertinatto reviewed May 21, 2026

View reviewed changes

ardaguclu mentioned this pull request May 21, 2026

NO-JIRA: Create Vault Secret for fake Vault setup #2238

Open

openshift-merge-bot Bot merged commit bfd7920 into openshift:master May 21, 2026
6 checks passed

		@@ -1,4 +1,4 @@
		package encryptiondata_test
		package encryptiondata

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 labels

Suggested reviewers

❌ Failed checks (2 warnings, 1 inconclusive)

Uh oh!

bertinatto May 13, 2026

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial May 21, 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!

p0lyn0mial commented May 15, 2026

Uh oh!

p0lyn0mial commented May 21, 2026

Uh oh!

bertinatto left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented May 21, 2026

Uh oh!

bertinatto May 21, 2026

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented May 21, 2026

Uh oh!

openshift-ci Bot commented May 21, 2026

Uh oh!

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