Skip to content

CNTRLPLANE-3237: Carry referenced Secret data from API to plugin lifecycle for KMS #2212

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

Merged
openshift-merge-bot merged 3 commits into from
May 22, 2026
Merged

CNTRLPLANE-3237: Carry referenced Secret data from API to plugin lifecycle for KMS #2212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4b1aca
Carry referenced Secret data from API to plugin lifecycle for KMS
ardaguclu May 11, 2026
1ffc8fb
Check AppRole authentication
ardaguclu May 21, 2026
daeed7b
Return if authentication type is unknown
ardaguclu May 21, 2026
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
43 changes: 41 additions & 2 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 @@ -42,6 +42,7 @@ const (
encryptionSecretMigrationInterval = time.Hour * 24 * 7 // one week
kmsEndpointFormat = "unix:///var/run/kmsplugin/kms-%d.sock"
defaultKMSTimeout = 10 * time.Second
openshiftConfigNS = "openshift-config"
)

// keyController creates new keys if necessary. It
Expand Down Expand Up @@ -117,6 +118,7 @@ func NewKeyController(
apiServerInformer.Informer(),
operatorClient.Informer(),
kubeInformersForNamespaces.InformersFor("openshift-config-managed").Core().V1().Secrets().Informer(),
// TODO: add informer for openshift-config namespace to watch referenced Secrets for KMS plugin secret data changes
Comment thread
ardaguclu marked this conversation as resolved.
deployer,
).ToController(
c.controllerInstanceName,
Expand Down Expand Up @@ -223,7 +225,7 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact

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

func (c *keyController) generateKeySecret(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, internalReason, externalReason string) (*corev1.Secret, error) {
bs := crypto.ModeToNewKeyFunc[currentMode]()
ks := state.KeyState{
Key: apiserverv1.Key{
Expand All @@ -281,6 +283,24 @@ func (c *keyController) generateKeySecret(keyID uint64, currentMode state.Mode,
},
Plugin: apiServerEncryption.KMS,
}

if secretName, expectedKeys, err := referencedSecretName(apiServerEncryption.KMS); err != nil {
return nil, err
} else if len(secretName) > 0 {
refSecret, err := c.secretClient.Secrets(openshiftConfigNS).Get(ctx, secretName, metav1.GetOptions{})
Comment thread
p0lyn0mial marked this conversation as resolved.
if err != nil {
return nil, fmt.Errorf("failed to get secret %s in %s: %w", secretName, openshiftConfigNS, err)
}
for _, key := range expectedKeys {
Comment thread
p0lyn0mial marked this conversation as resolved.
Copy link
Copy Markdown
Member

@bertinatto bertinatto May 21, 2026

Choose a reason for hiding this comment

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

this lgtm, just asking for my knowledge: I believe I mentioned about validating the secret before, but you mentioned the encryption controllers should not have semantic knowledge about the secret (which also makes sense). referencedSecretName now has that knowledge, and I'm fine with that, but just wondering if I missed any discussions about it

Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial May 21, 2026

Choose a reason for hiding this comment

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

I think we basically have two options:

Option 1 would be to restrict fields to only well-known ones essentially what we have today.
Option 2 would be to copy arbitrary fields.

Copying arbitrary fields seems too permissive. What’s the point of copying fields we won’t need?

The expected fields will have to be documented for users, just like we document them for, for example, identity providers.

Copy link
Copy Markdown
Member Author

@ardaguclu ardaguclu May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I agree about the reasons of working on selective set of data keys.

I think it is better to logically store provider specific details (i.e. Vault) in kms/vault.go. So that they won't be spread across all over the code base.

v, ok := refSecret.Data[key]
if !ok {
return nil, fmt.Errorf("secret %s in %s is missing required key %q", secretName, openshiftConfigNS, key)
}
if err := ks.KMS.PluginSecretData.Set(secretName, key, v); err != nil {
return nil, err
}
}
}
}
return secrets.FromKeyState(c.instanceName, ks)
}
Expand Down Expand Up @@ -383,6 +403,25 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern
return latestKeyID, "rotation-interval-has-passed", time.Since(latestKey.Migrated.Timestamp) > encryptionSecretMigrationInterval
}

