Add Wildcards support in ApplicationSet Source Namespaces #1988
Conversation
Signed-off-by: nmirasch <[email protected]>
WalkthroughThis pull request introduces wildcard pattern matching for ApplicationSet source namespaces in the ArgoCD operator. The Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as ReconcileArgoCD
participant Client as K8s Client
participant CR as ApplicationSet<br/>CR Spec
participant Match as Pattern Matcher
participant Deploy as Deployment Builder
participant RBAC as RBAC Provisioner
Reconciler->>CR: Read sourceNamespaces pattern<br/>(e.g., "team-*")
Reconciler->>Client: List all Namespaces
Client-->>Reconciler: [ns1, ns2, ..., team-1, team-2, ...]
Reconciler->>Match: Filter namespaces against pattern
Match-->>Reconciler: [team-1, team-2]
alt Namespaces resolved successfully
Reconciler->>Deploy: Build appset command with<br/>--applicationset-namespaces
Deploy-->>Reconciler: Deployment with matched namespaces
Reconciler->>RBAC: For each matched namespace,<br/>provision Role + RoleBinding
RBAC-->>Reconciler: RBAC resources created
else Error during resolution
Reconciler->>Reconciler: Log error & handle gracefully
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
controllers/argocd/role.go (1)
264-271: Error handling silently skips appset permissions on failure.When
getApplicationSetSourceNamespacesreturns an error, the code silently skips adding ApplicationSet policy rules. While this is a fail-safe approach (doesn't grant extra permissions on error), consider logging the error for debugging purposes:- appsetSourceNamespaces, err := r.getApplicationSetSourceNamespaces(cr) - if err == nil && contains(appsetSourceNamespaces, sourceNamespace) { + appsetSourceNamespaces, appsetErr := r.getApplicationSetSourceNamespaces(cr) + if appsetErr != nil { + log.Error(appsetErr, "failed to get ApplicationSet source namespaces, skipping appset rules") + } else if contains(appsetSourceNamespaces, sourceNamespace) { role.Rules = append(role.Rules, policyRuleForServerApplicationSetSourceNamespaces()...) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controllers/argocd/applicationset.go(5 hunks)controllers/argocd/applicationset_test.go(3 hunks)controllers/argocd/role.go(1 hunks)docs/usage/appsets-in-any-namespace.md(2 hunks)tests/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
controllers/argocd/applicationset.go (3)
controllers/argocd/argocd_controller.go (1)
ReconcileArgoCD(73-96)api/v1alpha1/argocd_types.go (1)
ArgoCD(61-67)api/v1beta1/argocd_types.go (1)
ArgoCD(61-67)
controllers/argocd/applicationset_test.go (2)
api/v1alpha1/argocd_types.go (1)
ArgoCDApplicationSet(138-166)api/v1beta1/argocd_types.go (1)
ArgoCDApplicationSet(171-223)
tests/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.go (4)
tests/ginkgo/fixture/fixture.go (1)
OutputDebugOnFail(491-588)tests/ginkgo/fixture/k8s/fixture.go (3)
ExistByName(103-120)NotExistByName(124-143)Update(146-163)tests/ginkgo/fixture/namespace/fixture.go (2)
HaveLabel(27-32)Update(35-51)tests/ginkgo/fixture/role/fixture.go (1)
Update(19-36)
⏰ 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). (4)
- GitHub Check: build-operator
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: build
🔇 Additional comments (10)
docs/usage/appsets-in-any-namespace.md (1)
17-63: Documentation updates look good.The documentation clearly explains the three patterns for configuring ApplicationSet source namespaces:
- Specific namespace names
- Glob patterns (e.g.,
team-*)- All namespaces wildcard (
*)The examples are well-structured and align with the implementation changes.
controllers/argocd/applicationset_test.go (3)
560-560: Command namespace order changed to alphabetical.The expected namespace order changed from
"foo,bar"to"bar,foo". This reflects the new behavior where namespaces are now listed alphabetically after expansion.
1261-1300: Good test coverage for wildcard pattern matching.The new test case for wildcard patterns properly validates:
- Pattern
team-*matchesteam-1andteam-2- Non-matching namespace
other-nsis correctly excluded- Namespace objects are now properly attached to the fake client
1320-1334: Robust test assertions with proper nil handling and sorting.The updated assertions correctly:
- Handle the new error return value
- Normalize nil slices to empty slices for comparison
- Sort both expected and actual slices to handle nondeterministic ordering from namespace listing
tests/ginkgo/sequential/1-037_validate_applicationset_in_any_namespace_test.go (2)
49-50: Debug output expanded for new test namespaces.Good addition of the new team-* namespaces to the debug output for easier troubleshooting of test failures.
739-972: Comprehensive E2E test for wildcard pattern matching.This test thoroughly validates the wildcard functionality:
- Creates multiple namespaces with
team-*pattern and one non-matching- Verifies RBAC resources in all matching namespaces
- Confirms non-matching namespace is excluded
- Tests dynamic provisioning when new matching namespace is created
- Verifies cleanup when pattern is narrowed
The test structure follows the existing patterns in the file and provides good coverage of edge cases.
controllers/argocd/applicationset.go (4)
75-100: Robust namespace expansion with proper error handling.The implementation correctly:
- Expands wildcard patterns using
getApplicationSetSourceNamespaces- Filters expanded namespaces to only those also in apps source namespaces
- Logs skipped namespaces at debug level (V(1))
- Only adds
--applicationset-namespaceswhen there are valid namespaces- Disables SCM providers only when appset namespaces exist
The nested error handling ensures the command construction continues even if namespace resolution fails.
1001-1012: Cleanup logic correctly uses pattern matching against raw patterns.The
removeUnmanagedApplicationSetSourceNamespaceResourcesfunction correctly usesglob.MatchStringInListagainst the rawcr.Spec.ApplicationSet.SourceNamespacespatterns (not expanded namespaces). This ensures that:
- Namespaces matching current patterns are kept
- Namespaces no longer matching are cleaned up
This is the correct approach for cleanup logic since we want to check if a namespace still matches any configured pattern.
662-674: Proper error handling with wrapped errors.Good use of
fmt.Errorfwith%wverb for error wrapping, providing proper error context for debugging.
35-36: New dependency on argo-cd v3 glob package.The import of
github.com/argoproj/argo-cd/v3/util/globis used for glob pattern matching withglob.MatchStringInList, providing consistent glob matching behavior aligned with ArgoCD. This is a reasonable and reliable dependency choice for namespace filtering logic.
| // getApplicationSetSourceNamespaces returns the list of actual namespaces that match the patterns | ||
| // specified in .spec.ApplicationSet.SourceNamespaces. It supports wildcard patterns (e.g., team-*). | ||
| func (r *ReconcileArgoCD) getApplicationSetSourceNamespaces(cr *argoproj.ArgoCD) ([]string, error) { | ||
| if cr.Spec.ApplicationSet == nil { | ||
| return []string(nil), nil | ||
| } | ||
| return []string(nil) | ||
|
|
||
| sourceNamespaces := []string{} | ||
| namespaces := &corev1.NamespaceList{} | ||
|
|
||
| if err := r.List(context.TODO(), namespaces, &client.ListOptions{}); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, namespace := range namespaces.Items { | ||
| if glob.MatchStringInList(cr.Spec.ApplicationSet.SourceNamespaces, namespace.Name, glob.REGEXP) { | ||
| sourceNamespaces = append(sourceNamespaces, namespace.Name) | ||
| } | ||
| } | ||
|
|
||
| return sourceNamespaces, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for applicationset-namespaces usage
rg -n "applicationset-namespaces" --type go -A3 -B3Repository: argoproj-labs/argocd-operator
Length of output: 9838
🏁 Script executed:
#!/bin/bash
# Check the context around line 90 in applicationset.go
sed -n '60,110p' controllers/argocd/applicationset.goRepository: argoproj-labs/argocd-operator
Length of output: 2209
🏁 Script executed:
#!/bin/bash
# Check the getApplicationSetSourceNamespaces function implementation
sed -n '1184,1210p' controllers/argocd/applicationset.goRepository: argoproj-labs/argocd-operator
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Check the test to understand what it expects for namespace ordering
sed -n '540,580p' controllers/argocd/applicationset_test.goRepository: argoproj-labs/argocd-operator
Length of output: 1341
🏁 Script executed:
#!/bin/bash
# Search for the test setup to see how the mock is configured
sed -n '500,550p' controllers/argocd/applicationset_test.goRepository: argoproj-labs/argocd-operator
Length of output: 1592
🏁 Script executed:
#!/bin/bash
# Search for the test implementation that checks the command
grep -n "TestReconcileApplicationSet_Deployments_Command" -A 200 controllers/argocd/applicationset_test.go | head -250Repository: argoproj-labs/argocd-operator
Length of output: 7271
🏁 Script executed:
#!/bin/bash
# Search for any sorting logic in applicationset.go
rg -n "sort" controllers/argocd/applicationset.go -iRepository: argoproj-labs/argocd-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check if the fake client sorts results - look at the test setup and fake client
rg -n "makeTestReconcilerClient" controllers/argocd/applicationset_test.go -A 5Repository: argoproj-labs/argocd-operator
Length of output: 5541
🏁 Script executed:
#!/bin/bash
# Find the definition of makeTestReconcilerClient
grep -n "func makeTestReconcilerClient" controllers/argocd/applicationset_test.go -A 20Repository: argoproj-labs/argocd-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Search for makeTestReconcilerClient definition in all test files
rg -n "func makeTestReconcilerClient" --type goRepository: argoproj-labs/argocd-operator
Length of output: 723
🏁 Script executed:
#!/bin/bash
# Check the makeTestReconcilerClient implementation
sed -n '68,100p' controllers/argocd/testing.goRepository: argoproj-labs/argocd-operator
Length of output: 909
🏁 Script executed:
#!/bin/bash
# Check if the fake client sorts - let's also look for any sorting in the broader context
# Let's check if there's any sort.Strings call or similar
rg -n "sort\|Sort" controllers/argocd/applicationset.go -C 3Repository: argoproj-labs/argocd-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Let me trace the entire flow - check what order namespaces are returned from the fake client
# by looking at the test more carefully. Let's see if there's any sorting in between
grep -n "contains" controllers/argocd/applicationset.go -B 5 -A 5Repository: argoproj-labs/argocd-operator
Length of output: 1540
🏁 Script executed:
#!/bin/bash
# Check if fake client implementation sorts - look for controllerruntime or similar
# Let's search for where namespaces might be sorted in the entire codebase
rg -n "sort\.Strings\|strings\.Join.*sort" --type go controllers/Repository: argoproj-labs/argocd-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Let's check the fake client's List implementation to see if it sorts
# First, let's understand: does the test pass with "bar,foo"?
# Let's look at what the fake client does
rg -n "fake.NewClientBuilder\|controllerruntime.*fake" --type go -B 3 -A 3 | head -50Repository: argoproj-labs/argocd-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Let's look more carefully at getApplicationSetSourceNamespaces and check if namespaces.Items might be pre-sorted
# Check the full function again and see if there's any processing after the loop
sed -n '1180,1220p' controllers/argocd/applicationset.goRepository: argoproj-labs/argocd-operator
Length of output: 883
Add sorting to ensure deterministic namespace ordering.
The function returns namespaces in the order they are returned by the Kubernetes API, which is not guaranteed to be deterministic. This can cause the --applicationset-namespaces command argument to differ between reconciliations, triggering unnecessary deployment updates.
Sort the sourceNamespaces slice before returning it to ensure consistent ordering across deployments.
🤖 Prompt for AI Agents
In controllers/argocd/applicationset.go around lines 1184-1205 the returned
sourceNamespaces slice is based on the non-deterministic order from the
Kubernetes API; sort the sourceNamespaces slice (e.g., with sort.Strings) before
returning to ensure deterministic ordering, and add the necessary "sort" import
to the file.
1 participant
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Adds wildcard pattern support (e.g., team-*) to .spec.applicationSet.sourceNamespaces in the ArgoCD Operator. The Operator automatically provisions RBAC permissions (Role and RoleBinding) for the argocd-applicationset-controller ServiceAccount in all namespaces that match the pattern.
Changes Added:
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/GITOPS-8278, https://issues.redhat.com/browse/GITOPS-8279
How to test changes / Special notes to the reviewer:
Create test namespaces
kubectl create namespace team-1 team-2 team-frontend team-backend other-ns
Create ArgoCD with wildcard pattern
kubectl get role wildcard-example-argocd-applicationset -n team-1
kubectl get rolebinding wildcard-example-argocd-applicationset -n team-1
kubectl get role wildcard-example-argocd-applicationset -n other-ns
Test dynamic provisioning
kubectl create namespace team-3 # Wait ~60 seconds, then verify resources are automatically created
kubectl get role wildcard-example-argocd-applicationset -n team-3
Verify deployment command
kubectl get deployment wildcard-example-applicationset-controller -n argocd \ -o jsonpath='{.spec.template.spec.containers[0].command[]}' | \ grep -o '--applicationset-namespaces [^ ]'# Should include: team-1,team-2,team-frontend,team-backend
Test cleanup
kubectl patch argocd wildcard-example -n argocd --type merge \ -p '{"spec":{"applicationSet":{"sourceNamespaces":["team-1"]}}}'# Wait ~60 seconds, then verify team-2, team-3, etc. have resources removed
Summary by CodeRabbit
New Features
team-*,*) enabling dynamic namespace targeting and automatic RBAC provisioning for matching namespaces.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.