feat: Edit annotations for the argocd-server service#2095
feat: Edit annotations for the argocd-server service#2095MaayanGalindez wants to merge 3 commits intoargoproj-labs:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CR
participant Controller as ArgoCD Controller
participant KubeAPI as Kubernetes API (Service)
participant Existing as Existing Service
User->>Controller: Create/Update ArgoCD CR with Service.Annotations
Controller->>Controller: Build desired Service (includes Annotations)
Controller->>KubeAPI: Get Existing Service
alt Service exists
KubeAPI->>Controller: Return Existing Service
Controller->>Controller: Merge/preserve AutoTLS annotations, compare annotations
alt Annotations differ
Controller->>KubeAPI: Update Service.annotations
KubeAPI->>Existing: Patch annotations
end
else Service missing
Controller->>KubeAPI: Create Service with annotations
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/argocd/service.go (1)
498-549:⚠️ Potential issue | 🟠 MajorAvoid wiping existing annotations when
spec.server.service.annotationsis unset.Right now
svc.Annotationsis only populated whenlen(...) > 0, but the update path always compares and overwrites annotations. This can remove existing annotations (including the OpenShift autoTLS annotation when the TLS secret already exists, or cluster-added annotations) even when users did not opt in to managing them. It also aliases the CR map directly.Consider gating annotation reconciliation on a non‑nil spec map and deep‑copying/merging with the autoTLS annotation instead of replacing the map.
🛠️ Suggested fix
- // Add annotations if specified without deleteing annotations from `ensureAutoTLSAnnotation()` - if len(cr.Spec.Server.Service.Annotations) > 0 { - if svc.Annotations == nil { - svc.Annotations = make(map[string]string) - svc.Annotations = cr.Spec.Server.Service.Annotations - } else { - for k, v := range cr.Spec.Server.Service.Annotations { - svc.Annotations[k] = v - } - } - } + // Add annotations if explicitly specified without clobbering AutoTLS or external annotations. + if cr.Spec.Server.Service.Annotations != nil { + desired := map[string]string{} + for k, v := range svc.Annotations { // preserves AutoTLS if set + desired[k] = v + } + for k, v := range cr.Spec.Server.Service.Annotations { + desired[k] = v + } + svc.Annotations = desired + } ... - if !reflect.DeepEqual(svc.Annotations, existingSVC.Annotations) { + if cr.Spec.Server.Service.Annotations != nil && + !reflect.DeepEqual(svc.Annotations, existingSVC.Annotations) { existingSVC.Annotations = svc.Annotations if changed { explanation += ", " } explanation += "service annotations" changed = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocd/service.go` around lines 498 - 549, The current logic overwrites existingSVC.Annotations with svc.Annotations (and aliases the CR map) even when cr.Spec.Server.Service.Annotations is nil/empty; change reconciliation so annotations are only replaced/merged when the CR explicitly provides a non-nil map: when building svc, do not assign svc.Annotations = cr.Spec.Server.Service.Annotations directly; instead create a new map copy and merge entries from cr.Spec.Server.Service.Annotations into svc.Annotations only if cr.Spec.Server.Service.Annotations != nil, and when updating existingSVC preserve any annotations not managed by the CR by merging the autoTLS annotation from ensureAutoTLSAnnotation and the CR-provided keys rather than replacing the entire map; ensure you deep-copy maps to avoid aliasing the CR's map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/argocd/service_test.go`:
- Around line 146-187: The tests "Server Service annotations update" and "Server
Service annotations update with Openshift auto TLS annotation" call
argoutil.SetRouteAPIFound(...) and do not restore the global flag, causing
cross-test leakage; update each subtest to capture the previous state (e.g.,
prev := argoutil.IsRouteAPIFound() or read current value), then defer restoring
it with argoutil.SetRouteAPIFound(prev) (or call SetRouteAPIFound(false) after
the first subtest) so the global Route API flag used by reconcileServerService
and related asserts is reset at test end.
---
Outside diff comments:
In `@controllers/argocd/service.go`:
- Around line 498-549: The current logic overwrites existingSVC.Annotations with
svc.Annotations (and aliases the CR map) even when
cr.Spec.Server.Service.Annotations is nil/empty; change reconciliation so
annotations are only replaced/merged when the CR explicitly provides a non-nil
map: when building svc, do not assign svc.Annotations =
cr.Spec.Server.Service.Annotations directly; instead create a new map copy and
merge entries from cr.Spec.Server.Service.Annotations into svc.Annotations only
if cr.Spec.Server.Service.Annotations != nil, and when updating existingSVC
preserve any annotations not managed by the CR by merging the autoTLS annotation
from ensureAutoTLSAnnotation and the CR-provided keys rather than replacing the
entire map; ensure you deep-copy maps to avoid aliasing the CR's map.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/v1alpha1/argocd_types.goapi/v1alpha1/zz_generated.deepcopy.goapi/v1beta1/argocd_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/service.gocontrollers/argocd/service_test.godeploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yaml
🔥 Files not summarized due to errors (1)
- bundle/manifests/argoproj.io_argocds.yaml: Error: Server error: no LLM provider could handle the message
Signed-off-by: Maayan Galindez <maayangg2005@gmail.com>
MaayanGalindez
force-pushed
the
feature/argocd-cr-service-annotation
branch
from
c212a12 to
2973d26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
controllers/argocd/service_test.go (1)
146-187: SetRouteAPIFound global not restored after subtests — already flagged.Both subtests mutate the package-level
routeAPIFoundflag without restoring it, which can affect subsequent tests in the same run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocd/service_test.go` around lines 146 - 187, The tests call the package-level flag setter argoutil.SetRouteAPIFound and do not restore the original routeAPIFound value, causing global state leakage between subtests; update each subtest that calls argoutil.SetRouteAPIFound (e.g., the "Server Service annotations update" and "Server Service annotations update with Openshift auto TLS annotation" cases) to capture the current routeAPIFound value before changing it and defer restoring it (or call SetRouteAPIFound with the saved value in a defer) immediately after saving so the global flag is returned to its original state after the subtest finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/argocd/service_test.go`:
- Line 173: The comment "Existing Server is found and has the argoCD new Server
Service Type" is stale/misleading for this subtest which asserts annotations;
update that comment (and the subtest description string if present) to reference
annotations (e.g., "Existing Server is found and has the argoCD Server
annotations") so the comment matches the test intent and assertions in the test
block.
In `@controllers/argocd/service.go`:
- Around line 498-508: The svc.Annotations nil-branch currently makes
svc.Annotations alias cr.Spec.Server.Service.Annotations and also wastes a make
call; instead always allocate a fresh map for svc.Annotations and copy entries
from cr.Spec.Server.Service.Annotations into it (same behavior as the non-nil
branch) to avoid mutating the CR in-memory; update the block that reads/sets
svc.Annotations (and thus what later gets assigned to existingSVC.Annotations)
so both branches use a newly allocated map and a loop to copy keys from
cr.Spec.Server.Service.Annotations.
---
Duplicate comments:
In `@controllers/argocd/service_test.go`:
- Around line 146-187: The tests call the package-level flag setter
argoutil.SetRouteAPIFound and do not restore the original routeAPIFound value,
causing global state leakage between subtests; update each subtest that calls
argoutil.SetRouteAPIFound (e.g., the "Server Service annotations update" and
"Server Service annotations update with Openshift auto TLS annotation" cases) to
capture the current routeAPIFound value before changing it and defer restoring
it (or call SetRouteAPIFound with the saved value in a defer) immediately after
saving so the global flag is returned to its original state after the subtest
finishes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/v1alpha1/argocd_types.goapi/v1alpha1/zz_generated.deepcopy.goapi/v1beta1/argocd_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/argoproj.io_argocds.yamlconfig/crd/bases/argoproj.io_argocds.yamlcontrollers/argocd/service.gocontrollers/argocd/service_test.godeploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- api/v1alpha1/argocd_types.go
- api/v1beta1/argocd_types.go
- api/v1beta1/zz_generated.deepcopy.go
Signed-off-by: Maayan Galindez <maayangg2005@gmail.com>
Signed-off-by: Maayan Galindez <maayangg2005@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controllers/argocd/service_test.go (1)
146-187:SetRouteAPIFoundis still not reset after each subtest — cross-test pollution remains.Both subtests mutate the package-level Route API flag but never restore it. After
TestReconcileServerServicecompletes, the flag is left astrue, which can affect subsequent tests in the same package.🧪 Suggested fix
t.Run("Server Service annotations update", func(t *testing.T) { // Reconcile with previous existing Server Service with different Annotations argoutil.SetRouteAPIFound(false) + t.Cleanup(func() { argoutil.SetRouteAPIFound(false) }) a.Spec.Server.Service.Annotations = map[string]string{"test.kubernetes.io/test": "test"} t.Run("Server Service annotations update with Openshift auto TLS annotation", func(t *testing.T) { // Reconcile with previous existing Server Service with different Annotations and the AutoTLSAnnotation argoutil.SetRouteAPIFound(true) + t.Cleanup(func() { argoutil.SetRouteAPIFound(false) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocd/service_test.go` around lines 146 - 187, Test subtests mutate the package-level Route API flag via argoutil.SetRouteAPIFound and never restore it, causing cross-test pollution; fix by capturing the original flag value before each subtest (or at test start) and restoring it after the subtest (use defer to reset) wherever argoutil.SetRouteAPIFound(...) is called in TestReconcileServerService (references: argoutil.SetRouteAPIFound, the subtests "Server Service annotations update" and "Server Service annotations update with Openshift auto TLS annotation", and the reconciler method r.reconcileServerService) so the global state is returned to its prior value for other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/argocd/service.go`:
- Around line 540-547: The code replaces existingSVC.Annotations with
svc.Annotations, which wipes annotations managed by other controllers; instead
merge annotations: iterate svc.Annotations and copy/overwrite those keys into
existingSVC.Annotations, and only remove keys that this controller previously
managed (track or hardcode the AutoTLS key handled by ensureAutoTLSAnnotation)
rather than clearing all absent keys; update the change detection logic around
svc.Annotations/existingSVC.Annotations and the explanation string to reflect
only additive updates or managed-key removals (refer to svc.Annotations,
existingSVC.Annotations and ensureAutoTLSAnnotation to locate the logic).
---
Duplicate comments:
In `@controllers/argocd/service_test.go`:
- Around line 146-187: Test subtests mutate the package-level Route API flag via
argoutil.SetRouteAPIFound and never restore it, causing cross-test pollution;
fix by capturing the original flag value before each subtest (or at test start)
and restoring it after the subtest (use defer to reset) wherever
argoutil.SetRouteAPIFound(...) is called in TestReconcileServerService
(references: argoutil.SetRouteAPIFound, the subtests "Server Service annotations
update" and "Server Service annotations update with Openshift auto TLS
annotation", and the reconciler method r.reconcileServerService) so the global
state is returned to its prior value for other tests.
| if !reflect.DeepEqual(svc.Annotations, existingSVC.Annotations) { | ||
| existingSVC.Annotations = svc.Annotations | ||
| if changed { | ||
| explanation += ", " | ||
| } | ||
| explanation += "service annotations" | ||
| changed = true | ||
| } |
There was a problem hiding this comment.
Full annotation replacement silently wipes annotations not managed by this controller.
svc.Annotations contains only AutoTLS and user-specified annotations. existingSVC (fetched from the API at line 513) may carry additional annotations set by other controllers, CNI plugins, service mesh, or kubectl/Terraform. Assigning existingSVC.Annotations = svc.Annotations at line 541 deletes all such external annotations on every reconcile that detects any diff.
Before this PR, ensureAutoTLSAnnotation only added or removed a single specific key, leaving all other annotations intact. This PR introduces a regression for any annotation not explicitly listed in cr.Spec.Server.Service.Annotations.
A minimal fix without tackling full ownership tracking is to merge user annotations additively and handle removals only for keys previously known to be operator-managed:
🐛 Proposed fix – merge user annotations instead of full replacement
- if !reflect.DeepEqual(svc.Annotations, existingSVC.Annotations) {
- existingSVC.Annotations = svc.Annotations
- if changed {
- explanation += ", "
- }
- explanation += "service annotations"
- changed = true
- }
+ // Merge user-specified annotations onto existingSVC without wiping externally-set keys.
+ for k, v := range cr.Spec.Server.Service.Annotations {
+ if existingSVC.Annotations == nil {
+ existingSVC.Annotations = make(map[string]string)
+ }
+ if existingSVC.Annotations[k] != v {
+ existingSVC.Annotations[k] = v
+ if !changed {
+ explanation = "service annotations"
+ } else {
+ explanation += ", service annotations"
+ }
+ changed = true
+ break // explanation already updated; loop continues updating the map
+ }
+ }
+ // Note: removal of annotations that were previously in the CR but are now
+ // absent requires ownership tracking (e.g. a "last-applied-annotations"
+ // annotation) and is deferred to a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/argocd/service.go` around lines 540 - 547, The code replaces
existingSVC.Annotations with svc.Annotations, which wipes annotations managed by
other controllers; instead merge annotations: iterate svc.Annotations and
copy/overwrite those keys into existingSVC.Annotations, and only remove keys
that this controller previously managed (track or hardcode the AutoTLS key
handled by ensureAutoTLSAnnotation) rather than clearing all absent keys; update
the change detection logic around svc.Annotations/existingSVC.Annotations and
the explanation string to reflect only additive updates or managed-key removals
(refer to svc.Annotations, existingSVC.Annotations and ensureAutoTLSAnnotation
to locate the logic).
| Type corev1.ServiceType `json:"type"` | ||
|
|
||
| // Annotations is the map of annotations to apply to the Server Service. | ||
| Annotations map[string]string `json:"annotations,omitempty"` |
There was a problem hiding this comment.
cant we rely on metadata.Annotations ?
There was a problem hiding this comment.
I’m thinking about renaming it to ExtraAnnotations and changing how it works.
I don’t want to rely on metadata.annotations because I’d prefer a more declarative way to add annotations to the server service. This would make it clearer that certain annotations are required for gRPC ingress to work with our CNI.
It should also help new team members, so they don’t have to guess that these annotations need to be added manually. In addition, it could make deploying Argo CD across multiple sites and instances easier and more consistent.
Thanks again for the review. I’d really appreciate any feedback. And if I misunderstood something or if you think this feature isn’t necessary, I’m happy to hear your opinion.
There was a problem hiding this comment.
IMHO, this can be achieved using the existing metadata.annotations field in Kubernetes. Since metadata is already a well-defined and structured section (like labels, annotations, etc.), it is more consistent and future-proof to rely on it instead of introducing a new custom annotations field in the CRD.
If we start adding separate fields for annotations now, we may eventually need similar fields for labels or other metadata as well. Continuously extending the CRD in this way could unnecessarily increase its length and complexity.
We should also keep in mind that Kubernetes enforces a size limit on annotations. The maximum total size of all annotations (combined keys and values) for a single object is 256 KiB (262,144 bytes). If this limit is exceeded, the API server will reject the object. This is an important constraint to consider in the design.
If an additional field is truly necessary, it would be better to follow the same metadata structure used by Kubernetes objects. That would keep the design consistent and aligned with Kubernetes conventions.
There was a problem hiding this comment.
Thanks for the detailed feedback, I definitely understand your point about keeping things aligned with the existing metadata structure and avoiding unnecessary CRD expansion. That makes sense from a consistency and long term maintenance perspective.
I’m not strongly attached to a specific implementation. The main goal here is to make it possible to add annotations to the Argo CD server Service in a way that can be defined and documented directly in the CR. It would also make this configuration automated for users who deploy the Argo CD CR via Terraform or other Argo CD instances, similar to how we currently handle ingress annotations.
If you feel this feature doesn’t provide enough value or doesn’t align with the project’s direction, I’m completely fine with closing the PR. But if there’s a preferred way to support this kind of feature, I’d really appreciate your guidance:
- How would you like to see this implemented?
- How should conflicts or overrides behave?
- Should there be additional restrictions or validation around which annotations can be set?
I’m happy to rework the proposal to better match the project’s conventions, just let me know what direction you’d prefer :).
2 participants
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
this PR adds the ability to control the argocd-server service annotations, most of the users won't benefit from this PR but it'll help for me and my team to have a cleaner argocd setup, we use terraform to create resources in argocd which requires gRPC ingress (already a feature) but in our case we need to add some annotations to the service before using gRPC in our CNI.
Have you updated the necessary documentation?
I don't think there's any required documentation to be updated or created
Which issue(s) this PR fixes:
Fixes #651
How to test changes / Special notes to the reviewer:
In the argocd manifest:
and then observe if there's annotations created in the argocd-server service,
Thanks for taking the time to review this :).
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests