encryption/controllers/kms_preflight_controller: read EncryptionKMSPreflightRequired condition and compare config hash by p0lyn0mial · Pull Request #2322 · openshift/library-go

p0lyn0mial · 2026-06-22T12:14:53Z

Summary by CodeRabbit

Bug Fixes
- KMS configuration preflight validation now includes proper readiness checks using a hash-based protocol to verify configuration consistency.
Tests
- Expanded KMS preflight test coverage with shared test fixtures and improved scenario validation.

…eflightRequired condition and compare config hash

coderabbitai · 2026-06-22T12:15:09Z

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: 54c44657-a52b-428f-981e-3c33a20c4dcc

📥 Commits

Reviewing files that changed from the base of the PR and between cb15bee and 603bb8f.

📒 Files selected for processing (2)

pkg/operator/encryption/controllers/kms_preflight_controller.go
pkg/operator/encryption/controllers/kms_preflight_controller_test.go

Walkthrough

kms_preflight_controller.go replaces the stubbed preflight with a preflightRequired function that reads the EncryptionKMSPreflightRequired operator condition, computes a deterministic FNV-32/JSON/base64 hash of the KMS provider config and referenced Secret/ConfigMap data, and compares it to the required hash. The constructor now accepts and stores a coreClient. Tests are refactored to use shared fixtures and expanded to cover all new preflight paths.

Changes

KMS Preflight Hash Protocol

Layer / File(s)	Summary
Controller state, constructor, and preflight logic `pkg/operator/encryption/controllers/kms_preflight_controller.go`	Adds `coreClient` field and `klog` import; updates `NewKMSPreflightController` to accept `coreClient`; replaces the `runPreflightChecks` stub with a `preflightRequired`-based dispatch that reads operator conditions, computes the current KMS config hash, and returns the hash when it matches or `nil` to wait.
Shared test fixtures and `TestKMSConfigHasher` refactor `pkg/operator/encryption/controllers/kms_preflight_controller_test.go`	Introduces package-level `wellKnownBaseVaultConfig`, `wellKnownBaseSecret`, and `wellKnownBaseConfigMap`; rewrites all `TestKMSConfigHasher` table scenarios to reference these shared baselines, covering field-change, ignored-key, missing-resource, and byte-shift cases.
`TestKMSPreflightController` expansion `pkg/operator/encryption/controllers/kms_preflight_controller_test.go`	Introduces `apiServerWithKMS`, a constant expected hash, and a richer scenario struct. Adds scenarios for preconditions not met, required condition absent/false/true, hash match/mismatch, missing Secret/ConfigMap, and empty-message; validates results with `ValidateOperatorClientConditions`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

jira/valid-reference

Suggested reviewers

