feat: allow ArgoCD Image Updater to watch specific namespaces#1488
feat: allow ArgoCD Image Updater to watch specific namespaces#1488grahamalama wants to merge 3 commits intoargoproj-labs:masterfrom
Conversation
grahamalama
requested review from
chengfang,
dkarpele,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
grahamalama
changed the title
WalkthroughAdds namespace-scoped operation via a new Changes
sequenceDiagram
participant Operator as Operator (CLI / Deployment)
participant Controller as argocd-image-updater (cmd/run)
participant Manager as controller-runtime Manager
participant K8s as Kubernetes API Server
Operator->>Controller: start with --watch-namespaces / env var
Controller->>Controller: parse CSV, validate namespaces
Controller->>Manager: provide cache.Options(DefaultNamespaces)
Manager->>K8s: initialize cache scoped to specified namespaces
Manager->>Controller: start controllers using scoped cache
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Add configuration to scope the controller to specific namespaces, enabling use of namespace-scoped Roles instead of ClusterRoles. Useful for multi-tenant environments where cluster-wide RBAC is not permitted. Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
grahamalama
force-pushed
the
limit-application-namespace
branch
from
8e5d73e to
cf636b0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1488 +/- ##
==========================================
+ Coverage 71.48% 72.33% +0.85%
==========================================
Files 50 51 +1
Lines 4667 4906 +239
==========================================
+ Hits 3336 3549 +213
- Misses 1133 1156 +23
- Partials 198 201 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/run.go`:
- Around line 184-192: Add a startup validation before creating the manager
(before ctrl.NewManager) that checks if leaderElectionNamespace is non-empty and
leader election is enabled and the controller is running in namespace-scoped
watch mode (i.e., the configured watched namespaces are a restricted set rather
than all namespaces); if so, verify the leaderElectionNamespace is included in
the watched namespace set (or warn/error if not) and fail fast or log a clear
warning explaining the RBAC lease restriction; update docs/install/cmd/run.md to
state that --leader-election-namespace must be inside the --watch-namespaces set
for namespace-scoped deployments (or be left empty to use the pod namespace) as
an alternative to runtime validation.
In `@docs/install/installation.md`:
- Line 199: The sentence containing the log message "Configuring
namespace-scoped operation" in the installation documentation is missing a
trailing period; edit the sentence to add a period at the end so it reads
"Configuring namespace-scoped operation." to match punctuation style.
🧹 Nitpick comments (1)
docs/install/cmd/run.md (1)
176-188: Flag documentation is placed between the two--metrics-*flags.The
--watch-namespacesblock splits--metrics-bind-addressand--metrics-secure. Consider moving it to maintain grouping of related flags (e.g., after--metrics-secureor in alphabetical order with the other flags).
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | ||
| Scheme: scheme, | ||
| Metrics: metricsServerOptions, | ||
| HealthProbeBindAddress: probeAddr, | ||
| LeaderElection: enableLeaderElection, | ||
| LeaderElectionID: "c21b75f2.argoproj.io", | ||
| LeaderElectionNamespace: leaderElectionNamespace, | ||
| Cache: cacheOptions, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check docs and config for leader election lease permissions
rg -n "leases" docs/ config/rbac/ --type md --type yaml 2>/dev/null || true
# Check for coordination.k8s.io API group
rg -n "coordination" docs/ config/rbac/ --type md --type yaml 2>/dev/null || trueRepository: argoproj-labs/argocd-image-updater
Length of output: 1547
🏁 Script executed:
# View the leader election role file
cat config/rbac/leader_election_role.yaml
# List all RBAC files
echo "=== All RBAC files ==="
fd . config/rbac/ --type f
# Check for any role bindings
rg -n "roleRef\|RoleBinding\|ClusterRoleBinding" config/rbac/ --type yamlRepository: argoproj-labs/argocd-image-updater
Length of output: 1153
🏁 Script executed:
# Check the leader election role binding
cat config/rbac/leader_election_role_binding.yaml
# Look at cmd/run.go to see how leaderElectionNamespace is handled
head -250 cmd/run.go | tail -100
# Search for namespace validation logic
rg -n "leaderElectionNamespace\|--leader-election-namespace" cmd/ --type go -A 3Repository: argoproj-labs/argocd-image-updater
Length of output: 4226
🏁 Script executed:
# Find where leaderElectionNamespace is defined as a flag
rg -n "leader-election-namespace\|leaderElectionNamespace" cmd/ --type go -B 2 -A 2
# Search for any validation that compares leaderElectionNamespace with watchNamespaces
rg -n "leaderElectionNamespace.*watchNamespaces\|watchNamespaces.*leaderElectionNamespace" --type goRepository: argoproj-labs/argocd-image-updater
Length of output: 60
🏁 Script executed:
# Let's look at a broader section of cmd/run.go to find flag definitions
cat cmd/run.go | head -100
# Also search for how leaderElectionNamespace is declared/used
rg -n "leaderElectionNamespace" . --type go
# Search for flag setup patterns
rg -n "flag\|Flag" cmd/run.go --type go | head -20Repository: argoproj-labs/argocd-image-updater
Length of output: 3690
🏁 Script executed:
# Look at the full flag definitions section around line 312
sed -n '290,320p' cmd/run.go
# Search for any validation logic after flag parsing that might check leaderElectionNamespace
rg -n "leaderElectionNamespace\|watchNamespaces\|WatchNamespaces" cmd/run.go --type go -A 2 -B 2
# Check if there are any error checks or warnings about namespace mismatches
rg -n "namespace.*mismatch\|lease.*permission\|RBAC\|error" cmd/run.go --type go | grep -i "leader\|election\|namespace"Repository: argoproj-labs/argocd-image-updater
Length of output: 2715
🏁 Script executed:
# Get the full section from flag parsing to manager start to check for any validation
sed -n '70,220p' cmd/run.go
# Also check if there's any validation in the controller itself
rg -n "DefaultNamespaces\|leaderElection\|lease" cmd/ --type go -B 3 -A 3Repository: argoproj-labs/argocd-image-updater
Length of output: 5908
🏁 Script executed:
# Search documentation for any mention of leader election namespace requirements
rg -n "leader" docs/ --type md -i -B 2 -A 2
# Check if there's any README or installation guide mentioning this
cat README.md 2>/dev/null | grep -i "leader\|election\|namespace" || echo "Not found in README"
# Check installation or deployment docs
find docs/ -type f \( -name "*.md" -o -name "*.yaml" \) | xargs rg -l "install\|deploy" 2>/dev/null | head -5Repository: argoproj-labs/argocd-image-updater
Length of output: 832
🏁 Script executed:
# Check if there's documentation about namespace-scoped RBAC requirements
rg -n "namespace.*scoped\|RBAC\|role.*binding" docs/install/ --type md -B 2 -A 2
# Look at the full installation doc
cat docs/install/cmd/run.md | head -150Repository: argoproj-labs/argocd-image-updater
Length of output: 4938
Add validation or warning when leader election namespace differs from watched namespaces.
With namespace-scoped RBAC, the leader-election-role Role only applies permissions in its bound namespace. If --leader-election-namespace is set to a namespace outside --watch-namespaces, the controller will fail to acquire the leadership lease due to missing RBAC permissions.
Add either:
- A startup validation that warns or errors if
--leader-election-namespaceis set and not included in--watch-namespaces(when namespace-scoped mode is enabled) - Documentation in
docs/install/cmd/run.mdclarifying that--leader-election-namespacemust be included in the watched set for namespace-scoped deployments, or left empty to use the controller pod's namespace
🤖 Prompt for AI Agents
In `@cmd/run.go` around lines 184 - 192, Add a startup validation before creating
the manager (before ctrl.NewManager) that checks if leaderElectionNamespace is
non-empty and leader election is enabled and the controller is running in
namespace-scoped watch mode (i.e., the configured watched namespaces are a
restricted set rather than all namespaces); if so, verify the
leaderElectionNamespace is included in the watched namespace set (or warn/error
if not) and fail fast or log a clear warning explaining the RBAC lease
restriction; update docs/install/cmd/run.md to state that
--leader-election-namespace must be inside the --watch-namespaces set for
namespace-scoped deployments (or be left empty to use the pod namespace) as an
alternative to runtime validation.
|
|
||
| Alternatively, you can add the `--watch-namespaces=argocd,team-a` flag to the container's `command` arguments in the deployment manifest, or set the `IMAGE_UPDATER_WATCH_NAMESPACES` environment variable. | ||
|
|
||
| The controller logs `Configuring namespace-scoped operation` on startup when this mode is active |
There was a problem hiding this comment.
Missing period at end of sentence.
Line 199 is missing a trailing period.
-The controller logs `Configuring namespace-scoped operation` on startup when this mode is active
+The controller logs `Configuring namespace-scoped operation` on startup when this mode is active.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The controller logs `Configuring namespace-scoped operation` on startup when this mode is active | |
| The controller logs `Configuring namespace-scoped operation` on startup when this mode is active. |
🤖 Prompt for AI Agents
In `@docs/install/installation.md` at line 199, The sentence containing the log
message "Configuring namespace-scoped operation" in the installation
documentation is missing a trailing period; edit the sentence to add a period at
the end so it reads "Configuring namespace-scoped operation." to match
punctuation style.
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
|
@dkarpele what does the |
|
Does #1490 addresses your requirements and concerns? |
|
@grahamalama thanks for your contribution. As we're working on high-priority features and fixes, issues and pr labelled |
I'm not exactly sure, but I don't think so. Even with this change, I think the controller will still search across all namespaces and as a result, require a |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/install.yaml (1)
618-664: Consider documenting RBAC implications for namespace-scoped installs.The PR's goal is to let operators use namespace-scoped Roles instead of ClusterRoles, but this
install.yamlstill shipsClusterRole/ClusterRoleBindingfor the manager (lines 618-664, 725-739). Users enabling--watch-namespaceswill need to manually replace these with per-namespaceRole/RoleBindingresources.It would be helpful to either:
- Add a comment in this file near the ClusterRole noting it can be replaced with Roles when
watch-namespacesis configured, or- Provide a separate overlay/example for namespace-scoped installs.
This isn't blocking, just a usability improvement for the feature's target audience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/install.yaml` around lines 618 - 664, The ClusterRole "argocd-image-updater-manager-role" is shipped for the manager but the PR enables namespace-scoped installs; update the install.yaml to document this by adding a clear comment immediately above the ClusterRole (and corresponding ClusterRoleBinding) stating that these ClusterRole/ClusterRoleBinding resources can be replaced with per-namespace Role/RoleBinding when using the --watch-namespaces option, and optionally add or reference a separate example/overlay for namespace-scoped installation so operators have a ready template to copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/install.yaml`:
- Around line 618-664: The ClusterRole "argocd-image-updater-manager-role" is
shipped for the manager but the PR enables namespace-scoped installs; update the
install.yaml to document this by adding a clear comment immediately above the
ClusterRole (and corresponding ClusterRoleBinding) stating that these
ClusterRole/ClusterRoleBinding resources can be replaced with per-namespace
Role/RoleBinding when using the --watch-namespaces option, and optionally add or
reference a separate example/overlay for namespace-scoped installation so
operators have a ready template to copy.
4 participants
In this PR, I'm essentially trying to re-enable what was introduced in #1112.
Adds configuration to scope the controller to specific namespaces, enabling use of namespace-scoped Roles instead of ClusterRoles. Useful for multi-tenant environments where cluster-wide RBAC is not desired.
Summary by CodeRabbit
New Features
Documentation