Kms preflight sandbox interceptor - #2381
Closed
p0lyn0mial wants to merge 4 commits into
Closed
Conversation
The preflight sandbox previously used testing/fstest.MapFS and manifestclient.NewTestingHTTPClient to simulate dry-run controller execution, importing a test-only package in production code. Replace it with a thin interceptingSecretsGetter that wraps the real corev1client.SecretsGetter. Reads fall through to the live cluster; writes matching a caller-supplied predicate are captured in an in-memory map and never reach the real API. A shared interceptor instance between the KeyController and StateController dry-runs means the intercepted key secret is visible to the StateController's List call without any pre-seeding or YAML serialization. staticEncryptionDeployer is removed from production code; both dry-run controllers now use the real encryptionDeployer directly, which is already known to be converged at the call site. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughKMS preflight now computes its encryption-config Secret through dry-run key and state controller execution, intercepts Secret writes, rewrites KMS endpoints for the preflight socket, and supplies the result during deployment. Key and state controllers are also exported with factory-compatible interfaces. ChangesKMS preflight encryption flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KMSPreflightController
participant KeyController
participant StateController
participant SecretClient
KMSPreflightController->>KeyController: Run dry-run Sync
KeyController->>SecretClient: Intercept key Secret write
KMSPreflightController->>StateController: Run dry-run Sync
StateController->>SecretClient: Intercept encryption-config write
KMSPreflightController->>KMSPreflightController: Rewrite KMS endpoint
KMSPreflightController->>KMSPreflightController: Deploy preflight workload with Secret
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/kms_preflight_controller_test.go (1)
846-860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover dry-run convergence and error paths.
The scenario table cannot configure
encryptionDeployer, so it only verifies successful deployment. Add cases where dry-run computation requires convergence (assert no Deploy) and where it fails (assert the controller reports degradation).Also applies to: 877-885
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go` around lines 846 - 860, Extend the scenario table and test setup around NewKMSPreflightController to allow configuring encryptionDeployer behavior. Add a dry-run convergence case that verifies Deploy is not called, and a dry-run failure case that verifies the controller reports degradation, while preserving the existing successful deployment coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox_test.go`:
- Around line 85-87: Update the failure message in the fakeKubeClient action
loop to avoid formatting the entire action object. Log only the action verb,
resource, and namespace, using the corresponding action accessors, so Secret
data and generated credentials cannot appear in CI output.
---
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 846-860: Extend the scenario table and test setup around
NewKMSPreflightController to allow configuring encryptionDeployer behavior. Add
a dry-run convergence case that verifies Deploy is not called, and a dry-run
failure case that verifies the controller reports degradation, while preserving
the existing successful deployment coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 98825896-905d-414b-9a80-8a432cb70354
📒 Files selected for processing (8)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/controllers/kms_preflight_interceptor.gopkg/operator/encryption/controllers/kms_preflight_sandbox.gopkg/operator/encryption/controllers/kms_preflight_sandbox_test.gopkg/operator/encryption/controllers/state_controller.go
Comment on lines
+85
to
+87
| for _, action := range fakeKubeClient.Actions() { | ||
| if action.GetVerb() == "create" || action.GetVerb() == "update" { | ||
| t.Fatalf("live client must not be mutated, got action %#v", action) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not serialize the full Secret action into CI logs.
%#v can recursively print the action’s Secret object and its Data, including generated key material or the Vault secret-id. Log only verb, resource, and namespace.
Proposed fix
- t.Fatalf("live client must not be mutated, got action %#v", action)
+ t.Fatalf("live client must not be mutated, got %s on %s in namespace %q",
+ action.GetVerb(), action.GetResource().Resource, action.GetNamespace())As per coding guidelines, “Flag test logging that may expose passwords, tokens, API keys, PII …”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, action := range fakeKubeClient.Actions() { | |
| if action.GetVerb() == "create" || action.GetVerb() == "update" { | |
| t.Fatalf("live client must not be mutated, got action %#v", action) | |
| for _, action := range fakeKubeClient.Actions() { | |
| if action.GetVerb() == "create" || action.GetVerb() == "update" { | |
| t.Fatalf("live client must not be mutated, got %s on %s in namespace %q", | |
| action.GetVerb(), action.GetResource().Resource, action.GetNamespace()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/encryption/controllers/kms_preflight_sandbox_test.go` around
lines 85 - 87, Update the failure message in the fakeKubeClient action loop to
avoid formatting the entire action object. Log only the action verb, resource,
and namespace, using the corresponding action accessors, so Secret data and
generated credentials cannot appear in CI output.
Source: Coding guidelines
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
xref: #2377
Summary by CodeRabbit