Review

[ardaguclu]

Reviewed version of #2249

Caller will use like this;

 encryption.TestInvalidKMSRecovery(ctx, t, encryption.InvalidKMSRecoveryScenario{
      BasicScenario:      basicScenario, 
      InvalidKMSProvider: invalidProvider,
      ValidKMSProvider:   validProvider,
      WaitForStuck: func(ctx context.Context, t testing.TB) {
          encryption.WaitForPodImagePullBackOff(ctx, t, kubeClient, ns, selector, 10*time.Minute)
      },
  })

Summary by CodeRabbit

• Tests
  • Enhanced encryption testing infrastructure with improved helper functions and additional test scenarios for validation.

[openshift-ci]

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu

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:

• test/library/encryption/OWNERS [ardaguclu]

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

[coderabbitai]

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: 2b037a2c-a24d-4be8-b550-66b49f91a279

📥 Commits

Reviewing files that changed from the base of the PR and between aa49560 and 2412c7e.

📒 Files selected for processing (2)

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

---

Walkthrough

This PR extends the encryption test library by exporting a key validation helper for reuse, adding a pod-status polling utility, and introducing a new KMS recovery scenario test that validates recovery from invalid KMS provider configurations via AESCBC fallback with key metadata verification.

Changes

Encryption test helpers and scenarios

Layer / File(s)	Summary
Export encryption key validation helper test/library/encryption/helpers.go	waitForNoNewEncryptionKey is renamed to WaitForNoNewEncryptionKey and exported; WaitForEncryptionKeyBasedOn is updated to call the exported version for consistent mode matching.
Add pod image pull backoff polling helper test/library/encryption/helpers.go	New exported WaitForPodImagePullBackOff polls pods by label selector for ImagePullBackOff or ErrImagePull container states, logging the first match and failing on timeout.
Add encryption application helper and KMS recovery scenario test/library/encryption/scenarios.go	Import metav1; add ApplyEncryption helper to update API server encryption; introduce InvalidKMSRecoveryScenario and TestInvalidKMSRecovery to validate recovery from invalid KMS by applying invalid config, waiting for stuck state, switching to AESCBC with key metadata verification, then applying valid KMS configuration.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 12 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Microshift Test Compatibility	⚠️ Warning	New test functions (TestInvalidKMSRecovery, ApplyEncryption) use config.openshift.io/v1 APIServer API which is unavailable on MicroShift (only Route and SecurityContextConstraints APIs are supported).	Add [apigroup:config.openshift.io] tag to any e2e tests calling these functions, or implement MicroShift compatibility checks with g.Skip().
Title check	❓ Inconclusive	The title 'Review' is vague and generic, providing no meaningful information about the specific changes made to the codebase.	Use a descriptive title that summarizes the main changes, such as 'Add KMS encryption recovery test and refactor encryption helpers' or similar.

✅ Passed checks (12 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	PR uses Go's standard testing package (testing.TB), not Ginkgo. Test step names are static (no dynamic info). Check not applicable to non-Ginkgo tests.
Test Structure And Quality	✅ Passed	Test meets all requirements: single responsibility (KMS recovery), proper setup/teardown, timeouts on async ops, assertion patterns match codebase, follows established step-based test design.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests are added in this PR. Changes are to test/library/encryption/, a helper library with test utility functions, not standalone Ginkgo tests. The check does not apply.
Topology-Aware Scheduling Compatibility	✅ Passed	PR contains only test helper functions and test scenarios (no deployment manifests, operator code, or controllers) that introduce scheduling constraints affecting topology-aware deployments.
Ote Binary Stdout Contract	✅ Passed	PR changes are test/library helper functions with no process-level code. All logging uses t.Logf (framework-intercepted). No stdout writes, klog, fmt.Print, or log.SetOutput issues found.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests (It/Describe/Context/When) added. Code uses testing.TB interface. No IPv4 assumptions, hardcoded IPs, or external connectivity requirements detected in new helpers.
No-Weak-Crypto	✅ Passed	PR adds encryption test helpers. No weak crypto algorithms, custom crypto implementations, or non-constant-time secret comparisons detected.
Container-Privileges	✅ Passed	PR modifies only Go test helper files (helpers.go, scenarios.go) with no container/K8s manifests defining privileged configs, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation.
No-Sensitive-Data-In-Logs	✅ Passed	New code logs only non-sensitive data: encryption types, pod names, timeouts, test step names. No passwords, tokens, API keys, or PII are exposed.

_{✏️ 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.}

[ardaguclu]

This is just for demonstrating the proposed idea
/close

[openshift-ci]

@ardaguclu: Closed this PR.

Details

In response to this:

This is just for demonstrating the proposed idea
/close

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.