Skip to content

Commit 617b5ef

Browse files
committed
GitOps: stop the Argo field list crashing the page, and show Secret changes
Expanding an Argo CD row with the Argo API connected crashed the GitOps page whenever Argo's diff had no field entries: the server sent fieldEntries as null and the UI read its length. The server now always sends a list, and the UI treats null as empty for older Radars. Also in the same response: - Changed Secret values were masked with one marker on both sides, so a rotation read as no difference. Each side now has its own marker. - The 50-entry cap now comes with fieldEntriesTruncated, so the panel says the list was cut instead of showing 50 as the count. - A resource missing on one side gets no field list; the YAML diff already shows the whole object. - Drift values are encoded without HTML escaping, so <, > and & no longer show as <, > and &.
1 parent c9962a3 commit 617b5ef

9 files changed

Lines changed: 215 additions & 45 deletions

File tree

‎docs/gitops.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ GitOps CRs that are themselves managed (a child Application from an app-of-apps
4646
Per managed resource, in one row:
4747

4848
- **Sync status** chip (`Synced`, `OutOfSync`, etc.) with the health chip beside it
49-
- **Field-level drift** computed inline from each resource's `kubectl.kubernetes.io/last-applied-configuration` annotation vs live cluster state. You see actual `spec.X removed / spec.Y added` entries — no `argocd app diff` round-trip needed. Live objects are pulled via a dedicated direct-GET path so the dynamic informer cache doesn't have to retain the annotation cluster-wide
49+
- **Field-level drift** computed inline from each resource's `kubectl.kubernetes.io/last-applied-configuration` annotation vs live cluster state. You see actual `spec.X removed / spec.Y added` entries — no `argocd app diff` round-trip needed. Live objects are pulled via a dedicated direct-GET path so the dynamic informer cache doesn't have to retain the annotation cluster-wide. When the [Argo CD API integration](#argo-cd-api-integration-deep-diff) is connected, an expanded row instead shows the field list from Argo's own diff above the YAML diff, which also covers Git changes that haven't synced yet. That list shows at most 50 fields and says so when it was cut. It is left out for a resource missing on one side (not yet created, or pending prune), where the YAML diff shows the whole object
5050
- **Recent events** (5 most recent, namespace-RBAC-filtered) so `ImagePullBackOff`, `FailedScheduling`, webhook denials, PVC pending, etc. show up next to the resource that caused them
5151
- **Open** button — drops into the standard resource drawer
5252

@@ -164,8 +164,10 @@ Behavior and guarantees:
164164
on the target resource's kind) before Argo is consulted. Users can never
165165
see manifests through Radar's token that Radar's RBAC would deny.
166166
- **Secret manifests are structurally redacted** — `data`/`stringData` values
167-
are masked with per-key changed/unchanged markers on both sides of the
168-
diff. There is no un-redact option.
167+
are masked per key. A value that is the same on both sides reads
168+
`<redacted:unchanged>`; a value that differs reads `<redacted:desired>` and
169+
`<redacted:live>`, so a rotated value still shows as a change. There is no
170+
un-redact option.
169171
- Local CLI/Desktop tokens live in `~/.radar/clusters.json` (written `0600`)
170172
and are never returned by Settings APIs. Unchanged credentials are preserved;
171173
explicit removal deletes only this context's saved settings and credentials.

‎internal/server/argo_diff.go‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,29 @@ import (
2626
// is serialized so redaction can't be bypassed through the annotation.
2727
const lastAppliedAnnotation = "kubectl.kubernetes.io/last-applied-configuration"
2828

29+
// Changed values get a different marker on each side so a rotated value still
30+
// reads as a difference in both the YAML pair and the field list.
2931
const (
3032
redactedUnchanged = "<redacted:unchanged>"
31-
redactedChanged = "<redacted:changed>"
33+
redactedDesired = "<redacted:desired>"
34+
redactedLive = "<redacted:live>"
3235
)
3336

3437
// argoResourceDiffResponse is the desired-vs-live diff for a single resource
3538
// managed by an Argo CD Application. Desired/Live are YAML manifest strings
3639
// (empty when that side doesn't exist); FieldEntries is a scannable per-field
37-
// summary derived from the same manifests. For Secrets the values are masked
38-
// and Redacted is true.
40+
// summary derived from the same manifests, empty when either side is missing
41+
// (a whole-object add or delete has no useful per-field breakdown), and
42+
// FieldEntriesTruncated says it was cut at the entry cap. For Secrets the
43+
// values are masked and Redacted is true.
3944
type argoResourceDiffResponse struct {
40-
Source string `json:"source"`
41-
Desired string `json:"desired"`
42-
Live string `json:"live"`
43-
FieldEntries []gitopsinsights.DriftEntry `json:"fieldEntries"`
44-
Redacted bool `json:"redacted"`
45-
Hook bool `json:"hook"`
45+
Source string `json:"source"`
46+
Desired string `json:"desired"`
47+
Live string `json:"live"`
48+
FieldEntries []gitopsinsights.DriftEntry `json:"fieldEntries"`
49+
FieldEntriesTruncated bool `json:"fieldEntriesTruncated,omitempty"`
50+
Redacted bool `json:"redacted"`
51+
Hook bool `json:"hook"`
4652
}
4753

4854
// handleArgoResourceDiff serves the desired-vs-live manifest diff for one
@@ -186,13 +192,19 @@ func (s *Server) handleArgoResourceDiff(w http.ResponseWriter, r *http.Request)
186192
redacted = true
187193
}
188194

195+
fieldEntries, truncated := []gitopsinsights.DriftEntry{}, false
196+
if len(desiredObj) > 0 && len(liveObj) > 0 {
197+
fieldEntries, truncated = gitopsinsights.DiffObjects(desiredObj, liveObj)
198+
}
199+
189200
s.writeJSON(w, argoResourceDiffResponse{
190-
Source: "argocd-api",
191-
Desired: manifestToYAML(desiredObj),
192-
Live: manifestToYAML(liveObj),
193-
FieldEntries: gitopsinsights.DiffObjects(desiredObj, liveObj),
194-
Redacted: redacted,
195-
Hook: item.Hook,
201+
Source: "argocd-api",
202+
Desired: manifestToYAML(desiredObj),
203+
Live: manifestToYAML(liveObj),
204+
FieldEntries: fieldEntries,
205+
FieldEntriesTruncated: truncated,
206+
Redacted: redacted,
207+
Hook: item.Hook,
196208
})
197209
}
198210

@@ -242,7 +254,7 @@ func isInClusterDestination(app *unstructured.Unstructured) bool {
242254
// material — service-account tokens, bootstrap tokens, the last-applied dump).
243255
// Key names stay visible so the operator still sees what changed.
244256
func redactSecretManifest(desired, live map[string]any) {
245-
redactSecretManifestWithMarkers(desired, live, redactedChanged, redactedChanged)
257+
redactSecretManifestWithMarkers(desired, live, redactedDesired, redactedLive)
246258
}
247259

248260
func redactSecretManifestWithMarkers(desired, live map[string]any, desiredChanged, liveChanged string) {

‎internal/server/argo_diff_test.go‎

Lines changed: 105 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"encoding/base64"
66
"encoding/json"
7+
"fmt"
78
"net/http"
89
"net/http/httptest"
910
"strings"
@@ -209,6 +210,98 @@ func TestArgoResourceDiff_HappyPath(t *testing.T) {
209210
}
210211
}
211212

213+
// serveArgoDiff runs the handler for one ConfigMap managed-set entry and
214+
// returns the raw body plus the decoded response.
215+
func serveArgoDiff(t *testing.T, item argoapi.ResourceDiff) (string, argoResourceDiffResponse) {
216+
t.Helper()
217+
seedInClusterArgoApp(t, "argocd", "guestbook")
218+
items := managedResourcesJSON(t, item)
219+
srv, _ := startFakeArgoServer(t, func(w http.ResponseWriter, r *http.Request) {
220+
_, _ = w.Write(items)
221+
})
222+
connectArgo(t, srv.URL)
223+
224+
req := newDiffRequest("argocd", "guestbook", "kind="+item.Kind+"&resourceNamespace="+item.Namespace+"&resourceName="+item.Name)
225+
w := httptest.NewRecorder()
226+
(&Server{}).handleArgoResourceDiff(w, req)
227+
if w.Code != http.StatusOK {
228+
t.Fatalf("status = %d, body = %s", w.Code, w.Body.String())
229+
}
230+
var resp argoResourceDiffResponse
231+
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
232+
t.Fatalf("decode: %v", err)
233+
}
234+
return w.Body.String(), resp
235+
}
236+
237+
func configMapManifest(data map[string]any) map[string]any {
238+
return map[string]any{
239+
"apiVersion": "v1", "kind": "ConfigMap",
240+
"metadata": map[string]any{"name": "settings", "namespace": "default"},
241+
"data": data,
242+
}
243+
}
244+
245+
// TestArgoResourceDiff_FieldEntriesNeverNull pins that an empty field list is
246+
// sent as [] (UI consumers read its length), for an in-sync resource and for a
247+
// resource missing on one side.
248+
func TestArgoResourceDiff_FieldEntriesNeverNull(t *testing.T) {
249+
same := configMapManifest(map[string]any{"mode": "steady"})
250+
cases := map[string]argoapi.ResourceDiff{
251+
"in sync": {
252+
Kind: "ConfigMap", Namespace: "default", Name: "settings",
253+
PredictedLiveState: jsonManifest(t, same), NormalizedLiveState: jsonManifest(t, same),
254+
},
255+
"missing from cluster": {
256+
Kind: "ConfigMap", Namespace: "default", Name: "settings",
257+
PredictedLiveState: jsonManifest(t, same),
258+
},
259+
"pending prune": {
260+
Kind: "ConfigMap", Namespace: "default", Name: "settings",
261+
NormalizedLiveState: jsonManifest(t, same),
262+
},
263+
}
264+
for name, item := range cases {
265+
t.Run(name, func(t *testing.T) {
266+
body, resp := serveArgoDiff(t, item)
267+
if !strings.Contains(body, `"fieldEntries":[]`) {
268+
t.Errorf("fieldEntries must serialize as []:\n%s", body)
269+
}
270+
if len(resp.FieldEntries) != 0 || resp.FieldEntriesTruncated {
271+
t.Errorf("fieldEntries = %+v, truncated = %v; want none", resp.FieldEntries, resp.FieldEntriesTruncated)
272+
}
273+
})
274+
}
275+
}
276+
277+
// TestArgoResourceDiff_FieldEntriesTruncated pins that a diff cut at the entry
278+
// cap says so, instead of presenting the cap as the exact count.
279+
func TestArgoResourceDiff_FieldEntriesTruncated(t *testing.T) {
280+
desired, live := map[string]any{}, map[string]any{}
281+
for i := 0; i < 60; i++ {
282+
key := fmt.Sprintf("key%02d", i)
283+
desired[key] = "desired"
284+
live[key] = "live"
285+
}
286+
body, resp := serveArgoDiff(t, argoapi.ResourceDiff{
287+
Kind: "ConfigMap", Namespace: "default", Name: "settings",
288+
PredictedLiveState: jsonManifest(t, configMapManifest(desired)),
289+
NormalizedLiveState: jsonManifest(t, configMapManifest(live)),
290+
})
291+
if !resp.FieldEntriesTruncated {
292+
t.Errorf("fieldEntriesTruncated = false for 60 differing fields:\n%s", body)
293+
}
294+
295+
_, small := serveArgoDiff(t, argoapi.ResourceDiff{
296+
Kind: "ConfigMap", Namespace: "default", Name: "settings",
297+
PredictedLiveState: jsonManifest(t, configMapManifest(map[string]any{"a": "1"})),
298+
NormalizedLiveState: jsonManifest(t, configMapManifest(map[string]any{"a": "2"})),
299+
})
300+
if small.FieldEntriesTruncated {
301+
t.Errorf("fieldEntriesTruncated = true for a one-field diff")
302+
}
303+
}
304+
212305
func TestArgoResourceDiff_SecretRedacted(t *testing.T) {
213306
seedInClusterArgoApp(t, "argocd", "guestbook")
214307
newPass := base64.StdEncoding.EncodeToString([]byte("BRAND-NEW-PASSWORD"))
@@ -268,20 +361,27 @@ func TestArgoResourceDiff_SecretRedacted(t *testing.T) {
268361
if !resp.Redacted {
269362
t.Errorf("Secret diff must set redacted=true")
270363
}
271-
// password differs → changed on both sides; shared identical → unchanged;
272-
// extra present only on live → changed.
273-
if !strings.Contains(resp.Desired, "password: "+redactedChanged) {
364+
// password differs → a different marker per side, so it still reads as a
365+
// change; shared identical → unchanged; extra present only on live → live marker.
366+
if !strings.Contains(resp.Desired, "password: "+redactedDesired) {
274367
t.Errorf("desired.password should be marked changed:\n%s", resp.Desired)
275368
}
276-
if !strings.Contains(resp.Live, "password: "+redactedChanged) {
369+
if !strings.Contains(resp.Live, "password: "+redactedLive) {
277370
t.Errorf("live.password should be marked changed:\n%s", resp.Live)
278371
}
279372
if !strings.Contains(resp.Desired, "shared: "+redactedUnchanged) {
280373
t.Errorf("desired.shared should be marked unchanged:\n%s", resp.Desired)
281374
}
282-
if !strings.Contains(resp.Live, "extra: "+redactedChanged) {
375+
if !strings.Contains(resp.Live, "extra: "+redactedLive) {
283376
t.Errorf("live-only key should be marked changed:\n%s", resp.Live)
284377
}
378+
paths := map[string]bool{}
379+
for _, e := range resp.FieldEntries {
380+
paths[e.Path] = true
381+
}
382+
if !paths["data.password"] || !paths["data.extra"] || paths["data.shared"] {
383+
t.Errorf("field entries = %+v; want data.password and data.extra, not data.shared", resp.FieldEntries)
384+
}
285385
}
286386

287387
// TestArgoResourceDiff_SecretRedactionFailClosed pins the fail-closed paths:

‎packages/k8s-ui/src/components/gitops/insights/ArgoResourceDiff.test.tsx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ describe('ArgoResourceDiff field list', () => {
4545
expect(html).not.toContain('Field diff')
4646
})
4747

48+
test('treats a null field list (sent by older Radars) as empty instead of crashing', () => {
49+
const html = renderToStaticMarkup(<ArgoResourceDiff diff={{ ...argoDiff, fieldEntries: null }} loading={false} fallbackDrift={lastApplied} />)
50+
expect(html).not.toContain('Field diff')
51+
expect(html).toContain('desired (Git-rendered)')
52+
})
53+
54+
test('says the list was cut short instead of presenting the cap as the count', () => {
55+
const html = renderToStaticMarkup(<ArgoResourceDiff diff={{ ...argoDiff, fieldEntriesTruncated: true }} loading={false} />)
56+
expect(html).toContain('showing first 50 entries')
57+
expect(html).not.toContain('2 fields')
58+
})
59+
4860
test("falls back to the last-applied list when Argo's diff fails to load", () => {
4961
const html = renderToStaticMarkup(<ArgoResourceDiff loading={false} error="Failed to fetch the diff from the Argo CD API server." fallbackDrift={lastApplied} />)
5062
expect(html).toContain('Field diff')

‎packages/k8s-ui/src/components/gitops/insights/ArgoResourceDiff.tsx‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,14 @@ export function ArgoResourceDiff({ diff, loading, error, fallbackDrift }: ArgoRe
6262
if (!diff) return fallbackPanel
6363

6464
const unchanged = !docsDiffer(diff.desired, diff.live)
65+
// Older Radars send null for an empty list.
66+
const fieldEntries = diff.fieldEntries ?? []
6567

6668
return (
6769
<div>
68-
{diff.fieldEntries.length > 0 && (
70+
{fieldEntries.length > 0 && (
6971
<div className="mb-3">
70-
<DriftPanel drift={{ entries: diff.fieldEntries, source: diff.source }} />
72+
<DriftPanel drift={{ entries: fieldEntries, source: diff.source, truncated: diff.fieldEntriesTruncated }} />
7173
</div>
7274
)}
7375
<div className="mb-2 flex items-center justify-between gap-2">

‎packages/k8s-ui/src/types/gitops-insights.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,13 +283,16 @@ export interface ArgoRevisionMetadata {
283283
// Response of GET /api/argo/applications/{ns}/{name}/resource-diff. `desired`
284284
// is the Git-rendered manifest, `live` the normalized cluster state — both YAML
285285
// for the line-by-line view. `fieldEntries` is the same diff structured per
286-
// path for a compact summary. `redacted` is set when Secret values were masked
286+
// path for a compact summary; Radars before the list was made non-null send
287+
// null when it is empty. `fieldEntriesTruncated` is set when the list was cut
288+
// at the server's entry cap. `redacted` is set when Secret values were masked
287289
// server-side; `hook` marks Argo sync-hook resources.
288290
export interface GitOpsResourceDiff {
289291
source: string
290292
desired: string
291293
live: string
292-
fieldEntries: GitOpsResourceDiffFieldEntry[]
294+
fieldEntries: GitOpsResourceDiffFieldEntry[] | null
295+
fieldEntriesTruncated?: boolean
293296
redacted: boolean
294297
hook: boolean
295298
}

‎pkg/gitops/insights/diffobjects.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,10 @@ import "sort"
1818
// applied upstream. This makes DiffObjects a field-level *summary* only — the
1919
// canonical, authoritative view is the desired/live YAML pair the caller
2020
// renders alongside these entries. Output is sorted by path and capped at
21-
// driftEntryCap.
22-
func DiffObjects(desired, live map[string]any) []DriftEntry {
23-
var entries []DriftEntry
21+
// driftEntryCap; truncated reports whether the cap cut entries off. The slice
22+
// is never nil, so an empty diff serializes as [] rather than null.
23+
func DiffObjects(desired, live map[string]any) (entries []DriftEntry, truncated bool) {
24+
entries = []DriftEntry{}
2425
for _, key := range unionKeys(desired, live) {
2526
switch key {
2627
case "status":
@@ -31,8 +32,7 @@ func DiffObjects(desired, live map[string]any) []DriftEntry {
3132
entries = diffValues(key, desired[key], live[key], entries)
3233
}
3334
}
34-
entries, _ = sortAndCapDriftEntries(entries)
35-
return entries
35+
return sortAndCapDriftEntries(entries)
3636
}
3737

3838
// diffMetadataScoped diffs only the labels and annotations sub-maps of two

‎pkg/gitops/insights/diffobjects_test.go‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ func TestDiffObjects(t *testing.T) {
8282

8383
for _, tc := range tests {
8484
t.Run(tc.name, func(t *testing.T) {
85-
entries := DiffObjects(tc.desired, tc.live)
85+
entries, truncated := DiffObjects(tc.desired, tc.live)
86+
if truncated {
87+
t.Errorf("truncated = true for %d entries", len(entries))
88+
}
8689
got := make(map[string]bool, len(entries))
8790
paths := make([]string, 0, len(entries))
8891
for _, e := range entries {
@@ -121,10 +124,13 @@ func TestDiffObjectsSortedAndCapped(t *testing.T) {
121124
lSpec[key] = "live"
122125
}
123126

124-
entries := DiffObjects(desired, live)
127+
entries, truncated := DiffObjects(desired, live)
125128
if len(entries) != driftEntryCap {
126129
t.Fatalf("entry count = %d, want cap %d", len(entries), driftEntryCap)
127130
}
131+
if !truncated {
132+
t.Errorf("truncated = false, want true when the diff exceeds the cap")
133+
}
128134
for i := 1; i < len(entries); i++ {
129135
if entries[i-1].Path > entries[i].Path {
130136
t.Fatalf("entries not sorted: %q > %q at index %d", entries[i-1].Path, entries[i].Path, i)
@@ -138,3 +144,36 @@ func TestDiffObjectsSortedAndCapped(t *testing.T) {
138144
t.Errorf("first entry = %q, want spec.field000 (sort applied before cap)", first)
139145
}
140146
}
147+
148+
// TestDiffObjectsNoDifferencesIsEmptyNotNil pins that an identical pair yields
149+
// an empty, non-nil slice: callers serialize it, and null breaks consumers that
150+
// read its length.
151+
func TestDiffObjectsNoDifferencesIsEmptyNotNil(t *testing.T) {
152+
obj := map[string]any{"spec": map[string]any{"replicas": int64(1)}}
153+
entries, truncated := DiffObjects(obj, obj)
154+
if entries == nil {
155+
t.Fatal("entries = nil, want an empty slice")
156+
}
157+
if len(entries) != 0 || truncated {
158+
t.Errorf("entries = %v, truncated = %v; want empty, false", entries, truncated)
159+
}
160+
}
161+
162+
// TestDiffObjectsValuesNotHTMLEscaped pins that values render as typed: a
163+
// redaction marker or a shell command must not show up as <...>.
164+
func TestDiffObjectsValuesNotHTMLEscaped(t *testing.T) {
165+
entries, _ := DiffObjects(
166+
map[string]any{"data": map[string]any{"password": "<redacted:desired>", "cmd": "a && b > c"}},
167+
map[string]any{"data": map[string]any{"password": "<redacted:live>", "cmd": "a"}},
168+
)
169+
got := map[string]string{}
170+
for _, e := range entries {
171+
got[e.Path] = e.Desired
172+
}
173+
if got["data.password"] != `"<redacted:desired>"` {
174+
t.Errorf("data.password desired = %s", got["data.password"])
175+
}
176+
if got["data.cmd"] != `"a && b > c"` {
177+
t.Errorf("data.cmd desired = %s", got["data.cmd"])
178+
}
179+
}

0 commit comments

Comments
 (0)