fix: use prefixed AppProject name while handling resyncs #666
Conversation
Assisted-by: Cursor Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
chetan-rns
requested review from
jannfis,
jgwest and
mikeshng
as code owners
WalkthroughExtract agent name from JSON-encoded remote ClientID and pass agentName into resync handlers; update principal callbacks and event processing to handle SourceNamespaces for autonomous agents; add E2E tests covering AppProject resync scenarios across principal/agent restarts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)test/e2e/resync_test.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chetan-rns
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/event.go (1)
586-607: Critical: Prefixing logic is unreachable due to conflicting mode check.The condition at line 587 returns an error if
agentMode != types.AgentModeManaged, which means only managed mode requests proceed past that point. However, line 599 checks foragentMode == types.AgentModeAutonomous, which will never be true since autonomous requests are rejected at line 588.This defeats the purpose of the fix for issue #660 - the prefixing logic for autonomous agents will never execute.
The mode check at line 587 should likely allow autonomous mode for
EventRequestUpdateevents, or the prefixing block should be relocated before the mode check:case event.EventRequestUpdate: - if agentMode != types.AgentModeManaged { - return fmt.Errorf("principal can only handle request update in the managed mode") - } - incoming := &event.RequestUpdate{} if err := ev.DataAs(incoming); err != nil { return err } // For autonomous agents, the agent sends RequestUpdate with the local AppProject name (e.g., "sample"), // but the principal stores it with a prefixed name (e.g., "agent-autonomous-sample"). // We need to add the prefix before looking it up locally. if agentMode == types.AgentModeAutonomous && incoming.Kind == "AppProject" { prefixedName, err := agentPrefixedProjectName(incoming.Name, agentName) if err != nil { return fmt.Errorf("could not prefix project name: %w", err) } incoming.Name = prefixedName } return resyncHandler.ProcessRequestUpdateEvent(ctx, agentName, incoming)
🧹 Nitpick comments (2)
agent/inbound_test.go (1)
1473-1482: Remove debug print statement.Line 1481 contains a debug print statement that should be removed before merging.
subjectJSON, err := json.Marshal(subject) if err != nil { t.Fatalf("Failed to marshal subject: %v", err) } a.remote.SetClientID(string(subjectJSON)) - fmt.Println("a.remote.ClientID()", a.remote.ClientID()) err = a.queues.Create(a.remote.ClientID())agent/inbound.go (1)
432-440: Consider usingstrings.TrimPrefixfor cleaner prefix stripping.The manual prefix stripping logic is correct but could be simplified using
strings.TrimPrefix.+ "strings" ... // For autonomous agents, the principal stores AppProjects with a prefixed name (agent-name + "-" + project-name). // When the principal sends a RequestUpdate, it uses the prefixed name. We need to strip the prefix // before looking up the resource locally. if incoming.Kind == "AppProject" { prefix := agentName + "-" - if len(incoming.Name) > len(prefix) && incoming.Name[:len(prefix)] == prefix { - incoming.Name = incoming.Name[len(prefix):] - } + incoming.Name = strings.TrimPrefix(incoming.Name, prefix) }Note:
strings.TrimPrefixreturns the original string unchanged if the prefix is not present, so the behavior is equivalent but cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
agent/inbound.go(5 hunks)agent/inbound_test.go(3 hunks)internal/resync/resync.go(1 hunks)principal/callbacks.go(2 hunks)principal/event.go(1 hunks)test/e2e/resync_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/resync/resync.go (2)
internal/logging/logfields/logfields.go (2)
Kind(58-58)Name(59-59)internal/logging/logging.go (1)
Trace(285-287)
agent/inbound_test.go (1)
internal/auth/interface.go (1)
AuthSubject(19-22)
principal/event.go (2)
pkg/types/types.go (1)
AgentModeAutonomous(31-31)internal/logging/logfields/logfields.go (2)
Kind(58-58)Name(59-59)
principal/callbacks.go (2)
internal/resources/resources.go (1)
NewResourceKeyFromAppProject(62-71)internal/backend/interface.go (1)
Namespace(124-127)
test/e2e/resync_test.go (3)
internal/backend/interface.go (2)
Namespace(124-127)AppProject(98-108)test/e2e/fixture/goreman.go (3)
StopProcess(10-13)IsProcessRunning(23-54)StartProcess(16-19)test/e2e/fixture/toxyproxy.go (1)
CheckReadiness(109-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run unit tests
- GitHub Check: Run end-to-end tests
- GitHub Check: Lint Go code
- GitHub Check: Build & cache Go code
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/resync/resync.go (1)
159-159: LGTM!The trace log addition is consistent with the existing logging pattern at line 198 and properly uses the
logfieldsconstants.principal/callbacks.go (2)
183-193: LGTM!The resource key handling correctly differentiates between autonomous and non-autonomous agents. For autonomous agents, using
SourceNamespaces[0]as the agent name aligns with how the principal sets this field inprocessAppProjectEvent, ensuring proper resource tracking for resync operations.
302-312: LGTM!The deletion callback mirrors the creation callback logic, properly removing resource keys under the correct agent name for autonomous agents via
SourceNamespaces[0].agent/inbound.go (1)
391-397: LGTM on agent name extraction.The JSON unmarshalling of
AuthSubjectcorrectly extracts the agent name from the client ID. Error handling properly propagates failures.test/e2e/resync_test.go (2)
831-878: LGTM on autonomous AppProject resync tests.The test correctly validates that AppProjects are retained on both clusters after principal restart. The expected spec transformation logic properly accounts for the autonomous-specific modifications (SourceNamespaces set to agent name, destinations pointing to agent cluster).
1380-1403: LGTM on createAutonomousAppProject helper.The helper correctly creates an AppProject on the autonomous agent cluster and waits for it to propagate to the principal with the expected prefixed name.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
==========================================
- Coverage 45.62% 45.56% -0.06%
==========================================
Files 90 90
Lines 9991 10011 +20
==========================================
+ Hits 4558 4562 +4
- Misses 4965 4978 +13
- Partials 468 471 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Chetan Banavikalmutt <[email protected]>
…bs#666) Signed-off-by: Chetan Banavikalmutt <[email protected]>
…bs#666) Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
3 participants
What does this PR do / why we need it:
Currently, the AppProjects coming from the autonomous agents are prefixed with the agent name to avoid duplicate resource names. When the agent/principal restarts, they exchange certain resync events to sync both the components. The principal needs to ensure that it is looking for the prefixed names when looking for resources locally.
Which issue(s) this PR fixes:
Fixes #660
How to test changes / Special notes to the reviewer:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.