From 73882635fd870325e00c52994419224f3e0b09b2 Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Thu, 21 May 2026 10:09:03 +0200 Subject: [PATCH 1/2] pkg/operator/encryption/kms: cleanup --- .../encryption/encryptiondata/secret.go | 30 ++++- .../encryption/encryptiondata/secret_test.go | 127 ++++++++++++++++-- pkg/operator/encryption/kms/helpers.go | 26 ---- pkg/operator/encryption/kms/helpers_test.go | 102 -------------- .../kms/pluginlifecycle/sidecar_test.go | 4 +- 5 files changed, 142 insertions(+), 147 deletions(-) diff --git a/pkg/operator/encryption/encryptiondata/secret.go b/pkg/operator/encryption/encryptiondata/secret.go index 0df884c62c..7e7b030dc1 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,10 +14,33 @@ 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-" + +// 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 +} + // EncryptionConfSecretName is the name of the final encryption config secret that is revisioned per apiserver rollout. const EncryptionConfSecretName = "encryption-config" @@ -36,7 +60,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 +117,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..cc8fdf6227 100644 --- a/pkg/operator/encryption/encryptiondata/secret_test.go +++ b/pkg/operator/encryption/encryptiondata/secret_test.go @@ -1,14 +1,13 @@ -package encryptiondata_test +package encryptiondata import ( "testing" "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiserverconfigv1 "k8s.io/apiserver/pkg/apis/apiserver/v1" - - "github.com/openshift/library-go/pkg/operator/encryption/encryptiondata" ) func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { @@ -16,7 +15,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { tests := []struct { name string - cfg *encryptiondata.Config + cfg *Config want []*apiserverconfigv1.KMSConfiguration wantError bool }{ @@ -27,12 +26,12 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "nil encryption returns error", - cfg: &encryptiondata.Config{}, + cfg: &Config{}, wantError: true, }, { name: "empty provider list returns empty slice", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{{ Resources: []string{"secrets"}, @@ -44,7 +43,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "single resource single KMS provider", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{{ Resources: []string{"secrets"}, @@ -68,7 +67,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "same keyID across resources is deduplicated", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{ { @@ -105,7 +104,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "multiple keyIDs sorted descending", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{{ Resources: []string{"secrets"}, @@ -146,7 +145,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "non-KMS providers are skipped", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{{ Resources: []string{"secrets"}, @@ -174,7 +173,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "mismatched duplicate keyID errors", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{ { @@ -206,7 +205,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }, { name: "invalid plugin name errors", - cfg: &encryptiondata.Config{ + cfg: &Config{ Encryption: &apiserverconfigv1.EncryptionConfiguration{ Resources: []apiserverconfigv1.ResourceConfiguration{{ Resources: []string{"secrets"}, @@ -226,7 +225,7 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := encryptiondata.ExtractUniqueAndSortedKMSConfigurations(tt.cfg) + got, err := ExtractUniqueAndSortedKMSConfigurations(tt.cfg) if tt.wantError { if err == nil { t.Fatal("expected error but got nil") @@ -242,3 +241,105 @@ func TestExtractUniqueAndSortedKMSConfigurations(t *testing.T) { }) } } + +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) + }) + } +} 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 08db74e449..2680f3b507 100644 --- a/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go +++ b/pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go @@ -9,7 +9,6 @@ import ( "github.com/openshift/api/features" "github.com/openshift/library-go/pkg/operator/configobserver/featuregates" "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" "k8s.io/apimachinery/pkg/api/resource" @@ -251,8 +250,7 @@ func TestAddKMSPluginSidecarToPodSpec(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{ From d617f820a9051aab0b0babcbb8a4c735ea8e7acf Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Thu, 21 May 2026 10:15:44 +0200 Subject: [PATCH 2/2] pkg/operator/encryption/kms: replace require calls with standard testing assertions --- .../encryption/encryptiondata/secret_test.go | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/pkg/operator/encryption/encryptiondata/secret_test.go b/pkg/operator/encryption/encryptiondata/secret_test.go index cc8fdf6227..efd2a84f21 100644 --- a/pkg/operator/encryption/encryptiondata/secret_test.go +++ b/pkg/operator/encryption/encryptiondata/secret_test.go @@ -5,7 +5,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiserverconfigv1 "k8s.io/apiserver/pkg/apis/apiserver/v1" ) @@ -285,13 +284,19 @@ func TestKeyIDFromPluginConfigSecretDataKey(t *testing.T) { t.Run(tt.name, func(t *testing.T) { keyID, found, err := keyIDFromPluginConfigSecretDataKey(tt.dataKey) if tt.wantError { - require.Error(t, err) + if err == nil { + t.Fatal("expected error but got nil") + } return } - require.NoError(t, err) - require.Equal(t, tt.wantFound, found) - if found { - require.Equal(t, tt.wantKeyID, keyID) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if found != tt.wantFound { + t.Fatalf("expected found=%v, got %v", tt.wantFound, found) + } + if found && keyID != tt.wantKeyID { + t.Fatalf("expected keyID=%q, got %q", tt.wantKeyID, keyID) } }) } @@ -335,11 +340,17 @@ func TestToPluginConfigSecretDataKeyFor(t *testing.T) { t.Run(tt.name, func(t *testing.T) { got, err := toPluginConfigSecretDataKeyFor(tt.keyID) if tt.wantError { - require.Error(t, err) + if err == nil { + t.Fatal("expected error but got nil") + } return } - require.NoError(t, err) - require.Equal(t, tt.wantKey, got) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.wantKey { + t.Fatalf("expected key=%q, got %q", tt.wantKey, got) + } }) } }