CNTRLPLANE-2241: kms: add helper for TechPreview V1 #2090

bertinatto · 2026-01-26T19:44:45Z

/assign @ardaguclu @benluddy @p0lyn0mial

Proof PR: openshift/cluster-kube-apiserver-operator#2015

bertinatto · 2026-01-26T19:54:52Z

/hold
For openshift/api#2669

pkg/operator/encryption/kms/helpers.go

ardaguclu · 2026-01-27T05:57:59Z

/lgtm
This looks good to me. I also opened #2091 for the new FG.

p0lyn0mial · 2026-01-27T10:32:02Z

pkg/operator/encryption/kms/helpers.go

+		if container.Name != containerName {
+			continue
+		}
+		pod.Spec.Containers[i].VolumeMounts = append(container.VolumeMounts,


should we check if the mount doesn't already exist ? :)

otherwise we might add it multiple times.
also worth adding a test case for this ^

Good point. This would be a problem if:

This function is called multiple times per sync;

Another routine adds a mount with the same name;

In both cases, I believe this function should fail rather than preventing the addition of the duplicate, and that's the currently behavior. Do you think it fail silently instead?

Alternatively, I could add a check with a different error, but I'm not sure it's worth it.

No strong opinions, though. I'm happy to change if you disagree.

BTW, in pkg/operator/v1helpers/helpers.go we do the same thing

Do you think it fail silently instead?

My idea was to simply check whether the volume/mount exists before adding it to the list.
I understand that the assumption is that this function will receive a pod that does not contain the volume/mounts.

If we don’t want to add these checks, would it make sense to at least document this assumption in the function’s docs?

Makes sense, added a doc

pkg/operator/encryption/kms/helpers.go

p0lyn0mial · 2026-01-27T10:34:48Z

pkg/operator/encryption/kms/helpers.go

+	}
+
+	directoryOrCreate := corev1.HostPathDirectoryOrCreate
+	pod.Spec.Volumes = append(pod.Spec.Volumes,


should we check if the volume doesn't already exist ? :)

otherwise we might add it multiple times.
also worth adding a test case for this ^

See my previous comment

pkg/operator/encryption/kms/helpers.go

p0lyn0mial · 2026-01-27T10:38:40Z

pkg/operator/encryption/kms/helpers.go

+// FIXME: this is a temporary solution to get KMS TP v1 out. We should come up with a different approach afterwards.
+func AddKMSPluginVolumeToPod(pod *corev1.Pod, containerName string, featureGateAccessor featuregates.FeatureGateAccess) error {
+	if !featureGateAccessor.AreInitialFeatureGatesObserved() {
+		return nil


i think this is what we usually return when FG hasn't been observed, right ?

Right, in that case it's not possible to make a decision

pkg/operator/encryption/kms/helpers.go

p0lyn0mial · 2026-01-27T10:49:24Z

pkg/operator/encryption/kms/helpers.go

+)
+
+// AddKMSPluginVolumeToPod conditionally adds the KMS plugin volume mount to the kube-apiserver container.
+// FIXME: this is a temporary solution to get KMS TP v1 out. We should come up with a different approach afterwards.


maybe we should also already Deprecate it :) ?

That makes sense :)

Changed it, but deprecated usually means "don't use this, use X instead", so it implies that an alternative exists

pkg/operator/encryption/kms/helpers.go

bertinatto · 2026-01-27T17:08:32Z

From https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-cluster-kube-apiserver-operator-2015-nightly-4.22-e2e-vsphere-ovn-techpreview/2016149344322850816:

$ omc get featuregate cluster -o jsonpath='{.status.featureGates[].enabled[?(@.name=="KMSEncryption")]}'
{"name":"KMSEncryption"}

$ omc -n openshift-kube-apiserver get pod/kube-apiserver-ci-op-5npgdx95-b7629-bbrjx-master-0 -o json | jq .spec.containers[0].volumeMounts[5]
{
  "mountPath": "/var/run/kmsplugin",
  "name": "kms-plugin-socket"
}

