CNTRLPLANE-3364: add the kms suite and migrate the kms tests of auth-o to ote#895
CNTRLPLANE-3364: add the kms suite and migrate the kms tests of auth-o to ote#895sandeepknd wants to merge 1 commit into
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:
WalkthroughThis PR adds end-to-end tests for KMS encryption in the cluster-authentication-operator. A new test suite is registered with serial execution constraints, and two test scenarios verify token encryption behavior during KMS enable/disable transitions and encryption provider migrations. The test logic is implemented in a new file with refactored thin entry points. ChangesKMS Encryption E2E Tests
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/assign @gangwgr |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption-kms/encryption_kms.go (1)
5-5: ⚡ Quick winAvoid nondeterministic provider selection in migration tests.
Line 69 randomizes provider choice and order, which makes failures harder to reproduce. Prefer a fixed provider list/order (or explicit deterministic subcases) for stable CI diagnostics.
As per coding guidelines, "Review Ginkgo test code for quality: ... (5) Consistency - follow existing repository patterns."Proposed deterministic variant
- "math/rand/v2" @@ - EncryptionProviders: library.ShuffleEncryptionProviders([]configv1.APIServerEncryption{{Type: configv1.EncryptionTypeKMS, KMS: librarykms.DefaultFakeKMSPluginConfig}, library.SupportedStaticEncryptionProviders[rand.IntN(len(library.SupportedStaticEncryptionProviders))]}), + EncryptionProviders: []configv1.APIServerEncryption{ + {Type: configv1.EncryptionTypeKMS, KMS: librarykms.DefaultFakeKMSPluginConfig}, + library.SupportedStaticEncryptionProviders[0], + },Also applies to: 69-69
🤖 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/e2e-encryption-kms/encryption_kms.go` at line 5, The test currently randomizes the provider selection/order (the providers slice and its rand.Shuffle/selection logic) which causes nondeterministic failures; replace the randomization with a deterministic order—either hardcode the providers slice in a stable order (e.g., []string{"aws","gcp","azure"}) or sort.Strings(providers) before use, and remove any use of math/rand/v2 or rand.Shuffle in the provider-selection code path so the migration tests run with a stable, reproducible provider list.
🤖 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.
Nitpick comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Line 5: The test currently randomizes the provider selection/order (the
providers slice and its rand.Shuffle/selection logic) which causes
nondeterministic failures; replace the randomization with a deterministic
order—either hardcode the providers slice in a stable order (e.g.,
[]string{"aws","gcp","azure"}) or sort.Strings(providers) before use, and remove
any use of math/rand/v2 or rand.Shuffle in the provider-selection code path so
the migration tests run with a stable, reproducible provider list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e62b6d6-dcfd-4fe7-a5e4-b453d6d4ca2c
📒 Files selected for processing (4)
cmd/cluster-authentication-operator-tests-ext/main.gopkg/dependencymagnet/dependencymagnet.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_test.go
|
/test e2e-aws-operator-encryption-kms-ote |
1 similar comment
|
/test e2e-aws-operator-encryption-kms-ote |
|
re-triggering the test e2e-aws-operator-encryption-kms-ote as it failed due to node not becoming ready. |
|
The tests (TestKMSEncryptionOnOff and TestKMSEncryptionProvidersMigration) got executed within the suite openshift/cluster-authentication-operator/encryption-kms. |
| Name: "openshift/cluster-authentication-operator/encryption-kms", | ||
| Parallelism: 1, | ||
| Qualifiers: []string{ | ||
| `name.contains("KMSEncryptionProvider")`, |
There was a problem hiding this comment.
| "github.com/openshift/cluster-authentication-operator/pkg/version" | ||
|
|
||
| _ "github.com/openshift/cluster-authentication-operator/test/e2e" | ||
| _ "github.com/openshift/cluster-authentication-operator/test/e2e-encryption-kms" |
There was a problem hiding this comment.
already added to pkg/dependencymagnet/dependencymagnet.go ?
There was a problem hiding this comment.
added only in this file. Referred other repo as well.
| }) | ||
| }) | ||
|
|
||
| func testKMSEncryptionOnOff(t testing.TB) { |
There was a problem hiding this comment.
were there any changes to this test ?
There was a problem hiding this comment.
the latest changes made to this test has been pulled in this PR as well.
| }) | ||
| } | ||
|
|
||
| func testKMSEncryptionProvidersMigration(t testing.TB) { |
There was a problem hiding this comment.
were there any changes to this test ?
There was a problem hiding this comment.
the latest changes made to this test (multiline parameters) has been pulled in this PR as well.
|
/assign @gangwgr |
sandeepknd
force-pushed
the
kms-suite-auth
branch
from
c352356 to
0a3eff5
Compare
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sandeepknd
force-pushed
the
kms-suite-auth
branch
from
0a3eff5 to
562ed4f
Compare
openshift-ci
Bot
removed
the
needs-rebase
sandeepknd
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
sandeepknd
force-pushed
the
kms-suite-auth
branch
2 times, most recently
from
0b99d30 to
31d729b
Compare
sandeepknd
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
sandeepknd
force-pushed
the
kms-suite-auth
branch
from
31d729b to
8b32248
Compare
sandeepknd
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sandeepknd: This pull request references CNTRLPLANE-3364 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sandeepknd: 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. |
|
job e2e-aws-operator-encryption-kms-ote got executed. Logs can be found here. It has successfully executed the tests TestKMSEncryptionOnOff and estKMSEncryptionProvidersMigration . |
|
All tests have passed. |
sandeepknd
changed the title
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@sandeepknd: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
4 participants
add the kms suite and migrate the kms encryption tests to ote
Summary by CodeRabbit