OCPBUGS-85280: encryption: detect orphaned namespace objects during key migration - #2364
Conversation
When a namespace is fully deleted from etcd but child objects remain, the NamespaceLifecycle admission plugin rejects Update requests with NotFound. The in-process migrator previously treated all NotFound errors as "object deleted, safe to skip," causing migration to report success. The encryption state machine then pruned the old encryption key, making the orphaned objects permanently undecryptable and crashing the kube-apiserver. After receiving NotFound on Update, the migrator now issues a GET (which bypasses admission and reads directly from etcd storage) to check whether the object still exists. If it does, the object is an orphan in a deleted namespace that cannot be re-encrypted. The migration is failed with an actionable error message, preventing the state machine from pruning the old encryption key.
openshift-ci-robot
added
jira/valid-reference
|
@sanchezl: This pull request references Jira Issue OCPBUGS-85280, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughThe in-process encryption migrator now verifies objects after ChangesOrphaned namespace migration handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InProcessMigrator
participant DynamicClient
participant ObjectStorage
InProcessMigrator->>DynamicClient: Update encrypted object
DynamicClient-->>InProcessMigrator: NotFound error
InProcessMigrator->>DynamicClient: Get encrypted object
DynamicClient->>ObjectStorage: Read object
ObjectStorage-->>DynamicClient: Object or NotFound
DynamicClient-->>InProcessMigrator: Existence result
InProcessMigrator-->>InProcessMigrator: Block migration or ignore error
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sanchezl 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/migrators/inprocess_test.go (1)
156-171: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen assertions to verify the specific branch, and add a case for the "verification failed" path.
Both subtests only check
result == nilvsresult != nil(lines 234-242), not the actual error content. This means the "orphaned object" case would also pass if some unrelated bug madeGetitself error out (hitting the "failed to verify existence" branch ininprocess.goat lines 154-157) rather than the intended "object confirmed to exist" branch (lines 158-161) — the test can't distinguish which of the two error paths was actually exercised. Consider asserting on error content (e.g.,strings.Containsfor the KCS reference or "blocking migration") and adding a third case where theGetreactor itself returns a non-NotFound error, to cover that branch too.♻️ Example assertion tightening
if tc.expectError { if result == nil { return false, fmt.Errorf("expected migration to fail for orphaned namespace object, but it succeeded") } + if !strings.Contains(result.Error(), "6769801") { + return false, fmt.Errorf("expected orphaned-namespace error, got: %v", result) + } t.Logf("migration correctly failed: %v", result)Also applies to: 226-244
🤖 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 `@pkg/operator/encryption/controllers/migrators/inprocess_test.go` around lines 156 - 171, Strengthen the assertions in the migration test cases around the existing result checks by verifying the expected error content for the orphaned-object branch, such as the KCS reference or “blocking migration,” while preserving the successful deleted-object case. Extend the test table and Get reactor setup to include a non-NotFound Get error, then assert that this case exercises the verification-failed path with its specific error.
🤖 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 `@pkg/operator/encryption/controllers/migrators/inprocess_test.go`:
- Around line 156-171: Strengthen the assertions in the migration test cases
around the existing result checks by verifying the expected error content for
the orphaned-object branch, such as the KCS reference or “blocking migration,”
while preserving the successful deleted-object case. Extend the test table and
Get reactor setup to include a non-NotFound Get error, then assert that this
case exercises the verification-failed path with its specific error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bf45d2f0-1a84-483a-8c15-f097774774fc
📒 Files selected for processing (2)
pkg/operator/encryption/controllers/migrators/inprocess.gopkg/operator/encryption/controllers/migrators/inprocess_test.go
|
@sanchezl: 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 looks good to me but I think it is better that @p0lyn0mial also looks at this PR |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sanchezl: This pull request references Jira Issue OCPBUGS-85280, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@sanchezl: This pull request references Jira Issue OCPBUGS-85280, which is valid. 3 validation(s) were run on this bug
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. |
3 participants
Summary
When a namespace is fully deleted from etcd but child objects remain (orphans), the NamespaceLifecycle admission plugin rejects Update requests with NotFound. The in-process encryption key migrator previously treated all NotFound errors as "object deleted, safe to skip," causing migration to report success. The encryption state machine then pruned the old encryption key, making the orphaned objects permanently undecryptable and crashing the kube-apiserver with
no matching key was found for the provided AES transformer.This change adds a GET probe after NotFound on Update. GET bypasses admission and reads directly from etcd storage. If the object still exists, it is an orphan that cannot be re-encrypted. The migration is failed with an actionable error message pointing to KCS 6769801, preventing the state machine from pruning the old encryption key.
Details
kube-storage-version-migratorand the in-tree KCM migrator (KEP-4192) have the same NotFound handling patternRelated
kube-storage-version-migrator(used by operators in production): OCPBUGS-85280: Detect orphaned namespace objects during storage version migration kubernetes-kube-storage-version-migrator#246Test plan
TestInProcessMigratorOrphanedNamespace/orphaned_object_in_deleted_namespace_blocks_migrationverifies migration fails when Update returns NotFound but GET succeeds (orphaned object)TestInProcessMigratorOrphanedNamespace/genuinely_deleted_object_is_skippedverifies existing behavior is preserved when both Update and GET return NotFound (object truly deleted)TestInProcessMigratorpasses unchanged (no regression)go test ./pkg/operator/encryption/...passes (14 packages, 0 failures)/verified by "TestInProcessMigratorOrphanedNamespace"