CNTRLPLANE-3237: Introduce kms to kms migration

pkg/operator/encryption/controllers/key_controller.go

@@ -28,6 +28,7 @@ import (
 
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/encryption/crypto"
+	"github.com/openshift/library-go/pkg/operator/encryption/kms"
 	"github.com/openshift/library-go/pkg/operator/encryption/secrets"
 	"github.com/openshift/library-go/pkg/operator/encryption/state"
 	"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
@@ -196,7 +197,7 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact
 
 	var commonReason *string
 	for gr, grKeys := range desiredEncryptionState {
-		latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs)
+		latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs, apiEncryptionConfiguration)
 		if !needed {
 			continue
 		}
@@ -320,7 +321,7 @@ func (c *keyController) getCurrentModeReasonAndEncryptionConfig(ctx context.Cont
 
 // needsNewKey checks whether a new key must be created for the given resource. If true, it also returns the latest
 // used key ID and a reason string.
-func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource) (uint64, string, bool) {
+func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource, currentApiServerEncryption configv1.APIServerEncryption) (uint64, string, bool) {
 	// we always need to have some encryption keys unless we are turned off
 	if len(grKeys.ReadKeys) == 0 {
 		return 0, "key-does-not-exist", currentMode != state.Identity
@@ -365,13 +366,14 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern
 
 	if currentMode == state.KMS {
 		// We are here because Encryption Mode is not changed
+		// However, we need to create a new key if migration-triggering fields
+		// in the KMS provider configuration have changed.
+		if kms.NeedsNewKey(latestKey.KMSConfig.Provider, currentApiServerEncryption.KMS) {
+			return latestKeyID, "kms-provider-changed", true
+		}
 
-		// For now in Tech Preview v1, we don't support configurational changes. Therefore,
-		// it is pointless comparing the secrets.
-
-		// For KMS mode, we don't do time-based rotation. Therefore, we shortcut here
-		// KMS keys are rotated externally by the KMS system.
-		// Moreover, we don't trigger new key when external reason is changed.
+		// For KMS mode, we don't do time-based rotation. KMS keys are rotated
+		// externally by the KMS provider. Moreover, we don't trigger new key when external reason is changed.
 		// Because it would lead to duplicate providers which is not allowed.
 		return 0, "", false
 	}

pkg/operator/encryption/controllers/key_controller_test.go

@@ -631,6 +631,160 @@ func TestKeyController(t *testing.T) {
 				}
 			},
 		},