// referencedSecretName returns the name of the secret referenced by the KMS plugin
// config and the specific data keys to carry from that secret. Only the listed keys
// are copied into the Key Secret; any other data in the referenced secret is ignored.
func referencedSecretName(plugin configv1.KMSPluginConfig) (string, []string, error) {
switch plugin.Type {
case configv1.VaultKMSProvider:
switch plugin.Vault.Authentication.Type {
case configv1.VaultAuthenticationTypeAppRole:
// The Vault AppRole secret must contain "role-id" and "secret-id" keys.
// These are the only keys carried into the encryption key secret.
return plugin.Vault.Authentication.AppRole.Secret.Name, []string{"role-id", "secret-id"}, nil
default:
return "", nil, fmt.Errorf("unsupported Vault authentication type %q", plugin.Vault.Authentication.Type)
}
Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial May 21, 2026

Choose a reason for hiding this comment

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

should we return an error on unrecognized types ?

Copy link
Copy Markdown
Member Author

@ardaguclu ardaguclu May 21, 2026

Choose a reason for hiding this comment

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

We should return error. Updated.

default:
return "", nil, fmt.Errorf("unsupported KMS provider type %q", plugin.Type)
}
}

// 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
162 changes: 158 additions & 4 deletions pkg/operator/encryption/controllers/key_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -336,9 +337,10 @@ func TestKeyController(t *testing.T) {
{Group: "", Resource: "secrets"},
},
targetNamespace: "kms",
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "create:secrets:openshift-config-managed", "create:events:kms"},
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
},
apiServerObjects: []runtime.Object{apiServerWithKMS},
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
Expand Down Expand Up @@ -381,6 +383,14 @@ func TestKeyController(t *testing.T) {
ts.Errorf("unexpected kms-plugin-config: %s", kmsPluginConfigData)
}

// Verify secret data is carried
if roleID := string(actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-secret-vault-approle-secret_role-id"]); roleID != "test-role-id" {
ts.Errorf("expected role-id secret data to be 'test-role-id', got %q", roleID)
}
if secretID := string(actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-secret-vault-approle-secret_secret-id"]); secretID != "test-secret-id" {
ts.Errorf("expected secret-id secret data to be 'test-secret-id', got %q", secretID)
}

// Verify internal reason
if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-key-does-not-exist" {
ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"])
Expand Down Expand Up @@ -418,10 +428,11 @@ func TestKeyController(t *testing.T) {
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
encryptiontesting.CreateEncryptionKeySecretWithRawKeyWithMode("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, []byte("61def964fb967f5d7c44a2af8dab6865"), "aescbc"),
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
},
apiServerObjects: []runtime.Object{apiServerWithKMS},
targetNamespace: "kms",
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "create:secrets:openshift-config-managed", "create:events:kms"},
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
wasSecretValidated := false
for _, action := range actions {
Expand Down Expand Up @@ -512,10 +523,11 @@ func TestKeyController(t *testing.T) {
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
encryptiontesting.CreateEncryptionKeySecretWithRawKeyWithMode("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, []byte("identity-key"), "identity"),
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
},
apiServerObjects: []runtime.Object{apiServerWithKMS},
targetNamespace: "kms",
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "create:secrets:openshift-config-managed", "create:events:kms"},
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
wasSecretValidated := false
for _, action := range actions {
Expand Down Expand Up @@ -573,6 +585,65 @@ func TestKeyController(t *testing.T) {
},
},

{
name: "degraded when KMS referenced secret does not exist",
targetGRs: []schema.GroupResource{
{Group: "", Resource: "secrets"},
},
targetNamespace: "kms",
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
},
apiServerObjects: []runtime.Object{apiServerWithKMS},
validateOperatorClientFunc: func(ts *testing.T, operatorClient v1helpers.OperatorClient) {
_, status, _, err := operatorClient.GetOperatorState()
if err != nil {
ts.Fatal(err)
}
for _, c := range status.Conditions {
if c.Type == "EncryptionKeyControllerDegraded" && c.Status == "True" {
if !strings.Contains(c.Message, "failed to get secret vault-approle-secret in openshift-config") {
ts.Errorf("unexpected degraded message: %s", c.Message)
}
return
}
}
ts.Fatal("expected EncryptionKeyControllerDegraded condition")
},
expectedError: fmt.Errorf(`failed to create key: failed to get secret vault-approle-secret in openshift-config: secrets "vault-approle-secret" not found`),
},