$ omc -n openshift-kube-apiserver get pod/kube-apiserver-ci-op-5npgdx95-b7629-bbrjx-master-0 -o json | jq .spec.volumes[5]
{
  "hostPath": {
    "path": "/var/run/kmsplugin",
    "type": "DirectoryOrCreate"
  },
  "name": "kms-plugin-socket"
}

/hold cancel

ardaguclu · 2026-01-28T08:24:19Z

This looks good to me. But I'll defer the tag to @p0lyn0mial

p0lyn0mial · 2026-01-28T15:37:17Z

pkg/operator/encryption/kms/helpers_test.go

+)
+
+func TestAddKMSPluginVolume(t *testing.T) {
+	tests := []struct {


This is a personal preference.

I really like it when test definitions include inputs and expected outputs. It allows me to review test cases without looking at the logic.

If you feel the same way, we could slightly update the unit test to include actualPod and expectedPod (with the expected volumes).

Ack; included actualPod and expectedPod

openshift-ci · 2026-01-28T18:50:13Z

@bertinatto: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

p0lyn0mial · 2026-01-29T08:24:05Z

/lgtm

openshift-ci · 2026-01-29T08:25:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, bertinatto, p0lyn0mial

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:

~~pkg/operator/encryption/OWNERS~~ [bertinatto,p0lyn0mial]

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

openshift-ci-robot · 2026-01-29T08:25:09Z

@bertinatto: This pull request references CNTRLPLANE-2241 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

/assign @ardaguclu @benluddy @p0lyn0mial

Proof PR: openshift/cluster-kube-apiserver-operator#2015

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.

openshift-ci bot assigned ardaguclu, benluddy and p0lyn0mial Jan 26, 2026

openshift-ci bot requested review from dgrisonnet and tkashem January 26, 2026 19:45

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2026

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2026

benluddy reviewed Jan 26, 2026

View reviewed changes

pkg/operator/encryption/kms/helpers.go Outdated Show resolved Hide resolved

bertinatto force-pushed the kms branch from 13c5a95 to 389d6e5 Compare January 26, 2026 20:03

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2026

p0lyn0mial reviewed Jan 27, 2026

View reviewed changes

bertinatto force-pushed the kms branch from 389d6e5 to 75dc4e0 Compare January 27, 2026 13:17

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2026

bertinatto force-pushed the kms branch from 75dc4e0 to 21ff953 Compare January 27, 2026 13:51

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2026

bertinatto force-pushed the kms branch from 21ff953 to 4b21a55 Compare January 27, 2026 18:54

p0lyn0mial reviewed Jan 28, 2026

View reviewed changes

bertinatto force-pushed the kms branch from 4b21a55 to 9af43be Compare January 28, 2026 17:11

kms: add helper for TechPreview V1

f24e67d

bertinatto force-pushed the kms branch from 9af43be to f24e67d Compare January 28, 2026 17:43

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2026

p0lyn0mial changed the title ~~kms: add helper for TechPreview V1~~ CNTRLPLANE-2241: kms: add helper for TechPreview V1 Jan 29, 2026

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 29, 2026

openshift-merge-bot bot merged commit 9382c11 into openshift:master Jan 29, 2026
5 checks passed

CNTRLPLANE-2241: kms: add helper for TechPreview V1 #2090

CNTRLPLANE-2241: kms: add helper for TechPreview V1 #2090

Uh oh!

Conversation

bertinatto commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bertinatto commented Jan 26, 2026

Uh oh!

Uh oh!

ardaguclu commented Jan 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bertinatto commented Jan 27, 2026

Uh oh!

ardaguclu commented Jan 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jan 28, 2026

Uh oh!

p0lyn0mial commented Jan 29, 2026

Uh oh!

openshift-ci bot commented Jan 29, 2026

Uh oh!

openshift-ci-robot commented Jan 29, 2026 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bertinatto commented Jan 26, 2026 •

edited

Loading

openshift-ci-robot commented Jan 29, 2026 •

edited by openshift-ci bot

Loading