WIP: Kms plugin pod mutation #2056
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| package targetconfigcontroller | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" | ||
| "github.com/openshift/library-go/pkg/operator/staticpod/installerpod" | ||
| corev1 "k8s.io/api/core/v1" | ||
| v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| corev1listers "k8s.io/client-go/listers/core/v1" | ||
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| // AddKMSPluginToPodSpec conditionally adds the KMS plugin volume mount to the specified container. | ||
| // It assumes the pod spec does not already contain the KMS volume or mount; no deduplication is performed. | ||
| // Deprecated: this is a temporary solution to get KMS TP v1 out. We should come up with a different approach afterwards. | ||
| func AddKMSPluginToPodSpec(podSpec *corev1.PodSpec, featureGateAccessor featuregates.FeatureGateAccess, secretLister corev1listers.SecretLister, targetNamespace string, kmsPluginImage string) error { | ||
| if podSpec == nil { | ||
| return fmt.Errorf("pod spec cannot be nil") | ||
| } | ||
|
|
||
| // if !featureGateAccessor.AreInitialFeatureGatesObserved() { | ||
| // return nil | ||
| // } | ||
| // | ||
| // featureGates, err := featureGateAccessor.CurrentFeatureGates() | ||
| // if err != nil { | ||
| // return fmt.Errorf("failed to get feature gates: %w", err) | ||
| // } | ||
| // | ||
| // if !featureGates.Enabled(features.FeatureGateKMSEncryption) { | ||
| // klog.Infof("kms is disabled: feature gate %s is disabled", features.FeatureGateKMSEncryption) | ||
| // return nil | ||
| // } | ||
|
|
||
| creds, err := secretLister.Secrets(targetNamespace).Get("vault-kms-credentials") | ||
| if err != nil { | ||
| klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err) | ||
| return nil | ||
|
Comment on lines
+37
to
+40
There was a problem hiding this comment. Handle non-NotFound secret retrieval errors explicitly. This currently treats every secret retrieval error as “KMS disabled,” which can silently bypass KMS on transient API/cache failures. Only “not found” should be treated as disabled; other errors should fail fast. 🔧 Proposed fix+import apierrors "k8s.io/apimachinery/pkg/api/errors"
...
creds, err := secretLister.Secrets(targetNamespace).Get("vault-kms-credentials")
if err != nil {
- klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
- return nil
+ if apierrors.IsNotFound(err) {
+ klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
+ return nil
+ }
+ return fmt.Errorf("failed to get vault-kms-credentials secret: %w", err)
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // At this point we know we should deploy the KMS plugin | ||
| if err := addKMSPluginSidecarToPodSpec(podSpec, "kms-plugin", kmsPluginImage, creds); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kms-plugin"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := addKMSPluginVolumeAndMountToPodSpec(podSpec, "kube-apiserver"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func addKMSPluginSidecarToPodSpec(podSpec *corev1.PodSpec, containerName string, image string, creds *corev1.Secret) error { | ||
| if podSpec == nil { | ||
| return fmt.Errorf("pod spec cannot be nil") | ||
| } | ||
|
|
||
| podSpec.Containers = append(podSpec.Containers, corev1.Container{ | ||
| Name: containerName, | ||
| Image: image, | ||
| ImagePullPolicy: corev1.PullAlways, | ||
| Command: []string{"/bin/sh", "-c"}, | ||
| Args: []string{fmt.Sprintf(` | ||
| echo "%s" > /tmp/secret-id | ||
|
Contributor
There was a problem hiding this comment. some "secrets" should not be visible in the pod template.
Member
Author
There was a problem hiding this comment. discussed offline, but just for context: right, this implementation is just a poc, once openshift/enhancements#1960 is implemented we'll be able to access the credentials as a mounted secret |
||
| exec /vault-kube-kms \ | ||
| -listen-address=unix:///var/run/kmsplugin/kms.sock \ | ||
| -vault-address=%s \ | ||
| -vault-namespace=%s \ | ||
| -transit-mount=transit \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ | ||
| -approle-role-id=%s \ | ||
| -approle-secret-id-path=/tmp/secret-id`, | ||
| string(creds.Data["VAULT_SECRET_ID"]), | ||
|
Contributor
There was a problem hiding this comment. what is |
||
| string(creds.Data["VAULT_ADDR"]), | ||
| string(creds.Data["VAULT_NAMESPACE"]), | ||
| string(creds.Data["VAULT_KEY_NAME"]), | ||
| string(creds.Data["VAULT_ROLE_ID"])), | ||
|
Comment on lines
+69
to
+84
There was a problem hiding this comment. Missing validation for required secret keys. The code assumes all required keys ( 🛡️ Suggested validationfunc validateCredentials(creds *corev1.Secret) error {
requiredKeys := []string{"VAULT_SECRET_ID", "VAULT_ADDR", "VAULT_NAMESPACE", "VAULT_KEY_NAME", "VAULT_ROLE_ID"}
for _, key := range requiredKeys {
if _, ok := creds.Data[key]; !ok {
return fmt.Errorf("missing required key %q in vault-kms-credentials secret", key)
}
}
return nil
}🤖 Prompt for AI Agents |
||
| }, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Privileged: ptrBool(true), | ||
| }, | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func addKMSPluginVolumeAndMountToPodSpec(podSpec *corev1.PodSpec, containerName string) error { | ||
| containerIndex := -1 | ||
| for i, container := range podSpec.Containers { | ||
| if container.Name == containerName { | ||
| containerIndex = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if containerIndex < 0 { | ||
| return fmt.Errorf("container %s not found", containerName) | ||
| } | ||
|
|
||
| container := &podSpec.Containers[containerIndex] | ||
| container.VolumeMounts = append(container.VolumeMounts, | ||
| corev1.VolumeMount{ | ||
| Name: "kms-plugin-socket", | ||
| MountPath: "/var/run/kmsplugin", | ||
| }, | ||
| ) | ||
|
|
||
| foundVolumeMount := false | ||
| for _, volumeMount := range podSpec.Volumes { | ||
| if volumeMount.Name == "kms-plugin-socket" { | ||
| foundVolumeMount = true | ||
| } | ||
| } | ||
|
|
||
| if !foundVolumeMount { | ||
| directoryOrCreate := corev1.HostPathDirectoryOrCreate | ||
| podSpec.Volumes = append(podSpec.Volumes, | ||
| corev1.Volume{ | ||
| Name: "kms-plugin-socket", | ||
| VolumeSource: corev1.VolumeSource{ | ||
| HostPath: &corev1.HostPathVolumeSource{ | ||
| Path: "/var/run/kmsplugin", | ||
| Type: &directoryOrCreate, | ||
| }, | ||
| }, | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func ptrBool(b bool) *bool { | ||
| return &b | ||
| } | ||
|
|
||
| func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc { | ||
| return func(pod *corev1.Pod) error { | ||
| creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{}) | ||
| if err != nil { | ||
| klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err) | ||
| return nil | ||
| } | ||
| klog.Infof("kms is ENABLED") | ||
|
|
||
| pod.Spec.Containers = append(pod.Spec.Containers, corev1.Container{ | ||
| Name: "kms-plugin", | ||
| Image: "quay.io/bertinatto/vault:v1", | ||
| ImagePullPolicy: corev1.PullAlways, | ||
| Command: []string{"/bin/sh", "-c"}, | ||
| Args: []string{fmt.Sprintf(` | ||
| echo "%s" > /tmp/secret-id | ||
| exec /vault-kube-kms \ | ||
| -listen-address=unix:///var/run/kmsplugin/kms.sock \ | ||
| -vault-address=%s \ | ||
| -vault-namespace=%s \ | ||
| -transit-mount=transit \ | ||
| -transit-key=%s \ | ||
| -log-level=debug-extended \ | ||
| -approle-role-id=%s \ | ||
| -approle-secret-id-path=/tmp/secret-id`, | ||
| string(creds.Data["VAULT_SECRET_ID"]), | ||
| string(creds.Data["VAULT_ADDR"]), | ||
| string(creds.Data["VAULT_NAMESPACE"]), | ||
| string(creds.Data["VAULT_KEY_NAME"]), | ||
| string(creds.Data["VAULT_ROLE_ID"])), | ||
| }, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Privileged: ptrBool(true), | ||
| }, | ||
| }) | ||
|
|
||
| return nil | ||
|
Comment on lines
+144
to
+168
There was a problem hiding this comment. Pod mutation path is incomplete and can produce a non-functional KMS setup.
♻️ Proposed consolidation/fix-func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions) installerpod.PodMutationFunc {
+func AddKMSPluginToPodSpecFn(o *installerpod.InstallOptions, kmsPluginImage string) installerpod.PodMutationFunc {
return func(pod *corev1.Pod) error {
+ if pod == nil {
+ return fmt.Errorf("pod cannot be nil")
+ }
creds, err := o.KubeClient.CoreV1().Secrets("openshift-kube-apiserver").Get(context.TODO(), "vault-kms-credentials", v1.GetOptions{})
if err != nil {
klog.Infof("kms is disabled: could not get vault-kms-credentials secret: %v", err)
return nil
}
- klog.Infof("kms is ENABLED")
-
- pod.Spec.Containers = append(pod.Spec.Containers, corev1.Container{
- Name: "kms-plugin",
- Image: "quay.io/bertinatto/vault:v1",
- ImagePullPolicy: corev1.PullAlways,
- Command: []string{"/bin/sh", "-c"},
- Args: []string{...},
- SecurityContext: &corev1.SecurityContext{
- Privileged: ptrBool(true),
- },
- })
+ if err := addKMSPluginSidecarToPodSpec(&pod.Spec, "kms-plugin", kmsPluginImage, creds); err != nil {
+ return err
+ }
+ if err := addKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kms-plugin"); err != nil {
+ return err
+ }
+ if err := addKMSPluginVolumeAndMountToPodSpec(&pod.Spec, "kube-apiserver"); err != nil {
+ return err
+ }
return nil
}
}🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 1363
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 3423
🏁 Script executed:
Repository: openshift/cluster-kube-apiserver-operator
Length of output: 414
Remove unused
ptrBoolfunction and fix installer wiring.The installer command wiring at lines 52-53 is correct—
AddKMSPluginToPodSpecFnis properly defined in the targetconfigcontroller package with the correct signature and integrates the KMS sidecar injection pathway as expected.However, the
ptrBoolfunction at lines 73-75 is dead code and should be removed. It is not called anywhere in this file. (Note: a separateptrBoolfunction exists inkms_sidecar.goand is used there, but the one inmain.goserves no purpose.)🤖 Prompt for AI Agents