Skip to content

Commit 3855e51

Browse files
ardagucluardaguclu
committed
In-place field update for KMS mode regardless of the convergence
When a KMS sidecar revision is stuck (e.g. wrong container image), the cluster-admin needs to update APIServer.spec.encryption.kms with corrected values. Previously, all encryption controllers blocked on revision convergence, so the fix could never be applied. This change allows the key_controller to update the latest key secret's KMS plugin config in-place for non-migration fields (image, TLS, AppRole) regardless of convergence state. If there is a change in migration-triggering fields, we should wait until convergence happens. State controller carries in-place fields updates only when there is no change in the encryption-config data key.
1 parent c877160 commit 3855e51

7 files changed

Lines changed: 383 additions & 27 deletions

File tree

‎pkg/operator/encryption/controllers/key_controller.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"time"
1212

1313
corev1 "k8s.io/api/core/v1"
14+
"k8s.io/apimachinery/pkg/api/equality"
1415
"k8s.io/apimachinery/pkg/api/errors"
1516
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1617
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -28,6 +29,7 @@ import (
2829

2930
"github.com/openshift/library-go/pkg/controller/factory"
3031
"github.com/openshift/library-go/pkg/operator/encryption/crypto"
32+
"github.com/openshift/library-go/pkg/operator/encryption/encoding"
3133
"github.com/openshift/library-go/pkg/operator/encryption/secrets"
3234
"github.com/openshift/library-go/pkg/operator/encryption/state"
3335
"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
@@ -171,6 +173,16 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact
171173
return err
172174
}
173175

