fix(config): install Image Updater into argocd namespace by default #1356
Conversation
dkarpele
requested review from
chengfang,
ishitasequeira,
jannfis and
pasha-codefresh
as code owners
WalkthroughCLI default for --argocd-namespace now reads ARGOCD_NAMESPACE; Makefile adds IMAGE_UPDATER_CONTROLLER_NAMESPACE and uses it for deploy/undeploy; many manifests removed hardcoded Changes
Possibly related PRs
Pre-merge checks✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (5)
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 @@
## master #1356 +/- ##
==========================================
- Coverage 70.80% 70.77% -0.04%
==========================================
Files 49 49
Lines 4528 4530 +2
==========================================
Hits 3206 3206
- Misses 1125 1127 +2
Partials 197 197 ☔ 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: 0
🧹 Nitpick comments (3)
config/manager/manager.yaml (1)
52-66: Manager deployment ARGOCD_NAMESPACE wiring and pull policy look reasonable
- Injecting
ARGOCD_NAMESPACEfromargocd-image-updater-config(argocd.namespace, optional) matches the pattern used inconfig/install.yamland gives a clean knob for cross-namespace setups.- Explicit
imagePullPolicy: IfNotPresentis fine for this kustomize/manager manifest, even with a:latesttag, since production installs rely onconfig/install.yaml.No functional issues spotted here.
If you want stricter “always refresh latest” behavior even in dev, you could leave the pull policy implicit so Kubernetes defaults to
Alwaysfor:latest, but that’s purely a workflow preference.docs/install/installation.md (1)
9-89: Namespace & RBAC docs clearly address same‑namespace and cross‑namespace setups, with minor flow nitsThe new “Choosing an installation namespace” section does a good job of:
- Making the default
argocd-namespace assumption explicit.- Calling out that ClusterRoleBinding
subjects[].namespacemust be adjusted when Argo CD or the updater runs outsideargocd.- Documenting the cross-namespace
Role/RoleBindingneeded for repo creds (secrets,configmaps,get/list/watch), which lines up with the controller’s needs.Two small polish opportunities you might consider:
Option 2 install flow – Step 1 applies the remote
install.yamldirectly, and Step 3 later tells the user to download and modify that same manifest to fix the ClusterRoleBinding subjects. To avoid a transient misconfigured state and potential confusion, you could recommend:
- “Download
install.yaml, update thesubjects[].namespacein all ClusterRoleBindings to<updater_namespace>, thenkubectl apply -n <updater_namespace> -f install.yaml,” instead of applying the unedited remote first.ARGOCD_NAMESPACE example – The snippet shows adding
ARGOCD_NAMESPACEvia a literalvalue: <argocd_namespace>, while the shipped manifests use aconfigMapKeyRef(argocd-image-updater-config/argocd.namespace). Both are correct, but a short note that you can either set a literalvalueor configure theargocd.namespacekey in theargocd-image-updater-configConfigMap would make it clearer how this ties back to the manifests.These are documentation refinements only; the technical guidance is sound.
config/install.yaml (1)
821-835: Controller ARGOCD_NAMESPACE env from ConfigMap cleanly enables cross‑namespace setupsInjecting
ARGOCD_NAMESPACEfromargocd-image-updater-config(argocd.namespace, optional) gives a central, declarative knob to point the controller at the Argo CD namespace without relying solely on CLI flags. It also keeps the default behavior unchanged when the key is absent (env empty → same-namespace assumptions).This is consistent with cmd flag defaults and with the manager manifest.
You might later document the
argocd.namespacekey explicitly alongside the ARGOCD_NAMESPACE/--argocd-namespaceoptions so users see all three configuration paths in one place.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Makefile(2 hunks)cmd/run.go(2 hunks)cmd/run_test.go(1 hunks)cmd/webhook.go(1 hunks)cmd/webhook_test.go(1 hunks)config/default/kustomization.yaml(1 hunks)config/default/metrics_service.yaml(0 hunks)config/install.yaml(4 hunks)config/manager/manager.yaml(1 hunks)config/network-policy/allow-metrics-traffic.yaml(0 hunks)config/prometheus/monitor.yaml(1 hunks)config/rbac/leader_election_role_binding.yaml(0 hunks)config/rbac/metrics_auth_role_binding.yaml(1 hunks)config/rbac/metrics_reader_role_binding.yaml(1 hunks)config/rbac/role_binding.yaml(1 hunks)config/rbac/service_account.yaml(0 hunks)docs/install/cmd/run.md(1 hunks)docs/install/installation.md(1 hunks)
💤 Files with no reviewable changes (4)
- config/network-policy/allow-metrics-traffic.yaml
- config/rbac/service_account.yaml
- config/default/metrics_service.yaml
- config/rbac/leader_election_role_binding.yaml
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/run_test.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
cmd/webhook_test.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
cmd/webhook.go (1)
registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
cmd/run.go (3)
pkg/version/version.go (2)
BinaryName(20-22)Version(15-18)pkg/argocd/util.go (1)
GetPrintableInterval(11-17)registry-scanner/pkg/env/env.go (1)
GetStringVal(31-37)
🪛 LanguageTool
docs/install/cmd/run.md
[style] ~17-~17: To form a complete sentence, be sure to include a subject.
Context: ...amespace the controller is running in. Can also be set with the ARGOCD_NAMESPACE...
(MISSING_IT_THERE)
docs/install/installation.md
[style] ~58-~58: Consider a more expressive alternative.
Context: ...talled (<updater_namespace>). To do this, download install.yaml and manua...
(DO_ACHIEVE)
⏰ 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). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Analyze (go)
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: build_image
🔇 Additional comments (15)
config/default/kustomization.yaml (1)
2-2: LGTM! Namespace configuration now deferred to deployment time.Commenting out the namespace field enables flexible namespace selection at deployment. This aligns with the Makefile changes that now specify the namespace via
IMAGE_UPDATER_CONTROLLER_NAMESPACEand the kubectl-nflag.config/rbac/role_binding.yaml (1)
15-15: LGTM! ServiceAccount namespace updated to match new default.The namespace change from
argocd-image-updater-systemtoargocdaligns the ClusterRoleBinding subject with the new default installation namespace.cmd/webhook.go (1)
77-77: LGTM! Environment-based default for ArgoCD namespace.The flag default now sources from the
ARGOCD_NAMESPACEenvironment variable, enabling flexible namespace configuration for multi-namespace deployments. This change is consistent with the corresponding update incmd/run.go.docs/install/cmd/run.md (1)
13-18: LGTM! Documentation accurately reflects environment-based namespace configuration.The updated flag documentation clearly explains:
- When the flag is required (cross-namespace deployments)
- Default behavior (controller's namespace)
- Environment variable alternative (
ARGOCD_NAMESPACE)The static analysis hint about sentence structure can be safely ignored as the fragment format is acceptable and clear in documentation context.
Makefile (3)
1-1: LGTM! New namespace variable enables flexible deployment configuration.The
IMAGE_UPDATER_CONTROLLER_NAMESPACEvariable defaults toargocdbut allows override via environment or command-line, supporting both standard and custom namespace deployments.
200-200: LGTM! Deploy target now respects namespace configuration.Adding the
-n ${IMAGE_UPDATER_CONTROLLER_NAMESPACE}flag ensures resources are deployed to the specified namespace, working in tandem with the commented-out namespace inconfig/default/kustomization.yaml.
204-204: LGTM! Undeploy target mirrors deploy namespace configuration.The namespace flag ensures cleanup occurs in the correct namespace, maintaining consistency with the deploy target.
cmd/run.go (2)
102-111: LGTM! Improved structured logging with conditional namespace field.The refactored startup logging uses a slice-based approach and only logs the
argocdNamespacewhen explicitly set, avoiding noise from empty values. This provides cleaner, more informative logs.
292-292: LGTM! Environment-based default for ArgoCD namespace flag.The flag default now sources from the
ARGOCD_NAMESPACEenvironment variable, consistent with the webhook command and supporting flexible multi-namespace deployments.config/rbac/metrics_auth_role_binding.yaml (1)
12-12: LGTM! ServiceAccount namespace updated for metrics authentication.The namespace change from
argocd-image-updater-systemtoargocdaligns with the new default installation namespace and is consistent with other RBAC binding updates in this PR.config/rbac/metrics_reader_role_binding.yaml (1)
12-12: LGTM! ServiceAccount namespace updated for metrics reader role.The namespace change from
argocd-image-updater-systemtoargocdcompletes the consistent namespace migration across all RBAC bindings in this PR.cmd/run_test.go (1)
44-55: argocd-namespace default assertion now correctly mirrors env-based behaviorUsing
env.GetStringVal("ARGOCD_NAMESPACE", "")in the assertion keeps the test aligned with the flag’s new default resolution and is consistent with the other env-driven defaults in this test.config/prometheus/monitor.yaml (1)
4-16: ServiceMonitor namespace aligns with new default install targetPointing the
ServiceMonitorat theargocdnamespace matches the new default deployment location for the controller. Users running in a different or dedicated monitoring namespace can still override this manifest as needed.cmd/webhook_test.go (1)
27-36: Webhook argocd-namespace flag test is consistent with env-based defaultAsserting
env.GetStringVal("ARGOCD_NAMESPACE", "")for--argocd-namespaceis consistent with the controller’s configuration model and other flag defaults in this test.config/install.yaml (1)
710-751: ClusterRoleBinding subject namespaces correctly moved to argocd, consistent with docsBinding the
argocd-image-updater-controllerServiceAccount in theargocdnamespace for the manager and metrics ClusterRoleBindings matches the new default install target and the warning you added in the docs about needing to adjust these when Argo CD/updater are in a different namespace.This makes the “argocd as default, override by editing subjects[].namespace when needed” behavior explicit in the manifests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/install.yaml (1)
721-750: Hardcoded argocd namespace in ClusterRoleBindings limits flexibility without manual customization.The three ClusterRoleBindings (lines 724, 737, 750) specify
namespace: argocdin their subjects. This assumes Argo CD runs in theargocdnamespace. Users installing into different namespaces must manually edit this file, which is documented in the installation guide but increases operational friction.Consider either:
- Parameterizing the namespace for ClusterRoleBindings in a kustomization base
- Documenting a sed/patch command for automation
- Providing separate overlay manifests for common namespace variations
This is acceptable as a known limitation if the primary use case is the
argocdnamespace, but could cause friction for the "Option 2: Install into a separate namespace" flow described in the installation guide.docs/install/cmd/run.md (1)
13-18: Minor documentation clarity improvement needed.Lines 15–18 describe the flag's behavior, but a blank line (line 17) separates the default behavior from the environment variable reference. Reformat to improve readability:
~The namespace where Argo CD is running. Required only if the Image Updater runs in a different namespace than Argo CD. ~Defaults to the namespace the controller is running in. ~ ~Can also be set with the `ARGOCD_NAMESPACE` environment variable. +The namespace where Argo CD is running. Required only if the Image Updater runs in a different namespace than Argo CD. +Defaults to the namespace the controller is running in. +Can also be set with the `ARGOCD_NAMESPACE` environment variable.This improves flow and addresses the static analysis style note about sentence structure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Makefile(2 hunks)cmd/run.go(2 hunks)cmd/run_test.go(1 hunks)cmd/webhook.go(1 hunks)cmd/webhook_test.go(1 hunks)config/default/kustomization.yaml(0 hunks)config/default/metrics_service.yaml(0 hunks)config/install.yaml(4 hunks)config/manager/manager.yaml(1 hunks)config/network-policy/allow-metrics-traffic.yaml(0 hunks)config/prometheus/monitor.yaml(1 hunks)config/rbac/leader_election_role_binding.yaml(0 hunks)config/rbac/metrics_auth_role_binding.yaml(1 hunks)config/rbac/metrics_reader_role_binding.yaml(1 hunks)config/rbac/role_binding.yaml(1 hunks)config/rbac/service_account.yaml(0 hunks)docs/install/cmd/run.md(1 hunks)docs/install/installation.md(1 hunks)
💤 Files with no reviewable changes (5)
- config/rbac/leader_election_role_binding.yaml
- config/rbac/service_account.yaml
- config/network-policy/allow-metrics-traffic.yaml
- config/default/kustomization.yaml
- config/default/metrics_service.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/run.go
- cmd/run_test.go
- config/rbac/role_binding.yaml
- config/rbac/metrics_auth_role_binding.yaml
- config/manager/manager.yaml
- Makefile
- cmd/webhook_test.go
- cmd/webhook.go
- config/prometheus/monitor.yaml
- config/rbac/metrics_reader_role_binding.yaml
🧰 Additional context used
🪛 LanguageTool
docs/install/installation.md
[style] ~58-~58: Consider a more expressive alternative.
Context: ...talled (<updater_namespace>). To do this, download install.yaml and manua...
(DO_ACHIEVE)
docs/install/cmd/run.md
[style] ~17-~17: To form a complete sentence, be sure to include a subject.
Context: ...amespace the controller is running in. Can also be set with the ARGOCD_NAMESPACE...
(MISSING_IT_THERE)
⏰ 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). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (3)
config/install.yaml (1)
821-826: ARGOCD_NAMESPACE environment variable correctly configured with optional override.The ARGOCD_NAMESPACE environment variable is sourced from a ConfigMap key (
argocd.namespace) withoptional: true, allowing users to override the default behavior via configuration. This aligns with PR objective to support multi-namespace deployments.Verify in downstream code (e.g.,
cmd/run.go) that an empty or missing environment variable correctly defaults to the controller's own namespace, as documented.docs/install/installation.md (2)
9-25: Installation namespace guidance is clear and well-structured.The two-option approach (recommended argocd namespace vs. separate namespace) provides good guidance. The warning (line 23–24) about manually updating ClusterRoleBinding namespaces correctly documents the limitation of hardcoded manifests. Option 1 follows best practices by minimizing configuration overhead.
26-89: Multi-namespace installation instructions are comprehensive and well-documented.The four-step Option 2 flow covers:
- Namespace creation and manifest deployment
- ARGOCD_NAMESPACE configuration (environment variable or CLI flag)
- ClusterRoleBinding namespace adjustment
- Cross-namespace RBAC setup in the Argo CD namespace
The cross-namespace Role/RoleBinding example (lines 66–89) correctly grants the required permissions (
get,list,watchonsecretsandconfigmaps) to access Argo CD resources from the updater namespace. This directly addresses PR issue #1341.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/install/installation.md (2)
26-62: Comprehensive multi-step guidance for Option 2.The separate namespace installation option is well-structured with clear steps. Providing three alternative methods for configuring the Argo CD namespace (environment variable, ConfigMap, or CLI flag at lines 44-61) gives users flexibility. However, it might be helpful to indicate which method is preferred or when to use each, though this is not blocking.
Consider briefly noting which configuration method is preferred for the Argo CD namespace setting (e.g., "environment variables are recommended" or "use ConfigMap for easier runtime changes").
63-68: RBAC adjustment instructions are clear but could use slightly more expressive language.The instructions for adjusting ClusterRoleBinding are clear and accurate. However, line 67 uses "download
install.yamland manually change" which is flagged by the style checker as potentially benefiting from more expressive wording.Consider rephrasing line 67 from "download
install.yamland manually change" to something like "downloadinstall.yaml, then update" or "download and modifyinstall.yamlto change" for better flow.As per static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/install/installation.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/install/installation.md
[style] ~67-~67: Consider a more expressive alternative.
Context: ...talled (<updater_namespace>). To do this, download install.yaml and manua...
(DO_ACHIEVE)
⏰ 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). (5)
- GitHub Check: Ensure registry-scanner Go modules synchronicity and run tests
- GitHub Check: Ensure code is correctly linted
- GitHub Check: Ensure unit tests are passing
- GitHub Check: Analyze (go)
- GitHub Check: build_image
🔇 Additional comments (2)
docs/install/installation.md (2)
9-25: Clear recommendation and helpful warning for namespace selection.Option 1 is appropriately marked as the recommended approach and provides the simplest path for users. The warning about manifest assumptions (lines 23-24) is valuable for users with non-default namespaces, ensuring they won't encounter silent failures.
69-98: Cross-namespace RBAC examples are well-documented and comprehensive.The Role and RoleBinding YAML examples clearly show how to grant the image updater service account read access to secrets and configmaps in the Argo CD namespace. The use of
<argocd_namespace>and<updater_namespace>placeholders is consistent and appropriate. The specification of["get", "list", "watch"]verbs at line 83 correctly aligns with the stated requirements for cross-namespace read permissions.
Signed-off-by: dkarpele <[email protected]>
|
in test/e2e/README.md, it still has a step to create ns argocd-image-updater-system, which is not used. Should it be updated too? |
Signed-off-by: dkarpele <[email protected]>
fixed |
3 participants
Closes #1341
Closes #1343 - wrong target file name was created because of missing
argocd-namespaceflag when using controller namespace different from ArgoCD ns.Closes #1338 - duplicates #1343
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.