+
+		{
+			name: "creates a new KMS key when VaultAddress changes",
+			targetGRs: []schema.GroupResource{
+				{Group: "", Resource: "secrets"},
+			},
+			initialObjects: []runtime.Object{
+				encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
+				encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
+			},
+			apiServerObjects: []runtime.Object{func() runtime.Object {
+				s := simpleAPIServer.DeepCopy()
+				changedConfig := encryptiontesting.DefaultKMSProviderConfig.DeepCopy()
+				changedConfig.Vault.VaultAddress = "https://vault-new.example.com"
+				s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: changedConfig}
+				return s
+			}()},
+			targetNamespace: "kms",
+			expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "create:secrets:openshift-config-managed", "create:events:kms"},
+			validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
+				for _, action := range actions {
+					if action.Matches("create", "secrets") {
+						createAction := action.(clientgotesting.CreateAction)
+						actualSecret := createAction.GetObject().(*corev1.Secret)
+
+						if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"] != "KMS" {
+							ts.Errorf("expected mode KMS, got %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"])
+						}
+						if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" {
+							ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"])
+						}
+						if actualSecret.Name != "encryption-key-kms-6" {
+							ts.Errorf("expected key ID 6, got %s", actualSecret.Name)
+						}
+
+						kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"]
+						providerConfig, err := encoding.DecodeKMSConfig(kmsProviderConfigData)
+						if err != nil {
+							ts.Fatalf("failed to encode KMS config: %v", err)
+						}
+						if providerConfig.Vault.VaultAddress != "https://vault-new.example.com" {
+							ts.Errorf("expected new VaultAddress, got %s", providerConfig.Vault.VaultAddress)
+						}
+						return
+					}
+				}
+				ts.Errorf("the secret wasn't created")
+			},
+		},
+
+		{
+			name: "creates a new KMS key when TransitKey changes",
+			targetGRs: []schema.GroupResource{
+				{Group: "", Resource: "secrets"},
+			},
+			initialObjects: []runtime.Object{
+				encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
+				encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
+			},
+			apiServerObjects: []runtime.Object{func() runtime.Object {
+				s := simpleAPIServer.DeepCopy()
+				changedConfig := encryptiontesting.DefaultKMSProviderConfig.DeepCopy()
+				changedConfig.Vault.TransitKey = "new-transit-key"
+				s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: changedConfig}
+				return s
+			}()},
+			targetNamespace: "kms",
+			expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "create:secrets:openshift-config-managed", "create:events:kms"},
+			validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
+				for _, action := range actions {
+					if action.Matches("create", "secrets") {
+						createAction := action.(clientgotesting.CreateAction)
+						actualSecret := createAction.GetObject().(*corev1.Secret)
+
+						if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" {
+							ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"])
+						}
+
+						kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-provider-config"]
+						providerConfig, err := encoding.DecodeKMSConfig(kmsProviderConfigData)
+						if err != nil {
+							ts.Fatalf("failed to encode KMS config: %v", err)
+						}
+						if providerConfig.Vault.TransitKey != "new-transit-key" {
+							ts.Errorf("expected new TransitKey, got %s", providerConfig.Vault.TransitKey)
+						}
+						return
+					}
+				}
+				ts.Errorf("the secret wasn't created")
+			},
+		},
+
+		{
+			name: "no-op when only KMSPluginImage changes (non-migration field)",
+			targetGRs: []schema.GroupResource{
+				{Group: "", Resource: "secrets"},
+			},
+			initialObjects: []runtime.Object{
+				encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
+				encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
+			},
+			apiServerObjects: []runtime.Object{func() runtime.Object {
+				s := simpleAPIServer.DeepCopy()
+				changedConfig := encryptiontesting.DefaultKMSProviderConfig.DeepCopy()
+				changedConfig.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000"
+				s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: changedConfig}
+				return s
+			}()},
+			targetNamespace: "kms",
+			expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
+		},
+
+		{
+			name: "no-op when only Authentication changes (non-migration field)",
+			targetGRs: []schema.GroupResource{
+				{Group: "", Resource: "secrets"},
+			},
+			initialObjects: []runtime.Object{
+				encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
+				encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
+			},
+			apiServerObjects: []runtime.Object{func() runtime.Object {
+				s := simpleAPIServer.DeepCopy()
+				changedConfig := encryptiontesting.DefaultKMSProviderConfig.DeepCopy()
+				changedConfig.Vault.Authentication.AppRole.Secret.Name = "new-approle-secret"
+				s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: changedConfig}
+				return s
+			}()},
+			targetNamespace: "kms",
+			expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
+		},
+
+		{
+			name: "no-op when only TLS changes (non-migration field)",
+			targetGRs: []schema.GroupResource{
+				{Group: "", Resource: "secrets"},
+			},
+			initialObjects: []runtime.Object{
+				encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
+				encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
+			},
+			apiServerObjects: []runtime.Object{func() runtime.Object {
+				s := simpleAPIServer.DeepCopy()
+				changedConfig := encryptiontesting.DefaultKMSProviderConfig.DeepCopy()
+				changedConfig.Vault.TLS = configv1.VaultTLSConfig{
+					CABundle: configv1.VaultConfigMapReference{Name: "my-ca"},
+				}
+				s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: changedConfig}
+				return s
+			}()},
+			targetNamespace: "kms",
+			expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
+		},
 	}
 
 	for _, scenario := range scenarios {

pkg/operator/encryption/kms/helpers.go

@@ -5,6 +5,7 @@ import (
 	"strconv"
 	"strings"
 
+	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/api/features"
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 	corev1 "k8s.io/api/core/v1"
@@ -34,6 +35,27 @@ func KeyIDFromProviderConfigSecretDataKey(dataKey string) (string, bool, error)
 	return keyID, true, nil
 }
 
+// NeedsNewKey returns true if the KMS provider configuration has changed
+// in a way that requires creating a new encryption key and migrating storage.
+// Only fields that affect the Key Encryption Key (KEK) trigger migration.
+// Fields like KMSPluginImage, TLS, and Authentication are non-migration fields.
+func NeedsNewKey(latest, current *configv1.KMSConfig) bool {
+	if latest.Type != current.Type {
+		// TODO: Integrate this with pre-flight checker
+		return true
+	}
+	if latest.Type == configv1.VaultKMSProvider {
+		if latest.Vault.VaultAddress != current.Vault.VaultAddress ||
+			latest.Vault.VaultNamespace != current.Vault.VaultNamespace ||
+			latest.Vault.TransitMount != current.Vault.TransitMount ||
+			latest.Vault.TransitKey != current.Vault.TransitKey {
+			// TODO: Integrate this with pre-flight checker
+			return true
+		}
+	}
+	return false
+}
+
 // 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.

pkg/operator/encryption/kms/helpers_test.go

@@ -114,6 +114,118 @@ func TestToProviderConfigSecretDataKeyFor(t *testing.T) {
 	}
 }
 
