Skip to content

CNTRLPLANE-3237:: pkg/operator/encryption/kms: cleanup #2221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
p0lyn0mial wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CNTRLPLANE-3237:: pkg/operator/encryption/kms: cleanup #2221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions pkg/operator/encryption/encryptiondata/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"sort"
"strconv"
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
Expand All @@ -13,16 +14,35 @@ import (
configv1 "github.com/openshift/api/config/v1"

"github.com/openshift/library-go/pkg/operator/encryption/encoding"
"github.com/openshift/library-go/pkg/operator/encryption/kms"
"github.com/openshift/library-go/pkg/operator/encryption/state"
)

const pluginConfigDataKeyPrefix = "kms-plugin-config-"

// EncryptionConfSecretName is the name of the final encryption config secret that is revisioned per apiserver rollout.
const EncryptionConfSecretName = "encryption-config"

// EncryptionConfSecretKey is the map data key used to store the raw bytes of the final encryption config.
const EncryptionConfSecretKey = "encryption-config"

func toPluginConfigSecretDataKeyFor(keyID string) (string, error) {
if _, err := strconv.ParseUint(keyID, 10, 64); err != nil {
return "", fmt.Errorf("invalid keyID %q: must be a non-negative integer", keyID)
}
return pluginConfigDataKeyPrefix + keyID, nil
}

func keyIDFromPluginConfigSecretDataKey(dataKey string) (string, bool, error) {
keyID, found := strings.CutPrefix(dataKey, pluginConfigDataKeyPrefix)
if !found || len(keyID) == 0 {
return "", false, nil
}
if _, err := strconv.ParseUint(keyID, 10, 64); err != nil {
return "", false, fmt.Errorf("invalid keyID %q: must be a non-negative integer", keyID)
}
return keyID, true, nil
}

