Adding KAS-O helpers and assertions#2349
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr 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 |
|
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 KAS encryption test helpers for fixed Secrets, expands Secret and ConfigMap encryption assertions, and updates encryption provider migration tests to handle multiple scenarios with shared provider-application and migration-wait logic. ChangesKAS Encryption Test Support
Estimated code review effort: 4 (Complex) | ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ 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.
🧹 Nitpick comments (3)
test/library/encryption/helpers_kas.go (2)
51-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win10-minute timeout seems excessive for a single etcd
Get.A hung etcd call would block the test for up to 10 minutes before failing, slowing down CI feedback significantly. Consider a much shorter bound (e.g., 30s–1min) consistent with a simple point read.
🤖 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/helpers_kas.go` around lines 51 - 62, The timeout in GetRawSecretOfLife is too long for a single etcd point read and can slow tests unnecessarily; reduce the context.WithTimeout duration in GetRawSecretOfLife to a much shorter bound (around 30s–1min) while keeping the existing clientSet.Etcd.Get and require checks unchanged.
19-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a timeout-bound context instead of
context.TODO().Get/Delete/Create calls here use
context.TODO()with no timeout, unlikeGetRawSecretOfLifein the same file which properly bounds its etcd call withcontext.WithTimeout. If the API server hangs, this helper can block indefinitely instead of failing the test with a clear timeout.🔧 Suggested fix
func CreateAndStoreSecretOfLife(t testing.TB, clientSet ClientSet, namespace string) runtime.Object { t.Helper() - ctx := context.TODO() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel()As per path instructions, Go files should use "context.Context for cancellation and timeouts."
🤖 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/helpers_kas.go` around lines 19 - 37, CreateAndStoreSecretOfLife currently uses context.TODO() for Secrets Get/Delete/Create, which leaves the helper without cancellation or a timeout. Update this helper to use a timeout-bound context like the nearby GetRawSecretOfLife logic, and make sure the same bounded context is passed through the clientSet.Kube.CoreV1().Secrets(namespace) operations so the test fails cleanly instead of hanging if the API server stalls.Source: Path instructions
test/library/encryption/scenarios.go (1)
194-197: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueProviders are read only from
scenarios[0]; other scenarios' lists are ignored.The doc comment states all scenarios must share the same
EncryptionProviders, but the code derivesproviderssolely fromscenarios[0]and never validates the rest. A caller passing a divergent list onscenarios[1..]gets silently wrong behavior rather than a clear failure. Consider asserting equality across scenarios up front.🤖 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 194 - 197, The provider list in ProvidersMigrationScenario is taken only from scenarios[0], so divergent EncryptionProviders on later scenarios are ignored. Update ProvidersMigrationScenario to validate that every scenario in the scenarios slice has the same EncryptionProviders before using the shared providers value, and fail fast with a clear test error if any mismatch is found. Use the existing ProvidersMigrationScenario logic and EncryptionProviders field to locate the check and add the equality assertion up front.
🤖 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/library/encryption/helpers_kas.go`:
- Around line 51-62: The timeout in GetRawSecretOfLife is too long for a single
etcd point read and can slow tests unnecessarily; reduce the context.WithTimeout
duration in GetRawSecretOfLife to a much shorter bound (around 30s–1min) while
keeping the existing clientSet.Etcd.Get and require checks unchanged.
- Around line 19-37: CreateAndStoreSecretOfLife currently uses context.TODO()
for Secrets Get/Delete/Create, which leaves the helper without cancellation or a
timeout. Update this helper to use a timeout-bound context like the nearby
GetRawSecretOfLife logic, and make sure the same bounded context is passed
through the clientSet.Kube.CoreV1().Secrets(namespace) operations so the test
fails cleanly instead of hanging if the API server stalls.
In `@test/library/encryption/scenarios.go`:
- Around line 194-197: The provider list in ProvidersMigrationScenario is taken
only from scenarios[0], so divergent EncryptionProviders on later scenarios are
ignored. Update ProvidersMigrationScenario to validate that every scenario in
the scenarios slice has the same EncryptionProviders before using the shared
providers value, and fail fast with a clear test error if any mismatch is found.
Use the existing ProvidersMigrationScenario logic and EncryptionProviders field
to locate the check and add the equality assertion up front.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 68426789-1623-4f79-b81e-05c7a1fd510f
📒 Files selected for processing (3)
test/library/encryption/assertion_kas.gotest/library/encryption/helpers_kas.gotest/library/encryption/scenarios.go
|
ok, could you leave a note on the functions that were copied? Also, could you paste a link to the code we copied them from? It would make the review easier for me. also, before we merge this PR, could we open a test PR to |
|
|
could you leave a note on the functions that were copied? Also, could you paste a link to the code we copied them from? It would make the review easier for me. |
added |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/library/encryption/helpers_kas.go (1)
22-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a bounded context instead of
context.TODO().
GetRawSecretOfLifein this same file uses a 10-minutecontext.WithTimeout, butCreateAndStoreSecretOfLifeusescontext.TODO()for its Get/Delete/Create calls, leaving no cancellation/timeout if the API server hangs.As per path instructions, "context.Context for cancellation and timeouts" should be used.
♻️ Proposed fix
t.Helper() - ctx := context.TODO() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel()🤖 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/helpers_kas.go` around lines 22 - 24, CreateAndStoreSecretOfLife currently uses context.TODO() for the Get/Delete/Create calls, so it has no cancellation or timeout. Update this helper to use a bounded context like GetRawSecretOfLife does, preferably with context.WithTimeout and a sensible duration, and make sure the resulting context is passed through the same client calls in CreateAndStoreSecretOfLife.Source: Path instructions
🤖 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/library/encryption/helpers_kas.go`:
- Around line 22-24: CreateAndStoreSecretOfLife currently uses context.TODO()
for the Get/Delete/Create calls, so it has no cancellation or timeout. Update
this helper to use a bounded context like GetRawSecretOfLife does, preferably
with context.WithTimeout and a sensible duration, and make sure the resulting
context is passed through the same client calls in CreateAndStoreSecretOfLife.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9371315-c7e1-4367-8947-9a7a5b87e6eb
📒 Files selected for processing (3)
test/library/encryption/assertion_kas.gotest/library/encryption/helpers_kas.gotest/library/encryption/scenarios.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/library/encryption/assertion_kas.go
- test/library/encryption/scenarios.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/library/encryption/helpers_kas.go (2)
22-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a bounded context instead of
context.TODO().
GetRawSecretOfLifeusescontext.WithTimeout(Line 56), but this function uses unboundedcontext.TODO()for the Get/Delete/Create calls. If the API server hangs, this test helper will block indefinitely instead of failing with a clear timeout. As per path instructions, "context.Context for cancellation and timeouts" should be used consistently.♻️ Suggested fix
func CreateAndStoreSecretOfLife(t testing.TB, clientSet ClientSet, namespace string) runtime.Object { t.Helper() - ctx := context.TODO() + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) + defer cancel()🤖 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/helpers_kas.go` around lines 22 - 40, CreateAndStoreSecretOfLife currently uses context.TODO() for Secrets Get/Delete/Create, which leaves these test helper calls unbounded. Update the helper to use a bounded context, ideally following the same pattern as GetRawSecretOfLife with context.WithTimeout, and pass that context into clientSet.Kube.CoreV1().Secrets(namespace) operations. Ensure the context is canceled/deferred properly so the helper fails fast instead of hanging if the API server stalls.Source: Path instructions
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nil check on
oldSecret.
clientSet.Kube.CoreV1().Secrets(namespace).Gettypically returns a non-nil zero-value object alongside aNotFounderror, sooldSecret != nilis effectively always true; the actual existence check islen(oldSecret.Name) > 0. Not a functional bug since the length check still guards correctly, but the nil check is misleading.🤖 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/helpers_kas.go` around lines 26 - 33, The existence check in the secret cleanup path is redundant because `Secrets(namespace).Get` already returns a non-nil object, so `oldSecret != nil` adds noise and can mislead readers. In `helpers_kas.go`, simplify the conditional around `oldSecret` in the secret cleanup logic to rely on the actual presence check already used (`len(oldSecret.Name) > 0`) and keep the delete flow in `clientSet.Kube.CoreV1().Secrets(namespace)` unchanged.
🤖 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/library/encryption/helpers_kas.go`:
- Around line 22-40: CreateAndStoreSecretOfLife currently uses context.TODO()
for Secrets Get/Delete/Create, which leaves these test helper calls unbounded.
Update the helper to use a bounded context, ideally following the same pattern
as GetRawSecretOfLife with context.WithTimeout, and pass that context into
clientSet.Kube.CoreV1().Secrets(namespace) operations. Ensure the context is
canceled/deferred properly so the helper fails fast instead of hanging if the
API server stalls.
- Around line 26-33: The existence check in the secret cleanup path is redundant
because `Secrets(namespace).Get` already returns a non-nil object, so `oldSecret
!= nil` adds noise and can mislead readers. In `helpers_kas.go`, simplify the
conditional around `oldSecret` in the secret cleanup logic to rely on the actual
presence check already used (`len(oldSecret.Name) > 0`) and keep the delete flow
in `clientSet.Kube.CoreV1().Secrets(namespace)` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66644bd0-4252-493f-a769-95d8cab3cc67
📒 Files selected for processing (3)
test/library/encryption/assertion_kas.gotest/library/encryption/helpers_kas.gotest/library/encryption/scenarios.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/library/encryption/assertion_kas.go
- test/library/encryption/scenarios.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/library/encryption/helpers_kas.go (1)
41-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning
*corev1.Secretdirectly.
SecretOfLifereturnsruntime.Objectand its only caller immediately type-asserts back to*corev1.Secret(line 36). Returning the concrete type avoids an unchecked assertion and is simpler for a helper with a single concrete construction.♻️ Proposed simplification
-func SecretOfLife(_ testing.TB, namespace string) runtime.Object { +func SecretOfLife(_ testing.TB, namespace string) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretOfLifeName, Namespace: namespace, }, Data: map[string][]byte{ "quote": []byte("I have no special talents. I am only passionately curious"), }, } }- secret, err := clientSet.Kube.CoreV1().Secrets(namespace).Create(ctx, rawSecret.(*corev1.Secret), metav1.CreateOptions{}) + secret, err := clientSet.Kube.CoreV1().Secrets(namespace).Create(ctx, rawSecret, metav1.CreateOptions{})Note: since this is copied from upstream KAS-O, changing the signature diverges from the source; weigh against keeping parity for easier future syncs.
🤖 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/helpers_kas.go` around lines 41 - 51, SecretOfLife currently returns runtime.Object even though it always constructs a *corev1.Secret, and its only caller type-asserts it back immediately. Update SecretOfLife in helpers_kas.go to return *corev1.Secret directly and adjust the caller to use the concrete type without an assertion. Keep the helper’s construction logic the same, and consider the upstream parity note before changing the signature.
🤖 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/library/encryption/helpers_kas.go`:
- Around line 41-51: SecretOfLife currently returns runtime.Object even though
it always constructs a *corev1.Secret, and its only caller type-asserts it back
immediately. Update SecretOfLife in helpers_kas.go to return *corev1.Secret
directly and adjust the caller to use the concrete type without an assertion.
Keep the helper’s construction logic the same, and consider the upstream parity
note before changing the signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf6ffb0f-31c5-4ba0-9701-eea71ee95eb4
📒 Files selected for processing (3)
test/library/encryption/assertion_kas.gotest/library/encryption/helpers_kas.gotest/library/encryption/scenarios.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/library/encryption/assertion_kas.go
- test/library/encryption/scenarios.go
gangwgr
force-pushed
the
update-helpers
branch
2 times, most recently
from
ad67a63 to
9dd8513
Compare
| if multi { | ||
| stepName = fmt.Sprintf("%s[%s]", stepName, scenario.ResourceName) | ||
| } | ||
| stepName := fmt.Sprintf("%s%s", prefix, strings.ToUpper(string(provider.Type))) |
There was a problem hiding this comment.
I think that we should run each scenario in parallel.
| if multi { | ||
| stepName = fmt.Sprintf("%s[%s]", stepName, scenario.ResourceName) | ||
| } | ||
| stepName := fmt.Sprintf("%s%s", prefix, strings.ToUpper(string(provider.Type))) |
There was a problem hiding this comment.
We will run into issues with updating the API when running in parallel. We have a few options here.
- we retry on a conflict
- only one scenario can set a provider, the remaining use the same provider and we also do 1
- ?
There was a problem hiding this comment.
Flow in runMigrationStepMultiOperator:
1.Snapshot GetLastKeyMeta for each operator
2. Single APIServer update (+ setup once)
3.Parallel wait/assert per scenario (KAS, Auth, OAS)
|
@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. |
| {Group: "", Resource: "configmaps"}, | ||
| } | ||
|
|
||
| func AssertSecretOfLifeEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { |
There was a problem hiding this comment.
let's open a new PR and move this function to the existing assertion.go file.
also I think we could rename the function to AssertWellKnownSecretOfLifeEncrypted(...)
There was a problem hiding this comment.
can we merge first this pr after your PR merged
then I will update your renaming suggestions in new pr what you think?
| } | ||
| } | ||
|
|
||
| func AssertSecretOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { |
There was a problem hiding this comment.
AssertWellKnownSecretOfLifeNotEncrypted
| } | ||
| } | ||
|
|
||
| func AssertSecretsAndConfigMaps(t testing.TB, clientSet ClientSet, expectedMode configv1.EncryptionType, namespace, labelSelector string) { |
There was a problem hiding this comment.
AssertWellKnownSecretsAndConfigMaps
| AssertLastMigratedKey(t, clientSet.Kube, DefaultTargetGRs, namespace, labelSelector) | ||
| } | ||
|
|
||
| func assertSecrets(t testing.TB, etcdClient EtcdClient, expectedMode string) { |
There was a problem hiding this comment.
this can be moved as is.
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func assertConfigMaps(t testing.TB, etcdClient EtcdClient, expectedMode string) { |
There was a problem hiding this comment.
this can be moved as is.
| configv1 "github.com/openshift/api/config/v1" | ||
| ) | ||
|
|
||
| var DefaultTargetGRs = []schema.GroupResource{ |
|
|
||
| const secretOfLifeName = "secret-of-life" | ||
|
|
||
| func CreateAndStoreSecretOfLife(t testing.TB, clientSet ClientSet, namespace string) runtime.Object { |
There was a problem hiding this comment.
let's open a new PR and move this function to the existing helpers.go file.
let's also rename the function to CreateAndStoreWellKnownSecretOfLife
| return secret | ||
| } | ||
|
|
||
| func SecretOfLife(_ testing.TB, namespace string) runtime.Object { |
| } | ||
| } | ||
|
|
||
| func GetRawSecretOfLife(t testing.TB, clientSet ClientSet, namespace string) string { |
There was a problem hiding this comment.
GetRawWellKnownSecretOfLife
2 participants
Adding KAS-O helpers and assertions
Summary by CodeRabbit