+func TestNeedsNewKey(t *testing.T) {
+	baseConfig := &configv1.KMSConfig{
+		Type: configv1.VaultKMSProvider,
+		Vault: configv1.VaultKMSConfig{
+			KMSPluginImage: "registry.example.com/kms-plugin@sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890",
+			VaultAddress:   "https://vault.example.com",
+			VaultNamespace: "ns1",
+			TransitMount:   "transit",
+			TransitKey:     "my-key",
+			Authentication: configv1.VaultAuthentication{
+				Type: configv1.VaultAuthenticationTypeAppRole,
+				AppRole: configv1.VaultAppRoleAuthentication{
+					Secret: configv1.VaultSecretReference{Name: "vault-approle-secret"},
+				},
+			},
+		},
+	}
+
+	tests := []struct {
+		name     string
+		latest   *configv1.KMSConfig
+		current  *configv1.KMSConfig
+		expected bool
+	}{
+		{
+			name:     "identical configs",
+			latest:   baseConfig.DeepCopy(),
+			current:  baseConfig.DeepCopy(),
+			expected: false,
+		},
+		{
+			name:   "different VaultAddress",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.VaultAddress = "https://vault-new.example.com"
+				return c
+			}(),
+			expected: true,
+		},
+		{
+			name:   "different VaultNamespace",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.VaultNamespace = "ns2"
+				return c
+			}(),
+			expected: true,
+		},
+		{
+			name:   "different TransitKey",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.TransitKey = "new-key"
+				return c
+			}(),
+			expected: true,
+		},
+		{
+			name:   "different TransitMount",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.TransitMount = "custom-transit"
+				return c
+			}(),
+			expected: true,
+		},
+		{
+			name:   "different KMSPluginImage only",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000"
+				return c
+			}(),
+			expected: false,
+		},
+		{
+			name:   "different TLS only",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.TLS = configv1.VaultTLSConfig{
+					CABundle: configv1.VaultConfigMapReference{Name: "my-ca"},
+				}
+				return c
+			}(),
+			expected: false,
+		},
+		{
+			name:   "different Authentication only",
+			latest: baseConfig.DeepCopy(),
+			current: func() *configv1.KMSConfig {
+				c := baseConfig.DeepCopy()
+				c.Vault.Authentication.AppRole.Secret.Name = "new-secret"
+				return c
+			}(),
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := NeedsNewKey(tt.latest, tt.current)
+			require.Equal(t, tt.expected, got)
+		})
+	}
+}
+
 func TestAddKMSPluginVolume(t *testing.T) {
 	directoryOrCreate := corev1.HostPathDirectoryOrCreate