Skip to content

test/encryption: add InvalidImageRecoveryScenario for KMS plugin imag… #2243

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

Draft
gangwgr wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

test/encryption: add InvalidImageRecoveryScenario for KMS plugin imag… #2243

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de1745f
test/encryption: add InvalidImageRecoveryScenario for KMS plugin imag…
gangwgr May 26, 2026
f307d1c
Rename AppRole datakeys to role-id and secret-id
ardaguclu May 22, 2026
9e7a937
In-place field update for KMS mode regardless of the convergence
ardaguclu May 13, 2026
d94ed61
test/kms: add InvalidImageVaultEncryptionProvider for degradation tests
gangwgr May 26, 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
82 changes: 80 additions & 2 deletions test/library/encryption/scenarios.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (
"testing"

"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/rand"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/rand"

configv1 "github.com/openshift/api/config/v1"
)
Expand Down Expand Up @@ -277,3 +277,81 @@ func TestEncryptionRotation(ctx context.Context, t testing.TB, scenario Rotation

// TODO: assert conditions - operator and encryption migration controller must report status as active not progressing, and not failing for all scenarios
}

// InvalidImageRecoveryScenario tests that an invalid KMS plugin image causes
// the cluster to degrade and that correcting the image restores normal operation.
//
// Arda's scenario:
// 1. Enable KMS with invalid image
// 2. Attempt to switch to aescbc (cluster should be stuck on KMS)
// 3. See degraded
// 4. Update with KMS with valid image
// 5. See everything works
type InvalidImageRecoveryScenario struct {
Copy link
Copy Markdown
Member

@ardaguclu ardaguclu May 26, 2026

Choose a reason for hiding this comment

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

I think this scenario nicely exercises the in-place field update case which will be added by #2241.

BasicScenario
// InvalidImageProvider is the KMS EncryptionProvider configured with an invalid
// (non-existent or broken) KMS plugin image. Enabling this should cause degradation.
InvalidImageProvider EncryptionProvider
// ValidImageProvider is the KMS EncryptionProvider configured with the correct
// KMS plugin image. Applying this after degradation should restore the cluster.
ValidImageProvider EncryptionProvider
// WaitForDegraded should block until the operator reports a degraded condition.
WaitForDegraded func(ctx context.Context, t testing.TB)
// WaitForRecovery should block until the operator reports a healthy condition
// and encryption is fully operational.
WaitForRecovery func(ctx context.Context, t testing.TB)
}

// TestEncryptionInvalidImageRecovery tests that:
// 1. Enabling KMS with an invalid plugin image causes degradation
// 2. The cluster remains stuck and cannot migrate to another mode (e.g. aescbc)
// 3. Fixing the KMS config with a valid image restores the cluster
func TestEncryptionInvalidImageRecovery(ctx context.Context, t testing.TB, scenario InvalidImageRecoveryScenario) {
e := NewE(t, PrintEventsOnFailure(scenario.OperatorNamespace))
require.NotNil(t, scenario.WaitForDegraded, "WaitForDegraded must not be nil")
require.NotNil(t, scenario.WaitForRecovery, "WaitForRecovery must not be nil")
require.Equal(t, configv1.EncryptionTypeKMS, scenario.InvalidImageProvider.Type, "InvalidImageProvider must be KMS type")
require.Equal(t, configv1.EncryptionTypeKMS, scenario.ValidImageProvider.Type, "ValidImageProvider must be KMS type")

cs := GetClients(e)

// step 1: enable KMS with invalid image (set config but don't wait for completion)
t.Log("Setting KMS encryption with invalid plugin image")
if scenario.InvalidImageProvider.Setup != nil {
scenario.InvalidImageProvider.Setup(ctx, e)
}
apiServer, err := cs.ApiServerConfig.Get(ctx, "cluster", metav1.GetOptions{})
require.NoError(t, err)
apiServer.Spec.Encryption = scenario.InvalidImageProvider.APIServerEncryption
_, err = cs.ApiServerConfig.Update(ctx, apiServer, metav1.UpdateOptions{})
require.NoError(t, err)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// step 2: attempt to switch to aescbc — cluster should remain stuck on KMS
t.Log("Attempting to switch to aescbc (cluster should remain stuck on KMS)")
apiServer, err = cs.ApiServerConfig.Get(ctx, "cluster", metav1.GetOptions{})
require.NoError(t, err)
apiServer.Spec.Encryption = configv1.APIServerEncryption{Type: configv1.EncryptionTypeAESCBC}
_, err = cs.ApiServerConfig.Update(ctx, apiServer, metav1.UpdateOptions{})
require.NoError(t, err)

// step 3: wait for degraded
t.Log("Waiting for operator to report degraded status")
scenario.WaitForDegraded(ctx, e)

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Assert the mode remains KMS after the failed switch attempt.

After Line 334, the test only waits for degraded status. It does not explicitly verify the cluster stayed on KMS, so step 2 can pass without validating the intended behavior.

Proposed fix 
 func TestEncryptionInvalidImageRecovery(ctx context.Context, t testing.TB, scenario InvalidImageRecoveryScenario) {
 	e := NewE(t, PrintEventsOnFailure(scenario.OperatorNamespace))
+	require.NotNil(t, scenario.AssertFunc, "AssertFunc must not be nil")
 	require.NotNil(t, scenario.WaitForDegraded, "WaitForDegraded must not be nil")
 	require.NotNil(t, scenario.WaitForRecovery, "WaitForRecovery must not be nil")
@@
 	// step 3: wait for degraded
 	t.Log("Waiting for operator to report degraded status")
 	scenario.WaitForDegraded(ctx, e)
+	scenario.AssertFunc(e, cs, configv1.EncryptionTypeKMS, scenario.Namespace, scenario.LabelSelector)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/library/encryption/scenarios.go` around lines 329 - 340, After calling
scenario.WaitForDegraded(ctx, e) add an explicit assertion that the cluster's
encryption mode remained KMS: re-fetch the APIServer via
cs.ApiServerConfig.Get(ctx, "cluster", metav1.GetOptions{}), then assert
apiServer.Spec.Encryption.Type equals configv1.EncryptionTypeKMS using
require/expect (e.g., require.Equal) so the test fails if the attempted switch
to aescbc actually applied; reference cs.ApiServerConfig.Get,
apiServer.Spec.Encryption.Type, and scenario.WaitForDegraded when locating where
to add the check.

// step 4: fix the config with valid KMS image
t.Log("Updating KMS encryption with valid plugin image to recover")
if scenario.ValidImageProvider.Setup != nil {
scenario.ValidImageProvider.Setup(ctx, e)
}
apiServer, err = cs.ApiServerConfig.Get(ctx, "cluster", metav1.GetOptions{})
require.NoError(t, err)
apiServer.Spec.Encryption = scenario.ValidImageProvider.APIServerEncryption
_, err = cs.ApiServerConfig.Update(ctx, apiServer, metav1.UpdateOptions{})
require.NoError(t, err)

// step 5: wait for recovery — everything should work
t.Log("Waiting for operator to recover with valid KMS image")
scenario.WaitForRecovery(ctx, e)

t.Log("Invalid image recovery test passed")
}