NO-JIRA: inject kms sidecar in preflight deployer#2321
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tjungblu: This pull request explicitly references no jira issue. DetailsIn 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds ChangesKMS Preflight Ephemeral Encryption Config and Sidecar Injection
Sequence DiagramsequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Caller,KubeClient: Deploy flow
Caller->>PodPreflightDeployer: Deploy(ctx, configHash, pluginConfig)
PodPreflightDeployer->>BuildEphemeralPreflightEncryptionConfig: pluginConfig, encryptionKeyID, socketEndpoint
BuildEphemeralPreflightEncryptionConfig->>KubeClient: Get Secret from openshift-config
BuildEphemeralPreflightEncryptionConfig->>KubeClient: Get ConfigMap from openshift-config
BuildEphemeralPreflightEncryptionConfig-->>PodPreflightDeployer: encryptiondata.Config
PodPreflightDeployer->>KubeClient: ApplySecret(ephemeral encryption Secret)
PodPreflightDeployer->>KMSPluginBuilder: FromEncryptionConfig(...).Apply(pod.Spec)
KMSPluginBuilder-->>PodPreflightDeployer: pod.Spec with sidecar injected
PodPreflightDeployer->>KubeClient: Create Pod
end
rect rgba(255, 200, 150, 0.5)
Note over Caller,KubeClient: Cleanup flow
Caller->>PodPreflightDeployer: Cleanup(ctx)
PodPreflightDeployer->>KubeClient: Delete Pod (ignore not-found)
PodPreflightDeployer->>KubeClient: Delete ephemeral encryption Secret (ignore not-found)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tjungblu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tjungblu
force-pushed
the
inject_sidecar
branch
2 times, most recently
from
57467e7 to
da70484
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/encryption/kms/preflight/deployer.go (1)
71-73: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winWrap pod creation failures with pod identity context.
Line 71 currently returns the raw create error, which makes controller logs harder to triage under failure.
Suggested patch
- _, err = d.coreClient.Pods(d.namespace).Create(ctx, pod, metav1.CreateOptions{}) - return err + if _, err = d.coreClient.Pods(d.namespace).Create(ctx, pod, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("failed to create preflight pod %s/%s: %w", d.namespace, preflightPodName, err) + } + return nil🤖 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/kms/preflight/deployer.go` around lines 71 - 73, The pod creation error returned on line 71 lacks context about which pod failed to create, making it difficult to triage failures in controller logs. Wrap the error returned from d.coreClient.Pods(d.namespace).Create() with the pod's identity information (such as the pod name) using error wrapping to provide clear context about which pod creation failed.pkg/operator/encryption/kms/pluginlifecycle/builder_test.go (1)
88-101: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression case for override + multi-plugin rejection.
Line 88 covers the happy path, but the new guard in
Apply(socket endpoint override supports a single KMS plugin only) should also be asserted in this table to prevent regressions.🤖 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/kms/pluginlifecycle/builder_test.go` around lines 88 - 101, The test table in this file covers the happy path for socket endpoint override but lacks a regression test case to verify the validation guard that rejects socket endpoint overrides when multiple KMS plugins are configured. Add a new test case in the same table-driven test structure (after or alongside the existing "socket endpoint override" case) that configures multiple KMS plugins in the encryption config and attempts to apply a socket endpoint override via WithSocketEndpoint, then verify in the test that the Apply method properly rejects this configuration with an appropriate error indicating that socket endpoint override supports a single KMS plugin only.
🤖 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.
Nitpick comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/builder_test.go`:
- Around line 88-101: The test table in this file covers the happy path for
socket endpoint override but lacks a regression test case to verify the
validation guard that rejects socket endpoint overrides when multiple KMS
plugins are configured. Add a new test case in the same table-driven test
structure (after or alongside the existing "socket endpoint override" case) that
configures multiple KMS plugins in the encryption config and attempts to apply a
socket endpoint override via WithSocketEndpoint, then verify in the test that
the Apply method properly rejects this configuration with an appropriate error
indicating that socket endpoint override supports a single KMS plugin only.
In `@pkg/operator/encryption/kms/preflight/deployer.go`:
- Around line 71-73: The pod creation error returned on line 71 lacks context
about which pod failed to create, making it difficult to triage failures in
controller logs. Wrap the error returned from
d.coreClient.Pods(d.namespace).Create() with the pod's identity information
(such as the pod name) using error wrapping to provide clear context about which
pod creation failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59370520-0596-4b40-b42f-31838c378ade
📒 Files selected for processing (5)
pkg/operator/encryption/kms/pluginlifecycle/builder.gopkg/operator/encryption/kms/pluginlifecycle/builder_test.gopkg/operator/encryption/kms/preflight/cmd.gopkg/operator/encryption/kms/preflight/deployer.gopkg/operator/encryption/kms/preflight/deployer_test.go
💤 Files with no reviewable changes (1)
- pkg/operator/encryption/kms/preflight/cmd.go
|
|
||
| type kmsConfigHasher struct { | ||
| provider kmsProviderConfig | ||
| provider KMSProviderConfig |
There was a problem hiding this comment.
Instead of exposing this, can't we move the buildephemeralencryptionconfig function here?
| // Deploy creates the preflight checker with the given config hash | ||
| Deploy(ctx context.Context, configHash string) error | ||
| // Deploy creates the preflight checker with the given config hash and KMS plugin configuration. | ||
| Deploy(ctx context.Context, configHash string, pluginConfig configv1.KMSPluginConfig) error |
There was a problem hiding this comment.
and pass encryptionconfig name in the namespace here?
There was a problem hiding this comment.
the other comment I understood, I've moved the ephemeral builder to the other package. What should I replace this with?
| } | ||
|
|
||
| err = pluginlifecycle.NewKMSPluginBuilder(). | ||
| FromEncryptionConfig(preflightEncryptionConfigSecretName, encryptionConfig). |
There was a problem hiding this comment.
builder only needs an existed secret name
There was a problem hiding this comment.
that's fair, but the pods needs a real secret to run and I need to create it. What should I change here?
| } | ||
|
|
||
| func ephemeralEncryptionConfigSecret(namespace string, cfg *encryptiondata.Config) (*corev1.Secret, error) { | ||
| return encryptiondata.ToSecret(namespace, preflightEncryptionConfigSecretName, cfg) |
There was a problem hiding this comment.
This may also need a flag to omit the finalizer, otherwise we won't be able to delete it easily
There was a problem hiding this comment.
Manually deleting the finalizer from the value returned from ToSecret might work?
tjungblu
force-pushed
the
inject_sidecar
branch
2 times, most recently
from
c5a586b to
84b0efc
Compare
Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
|
@tjungblu: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| } | ||
|
|
||
| func (d *PodPreflightDeployer) Deploy(ctx context.Context, configHash string) error { | ||
| func (d *PodPreflightDeployer) Deploy(ctx context.Context, configHash string, pluginConfig configv1.KMSPluginConfig) error { |
There was a problem hiding this comment.
Logical flow in that function makes sense to me. I think, we just may need to agree upon the how we align built-in functions.
3 participants
Summary by CodeRabbit
Bug Fixes
Tests