From b6df00e865c8bc05ee72f003fccac54562818957 Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Wed, 13 May 2026 16:47:53 +0200 Subject: [PATCH] pkg/operator/encryption/kms: cleanup --- .../encryption/encryptiondata/secret.go | 26 +++- .../encryption/encryptiondata/secret_test.go | 124 ++++++++++++++++++ pkg/operator/encryption/kms/helpers.go | 26 ---- pkg/operator/encryption/kms/helpers_test.go | 102 -------------- .../kms/pluginlifecycle/sidecar_test.go | 7 +- 5 files changed, 149 insertions(+), 136 deletions(-) diff --git a/pkg/operator/encryption/encryptiondata/secret.go b/pkg/operator/encryption/encryptiondata/secret.go index 0df884c62c..92e815218b 100644 --- a/pkg/operator/encryption/encryptiondata/secret.go +++ b/pkg/operator/encryption/encryptiondata/secret.go @@ -4,6 +4,7 @@ import ( "fmt" "sort" "strconv" + "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -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 { @@ -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) } @@ -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 } diff --git a/pkg/operator/encryption/encryptiondata/secret_test.go b/pkg/operator/encryption/encryptiondata/secret_test.go index 856e6b5b04..39f8d9784c 100644 --- a/pkg/operator/encryption/encryptiondata/secret_test.go +++ b/pkg/operator/encryption/encryptiondata/secret_test.go @@ -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 + } + secret := &corev1.Secret{Data: data} + _, err := encryptiondata.FromSecret(secret) + if tt.wantError && err == nil { + 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) + 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} diff --git a/pkg/operator/encryption/kms/helpers.go b/pkg/operator/encryption/kms/helpers.go index 8a87f83e6b..c5031ce2ab 100644 --- a/pkg/operator/encryption/kms/helpers.go +++ b/pkg/operator/encryption/kms/helpers.go @@ -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-" 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. diff --git a/pkg/operator/encryption/kms/helpers_test.go b/pkg/operator/encryption/kms/helpers_test.go index 365d063b2b..626c93f375 100644 --- a/pkg/operator/encryption/kms/helpers_test.go +++ b/pkg/operator/encryption/kms/helpers_test.go @@ -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 diff --git a/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go b/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go index 8ae3df3beb..a092f1565c 100644 --- a/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go +++ b/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go @@ -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" @@ -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{ @@ -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{