Skip to content

CNTRLPLANE-3237: Introduce kms to kms migration #2192

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
ardaguclu wants to merge 1 commit into
base: master
Choose a base branch
from
+382 −23
Open

CNTRLPLANE-3237: Introduce kms to kms migration #2192

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
84 changes: 69 additions & 15 deletions pkg/operator/encryption/controllers/key_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,18 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact
// fills up the state with all resources and set identity write key if write key secrets
// are missing.

var providerCfg kmsProviderConfig = noopKMSProviderConfig{}
if currentMode == state.KMS {
var err error
providerCfg, err = newKMSProviderConfig(apiEncryptionConfiguration.KMS)
if err != nil {
return err
}
}

var commonReason *string
for gr, grKeys := range desiredEncryptionState {
latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs)
latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs, providerCfg)
if !needed {
continue
}
Expand All @@ -228,7 +237,7 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact

sort.Sort(sort.StringSlice(reasons))
internalReason := strings.Join(reasons, ", ")
keySecret, err := c.generateKeySecret(ctx, newKeyID, currentMode, apiEncryptionConfiguration, internalReason, externalReason)
keySecret, err := c.generateKeySecret(ctx, newKeyID, currentMode, apiEncryptionConfiguration, providerCfg, internalReason, externalReason)
if err != nil {
return fmt.Errorf("failed to create key: %v", err)
}
Expand Down Expand Up @@ -265,7 +274,7 @@ func (c *keyController) validateExistingSecret(ctx context.Context, keySecret *c
return nil // we made this key earlier
}

func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, internalReason, externalReason string) (*corev1.Secret, error) {
func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, providerCfg kmsProviderConfig, internalReason, externalReason string) (*corev1.Secret, error) {
bs := crypto.ModeToNewKeyFunc[currentMode]()
ks := state.KeyState{
Key: apiserverv1.Key{
Expand All @@ -287,11 +296,6 @@ func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, cur
Plugin: apiServerEncryption.KMS,
}

providerCfg, err := newKMSProviderConfig(apiServerEncryption.KMS)
if err != nil {
return nil, err
}

if secretName, expectedKeys, err := providerCfg.referencedSecretName(); err != nil {
return nil, err
} else if len(secretName) > 0 {
Expand Down Expand Up @@ -363,7 +367,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, providerCfg kmsProviderConfig) (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
Expand Down Expand Up @@ -408,13 +412,25 @@ 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 latestKey.KMS == nil {
// A KMS-mode key without KMS state indicates a corrupted key secret.
// Do not create a new key on corrupted data.
klog.Errorf("KMS-mode key %q has nil KMS state, possibly corrupted key secret; skipping new key creation", latestKey.Key.Name)
return 0, "", false
}
migrationNeeded, err := providerCfg.migrationRequired(latestKey.KMS.Plugin)
if err != nil {
klog.Errorf("Failed to check KMS migration: %v", err)
return 0, "", false
}
if migrationNeeded {
return latestKeyID, "kms-provider-changed", true

@p0lyn0mial p0lyn0mial May 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how could we e2e test this change ? :)

@ardaguclu ardaguclu May 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We'll update kms plugin deployment in library-go with the fake Vault kms plugin. We'll have actual Vault kms plugin coming from stepregistry. We'll move from one to another :)

@ardaguclu ardaguclu May 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, there is simpler way: We have already run multiple kms plugins (km-1.sock, kms-2.sock, etc.). We'll just migrate from one to another.

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 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
}
Expand Down Expand Up @@ -442,7 +458,21 @@ type kmsProviderConfig interface {
// config and the specific data keys to carry from that configmap. Only the listed keys
// are copied into the Key Secret; any other data in the referenced configmap is ignored.
referencedConfigMapName() (string, []string, error)
// migrationRequired reports whether switching from latest (stored in
// the key secret) to this provider config requires a new encryption key.
migrationRequired(latest configv1.KMSPluginConfig) (bool, error)
}

// noopKMSProviderConfig is a safe zero-value implementation used for non-KMS modes.
// All methods return empty/false so callers never need nil checks.
type noopKMSProviderConfig struct{}

func (noopKMSProviderConfig) referencedSecretName() (string, []string, error) { return "", nil, nil }
func (noopKMSProviderConfig) referencedConfigMapName() (string, []string, error) { return "", nil, nil }
func (noopKMSProviderConfig) migrationRequired(configv1.KMSPluginConfig) (bool, error) {
return false, fmt.Errorf("migrationRequired called on non-KMS provider")
}
func (noopKMSProviderConfig) sourceConfig() interface{} { return nil }

func newKMSProviderConfig(plugin configv1.KMSPluginConfig) (kmsProviderConfig, error) {
switch plugin.Type {
Expand Down Expand Up @@ -479,6 +509,30 @@ func (v *vaultProviderConfig) referencedConfigMapName() (string, []string, error
return v.vault.TLS.CABundle.Name, []string{"ca-bundle.crt"}, nil
}

@bertinatto bertinatto Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in newKMSProviderConfig don't we want to support anything other than Vault, like the upstream mock plugin for testing?

@ardaguclu ardaguclu Jun 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we would only want to extend this with the providers that we officially support. In that case currently is Vault.


func (v *vaultProviderConfig) migrationRequired(latest configv1.KMSPluginConfig) (bool, error) {
if latest.Type != configv1.VaultKMSProvider {
klog.V(2).Infof("KMS migration required: provider type changed from %q to %q", latest.Type, configv1.VaultKMSProvider)
return true, nil
}
if v.vault.VaultAddress != latest.Vault.VaultAddress {
klog.V(2).Infof("KMS migration required: VaultAddress changed from %q to %q", latest.Vault.VaultAddress, v.vault.VaultAddress)
return true, nil
}
if v.vault.VaultNamespace != latest.Vault.VaultNamespace {
klog.V(2).Infof("KMS migration required: VaultNamespace changed from %q to %q", latest.Vault.VaultNamespace, v.vault.VaultNamespace)
return true, nil
}
if v.vault.TransitMount != latest.Vault.TransitMount {
klog.V(2).Infof("KMS migration required: TransitMount changed from %q to %q", latest.Vault.TransitMount, v.vault.TransitMount)
return true, nil
}
if v.vault.TransitKey != latest.Vault.TransitKey {
klog.V(2).Infof("KMS migration required: TransitKey changed from %q to %q", latest.Vault.TransitKey, v.vault.TransitKey)
return true, nil
}
return false, nil
}

// TODO make this un-settable once set
// ex: we could require the tech preview no upgrade flag to be set before we will honor this field
type unsupportedEncryptionConfig struct {
Expand Down
Loading