func FromSecret(encryptionConfigSecret *corev1.Secret) (*Config, error) {
data, ok := encryptionConfigSecret.Data[EncryptionConfSecretKey]
if !ok {
Expand All @@ -36,7 +56,7 @@ func FromSecret(encryptionConfigSecret *corev1.Secret) (*Config, error) {
for key, value := range encryptionConfigSecret.Data {
// Not all data keys are plugin configs — the Secret also contains the
// encryption-config entry, so skip keys that don't match the pattern.
keyID, found, err := kms.KeyIDFromPluginConfigSecretDataKey(key)
keyID, found, err := keyIDFromPluginConfigSecretDataKey(key)
if err != nil {
return nil, fmt.Errorf("failed to extract keyID from data key %s: %w", key, err)
}
Expand Down Expand Up @@ -93,7 +113,7 @@ func ToSecret(ns, name string, secretData *Config) (*corev1.Secret, error) {
if err != nil {
return nil, fmt.Errorf("failed to encode KMS plugin config for key %s: %w", keyID, err)
}
dataKey, err := kms.ToPluginConfigSecretDataKeyFor(keyID)
dataKey, err := toPluginConfigSecretDataKeyFor(keyID)
if err != nil {
return nil, err
}
Expand Down
124 changes: 124 additions & 0 deletions pkg/operator/encryption/encryptiondata/secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,136 @@ import (
"time"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apiserverconfigv1 "k8s.io/apiserver/pkg/apis/apiserver/v1"

configv1 "github.com/openshift/api/config/v1"

"github.com/openshift/library-go/pkg/operator/encryption/encryptiondata"
)

func TestFromSecretPluginDataKeyHandling(t *testing.T) {
baseSecret := func(t *testing.T) map[string][]byte {
t.Helper()
cfg := &encryptiondata.Config{
Encryption: &apiserverconfigv1.EncryptionConfiguration{
Resources: []apiserverconfigv1.ResourceConfiguration{{
Resources: []string{"secrets"},
}},
},
}
secret, err := encryptiondata.ToSecret("ns", "name", cfg)
if err != nil {
t.Fatalf("failed to create base secret: %v", err)
}
return secret.Data
}

tests := []struct {
name string
extraKeys map[string][]byte
wantError bool
}{
{
name: "no plugin config keys",
},
{
name: "non-integer suffix is rejected",
extraKeys: map[string][]byte{"kms-plugin-config-abc": {}},
wantError: true,
},
{
name: "negative suffix is rejected",
extraKeys: map[string][]byte{"kms-plugin-config--1": {}},
wantError: true,
},
{
name: "empty suffix is ignored",
extraKeys: map[string][]byte{"kms-plugin-config-": {}},
},
{
name: "unrelated keys are ignored",
extraKeys: map[string][]byte{"some-other-key": {}},
},
{
name: "empty string key is ignored",
extraKeys: map[string][]byte{"": {}},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
data := baseSecret(t)
for k, v := range tt.extraKeys {
data[k] = v
}
Copy link
Copy Markdown
Member

@bertinatto bertinatto May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maps.Copy(data, tt.extraKeys)

secret := &corev1.Secret{Data: data}
_, err := encryptiondata.FromSecret(secret)
if tt.wantError && err == nil {
Copy link
Copy Markdown
Member

@bertinatto bertinatto May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we unify whether we use github.com/stretchr/testify/require in library-go unit tests? Right now it's a mix of require and manual if/t.Fatal checks

t.Fatal("expected error but got nil")
}
if !tt.wantError && err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
}
}

func TestToSecretPluginKeyIDValidation(t *testing.T) {
tests := []struct {
name string
keyID string
wantError bool
}{
{
name: "valid keyID",
keyID: "1",
},
{
name: "valid large keyID",
keyID: "42",
},
{
name: "non-integer keyID",
keyID: "abc",
wantError: true,
},
{
name: "empty keyID",
keyID: "",
wantError: true,
},
{
name: "negative keyID",
keyID: "-1",
wantError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg := &encryptiondata.Config{
Encryption: &apiserverconfigv1.EncryptionConfiguration{
Resources: []apiserverconfigv1.ResourceConfiguration{{
Resources: []string{"secrets"},
}},
},
KMSPlugins: map[string]configv1.KMSPluginConfig{
tt.keyID: {},
},
}
_, err := encryptiondata.ToSecret("ns", "name", cfg)
Copy link
Copy Markdown
Member

@bertinatto bertinatto May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the idea of this new test is to test toPluginConfigSecretDataKeyFor via ToSecret. In general I think it's better to unit test the function directly because then you can check exactly what it produces for a given input. For example, the old test verified that for a given key 42, the function would produce a data key kms-plugin-config-42, but now it only checks that the function didn't return any errors.

to test toPluginConfigSecretDataKeyFor we'd need to change the package directive in this file to encryptiondata though (it's currently encryptiondata_test)

Copy link
Copy Markdown
Contributor Author

@p0lyn0mial p0lyn0mial May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we could create a new test file which would be in the same pkg as the production code to test the priv methods. then we could just move the prev tests. wdty ?

if tt.wantError && err == nil {
t.Fatalf("expected error for keyID %q", tt.keyID)
}
if !tt.wantError && err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
}
}

func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) {
timeout := &metav1.Duration{Duration: 10 * time.Second}

Expand Down
26 changes: 0 additions & 26 deletions pkg/operator/encryption/kms/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,12 @@ package kms

import (
"fmt"
"strconv"
"strings"

"github.com/openshift/api/features"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
corev1 "k8s.io/api/core/v1"
)

const pluginConfigDataKeyPrefix = "kms-plugin-config-"

// ToPluginConfigSecretDataKeyFor constructs the data key for storing a KMS plugin config in the encryption-config Secret.
// The keyID must be a valid non-negative integer string.
func ToPluginConfigSecretDataKeyFor(keyID string) (string, error) {
if _, err := strconv.ParseUint(keyID, 10, 64); err != nil {
return "", fmt.Errorf("invalid keyID %q: must be a non-negative integer", keyID)
}
return pluginConfigDataKeyPrefix + keyID, nil
}

// KeyIDFromPluginConfigSecretDataKey extracts the keyID from a kms-plugin-config data key.
// Returns the keyID and true if the key matches the "kms-plugin-config-<keyID>" pattern.
func KeyIDFromPluginConfigSecretDataKey(dataKey string) (string, bool, error) {
keyID, found := strings.CutPrefix(dataKey, pluginConfigDataKeyPrefix)
if !found || len(keyID) == 0 {
return "", false, nil
}
if _, err := strconv.ParseUint(keyID, 10, 64); err != nil {
return "", false, fmt.Errorf("invalid keyID %q: must be a non-negative integer", keyID)
}
return keyID, true, nil
}

// AddKMSPluginVolumeAndMountToPodSpec 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.
Expand Down
102 changes: 0 additions & 102 deletions pkg/operator/encryption/kms/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,108 +12,6 @@ import (
"github.com/stretchr/testify/require"
)

func TestKeyIDFromPluginConfigSecretDataKey(t *testing.T) {
tests := []struct {
name string
dataKey string
wantKeyID string
wantFound bool
wantError bool
}{
{
name: "valid key",
dataKey: "kms-plugin-config-1",
wantKeyID: "1",
wantFound: true,
},
{
name: "valid key with large ID",
dataKey: "kms-plugin-config-42",
wantKeyID: "42",
wantFound: true,
},
{
name: "encryption-config key",
dataKey: "encryption-config",
},
{
name: "non-integer keyID",
dataKey: "kms-plugin-config-abc",
wantError: true,
},
{
name: "missing keyID",
dataKey: "kms-plugin-config-",
},
{
name: "empty string",
dataKey: "",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
keyID, found, err := KeyIDFromPluginConfigSecretDataKey(tt.dataKey)
if tt.wantError {
require.Error(t, err)
return
}
require.NoError(t, err)
require.Equal(t, tt.wantFound, found)
if found {
require.Equal(t, tt.wantKeyID, keyID)
}
})
}
}

func TestToPluginConfigSecretDataKeyFor(t *testing.T) {
tests := []struct {
name string
keyID string
wantKey string
wantError bool
}{
{
name: "valid keyID",
keyID: "1",
wantKey: "kms-plugin-config-1",
},
{
name: "valid large keyID",
keyID: "42",
wantKey: "kms-plugin-config-42",
},
{
name: "non-integer keyID",
keyID: "abc",
wantError: true,
},
{
name: "empty keyID",
keyID: "",
wantError: true,
},
{
name: "negative keyID",
keyID: "-1",
wantError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := ToPluginConfigSecretDataKeyFor(tt.keyID)
if tt.wantError {
require.Error(t, err)
return
}
require.NoError(t, err)
require.Equal(t, tt.wantKey, got)
})
}
}

func TestAddKMSPluginVolume(t *testing.T) {
directoryOrCreate := corev1.HostPathDirectoryOrCreate

Expand Down
7 changes: 2 additions & 5 deletions pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/library-go/pkg/operator/encryption/encoding"
"github.com/openshift/library-go/pkg/operator/encryption/kms"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -45,8 +44,7 @@ func TestInjectIntoPodSpec(t *testing.T) {
pluginConfigBytes, err := encoding.EncodeKMSPluginConfig(*vaultConfig)
require.NoError(t, err)

pluginConfigKey, err := kms.ToPluginConfigSecretDataKeyFor("555")
require.NoError(t, err)
pluginConfigKey := "kms-plugin-config-555"

encryptionConfig := &apiserverv1.EncryptionConfiguration{
Resources: []apiserverv1.ResourceConfiguration{
Expand Down Expand Up @@ -195,8 +193,7 @@ func TestInjectIntoPodSpec(t *testing.T) {
pluginConfig2Bytes, err := encoding.EncodeKMSPluginConfig(*vaultConfig2)
require.NoError(t, err)

pluginConfigKey2, err := kms.ToPluginConfigSecretDataKeyFor("777")
require.NoError(t, err)
pluginConfigKey2 := "kms-plugin-config-777"

multiEncConfig := &apiserverv1.EncryptionConfiguration{
Resources: []apiserverv1.ResourceConfiguration{
Expand Down