Added new test case for update image scenario#2285
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a pod polling helper that waits until all listed pods satisfy a predicate, and a KMS in-place update scenario + test that applies initial/updated KMS providers, asserts no new encryption key is created, and invokes a propagation verification callback. ChangesKMS In-Place Update and Helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/library/encryption/scenarios.go`:
- Around line 419-426: The kmsPluginImage helper dereferences encryption.KMS and
encryption.KMS.Vault without nil checks; add defensive guards at the top of
kmsPluginImage(encryption configv1.APIServerEncryption) to verify encryption.KMS
!= nil and encryption.KMS.Vault != nil before accessing Type or KMSPluginImage,
and return a clear error (e.g., "missing KMS configuration" or "missing Vault
configuration") when those are nil; keep the existing switch on
configv1.VaultKMSProvider and only return the Vault.KMSPluginImage after the nil
checks succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a46b0da8-686d-490c-bc94-4ad56a02d520
📒 Files selected for processing (1)
test/library/encryption/scenarios.go
Comment on lines
+419
to
+426
| func kmsPluginImage(encryption configv1.APIServerEncryption) (string, error) { | ||
| switch encryption.KMS.Type { | ||
| case configv1.VaultKMSProvider: | ||
| return encryption.KMS.Vault.KMSPluginImage, nil | ||
| default: | ||
| return "", fmt.Errorf("unsupported KMS provider type %q", encryption.KMS.Type) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil pointer guards to prevent runtime panics.
The function accesses encryption.KMS.Type (line 420) and encryption.KMS.Vault.KMSPluginImage (line 422) without checking whether encryption.KMS or encryption.KMS.Vault are nil. While current callers validate the encryption type before calling this helper, the function should be defensive against nil pointer dereferences to prevent runtime panics if called in other contexts.
🛡️ Proposed fix to add nil guards
func kmsPluginImage(encryption configv1.APIServerEncryption) (string, error) {
+ if encryption.KMS == nil {
+ return "", fmt.Errorf("KMS configuration is nil")
+ }
switch encryption.KMS.Type {
case configv1.VaultKMSProvider:
+ if encryption.KMS.Vault == nil {
+ return "", fmt.Errorf("Vault KMS configuration is nil")
+ }
return encryption.KMS.Vault.KMSPluginImage, nil
default:
return "", fmt.Errorf("unsupported KMS provider type %q", encryption.KMS.Type)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func kmsPluginImage(encryption configv1.APIServerEncryption) (string, error) { | |
| switch encryption.KMS.Type { | |
| case configv1.VaultKMSProvider: | |
| return encryption.KMS.Vault.KMSPluginImage, nil | |
| default: | |
| return "", fmt.Errorf("unsupported KMS provider type %q", encryption.KMS.Type) | |
| } | |
| } | |
| func kmsPluginImage(encryption configv1.APIServerEncryption) (string, error) { | |
| if encryption.KMS == nil { | |
| return "", fmt.Errorf("KMS configuration is nil") | |
| } | |
| switch encryption.KMS.Type { | |
| case configv1.VaultKMSProvider: | |
| if encryption.KMS.Vault == nil { | |
| return "", fmt.Errorf("Vault KMS configuration is nil") | |
| } | |
| return encryption.KMS.Vault.KMSPluginImage, nil | |
| default: | |
| return "", fmt.Errorf("unsupported KMS provider type %q", encryption.KMS.Type) | |
| } | |
| } |
🤖 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 419 - 426, The
kmsPluginImage helper dereferences encryption.KMS and encryption.KMS.Vault
without nil checks; add defensive guards at the top of kmsPluginImage(encryption
configv1.APIServerEncryption) to verify encryption.KMS != nil and
encryption.KMS.Vault != nil before accessing Type or KMSPluginImage, and return
a clear error (e.g., "missing KMS configuration" or "missing Vault
configuration") when those are nil; keep the existing switch on
configv1.VaultKMSProvider and only return the Vault.KMSPluginImage after the nil
checks succeed.
ardaguclu
reviewed
Jun 9, 2026
| require.NoError(t, err) | ||
| require.NotEqual(t, initialImage, updatedImage, "UpdatedProvider kmsPluginImage must differ from Provider") | ||
|
|
||
| var prevKeyMeta EncryptionKeyMeta |
Member
There was a problem hiding this comment.
We don't need this. Maybe it is better to call GetLastKeyMeta every time.
| ApplyEncryption(ctx, t, scenario.UpdatedProvider.APIServerEncryption) | ||
| }}, | ||
| {name: "WaitForNoNewEncryptionKey", testFunc: func(t testing.TB) { | ||
| WaitForNoNewEncryptionKey(t, clientSet.Kube, prevKeyMeta, |
Member
There was a problem hiding this comment.
Calling WaitForNoNewEncryptionKey definitely makes sense. But I wonder how we will ensure that the kms plugin container uses the new image.
gangwgr
force-pushed
the
new-case1
branch
3 times, most recently
from
b6f51ab to
0a27404
Compare
Contributor
Author
|
/test unit |
ardaguclu
reviewed
Jun 10, 2026
| // rolls out a new sidecar image without creating a new encryption key. | ||
| // The caller supplies Provider (initial valid config) and UpdatedProvider (same config | ||
| // with only kmsPluginImage changed). | ||
| type KMSPluginImageUpdateScenario struct { |
Member
There was a problem hiding this comment.
As we did for invalidconfig scenario, I think it is better to rename this to more generic like KMSInPlaceUpdateScenario.
| // WaitForUpdatedImage is called after the image update to verify the new image | ||
| // is running (e.g. by polling API server pod init containers). Caller-provided, | ||
| // same pattern as WaitForStuck in KMSInvalidEncryptionRecoveryScenario. | ||
| WaitForUpdatedImage func(ctx context.Context, t testing.TB) |
Member
There was a problem hiding this comment.
In order to keep it generic, we can rename it to WaitForPropagation.
| scenario.AssertFunc(t, clientSet, scenario.Provider.Type, | ||
| scenario.Namespace, scenario.LabelSelector) | ||
| }}, | ||
| {name: "UpdateKMSPluginImage", testFunc: func(t testing.TB) { |
| scenario.Namespace, scenario.LabelSelector) | ||
| }}, | ||
| {name: "WaitForUpdatedImage", testFunc: func(t testing.TB) { | ||
| scenario.WaitForUpdatedImage(ctx, t) |
Member
There was a problem hiding this comment.
WaitForPropagation and callers will use custom wait command as we did for invalidconfigrecovery.
| // WaitForPodInitContainerImage polls pods until each has a running init container | ||
| // using the expected image. Init containers are scanned the same way as in | ||
| // WaitForPodImagePullBackOff. | ||
| func WaitForPodInitContainerImage(ctx context.Context, t testing.TB, kubeClient kubernetes.Interface, namespace, labelSelector, expectedImage string) { |
Member
There was a problem hiding this comment.
What about updating this function to be generic;
// WaitForPodContainerCondition polls pods until every pod has at least one
// container status (init or regular, depending on the predicate) that satisfies
// the given condition.
func WaitForPodContainerCondition(
ctx context.Context,
t testing.TB,
kubeClient kubernetes.Interface,
namespace, labelSelector, conditionDesc string,
match func(pod corev1.Pod) bool,
) {
t.Helper()
t.Logf("Waiting up to %s for pods in %s (selector=%s) to satisfy: %s",
waitPollTimeout, namespace, labelSelector, conditionDesc)
err := wait.PollUntilContextTimeout(ctx, waitPollInterval, waitPollTimeout, true,
func(ctx context.Context) (bool, error) {
pods, err := kubeClient.CoreV1().Pods(namespace).List(ctx,
metav1.ListOptions{LabelSelector: labelSelector})
if err != nil {
t.Logf("Error listing pods: %v", err)
return false, nil
}
if len(pods.Items) == 0 {
t.Logf("No pods found yet in %s", namespace)
return false, nil
}
for _, pod := range pods.Items {
if !match(pod) {
t.Logf("Pod %s does not yet satisfy condition: %s", pod.Name, conditionDesc)
return false, nil
}
}
return true, nil
})
require.NoError(t, err, "timed out waiting for pods in %s to satisfy: %s",
namespace, conditionDesc)
}Callers will use it like this;
WaitForPropagation: func(ctx context.Context, t testing.TB) {
WaitForPodContainerCondition(ctx, t, kubeClient, namespace, labelSelector,
fmt.Sprintf("running init container with image %q", expectedImage),
func(pod corev1.Pod) bool {
if pod.Status.Phase != corev1.PodRunning {
return false
}
for _, cs := range pod.Status.InitContainerStatuses {
if cs.State.Running != nil && cs.Image == expectedImage {
return true
}
}
return false
},
)
},
Member
|
Thank you |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
added
lgtm
ardaguclu
reviewed
Jun 10, 2026
Comment on lines
+355
to
+402
| } | ||
|
|
||
| // KMSInPlaceUpdateScenario tests that updating an in-place KMS config field | ||
| // (e.g. kmsPluginImage) takes effect without creating a new encryption key. | ||
| // The caller supplies Provider (initial valid config) and UpdatedProvider (same config | ||
| // with one in-place field changed). | ||
| type KMSInPlaceUpdateScenario struct { | ||
| BasicScenario | ||
| Provider EncryptionProvider | ||
| UpdatedProvider EncryptionProvider | ||
| // WaitForPropagation is called after the in-place update to verify the change | ||
| // took effect. Caller-provided, same pattern as WaitForStuck in | ||
| // KMSInvalidEncryptionRecoveryScenario. | ||
| WaitForPropagation func(ctx context.Context, t testing.TB) | ||
| } | ||
|
|
||
| // TestKMSInPlaceUpdate validates in-place KMS config field updates: | ||
| // 1. Apply valid provider and verify migration | ||
| // 2. Update in-place field and verify no new encryption key is created | ||
| // 3. WaitForPropagation — caller verifies the change took effect | ||
| func TestKMSInPlaceUpdate(ctx context.Context, t testing.TB, scenario KMSInPlaceUpdateScenario) { | ||
| e := NewE(t, PrintEventsOnFailure(scenario.OperatorNamespace)) | ||
| clientSet := GetClients(e) | ||
|
|
||
| require.NotNil(t, scenario.Provider.Setup, "Provider.Setup must not be nil") | ||
| require.NotNil(t, scenario.UpdatedProvider.Setup, "UpdatedProvider.Setup must not be nil") | ||
| require.NotNil(t, scenario.WaitForPropagation, "WaitForPropagation must not be nil") | ||
| require.Equal(t, configv1.EncryptionTypeKMS, scenario.Provider.Type, "Provider must use KMS encryption type") | ||
| require.Equal(t, configv1.EncryptionTypeKMS, scenario.UpdatedProvider.Type, "UpdatedProvider must use KMS encryption type") | ||
|
|
||
| steps := []testStep{ | ||
| {name: "ApplyValidProviderAndVerifyMigration", testFunc: func(t testing.TB) { | ||
| SetAndWaitForEncryptionType(ctx, t, scenario.Provider, scenario.TargetGRs, | ||
| scenario.Namespace, scenario.LabelSelector) | ||
| scenario.AssertFunc(t, clientSet, scenario.Provider.Type, | ||
| scenario.Namespace, scenario.LabelSelector) | ||
| }}, | ||
| {name: "UpdateInPlaceField", testFunc: func(t testing.TB) { | ||
| keyMeta, err := GetLastKeyMeta(t, clientSet.Kube, | ||
| scenario.Namespace, scenario.LabelSelector) | ||
| require.NoError(t, err) | ||
| scenario.UpdatedProvider.Setup(ctx, t) | ||
| ApplyEncryption(ctx, t, scenario.UpdatedProvider.APIServerEncryption) | ||
| WaitForNoNewEncryptionKey(t, clientSet.Kube, keyMeta, | ||
| scenario.Namespace, scenario.LabelSelector) | ||
| }}, | ||
| {name: "WaitForPropagation", testFunc: func(t testing.TB) { | ||
| scenario.WaitForPropagation(ctx, t) |
Member
There was a problem hiding this comment.
Maybe we should get last key as in line: 393 and pass it to waitforpropagation. So that if we find a matching container, we can compare container name with our key.
| t.Logf("No pods found yet in %s", namespace) | ||
| return false, nil | ||
| } | ||
| for _, pod := range pods.Items { |
Member
There was a problem hiding this comment.
Suggested change
| for _, pod := range pods.Items { | |
| for _, pod := range pods.Items { | |
| if match(pod, keyName) { | |
| return true, nil | |
| } | |
| } | |
| t.Logf("Pod %s does not yet satisfy condition: %s", pod.Name, conditionDesc) | |
| return false, nil | |
Member
There was a problem hiding this comment.
Don't we need to continue iteration over the pods until we see match. So maybe we should use this ^^
Member
|
/lgtm |
openshift-ci
Bot
added
lgtm
ardaguclu
reviewed
Jun 10, 2026
| // WaitForPodContainerCondition polls pods until every pod satisfies the given condition. | ||
| // keyName is the current encryption key secret name, passed to match so callers can | ||
| // correlate pod container names with the active key (e.g. vault-kms-plugin-42). | ||
| func WaitForPodContainerCondition(ctx context.Context, t testing.TB, kubeClient kubernetes.Interface, namespace, labelSelector, keyName, conditionDesc string, match func(pod corev1.Pod, keyName string) bool) { |
Member
|
/lgtm cancel |
openshift-ci
Bot
removed
lgtm
Contributor
Author
|
@ardaguclu holding pr to test with your pr, will unhold later |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
ardaguclu
reviewed
Jun 10, 2026
| if match(pod, keyName) { | ||
| return true, nil | ||
| } | ||
| t.Logf("Pod %s does not yet satisfy condition", pod.Name) |
Member
There was a problem hiding this comment.
This pod may not be the one we are looking for. Can we move this log just above the return false, nil
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, gangwgr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
@gangwgr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added new test case for update image scenario
Summary by CodeRabbit