deads2k

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically describes the main changes: reading the EncryptionKMSPreflightRequired condition and comparing config hashes in the kms_preflight_controller.
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 test file uses standard Go testing conventions (testing.T with table-driven tests), not Ginkgo. No Ginkgo test declarations (It, Describe, Context, etc.) are present. All 27 test scenario names...
Test Structure And Quality	✅ Passed	Test file uses standard Go testing (not Ginkgo), making check inapplicable. Table-driven tests show single responsibility per scenario; fake clients eliminate cleanup concerns; assertions mostly ha...
Microshift Test Compatibility	✅ Passed	PR contains no Ginkgo e2e tests. The modified test file uses standard Go testing package with func TestXxx(t *testing.T) syntax, not Ginkgo e2e test patterns.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests added. PR modifies unit tests using standard Go testing package, not Ginkgo framework. Check is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies only controller logic and tests for KMS preflight validation. No deployment manifests, scheduling constraints, affinity rules, node selectors, PDBs, or topology assumptions are introduced.
Ote Binary Stdout Contract	✅ Passed	The klog.V(4).Infof call in kms_preflight_controller.go is within the sync() controller callback (not process-level code), and klog v2 defaults to writing to stderr, not stdout.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests added. The PR only modifies a controller implementation and its Go unit tests, not Ginkgo e2e tests.
No-Weak-Crypto	✅ Passed	No flagged weak crypto (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto, or non-constant-time secret comparisons found. FNV-32 is used for configuration hash change detection (not cryptogr...
Container-Privileges	✅ Passed	PR only modifies Go source files for KMS preflight controller logic; no Kubernetes manifests with container privilege settings are introduced or modified.
No-Sensitive-Data-In-Logs	✅ Passed	The PR logs only FNV-32 hashes (base64-encoded), which are non-reversible cryptographic digests. No passwords, tokens, API keys, PII, or sensitive data are exposed in logs.

_{✏️ 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-06-22T12:16:31Z

[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

ardaguclu · 2026-06-22T12:22:00Z

+		return "", fmt.Errorf("failed to get apiserver config: %w", err)
+	}
+
+	providerCfg, err := newKMSProviderConfig(apiServer.Spec.Encryption.KMS)


@tjungblu we have accessed providerCfg. This can be used for encryption-config.

maybe @p0lyn0mial can make up his mind on what he wants to pass down to the Deployer. I'll take whatever works.

not sure if i understand your comment.

ardaguclu · 2026-06-22T12:23:42Z

+	// picks up the config change (via apiServerInformer), which triggers us through
+	// operatorClient.Informer(). The minute-based resync is a backstop.
+	if currentHash != requiredHash {
+		klog.V(4).Infof("KMS config hash changed: required=%s, current=%s; waiting for the key-controller to post an updated condition", requiredHash, currentHash)


It might make sense adding a TODO about we should cleanup the running pods. They are obsolete now.

openshift-ci · 2026-06-22T13:33:20Z

@p0lyn0mial: 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/verify	`603bb8f`	link	true	`/test verify`

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.

tjungblu · 2026-06-22T14:39:22Z


 func (c *kmsPreflightController) runPreflightChecks(ctx context.Context) error {
-	return fmt.Errorf("implement me")
+	requiredHash, err := c.preflightRequired(ctx)


don't you need to pass the config along that matches this hash here? the deployer can't deploy from the hash itself

tjungblu · 2026-06-22T14:52:37Z

+	// picks up the config change (via apiServerInformer), which triggers us through
+	// operatorClient.Informer(). The minute-based resync is a backstop.
+	if currentHash != requiredHash {
+		klog.V(4).Infof("KMS config hash changed: required=%s, current=%s; waiting for the key-controller to post an updated condition", requiredHash, currentHash)


can we record an event for this?

tjungblu · 2026-06-22T14:54:32Z

+
+// preflightRequired returns the config hash that needs preflight validation,
+// or an empty string when no preflight is needed.
+func (c *kmsPreflightController) preflightRequired(ctx context.Context) (string, error) {


this reads like it will return a bool, requiredPreflightHash maybe?

depends on whether you intend to return the config along

I think our plan for the first iteration was to "just" deploy the preflight pod. The next iteration will focus on wiring the configuration.

tjungblu · 2026-06-22T14:56:55Z

+// or an empty string when no preflight is needed.
+func (c *kmsPreflightController) preflightRequired(ctx context.Context) (string, error) {
+	_, operatorStatus, _, err := c.operatorClient.GetOperatorState()
+	if err != nil {


you mention

which triggers this controller ... via the operatorClient informer.

earlier, why use a live client here?

encryption/controllers/kms_preflight_controller: read EncryptionKMSPr…

603bb8f

…eflightRequired condition and compare config hash

openshift-ci Bot requested review from ardaguclu and dgrisonnet June 22, 2026 12:16

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

tjungblu mentioned this pull request Jun 22, 2026

[Auto-Generated] KMS Team PR Dashboard #2268

Open

ardaguclu reviewed Jun 22, 2026

View reviewed changes

tjungblu reviewed Jun 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encryption/controllers/kms_preflight_controller: read EncryptionKMSPreflightRequired condition and compare config hash#2322

encryption/controllers/kms_preflight_controller: read EncryptionKMSPreflightRequired condition and compare config hash#2322
p0lyn0mial wants to merge 1 commit into
openshift:masterfrom
p0lyn0mial:kms-preflight-read-required-condition

p0lyn0mial commented Jun 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 22, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

openshift-ci Bot commented Jun 22, 2026

Uh oh!

ardaguclu Jun 22, 2026

Uh oh!

tjungblu Jun 22, 2026

Uh oh!

p0lyn0mial Jun 22, 2026

Uh oh!

ardaguclu Jun 22, 2026

Uh oh!

openshift-ci Bot commented Jun 22, 2026

Uh oh!

tjungblu Jun 22, 2026

Uh oh!

tjungblu Jun 22, 2026

Uh oh!

tjungblu Jun 22, 2026

Uh oh!

p0lyn0mial Jun 23, 2026

Uh oh!

tjungblu Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

p0lyn0mial commented Jun 22, 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 Jun 22, 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

❌ Failed checks (1 warning)

Uh oh!

openshift-ci Bot commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

p0lyn0mial commented Jun 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 22, 2026 •

edited

Loading