176+
// Apply in-place KMS plugin config updates (e.g. image, TLS) to the latest key
177+
// secret regardless of convergence. This unblocks stuck revisions and propagates
178+
// operational fixes like CVE image updates. Changes to migration-triggering fields
179+
// (transit key, vault address) are skipped via kmsMigrationRequired.
180+
if currentMode == state.KMS {
181+
if err := c.maybeUpdateKMSPluginConfigInPlace(ctx, syncContext, apiEncryptionConfiguration); err != nil {
182+
return err
183+
}
184+
}
185+
174186
currentConfig, desiredEncryptionState, secrets, isProgressingReason, err := statemachine.GetEncryptionConfigAndState(ctx, c.deployer, c.secretClient, c.encryptionSecretSelector, encryptedGRs)
175187
if err != nil {
176188
return err
@@ -262,6 +274,81 @@ func (c *keyController) validateExistingSecret(ctx context.Context, keySecret *c
262274
return nil // we made this key earlier
263275
}
264276

277+
// maybeUpdateKMSPluginConfigInPlace updates the latest key secret's KMS plugin
278+
// config when only in-place-safe fields changed (image, TLS, authentication).
279+
func (c *keyController) maybeUpdateKMSPluginConfigInPlace(ctx context.Context, syncContext factory.SyncContext, apiServerEncryption configv1.APIServerEncryption) error {
280+
keySecrets, err := secrets.ListKeySecrets(ctx, c.secretClient, c.encryptionSecretSelector)
281+
if err != nil {
282+
return err
283+
}
284+
backedKeys := secrets.ToKeyStates(keySecrets)
285+
if len(backedKeys) == 0 {
286+
return nil
287+
}
288+
latest := backedKeys[0]
289+
290+
// Any mode mismatch (e.g. KMS <-> AESCBC) requires a migration, not an in-place
291+
// update. The normal needsNewKey path handles this after convergence.
292+
if latest.Mode != state.KMS {
293+
return nil
294+
}
295+
// This should never happen under normal operation because ToKeyState enforces
296+
// that KMS mode keys have a plugin config. This can only occur if someone
297+
// manually edited the key secret and removed the kms-plugin-config data field.
298+
// To mitigate, re-add the removed kms-plugin-config data to the key secret.
299+
if !latest.HasKMSPlugin() {
300+
return fmt.Errorf("latest KMS key %s is missing plugin config", latest.Key.Name)
301+
}
302+
// Skip when migration-triggering fields changed (needs a new key, not an in-place
303+
// update) or when the plugin config is already up-to-date.
304+
if kmsMigrationRequired(latest.KMS.Plugin, apiServerEncryption.KMS) ||
305+
equality.Semantic.DeepEqual(latest.KMS.Plugin, apiServerEncryption.KMS) {
306+
return nil
307+
}
308+
309+
keySecret, err := secrets.FromKeyState(c.instanceName, latest)
310+
if err != nil {
311+
return err
312+
}
313+
s, err := c.secretClient.Secrets(keySecret.Namespace).Get(ctx, keySecret.Name, metav1.GetOptions{})
314+
if err != nil {
315+
return fmt.Errorf("failed to get key secret %s/%s: %v", keySecret.Namespace, keySecret.Name, err)
316+
}
317+
pluginData, err := encoding.EncodeKMSPluginConfig(apiServerEncryption.KMS)
318+
if err != nil {
319+
return fmt.Errorf("failed to encode KMS plugin config: %v", err)
320+
}
321+
s.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] = pluginData
322+
_, updateErr := c.secretClient.Secrets(s.Namespace).Update(ctx, s, metav1.UpdateOptions{})
323+
if errors.IsConflict(updateErr) {
324+
return nil
325+
}
326+
if updateErr == nil {
327+
syncContext.Recorder().Eventf("EncryptionKeyKMSPluginConfigUpdated", "Updated KMS plugin config on key secret %q in-place", s.Name)
328+
}
329+
return updateErr
330+
}
331+
332+
// kmsMigrationRequired reports whether the KMS config change between latest
333+
// (stored in the key secret) and current (from the APIServer CR) involves
334+
// migration-triggering fields that require a new encryption key.
335+
// Returns false when only in-place-safe fields differ (image, TLS, authentication)
336+
// or when configs are identical.
337+
func kmsMigrationRequired(latest, current configv1.KMSPluginConfig) bool {
338+
if latest.Type != current.Type {
339+
return true
340+
}
341+
if latest.Type == configv1.VaultKMSProvider {
342+
if latest.Vault.VaultAddress != current.Vault.VaultAddress ||
343+
latest.Vault.VaultNamespace != current.Vault.VaultNamespace ||
344+
latest.Vault.TransitMount != current.Vault.TransitMount ||
345+
latest.Vault.TransitKey != current.Vault.TransitKey {
346+
return true
347+
}
348+
}
349+
return false
350+
}
351+
265352
func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, internalReason, externalReason string) (*corev1.Secret, error) {
266353
bs := crypto.ModeToNewKeyFunc[currentMode]()
267354
ks := state.KeyState{

‎pkg/operator/encryption/controllers/key_controller_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ func TestKeyController(t *testing.T) {
337337
{Group: "", Resource: "secrets"},
338338
},
339339
targetNamespace: "kms",
340-
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"},
340+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
341341
initialObjects: []runtime.Object{
342342
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
343343
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
@@ -417,7 +417,7 @@ func TestKeyController(t *testing.T) {
417417
},
418418
apiServerObjects: []runtime.Object{apiServerWithKMS},
419419
targetNamespace: "kms",
420-
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
420+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
421421
},
422422

423423
{
@@ -432,7 +432,7 @@ func TestKeyController(t *testing.T) {
432432
},
433433
apiServerObjects: []runtime.Object{apiServerWithKMS},
434434
targetNamespace: "kms",
435-
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"},
435+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
436436
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
437437
wasSecretValidated := false
438438
for _, action := range actions {
@@ -498,7 +498,7 @@ func TestKeyController(t *testing.T) {
498498
apiServerObjects: []runtime.Object{apiServerWithKMS},
499499
targetNamespace: "kms",
500500
// Should be no-op because KMS keys don't have time-based rotation
501-
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
501+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
502502
},
503503
{
504504
name: "no-op when latest KMS key is not migrated yet",
@@ -512,7 +512,7 @@ func TestKeyController(t *testing.T) {
512512
apiServerObjects: []runtime.Object{apiServerWithKMS},
513513
targetNamespace: "kms",
514514
// Should be no-op because migration hasn't completed yet
515-
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
515+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
516516
},
517517

518518
{
@@ -527,7 +527,7 @@ func TestKeyController(t *testing.T) {
527527
},
528528
apiServerObjects: []runtime.Object{apiServerWithKMS},
529529
targetNamespace: "kms",
530-
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"},
530+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},
531531
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
532532
wasSecretValidated := false
533533
for _, action := range actions {
@@ -591,7 +591,7 @@ func TestKeyController(t *testing.T) {
591591
{Group: "", Resource: "secrets"},
592592
},
593593
targetNamespace: "kms",
594-
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
594+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
595595
initialObjects: []runtime.Object{
596596
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
597597
},
@@ -620,7 +620,7 @@ func TestKeyController(t *testing.T) {
620620
{Group: "", Resource: "secrets"},
621621
},
622622
targetNamespace: "kms",
623-
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
623+
expectedActions: []string{"list:secrets:openshift-config-managed", "list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config"},
624624
initialObjects: []runtime.Object{
625625
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
626626
&corev1.Secret{

‎pkg/operator/encryption/controllers/state_controller.go‎

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"fmt"
66
"time"
77

8+
"k8s.io/apimachinery/pkg/api/equality"
9+
apierrors "k8s.io/apimachinery/pkg/api/errors"
810
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
911
"k8s.io/apimachinery/pkg/runtime/schema"
1012
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -16,6 +18,7 @@ import (
1618
configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1"
1719
"github.com/openshift/library-go/pkg/controller/factory"
1820
"github.com/openshift/library-go/pkg/operator/encryption/encryptiondata"
21+
"github.com/openshift/library-go/pkg/operator/encryption/secrets"
1922
"github.com/openshift/library-go/pkg/operator/encryption/state"
2023
"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
2124
"github.com/openshift/library-go/pkg/operator/events"
@@ -127,11 +130,19 @@ type eventWithReason struct {
127130
}
128131

129132
func (c *stateController) generateAndApplyCurrentEncryptionConfigSecret(ctx context.Context, queue workqueue.RateLimitingInterface, recorder events.Recorder, encryptedGRs []schema.GroupResource) error {
133+
encryptionConfigNamespace := "openshift-config-managed"
134+
encryptionConfigName := fmt.Sprintf("%s-%s", encryptiondata.EncryptionConfSecretName, c.instanceName)
135+
130136
currentConfig, desiredEncryptionState, encryptionSecrets, transitioningReason, err := statemachine.GetEncryptionConfigAndState(ctx, c.deployer, c.secretClient, c.encryptionSecretSelector, encryptedGRs)
131137
if err != nil {
132138
return err
133139
}
134140
if len(transitioningReason) > 0 {
141+
// Even when not converged, propagate in-place KMS plugin config changes
142+
// so the revision controller can create a new revision with corrected sidecar config.
143+
if err := c.maybeUpdateKMSDataInEncryptionConfigSecret(ctx, recorder, encryptionConfigNamespace, encryptionConfigName); err != nil {
144+
return err
145+
}
135146
queue.AddAfter(stateWorkKey, 2*time.Minute)
136147
return nil
137148
}
@@ -147,7 +158,7 @@ func (c *stateController) generateAndApplyCurrentEncryptionConfigSecret(ctx cont
147158
if err != nil {
148159
return err
149160
}
150-
changed, err := c.applyEncryptionConfigSecret(ctx, desiredSecretData, recorder)
161+
changed, err := c.applyEncryptionConfigSecret(ctx, desiredSecretData, encryptionConfigNamespace, encryptionConfigName, recorder)
151162
if err != nil {
152163
return err
153164
}
@@ -163,8 +174,61 @@ func (c *stateController) generateAndApplyCurrentEncryptionConfigSecret(ctx cont
163174
return nil
164175
}
165176

166-
func (c *stateController) applyEncryptionConfigSecret(ctx context.Context, secretData *encryptiondata.Config, recorder events.Recorder) (bool, error) {
167-
s, err := encryptiondata.ToSecret("openshift-config-managed", fmt.Sprintf("%s-%s", encryptiondata.EncryptionConfSecretName, c.instanceName), secretData)
177+
// maybeUpdateKMSDataInEncryptionConfigSecret propagates in-place KMS plugin
178+
// config changes to the encryption-config secret during non-convergence. It enriches
179+
// the existing state with current key secrets (picking up updated plugin configs from
180+
// key_controller), guards that the EncryptionConfiguration is unchanged (no key
181+
// promotion or structural changes), and applies only kms-plugin-config updates.
182+
func (c *stateController) maybeUpdateKMSDataInEncryptionConfigSecret(ctx context.Context, recorder events.Recorder, namespace, name string) error {
183+
keySecrets, err := secrets.ListKeySecrets(ctx, c.secretClient, c.encryptionSecretSelector)
184+
if err != nil {
185+
return err
186+
}
187+
188+
existingSecret, err := c.secretClient.Secrets(namespace).Get(ctx, name, metav1.GetOptions{})
189+
if apierrors.IsNotFound(err) {
190+
return nil
191+
}
192+
if err != nil {
193+
return err
194+
}
195+
196+
existingConfig, err := encryptiondata.FromSecret(existingSecret)
197+
if err != nil {
198+
return err
199+
}
200+
if existingConfig == nil || len(existingConfig.KMSPlugins) == 0 {
201+
return nil
202+
}
203+
204+
// Round-trip through ToEncryptionState → FromEncryptionState to pick up
205+
// updated KMS plugin configs from key secrets while preserving the existing
206+
// EncryptionConfiguration structure. ToEncryptionState enriches each key in
207+
// the config with its backed secret data (including any in-place plugin config
208+
// updates), and FromEncryptionState rebuilds the Config from the enriched state.
209+
enrichedState, _ := encryptiondata.ToEncryptionState(existingConfig, keySecrets)
210+
if enrichedState == nil {
211+
return nil
212+
}
213+
214+
rebuiltConfig, err := encryptiondata.FromEncryptionState(enrichedState)
215+
if err != nil {
216+
return err
217+
}
218+
219+
// Only proceed if the provider list, key ordering, and write key designation
220+
// are unchanged. Structural changes require convergence to avoid a server
221+
// encrypting with a key another server hasn't observed.
222+
if !equality.Semantic.DeepEqual(existingConfig.Encryption.Resources, rebuiltConfig.Encryption.Resources) {
223+
return nil
224+
}
225+
226+
_, err = c.applyEncryptionConfigSecret(ctx, rebuiltConfig, namespace, name, recorder)
227+
return err
228+
}
229+
230+
func (c *stateController) applyEncryptionConfigSecret(ctx context.Context, secretData *encryptiondata.Config, namespace, name string, recorder events.Recorder) (bool, error) {
231+
s, err := encryptiondata.ToSecret(namespace, name, secretData)
168232
if err != nil {
169233
return false, err
170234
}

0 commit comments

Comments
 (0)