{
name: "degraded when KMS referenced secret is missing a required key",
targetGRs: []schema.GroupResource{
{Group: "", Resource: "secrets"},
},
targetNamespace: "kms",
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Name: "vault-approle-secret", Namespace: "openshift-config"},
Data: map[string][]byte{
"role-id": []byte("test-role-id"),
},
Type: corev1.SecretTypeOpaque,
},
},
apiServerObjects: []runtime.Object{apiServerWithKMS},
validateOperatorClientFunc: func(ts *testing.T, operatorClient v1helpers.OperatorClient) {
expectedCondition := operatorv1.OperatorCondition{
Type: "EncryptionKeyControllerDegraded",
Status: "True",
Reason: "Error",
Message: `failed to create key: secret vault-approle-secret in openshift-config is missing required key "secret-id"`,
}
encryptiontesting.ValidateOperatorClientConditions(ts, operatorClient, []operatorv1.OperatorCondition{expectedCondition})
},
expectedError: errors.New(`failed to create key: secret vault-approle-secret in openshift-config is missing required key "secret-id"`),
},

{
name: "creates a new AESCBC key when switching from KMS to AESCBC",
targetGRs: []schema.GroupResource{
Expand Down Expand Up @@ -667,7 +738,7 @@ func TestKeyController(t *testing.T) {
// - target namespace: pods and secrets
// - openshift-config-managed: secrets
// note that the informer factory is not used in the test - it's only needed to create the controller
kubeInformers := v1helpers.NewKubeInformersForNamespaces(fakeKubeClient, "openshift-config-managed", scenario.targetNamespace)
kubeInformers := v1helpers.NewKubeInformersForNamespaces(fakeKubeClient, "openshift-config-managed", "openshift-config", scenario.targetNamespace)
fakeSecretClient := fakeKubeClient.CoreV1()
fakePodClient := fakeKubeClient.CoreV1()
fakeConfigClient := configv1clientfake.NewSimpleClientset(scenario.apiServerObjects...)
Expand Down Expand Up @@ -708,6 +779,89 @@ func TestKeyController(t *testing.T) {
}
}

func TestReferencedSecretName(t *testing.T) {
scenarios := []struct {
name string
plugin configv1.KMSPluginConfig
expectedName string
expectedDataKeys []string
expectedError bool
}{
{
name: "Vault with AppRole authentication returns secret name and keys",
plugin: configv1.KMSPluginConfig{
Type: configv1.VaultKMSProvider,
Vault: configv1.VaultKMSPluginConfig{
Authentication: configv1.VaultAuthentication{
Type: configv1.VaultAuthenticationTypeAppRole,
AppRole: configv1.VaultAppRoleAuthentication{
Secret: configv1.VaultSecretReference{Name: "my-approle-secret"},
},
},
},
},
expectedName: "my-approle-secret",
expectedDataKeys: []string{"role-id", "secret-id"},
},
{
name: "Vault with unknown authentication type returns error",
plugin: configv1.KMSPluginConfig{
Type: configv1.VaultKMSProvider,
Vault: configv1.VaultKMSPluginConfig{
Authentication: configv1.VaultAuthentication{
Type: "UnknownAuth",
},
},
},
expectedError: true,
},
{
name: "Vault with empty authentication type returns error",
plugin: configv1.KMSPluginConfig{
Type: configv1.VaultKMSProvider,
Vault: configv1.VaultKMSPluginConfig{},
},
expectedError: true,
},
{
name: "unknown KMS provider returns error",
plugin: configv1.KMSPluginConfig{Type: "UnknownProvider"},
expectedError: true,
},
{
name: "empty plugin config returns error",
plugin: configv1.KMSPluginConfig{},
expectedError: true,
},
}

for _, scenario := range scenarios {
t.Run(scenario.name, func(t *testing.T) {
name, dataKeys, err := referencedSecretName(scenario.plugin)
if scenario.expectedError {
if err == nil {
t.Fatal("expected error, got nil")
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if name != scenario.expectedName {
t.Errorf("expected secret name %q, got %q", scenario.expectedName, name)
}
if len(dataKeys) != len(scenario.expectedDataKeys) {
t.Fatalf("expected %d data keys, got %d", len(scenario.expectedDataKeys), len(dataKeys))
}
for i, key := range dataKeys {
if key != scenario.expectedDataKeys[i] {
t.Errorf("expected data key[%d] %q, got %q", i, scenario.expectedDataKeys[i], key)
}
}
})
}
}

var flatEncryptionJSON = `
{
"encryption": {
Expand Down
Loading