DO-NOT-MERGE: static pod deployer#2345
Draft
tjungblu wants to merge 1 commit into
Draft
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds static-pod support for KMS preflight by introducing new pod templates, refactoring template rendering, and implementing a ChangesKMS preflight static-pod flow
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/operator/encryption/kms/preflight/static_pod_deployer_test.go (1)
244-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert rollback on installer-create failure.
This test would still pass if
Deployleaked the copied Secret or ConfigMap after Line 247’s forced failure. After adding rollback, assert the cleanup delete actions or absence of those created objects.🤖 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/static_pod_deployer_test.go` around lines 244 - 253, Update TestStaticPodPreflightDeployer_Deploy_installerCreateFailure so it verifies rollback, not just the create error. After forcing the pod create failure in Deploy on StaticPodPreflightDeployer, assert that the copied Secret and ConfigMap created earlier are cleaned up by checking the kubeClient actions for delete calls or by confirming those objects no longer exist. Use the existing helpers around newTestStaticPodDeployer, preflightInstallerPodName, and Deploy to keep the test focused on the installer-create failure path.
🤖 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/kms/preflight/assets/kms-preflight-installer-pod.yaml`:
- Around line 37-39: The pod spec is missing resource limits for both
containers, so update the container definitions for cleanup and installer in
kms-preflight-installer-pod.yaml to include cpu and memory limits alongside the
existing requests. Ensure the limit settings are added consistently to each
container block so both the init container and main container satisfy the
resource policy.
- Around line 33-35: The installer pod securityContext is still fully privileged
and runs as root without any additional hardening. Review the installer pod
manifest and the relevant securityContext block to either justify the required
elevated access or reduce it where possible; if privileged/root is unavoidable,
add the missing hardening settings in the same installer pod definition (for
example by tightening capabilities and related security options) so the elevated
privileges are explicitly scoped and documented.
In `@pkg/operator/encryption/kms/preflight/static_pod_deployer.go`:
- Around line 110-129: Cleanup in StaticPodPreflightDeployer.Cleanup only
removes Kubernetes resources and leaves the host-side static-pod manifest and
copied resources behind. Update the cleanup flow to also invoke the
installer/static-pod removal path that deletes the host manifest and
static-pod-resources before or alongside deleting the Secret, ConfigMap, and
installer Pod, so kubelet cannot keep a stale mirror pod running.
- Around line 50-95: The preflight deploy flow in static_pod_deployer.go leaves
partially created resources behind when later steps fail, especially after the
secret/configmap creation and before installer pod creation. Update the deploy
logic around the sequence in the deploy method that calls
generateStaticPodTemplate, pluginlifecycle.NewKMSPluginBuilder().Apply, and
d.coreClient.ConfigMaps(...).Create so it tracks created resources and rolls
them back on any subsequent error, including the installer pod creation failure.
Ensure cleanup covers the copied encryption config Secret and the manifest
ConfigMap, and return the original error after rollback.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/preflight/static_pod_deployer_test.go`:
- Around line 244-253: Update
TestStaticPodPreflightDeployer_Deploy_installerCreateFailure so it verifies
rollback, not just the create error. After forcing the pod create failure in
Deploy on StaticPodPreflightDeployer, assert that the copied Secret and
ConfigMap created earlier are cleaned up by checking the kubeClient actions for
delete calls or by confirming those objects no longer exist. Use the existing
helpers around newTestStaticPodDeployer, preflightInstallerPodName, and Deploy
to keep the test focused on the installer-create failure path.
🪄 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: 75e22f92-226f-4837-864b-a606c55d3dfa
📒 Files selected for processing (6)
pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yamlpkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlpkg/operator/encryption/kms/preflight/pod_template.gopkg/operator/encryption/kms/preflight/pod_template_test.gopkg/operator/encryption/kms/preflight/static_pod_deployer.gopkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
Comment on lines
+33
to
+35
| securityContext: | ||
| privileged: true | ||
| runAsUser: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg 'pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yaml|pkg/operator/encryption/kms/preflight|kms-preflight'
printf '\n--- manifest ---\n'
cat -n pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yaml | sed -n '1,220p'
printf '\n--- references ---\n'
rg -n "kms-preflight-installer-pod|privileged: true|runAsUser: 0|allowPrivilegeEscalation|readOnlyRootFilesystem|capabilities:" pkg/operator/encryption/kms/preflight -S
printf '\n--- surrounding code ---\n'
fd -a . pkg/operator/encryption/kms/preflightRepository: openshift/library-go
Length of output: 5957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- pod_template.go ---\n'
cat -n pkg/operator/encryption/kms/preflight/pod_template.go | sed -n '1,240p'
printf '\n--- deployer.go ---\n'
cat -n pkg/operator/encryption/kms/preflight/deployer.go | sed -n '1,240p'
printf '\n--- deployer_test.go ---\n'
cat -n pkg/operator/encryption/kms/preflight/deployer_test.go | sed -n '1,220p'
printf '\n--- installer pod template tests ---\n'
cat -n pkg/operator/encryption/kms/preflight/pod_template_test.go | sed -n '1,260p'Repository: openshift/library-go
Length of output: 27824
Installer pod still runs as root with full privilege
These host-write containers likely need elevated access, but the manifest still leaves them as privileged: true and runAsUser: 0 without any clear justification or extra hardening. If that access is required, document why it cannot be reduced; otherwise scope it down and add the missing securityContext settings where compatible.
🤖 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/assets/kms-preflight-installer-pod.yaml`
around lines 33 - 35, The installer pod securityContext is still fully
privileged and runs as root without any additional hardening. Review the
installer pod manifest and the relevant securityContext block to either justify
the required elevated access or reduce it where possible; if privileged/root is
unavoidable, add the missing hardening settings in the same installer pod
definition (for example by tightening capabilities and related security options)
so the elevated privileges are explicitly scoped and documented.
Sources: Coding guidelines, Path instructions
Comment on lines
+37
to
+39
| requests: | ||
| memory: 10Mi | ||
| cpu: 10m |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing resource limits on both containers.
Only requests are defined; no limits are set for either the cleanup init container or the installer container.
As per path instructions, "Resource limits (cpu, memory) on every container."
🔧 Proposed fix
resources:
requests:
memory: 10Mi
cpu: 10m
+ limits:
+ memory: 10Mi
+ cpu: 10mAlso applies to: 69-71
🤖 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/assets/kms-preflight-installer-pod.yaml`
around lines 37 - 39, The pod spec is missing resource limits for both
containers, so update the container definitions for cleanup and installer in
kms-preflight-installer-pod.yaml to include cpu and memory limits alongside the
existing requests. Ensure the limit settings are added consistently to each
container block so both the init container and main container satisfy the
resource policy.
Source: Path instructions
Comment on lines
+50
to
+95
| if _, err := d.coreClient.Secrets(d.namespace).Create(ctx, encryptionConfigSecret, metav1.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to create preflight encryption config secret: %w", err) | ||
| } | ||
|
|
||
| pod, err := generateStaticPodTemplate( | ||
| preflightPodName, | ||
| d.namespace, | ||
| configHash, | ||
| d.operatorImage, | ||
| d.operatorCommand, | ||
| d.kmsCallTimeout, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate preflight pod template: %w", err) | ||
| } | ||
|
|
||
| err = pluginlifecycle.NewKMSPluginBuilder(). | ||
| WithSecretRequired(). | ||
| FromEncryptionConfigSecret(d.namespace, preflightEncryptionConfigSecretName, d.coreClient). | ||
| AsStaticPod(). | ||
| Apply(ctx, &pod.Spec, preflightCheckContainerName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to apply preflight plugin: %w", err) | ||
| } | ||
|
|
||
| podManifest := resourceread.WritePodV1OrDie(pod) | ||
| if _, err = d.coreClient.ConfigMaps(d.namespace).Create(ctx, &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: d.namespace, | ||
| Name: preflightPodConfigMapPrefix, | ||
| Labels: labels, | ||
| }, | ||
| Data: map[string]string{ | ||
| "pod.yaml": podManifest, | ||
| }, | ||
| }, metav1.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to create preflight pod configmap: %w", err) | ||
| } | ||
|
|
||
| installerPod, err := generateInstallerPodTemplate(d.namespace, d.operatorImage) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate preflight installer pod: %w", err) | ||
| } | ||
|
|
||
| if _, err = d.coreClient.Pods(d.namespace).Create(ctx, installerPod, metav1.CreateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to create preflight installer pod: %w", err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Roll back resources created during partial deploy failures.
After the preflight Secret is created, later failures leave the copied encryption config Secret and possibly the manifest ConfigMap behind; Line 94’s installer-create failure path currently returns without cleanup.
🧹 Proposed rollback pattern
-func (d *StaticPodPreflightDeployer) Deploy(ctx context.Context, configHash string, encryptionConfigSecret *corev1.Secret) error {
+func (d *StaticPodPreflightDeployer) Deploy(ctx context.Context, configHash string, encryptionConfigSecret *corev1.Secret) (retErr error) {
@@
if _, err := d.coreClient.Secrets(d.namespace).Create(ctx, encryptionConfigSecret, metav1.CreateOptions{}); err != nil {
return fmt.Errorf("failed to create preflight encryption config secret: %w", err)
}
+ defer func() {
+ if retErr != nil {
+ if cleanupErr := d.Cleanup(ctx); cleanupErr != nil {
+ retErr = errors.Join(retErr, cleanupErr)
+ }
+ }
+ }()📝 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
| if _, err := d.coreClient.Secrets(d.namespace).Create(ctx, encryptionConfigSecret, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight encryption config secret: %w", err) | |
| } | |
| pod, err := generateStaticPodTemplate( | |
| preflightPodName, | |
| d.namespace, | |
| configHash, | |
| d.operatorImage, | |
| d.operatorCommand, | |
| d.kmsCallTimeout, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate preflight pod template: %w", err) | |
| } | |
| err = pluginlifecycle.NewKMSPluginBuilder(). | |
| WithSecretRequired(). | |
| FromEncryptionConfigSecret(d.namespace, preflightEncryptionConfigSecretName, d.coreClient). | |
| AsStaticPod(). | |
| Apply(ctx, &pod.Spec, preflightCheckContainerName) | |
| if err != nil { | |
| return fmt.Errorf("failed to apply preflight plugin: %w", err) | |
| } | |
| podManifest := resourceread.WritePodV1OrDie(pod) | |
| if _, err = d.coreClient.ConfigMaps(d.namespace).Create(ctx, &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: d.namespace, | |
| Name: preflightPodConfigMapPrefix, | |
| Labels: labels, | |
| }, | |
| Data: map[string]string{ | |
| "pod.yaml": podManifest, | |
| }, | |
| }, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight pod configmap: %w", err) | |
| } | |
| installerPod, err := generateInstallerPodTemplate(d.namespace, d.operatorImage) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate preflight installer pod: %w", err) | |
| } | |
| if _, err = d.coreClient.Pods(d.namespace).Create(ctx, installerPod, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight installer pod: %w", err) | |
| func (d *StaticPodPreflightDeployer) Deploy(ctx context.Context, configHash string, encryptionConfigSecret *corev1.Secret) (retErr error) { | |
| if _, err := d.coreClient.Secrets(d.namespace).Create(ctx, encryptionConfigSecret, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight encryption config secret: %w", err) | |
| } | |
| defer func() { | |
| if retErr != nil { | |
| if cleanupErr := d.Cleanup(ctx); cleanupErr != nil { | |
| retErr = errors.Join(retErr, cleanupErr) | |
| } | |
| } | |
| }() | |
| pod, err := generateStaticPodTemplate( | |
| preflightPodName, | |
| d.namespace, | |
| configHash, | |
| d.operatorImage, | |
| d.operatorCommand, | |
| d.kmsCallTimeout, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate preflight pod template: %w", err) | |
| } | |
| err = pluginlifecycle.NewKMSPluginBuilder(). | |
| WithSecretRequired(). | |
| FromEncryptionConfigSecret(d.namespace, preflightEncryptionConfigSecretName, d.coreClient). | |
| AsStaticPod(). | |
| Apply(ctx, &pod.Spec, preflightCheckContainerName) | |
| if err != nil { | |
| return fmt.Errorf("failed to apply preflight plugin: %w", err) | |
| } | |
| podManifest := resourceread.WritePodV1OrDie(pod) | |
| if _, err = d.coreClient.ConfigMaps(d.namespace).Create(ctx, &corev1.ConfigMap{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Namespace: d.namespace, | |
| Name: preflightPodConfigMapPrefix, | |
| Labels: labels, | |
| }, | |
| Data: map[string]string{ | |
| "pod.yaml": podManifest, | |
| }, | |
| }, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight pod configmap: %w", err) | |
| } | |
| installerPod, err := generateInstallerPodTemplate(d.namespace, d.operatorImage) | |
| if err != nil { | |
| return fmt.Errorf("failed to generate preflight installer pod: %w", err) | |
| } | |
| if _, err = d.coreClient.Pods(d.namespace).Create(ctx, installerPod, metav1.CreateOptions{}); err != nil { | |
| return fmt.Errorf("failed to create preflight installer pod: %w", err) | |
| } |
🤖 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/static_pod_deployer.go` around lines 50
- 95, The preflight deploy flow in static_pod_deployer.go leaves partially
created resources behind when later steps fail, especially after the
secret/configmap creation and before installer pod creation. Update the deploy
logic around the sequence in the deploy method that calls
generateStaticPodTemplate, pluginlifecycle.NewKMSPluginBuilder().Apply, and
d.coreClient.ConfigMaps(...).Create so it tracks created resources and rolls
them back on any subsequent error, including the installer pod creation failure.
Ensure cleanup covers the copied encryption config Secret and the manifest
ConfigMap, and return the original error after rollback.
Comment on lines
+110
to
+129
| func (d *StaticPodPreflightDeployer) Cleanup(ctx context.Context) error { | ||
| var errs []error | ||
|
|
||
| installerPodName := preflightInstallerPodName | ||
| err := d.coreClient.Pods(d.namespace).Delete(ctx, installerPodName, metav1.DeleteOptions{}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| errs = append(errs, fmt.Errorf("failed to delete installer pod %s/%s: %w", d.namespace, installerPodName, err)) | ||
| } | ||
|
|
||
| err = d.coreClient.Secrets(d.namespace).Delete(ctx, preflightEncryptionConfigSecretName, metav1.DeleteOptions{}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| errs = append(errs, fmt.Errorf("failed to delete secret %s/%s: %w", d.namespace, preflightEncryptionConfigSecretName, err)) | ||
| } | ||
|
|
||
| err = d.coreClient.ConfigMaps(d.namespace).Delete(ctx, preflightPodConfigMapPrefix, metav1.DeleteOptions{}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| errs = append(errs, fmt.Errorf("failed to delete configmap %s/%s: %w", d.namespace, preflightPodConfigMapPrefix, err)) | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cleanup needs to remove the host static-pod artifacts.
The installer writes the manifest and supporting secret material onto the host, but Cleanup only deletes Kubernetes API objects. Deleting the ConfigMap/Secret does not remove the already-copied files, so kubelet can keep the mirror pod running with stale resources. Add a cleanup installer/static-pod removal path that deletes the host manifest and copied static-pod-resources before/while removing the API objects.
🤖 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/static_pod_deployer.go` around lines
110 - 129, Cleanup in StaticPodPreflightDeployer.Cleanup only removes Kubernetes
resources and leaves the host-side static-pod manifest and copied resources
behind. Update the cleanup flow to also invoke the installer/static-pod removal
path that deletes the host manifest and static-pod-resources before or alongside
deleting the Secret, ConfigMap, and installer Pod, so kubelet cannot keep a
stale mirror pod running.
tjungblu
force-pushed
the
static_pod_kms_preflight
branch
from
bd28e37 to
66565e6
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kms/preflight/assets/kms-preflight-pod.yaml`:
- Around line 27-29: The static-pod branch in kms-preflight-pod.yaml sets
hostNetwork: true without the required justification comment. Add an inline
manifest comment near the .StaticPod conditional that explains why hostNetwork
is necessary for the static-pod variant (for example, API server reachability
before CNI is ready) and explicitly notes that no lower-privilege alternative is
available. Keep the change scoped to the hostNetwork stanza and preserve the
existing .StaticPod gating.
🪄 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: 99932d52-d51d-4d52-8b86-5f368f6e7dd7
📒 Files selected for processing (6)
pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yamlpkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlpkg/operator/encryption/kms/preflight/pod_template.gopkg/operator/encryption/kms/preflight/pod_template_test.gopkg/operator/encryption/kms/preflight/static_pod_deployer.gopkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/encryption/kms/preflight/pod_template_test.go
- pkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
- pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yaml
- pkg/operator/encryption/kms/preflight/pod_template.go
- pkg/operator/encryption/kms/preflight/static_pod_deployer.go
tjungblu
force-pushed
the
static_pod_kms_preflight
branch
from
66565e6 to
6e06658
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a4c3cf6-200c-4ddc-abd8-70b38314f27a
📒 Files selected for processing (7)
pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yamlpkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlpkg/operator/encryption/kms/preflight/deployer.gopkg/operator/encryption/kms/preflight/pod_template.gopkg/operator/encryption/kms/preflight/pod_template_test.gopkg/operator/encryption/kms/preflight/static_pod_deployer.gopkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
💤 Files with no reviewable changes (1)
- pkg/operator/encryption/kms/preflight/deployer.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
- pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yaml
- pkg/operator/encryption/kms/preflight/static_pod_deployer.go
tjungblu
force-pushed
the
static_pod_kms_preflight
branch
from
6e06658 to
4497168
Compare
tjungblu
commented
Jul 3, 2026
| # This is a one-shot preflight check, not a long-running pod. | ||
| # The operator creates it, waits for completion, and inspects the result. | ||
| restartPolicy: Never | ||
| {{- if not .StaticPod}} |
Contributor
Author
There was a problem hiding this comment.
not possible to use a service account anymore due to:
https://github.com/kubernetes/kubernetes/blob/cbe5e3096ec29950198ed0b77ed9c611f3183c14/pkg/kubelet/config/common.go#L151-L159
This is just an e2e, not destined for review, we pull individual refactorings out. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
force-pushed
the
static_pod_kms_preflight
branch
from
4497168 to
8fe795e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kms/preflight/static_pod_deployer.go`:
- Around line 90-99: The ConfigMap created in static_pod_deployer.go is
incorrectly storing credential data from node-kubeconfigs alongside pod.yaml in
kms-preflight-pod. Update the preflight pod setup in the staticPodDeployer flow
so pod.yaml remains in the ConfigMap, but lb-int.kubeconfig is provided via a
Secret volume or by mounting node-kubeconfigs directly; use the existing
ConfigMap creation and pod manifest assembly points to separate non-sensitive
config from kubeconfig material.
🪄 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: 128c6559-8472-44e1-acc1-8197ed0e5bcb
📒 Files selected for processing (7)
pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yamlpkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlpkg/operator/encryption/kms/preflight/deployer.gopkg/operator/encryption/kms/preflight/pod_template.gopkg/operator/encryption/kms/preflight/pod_template_test.gopkg/operator/encryption/kms/preflight/static_pod_deployer.gopkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
💤 Files with no reviewable changes (1)
- pkg/operator/encryption/kms/preflight/deployer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/encryption/kms/preflight/pod_template.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/kms/preflight/static_pod_deployer.go`:
- Around line 193-207: The node kubeconfig fallback in nodeKubeconfigData is
broken because mustAsset("assets/lb-int.kubeconfig") references an asset that
does not exist, so a NotFound on node-kubeconfigs can panic the deploy flow.
Update StaticPodPreflightDeployer.nodeKubeconfigData to return a valid fallback
source that is actually present or otherwise handle the NotFound case without
using mustAsset, and keep the existing secret/error handling paths intact so the
function fails cleanly instead of crashing.
🪄 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: 0580546c-495d-4657-8378-afb604d30132
📒 Files selected for processing (7)
pkg/operator/encryption/kms/preflight/assets/kms-preflight-installer-pod.yamlpkg/operator/encryption/kms/preflight/assets/kms-preflight-pod.yamlpkg/operator/encryption/kms/preflight/deployer.gopkg/operator/encryption/kms/preflight/pod_template.gopkg/operator/encryption/kms/preflight/pod_template_test.gopkg/operator/encryption/kms/preflight/static_pod_deployer.gopkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
💤 Files with no reviewable changes (1)
- pkg/operator/encryption/kms/preflight/deployer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/kms/preflight/pod_template_test.go
- pkg/operator/encryption/kms/preflight/static_pod_deployer_test.go
Comment on lines
+193
to
+207
| func (d *StaticPodPreflightDeployer) nodeKubeconfigData(ctx context.Context) ([]byte, error) { | ||
| secret, err := d.coreClient.Secrets(d.namespace).Get(ctx, nodeKubeconfigsSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return mustAsset("assets/lb-int.kubeconfig"), nil | ||
| } | ||
| return nil, fmt.Errorf("failed to get %s/%s secret: %w", d.namespace, nodeKubeconfigsSecretName, err) | ||
| } | ||
|
|
||
| kubeconfig, ok := secret.Data[nodeKubeconfigSecretKey] | ||
| if !ok || len(kubeconfig) == 0 { | ||
| return nil, fmt.Errorf("secret %s/%s is missing %q data", d.namespace, nodeKubeconfigsSecretName, nodeKubeconfigSecretKey) | ||
| } | ||
| return kubeconfig, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the embedded fallback asset and mustAsset resolution
fd -t f 'lb-int.kubeconfig' pkg/operator/encryption/kms/preflight
rg -nP '\bmustAsset\b' -C3 pkg/operator/encryption/kms/preflight
fd -t f 'lb-int.kubeconfig' pkg/operator/encryption/kms/preflight/assets --exec sed -n '1,40p' {}Repository: openshift/library-go
Length of output: 2899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== asset listing ==\n'
fd -t f 'lb-int.kubeconfig' pkg/operator/encryption/kms/preflight
printf '\n== asset contents ==\n'
sed -n '1,120p' pkg/operator/encryption/kms/preflight/assets/lb-int.kubeconfig
printf '\n== references to lb-int.kubeconfig ==\n'
rg -n 'lb-int\.kubeconfig|nodeKubeconfigsSecretName|nodeKubeconfigData\(' pkg/operator/encryption/kms/preflight -C 3
printf '\n== nearby deploy flow ==\n'
sed -n '1,260p' pkg/operator/encryption/kms/preflight/static_pod_deployer.goRepository: openshift/library-go
Length of output: 307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate asset file ==\n'
fd -a -t f 'lb-int.kubeconfig' .
printf '\n== locate assets directories under preflight ==\n'
fd -a -t d 'assets' pkg/operator/encryption/kms/preflight
printf '\n== search for fallback asset usage ==\n'
rg -n 'lb-int\.kubeconfig|mustAsset\(' pkg/operator/encryption/kms/preflight -C 3
printf '\n== file inventory around preflight ==\n'
git ls-files pkg/operator/encryption/kms/preflightRepository: openshift/library-go
Length of output: 7868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== nodeKubeconfigData call flow ==\n'
sed -n '1,260p' pkg/operator/encryption/kms/preflight/static_pod_deployer.go
printf '\n== tests around nodeKubeconfigData / missing secret ==\n'
rg -n 'nodeKubeconfigData|node-kubeconfigs|lb-int\.kubeconfig|NotFound|missing "lb-int\.kubeconfig"' pkg/operator/encryption/kms/preflight/*_test.go -C 4
printf '\n== static pod deployer tests ==\n'
sed -n '1,260p' pkg/operator/encryption/kms/preflight/static_pod_deployer_test.goRepository: openshift/library-go
Length of output: 20401
Missing fallback asset will panic on NotFound. nodeKubeconfigData() calls mustAsset("assets/lb-int.kubeconfig"), but that file isn’t present under pkg/operator/encryption/kms/preflight/assets/. A missing node-kubeconfigs Secret will crash the deploy path instead of failing cleanly.
🤖 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/static_pod_deployer.go` around lines
193 - 207, The node kubeconfig fallback in nodeKubeconfigData is broken because
mustAsset("assets/lb-int.kubeconfig") references an asset that does not exist,
so a NotFound on node-kubeconfigs can panic the deploy flow. Update
StaticPodPreflightDeployer.nodeKubeconfigData to return a valid fallback source
that is actually present or otherwise handle the NotFound case without using
mustAsset, and keep the existing secret/error handling paths intact so the
function fails cleanly instead of crashing.
Source: Linters/SAST tools
This was referenced Jul 7, 2026
openshift-merge-bot Bot
pushed a commit
that referenced
this pull request
Jul 8, 2026
to facilitate static pods, this is a spin-off from #2345. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
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. |
3 tasks
tjungblu
added a commit
to tjungblu/library-go
that referenced
this pull request
Jul 9, 2026
Alternative e2e to openshift#2345 that deploys the preflight static pod through revision snapshots and the shared installerpod installer instead of a custom shell-based installer pod. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
added a commit
to tjungblu/library-go
that referenced
this pull request
Jul 9, 2026
Alternative e2e to openshift#2345 that deploys the preflight static pod through revision snapshots and the shared installerpod installer instead of a custom shell-based installer pod. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
tjungblu
added a commit
to tjungblu/library-go
that referenced
this pull request
Jul 9, 2026
Alternative e2e to openshift#2345 that deploys the preflight static pod through revision snapshots and the shared installerpod installer instead of a custom shell-based installer pod. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>
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
1 participant
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.
This is just an e2e, not destined for review, we pull individual refactorings out.
Summary by CodeRabbit
New Features
Refactor
Tests