feat: support destination based agent mapping #726
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:
📝 WalkthroughWalkthroughAdds destination-based mapping allowing applications to be routed by spec.destination.name instead of agent namespace. Introduces agent/principal flags and options, propagates mapping through Redis proxy, resync, callbacks, ApplicationManager/AppProject, adds optional namespace creation, CLI flags, docs, unit and E2E tests, and RBAC/chart updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Principal
participant RedisProxy
participant Agent
participant AppManager
Principal->>Principal: receive App create/update (has spec.destination.name)
Principal->>Principal: determine agentName (destinationBasedMapping? use destination.name : use namespace)
Principal->>RedisProxy: publish event keyed by agentName
RedisProxy->>Agent: forward pub/sub to agent queue (agentName)
Agent->>Agent: resolve target namespace (getTargetNamespaceForApp)
Agent->>AppManager: create/update Application in resolved namespace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
==========================================
+ Coverage 46.18% 46.48% +0.30%
==========================================
Files 101 101
Lines 12513 12709 +196
==========================================
+ Hits 5779 5908 +129
- Misses 6179 6233 +54
- Partials 555 568 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chetan-rns
force-pushed
the
destination-mapping
branch
2 times, most recently
from
24f36df to
5137c23
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/callbacks.go (1)
100-144:⚠️ Potential issue | 🟠 MajorGuard against empty agent name on updates.
When
spec.destination.nameis empty,updateAppCallbackwill create/route to a queue keyed by"", while create/delete already short‑circuit. Add the same guard here to avoid misrouting and silent queue creation.🛠️ Suggested fix
- agentName := s.getAgentNameForApp(new) - - logCtx := log().WithFields(logrus.Fields{ + agentName := s.getAgentNameForApp(new) + + logCtx := log().WithFields(logrus.Fields{ "component": "EventCallback", "queue": agentName, "event": "application_update", "application_name": old.Name, }) + if agentName == "" { + logCtx.Error("Failed to get agent name for application") + return + }
🤖 Fix all issues with AI agents
In `@agent/inbound.go`:
- Around line 910-941: The agent currently may create namespaces without
checking the allowlist; before calling ensureNamespaceExists() in
createApplication() and updateApplication() add a validation step that, when
a.createNamespace and a.destinationBasedMapping are enabled, checks the target
namespace against the agent's allowedNamespaces (or equivalent allowlist) and
rejects the request with an error if the namespace is not permitted; ensure this
check runs prior to any call to ensureNamespaceExists() (and returns a clear,
early error) so disallowed namespaces are never passed to
ensureNamespaceExists() or created.
In `@docs/concepts/agent-mapping.md`:
- Around line 99-102: Hyphenate the compound modifiers in the prose: change
"destination based mapping" to "destination-based mapping" and "same namespace
as the principal" context if present change "namespace based" to
"namespace-based" for consistency; update the sentence in the Create Namespace
note (and the other occurrence referenced) to use "destination-based" and
"namespace-based" where appropriate so it matches the rest of the document and
grammar checks.
- Around line 9-16: The Markdown table in docs/concepts/agent-mapping.md
violates MD060 due to inconsistent column pipe spacing; fix it by aligning pipes
and cell padding so each row uses the same number of spaces around pipe
separators (ensure header, separator, and all rows match), e.g., adjust the
header row "| Feature | Namespace-Based Mapping | Destination-Based Mapping |",
the separator row
"|---------|------------------------|---------------------------|", and each
subsequent row so columns are vertically aligned and consistent across the
table.
In `@principal/redisproxy/redisproxy.go`:
- Around line 826-835: The current fallback in the agent resolution always
returns namespace even when an agentLookupFn is configured but returns no match;
change the behavior in the branch that checks rp.agentLookupFn so that if
rp.agentLookupFn(namespace, appName) returns an empty string you return "" (and
nil error) instead of falling through to return namespace, or alternatively
update the comment/contract to explicitly state that a configured agentLookupFn
will cause an empty result to be treated as “no route” and not fall back to
namespace; locate the logic using rp.agentLookupFn, the variables namespace and
appName, and the final return of namespace to implement this change.
In `@test/e2e/dest_mapping_test.go`:
- Around line 51-53: The TearDownSuite currently resets SourceNamespaces to "*"
and deletes the ConfigMap key without restoring prior state; update
DestinationMappingTestSuite to capture the original config in SetupSuite (add
fields on DestinationMappingTestSuite like origSourceNamespaces and
origConfigMapData), read and save the existing SourceNamespaces/ConfigMap values
there, and then in TearDownSuite restore those originals (write back the saved
SourceNamespaces or remove the key if it was absent) using the same ConfigMap
update functions; ensure error handling around read/write operations in
SetupSuite and TearDownSuite so tests don’t leak state.
🧹 Nitpick comments (1)
principal/redisproxy/redisproxy.go (1)
400-405: Avoid re-deriving agentName inside handleAgentGet.
The caller already resolved it; re-parsing can diverge or do extra work.Proposed diff
- agentName, err := rp.extractAgentNameFromRedisCommandKey(key, logCtx) - if err != nil { - logCtx.WithError(err).Error("unable to get agent name: " + key) - return err - } - logCtx.Trace("sending get command to agent")
chetan-rns
force-pushed
the
destination-mapping
branch
from
77cc813 to
3e6877d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
principal/callbacks.go (1)
100-147:⚠️ Potential issue | 🟠 MajorGuard against empty
destination.namein update flow.
When destination-based mapping is enabled anddestination.namebecomes empty,agentNameis empty but the update path still creates a queue and dispatches events to the empty name. This can misroute updates and create a phantom queue.Consider returning early after handling agent change if
agentNameis empty.🛠️ Suggested fix
if s.destinationBasedMapping { s.handleAppAgentChange(ctx, old, new, logCtx) } +if agentName == "" { + logCtx.Warn("Destination name is empty; skipping update dispatch") + return +}
🤖 Fix all issues with AI agents
In `@agent/inbound.go`:
- Around line 525-534: When destination-based mapping is enabled
(a.destinationBasedMapping) enforce the namespace allowlist by calling
a.isNamespaceAllowed(targetNamespace) regardless of a.createNamespace; only gate
actual namespace creation on a.createNamespace by calling
a.ensureNamespaceExists(targetNamespace) when createNamespace is true. Update
the conditional in the inbound handling code (the block using a.createNamespace,
a.destinationBasedMapping, a.isNamespaceAllowed and a.ensureNamespaceExists) so
that isNamespaceAllowed runs whenever destination-based mapping is true and
ensureNamespaceExists runs only if createNamespace is true; apply the same
change to the other similar block that uses these same symbols.
In `@docs/concepts/agent-mapping.md`:
- Around line 103-109: Wrap the indented code block inside the "Allowed
Namespaces" admonition with markdownlint disable/enable comments for MD046 so
the linter ignores the indented-block rule only for that section; specifically,
insert the markdownlint HTML comment directive to disable MD046 immediately
before the indented code block and re-enable it immediately after, targeting the
admonition titled "Allowed Namespaces" in docs/concepts/agent-mapping.md.
In `@principal/callbacks.go`:
- Around line 817-826: The GetAgentForApp implementation contradicts its
comment: when s.destinationBasedMapping is true but s.appToAgent is nil it
currently returns the namespace, which silently falls back to namespace-based
routing; update GetAgentForApp so that when s.destinationBasedMapping (or
destination-based mapping is enabled) and s.appToAgent == nil it returns an
empty string instead of namespace, otherwise keep building qualifiedName :=
fmt.Sprintf("%s/%s", namespace, name) and returning
s.appToAgent.Get(qualifiedName).
In `@test/e2e/dest_mapping_test.go`:
- Around line 484-490: The call to fixture.EnsureUpdate that appends a finalizer
to the Deployment is missing an error check so failures are ignored; update the
code around EnsureUpdate (the call that uses fixture.EnsureUpdate(suite.Ctx,
suite.ManagedAgentClient, depl, func(obj fixture.KubeObject) { ... })) to
capture its returned error into a variable and immediately assert/handle it
(e.g., require.NoError or t.Fatalf) so the test fails if the finalizer update
did not succeed, and also check the earlier err variable after modifying
depl.Finalizers to ensure both update attempts are validated.
- Around line 269-292: The deferred cleanup unconditionally deletes the test
namespace(s); change the logic to track whether each namespace was actually
created and only call fixture.EnsureDeletion when the test created it.
Concretely, add a bool flag (e.g., principalNsCreated) set true when
suite.PrincipalClient.Create(ns) succeeds (and false if errors.IsAlreadyExists),
use that flag in the first defer to skip deletion when false; do the same for
the agent-side namespace (agentNs and a separate agentNsCreated) in the second
defer that currently calls fixture.EnsureDeletion on agentNs, ensuring you check
the corresponding creation result before attempting deletion.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent/inbound.go (1)
622-653:⚠️ Potential issue | 🟠 MajorUse the target namespace when deleting applications.
deleteApplicationsets the target namespace on the app, buta.appManager.Deletestill usesa.namespace. In destination-based mapping this can delete the wrong namespace or fail to delete at all. Use the app’s resolved namespace instead.🔧 Proposed fix
- err = a.appManager.Delete(a.context, a.namespace, app, &deletionPropagation) + err = a.appManager.Delete(a.context, app.Namespace, app, &deletionPropagation)principal/callbacks.go (1)
100-147:⚠️ Potential issue | 🟠 MajorGuard against empty
agentNamebefore queue operations.
Ifspec.destination.nameis empty, this can create/lookup a queue with an empty key and misroute updates. Add the same early return used in create/delete.🔧 Proposed fix
agentName := s.getAgentNameForApp(new) + if agentName == "" { + logCtx.Error("Failed to get agent name for application") + return + } logCtx := log().WithFields(logrus.Fields{ "component": "EventCallback", "queue": agentName,
🤖 Fix all issues with AI agents
In `@principal/callbacks.go`:
- Around line 782-865: The handler handleAppAgentChange should early-return for
autonomous resources to avoid routing/deleting transient autonomous updates; add
a guard at the top of handleAppAgentChange that checks
isResourceFromAutonomousAgent(old) || isResourceFromAutonomousAgent(new) and
returns immediately if true. This will prevent calls to untrackAppToAgent,
trackAppToAgent, resources.Remove/resources.Add and enqueueing delete events on
queues for autonomous apps.
In `@test/e2e/dest_mapping_test.go`:
- Around line 146-148: In DestinationMappingTestSuite.TearDownSuite() remove the
incorrect call to suite.BaseSuite.TearDownTest() (per-test teardown) — delete
that line and keep only suite-level cleanup code (e.g., retains suite.Require()
usage or add any suite-level teardown logic here); do not attempt to call a
non-existent BaseSuite.TearDownSuite().
🧹 Nitpick comments (1)
test/e2e/dest_mapping_test.go (1)
484-489: Drop the redundant pre-append of the finalizer.
The directdepl.Finalizersappend is overwritten byEnsureUpdate’s internal GET, so it doesn’t affect the update.♻️ Simplify finalizer update
- depl.Finalizers = append(depl.Finalizers, "test-e2e/my-test-finalizer") - err = fixture.EnsureUpdate(suite.Ctx, suite.ManagedAgentClient, depl, func(obj fixture.KubeObject) { + err = fixture.EnsureUpdate(suite.Ctx, suite.ManagedAgentClient, depl, func(obj fixture.KubeObject) { depl := obj.(*appsv1.Deployment) depl.Finalizers = append(depl.Finalizers, "test-e2e/my-test-finalizer") })
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@principal/callbacks.go`:
- Around line 100-111: The update callback is missing a check that agentName
(from s.getAgentNameForApp(new)) is non-empty before using it; add the same
validation pattern used in newAppCallback and deleteAppCallback: after computing
agentName and before using it or calling s.handleAppAgentChange(ctx, old, new,
logCtx), verify agentName != "" and if empty log a warning/error on logCtx and
return early so destinationBasedMapping paths won't operate on an invalid queue
key (references: getAgentNameForApp, agentName, destinationBasedMapping,
handleAppAgentChange, newAppCallback, deleteAppCallback).
🧹 Nitpick comments (3)
principal/callbacks_test.go (3)
1284-1404: Clarify empty destination behavior in the test comment.Line 1394 says “fall back to namespace,” but the test asserts no processing. Align the comment with the expected behavior to avoid confusion.
💡 Suggested fix
- Name: "", // Empty - should fall back to namespace + Name: "", // Empty - should skip processing when destination-based mapping is enabled
1724-1898: Assert queue items directly to avoid ambiguity.Line 1760 uses
s.queues.Len(), which may reflect queue count rather than queued items. Consider asserting the send queue length so the test explicitly verifies the create event was queued.🔧 Suggested tweak
s.newAppCallback(oldApp) - assert.Equal(t, 1, s.queues.Len()) - drainQueue(t, s.queues.SendQ("target-cluster")) + preQ := s.queues.SendQ("target-cluster") + require.NotNil(t, preQ) + assert.Equal(t, 1, preQ.Len()) + drainQueue(t, preQ)
1901-2159: Pre-populate old resources to verify removal logic.Right now, the delete-side resource assertions can pass even if removal is broken because the old resources are never added. Seeding the old agent’s resources strengthens the test’s guarantee.
🧪 Suggested enhancement
// Create queues for agents involved if tt.expectedDeleteAgent != "" { s.queues.Create(tt.expectedDeleteAgent) } if tt.expectedAddAgent != "" { s.queues.Create(tt.expectedAddAgent) } + if tt.expectedDeleteAgent != "" { + s.resources.Add(tt.expectedDeleteAgent, resources.NewResourceKeyFromApp(tt.oldApp)) + } logCtx := logrus.WithField("test", "handleAppAgentChange")
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@principal/callbacks.go`:
- Around line 106-117: Compute agentName via s.getAgentNameForApp(new) as before
but do not early-return when agentName == ""; instead, still call
s.handleAppAgentChange(ctx, old, new, logCtx) when s.destinationBasedMapping is
true so cleanup runs even if destination.name became empty; keep the
logCtx.Error("Failed to get agent name for application") for visibility, and
only add logCtx = logCtx.WithField("queue", agentName) when agentName is
non-empty to avoid adding an empty field.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@principal/callbacks_test.go`:
- Around line 2224-2227: The assertion message is misleading: update the
NotContainsf message used with resources.NewResourceKeyFromApp(tt.oldApp) and
s.resources.Get(tt.expectedDeleteAgent).GetAll() (inside the
tt.expectedDeleteAgent != "" branch) to reflect that the resource should be
removed from the old agent (e.g. "Resource should be removed from old agent" or
similar) instead of "Resource should be added to new agent".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@principal/callbacks_test.go`:
- Around line 1977-2235: The test fails to seed state for the old agent so the
removal assertion is vacuous; before calling s.handleAppAgentChange add the app
resource to s.resources for tt.expectedDeleteAgent (use
resources.NewResourceKeyFromApp(tt.oldApp) and add that key to
s.resources.Get(tt.expectedDeleteAgent)) and set the app->agent mapping in
s.appToAgent to the old agent (use tt.oldApp.QualifiedName()) so the handler
actually removes something to validate removal behavior.
510fa4e to
c6e3c52
Compare
drewbailey
left a comment
drewbailey
left a comment
There was a problem hiding this comment.
Two small things that are probably worth calling out,
- The agent-clusterrole needs permissions to manage argoproj.io resources similar to the agent-role thats included in the install dir.
- Agent allowed namespaces is not currently wired up in the agent-deplyoment / cm
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)
install/kubernetes/agent/agent-clusterrole.yaml (1)
10-19:⚠️ Potential issue | 🟠 MajorGate cluster-wide namespace creation behind an optional binding.
Granting
namespaces/createat cluster scope is a high-privilege expansion. The agent uses this to auto-create namespaces (whenARGOCD_AGENT_CREATE_NAMESPACEis enabled), but this should not be part of the default install. Split this into a dedicated ClusterRole and bind it only when the feature flag is enabled.💡 Suggested split to keep default RBAC minimal
resources: - namespaces verbs: - - create - get - list - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: argocd-agent-agent-namespace-create +rules: +- apiGroups: + - "" + resources: + - namespaces + verbs: + - create
🤖 Fix all issues with AI agents
In `@install/kubernetes/agent/agent-clusterrole.yaml`:
- Around line 19-30: The ClusterRole granting verbs
(create,get,list,watch,update,delete,patch) on the argoproj.io resource
"applications" is currently cluster-wide; restrict it by converting this broad
ClusterRole binding into namespace-scoped RBAC (create a Role with the same
resources/verbs and bind it via RoleBinding only in each allowed namespace) or
keep the ClusterRole but remove/warn against cluster-scoped binding and instead
create RoleBindings conditionally per allowed namespace (or add a managed-mode
installation path that only creates RoleBindings for allowed-namespaces). Update
the manifest references to the "applications" resource and any
ClusterRoleBinding usage so permissions are applied per-namespace (or behind a
managed-mode/allowed-namespaces conditional) rather than cluster-wide.
🧹 Nitpick comments (1)
install/helm-repo/argocd-agent-agent/templates/agent-clusterrole.yaml (1)
15-30: Consider least‑privilege scoping for Application CRUD.This ClusterRole grants cluster‑wide create/update/delete/patch on Applications (and namespace create/get). If allowed‑namespaces or destination‑based mapping is intended to restrict scope, consider making this rule conditional or offering Role/RoleBinding alternatives per namespace to reduce blast radius.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/concepts/agent-mapping.md`:
- Around line 100-101: Change the phrasing "in the same namespace as the
principal" by replacing "on the same namespace" with "in the same namespace" in
the sentence that begins "With destination-based mapping" (also update the
follow-up sentence that mentions the `--create-namespace` flag if it uses "on"
there) so both occurrences use the standard "in the same namespace" wording.
- Line 64: Replace the namespace placeholder `{agentName}/` with `{agentName}`
in the "Destination-based mapping" sentence so it no longer implies a filesystem
path; update the text that currently reads "autonomous apps are always stored in
the `{agentName}/` namespace" to use `{agentName}` (remove the trailing slash)
to clarify it's a namespace name rather than a path.
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
Signed-off-by: Chetan Banavikalmutt <[email protected]>
chetan-rns
force-pushed
the
destination-mapping
branch
from
c5705d8 to
5b57823
Compare
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/redisproxy/redisproxy.go (1)
400-404:⚠️ Potential issue | 🟡 MinorAvoid re-resolving agent name inside
handleAgentGet.
A second lookup can return empty and still proceed to send to an invalid agent. Prefer the already-resolved parameter or guard against empty.🛠️ Proposed fix
- agentName, err := rp.extractAgentNameFromRedisCommandKey(key, logCtx) - if err != nil { - logCtx.WithError(err).Error("unable to get agent name: " + key) - return err - } + if agentName == "" { + logCtx.Error("empty agent name for key: " + key) + return fmt.Errorf("empty agent name for key: %s", key) + }
🤖 Fix all issues with AI agents
In `@docs/concepts/agent-mapping.md`:
- Line 191: Replace the inconsistent use of "ArgoCD" with the official two-word
form "Argo CD" in this document by locating and updating the string "ArgoCD"
(specifically the occurrences matching the phrase in the diff and at the
instances referenced around lines where "Apps-in-Any-Namespace" and other
mentions appear) so all mentions of the project use "Argo CD" consistently.
🧹 Nitpick comments (6)
install/helm-repo/argocd-agent-agent/templates/agent-clusterrole.yaml (1)
14-30: Consider gating the new permissions on their respective feature flags.The
createverb onnamespacesis only needed whencreateNamespaceis enabled, and theargoproj.io/applicationsrule is only needed for destination-based mapping. Since this is a Helm template, you could conditionally include these using{{- if .Values.createNamespace }}/{{- if .Values.destinationBasedMapping }}to follow least-privilege by default.That said, I see from prior discussion that the broad ClusterRole is intentional and users are expected to customize RBAC per their deployment. If that's the chosen approach, this is fine as-is — just noting the option for tighter defaults. Based on learnings, the ClusterRole is intentionally broad because allowed namespaces vary by mapping mode, and documentation guides users to use namespace-scoped RoleBindings.
test/e2e/redis_proxy_test.go (1)
113-113: Nit: TheresourceTreeURLdoes not URL-encodeappNamespace.If the namespace ever contains special characters (unlikely but possible with destination-based mapping enabling arbitrary namespaces), this could break the URL. Currently low risk given the test uses
"agent-managed".docs/concepts/agent-mapping.md (1)
195-207: AppProject example could be more specific.The placeholder
<namespaces>is vague. Consider showing a concrete example (e.g.,"*"or specific namespace names) to make it immediately actionable, especially since this is a required configuration for the feature.principal/callbacks_test.go (1)
1673-1673: Remove debug print from test output.
It adds noise to test logs without assertions.🧹 Proposed cleanup
- fmt.Println(app.Operation.String())agent/agent.go (1)
566-574: Consider using an explicit empty slice instead of nil for clarity.When
destinationBasedMappingis true,namespacesremainsnil. While this likely works (nil slice = no namespace restriction = all namespaces), it depends on theListimplementation treatingniland[]string{}equivalently. An explicit empty slice would make the "all namespaces" intent clearer.💡 Suggested change
- var namespaces []string - if !a.destinationBasedMapping { - namespaces = []string{a.namespace} + namespaces := []string{a.namespace} + if a.destinationBasedMapping { + // Watch all namespaces when destination-based mapping is enabled + namespaces = []string{} }agent/inbound.go (1)
150-152: Redundant namespace setting —getTargetNamespaceForAppis called both here and again insidecreateApplication/updateApplication/deleteApplication.The namespace is set on
incomingApphere, and then overwritten in each handler method. While the result is the same (idempotent), this double-assignment adds cognitive overhead. Consider setting it only in the handler methods (which need to be self-contained for when they're called from other paths), and removing it fromprocessIncomingApplication.
|
I'm justing testing this PR locally, and there's one thing I stumbled over: When you create an application in the control plane's namespace (i.e. |
|
I think regarding my previous comment about an application in the control plane namespace, we may ignore the situation for the sake of getting this PR merged and fix the issue in a follow-up PR. If we aren't able to come up with a good solution until the next release, we document it as known issue. |
Signed-off-by: Chetan Banavikalmutt <[email protected]>
@jannfis The Redis keys for apps created in the argocd installation namespace will only have the app name. i.e. |
|
Oops. I'm sorry, I didn't intend to merge main into this branch. |
5 participants
What does this PR do / why we need it:
Support mapping applications to target agents based on the
.spec.destination.nameinstead of the app namespace.ARGOCD_PRINCIPAL_DESTINATION_BASED_MAPPINGandARGOCD_AGENT_DESTINATION_BASED_MAPPINGto toggle the featureARGOCD_AGENT_CREATE_NAMESPACEto create namespaces on the flyWhich issue(s) this PR fixes:
Fixes #593
How to test changes / Special notes to the reviewer:
--destination-based-mappingand--allowed-namespacesconfigureddestination-based-mapping,--create-namespace, and--allowed-namespacesconfiguredtestspec.destination.namepointing to the agent name (agent-managed)testand sync the app to this test namespace instead of the agent's namespace.Checklist
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
Chores