Skip to content

Commit 34a5202

Browse files
committed
Keep Helm release payloads out of the Secret cache
Radar's Secret informer cached every Helm release Secret with its full encoded release (30-400KB each, up to 10 revisions per release). At 300 releases that was about 390MB of Go heap held for nothing: the Helm views and Issues read release Secrets from the API server, not the cache. The informer transform now replaces each value of a helm.sh/release.v1 Secret with a SHA-256 digest of it. Keys stay, so key counts and missing-key checks are unchanged, and the timeline still records a rewrite as "data modified keys", since a changed value still compares as changed. Paths that serve cached Secrets drop the digests, and opening one of these Secrets reads it from the API server as the caller.
1 parent 617b5ef commit 34a5202

7 files changed

Lines changed: 541 additions & 9 deletions

File tree

‎internal/k8s/fetch.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"k8s.io/apimachinery/pkg/runtime"
1717
"k8s.io/apimachinery/pkg/runtime/schema"
1818

19+
"github.com/skyhook-io/radar/pkg/k8score"
1920
"github.com/skyhook-io/radar/pkg/resourceid"
2021
)
2122

@@ -349,14 +350,14 @@ func FetchResourceList(cache *ResourceCache, kind string, namespaces []string) (
349350
if err != nil {
350351
return nil, err
351352
}
352-
return ToRuntimeObjects(items), nil
353+
return ToRuntimeObjects(ServableSecrets(items)), nil
353354
},
354355
func(ns string) ([]runtime.Object, error) {
355356
items, err := cache.Secrets().Secrets(ns).List(labels.Everything())
356357
if err != nil {
357358
return nil, err
358359
}
359-
return ToRuntimeObjects(items), nil
360+
return ToRuntimeObjects(ServableSecrets(items)), nil
360361
},
361362
)
362363
case "events":
@@ -717,7 +718,11 @@ func FetchResource(cache *ResourceCache, kind, namespace, name string) (runtime.
717718
if cache.Secrets() == nil {
718719
return nil, fmt.Errorf("forbidden: secrets")
719720
}
720-
return cache.Secrets().Secrets(namespace).Get(name)
721+
secret, err := cache.Secrets().Secrets(namespace).Get(name)
722+
if err != nil {
723+
return nil, err
724+
}
725+
return k8score.WithoutHelmReleaseValues(secret), nil
721726
case "events":
722727
if cache.Events() == nil {
723728
return nil, fmt.Errorf("forbidden: events")
@@ -908,3 +913,13 @@ func SetTypeMeta(resource any) {
908913
r.Kind = "ClusterRoleBinding"
909914
}
910915
}
916+
917+
// ServableSecrets returns cached Secrets as they may be served: Helm release
918+
// Secrets without values (see k8score.WithoutHelmReleaseValues).
919+
func ServableSecrets(items []*corev1.Secret) []*corev1.Secret {
920+
out := make([]*corev1.Secret, len(items))
921+
for i, item := range items {
922+
out[i] = k8score.WithoutHelmReleaseValues(item)
923+
}
924+
return out
925+
}
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package k8s
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
corev1 "k8s.io/api/core/v1"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
"k8s.io/client-go/kubernetes/fake"
10+
11+
"github.com/skyhook-io/radar/pkg/k8score"
12+
)
13+
14+
func helmReleaseSecret(rv, status, payload string) *corev1.Secret {
15+
return &corev1.Secret{
16+
ObjectMeta: metav1.ObjectMeta{
17+
Name: "sh.helm.release.v1.web.v1", Namespace: "apps", UID: "uid-web-1", ResourceVersion: rv,
18+
Labels: map[string]string{"owner": "helm", "name": "web", "status": status, "version": "1"},
19+
},
20+
Type: k8score.HelmReleaseSecretType,
21+
Data: map[string][]byte{"release": []byte(payload)},
22+
}
23+
}
24+
25+
func cachedHelmReleaseSecret(t *testing.T, rv, status, payload string) *corev1.Secret {
26+
t.Helper()
27+
obj, err := k8score.DropManagedFields(helmReleaseSecret(rv, status, payload))
28+
if err != nil {
29+
t.Fatal(err)
30+
}
31+
return obj.(*corev1.Secret)
32+
}
33+
34+
// Helm hooks rewrite a release Secret with the same labels. The cache holds
35+
// digests, not payloads, and the timeline must record the rewrite exactly as
36+
// it does when it can compare the payloads.
37+
func TestComputeDiff_HelmReleaseRewriteMatchesUncachedDiff(t *testing.T) {
38+
uncached := ComputeDiff("Secret", helmReleaseSecret("10", "pending-upgrade", "hook-1"), helmReleaseSecret("11", "pending-upgrade", "hook-2"))
39+
cached := ComputeDiff("Secret",
40+
cachedHelmReleaseSecret(t, "10", "pending-upgrade", "hook-1"),
41+
cachedHelmReleaseSecret(t, "11", "pending-upgrade", "hook-2"))
42+
if uncached == nil || cached == nil || cached.Summary != uncached.Summary || cached.Summary != "data modified keys: [release]" {
43+
t.Fatalf("cached diff = %+v, uncached diff = %+v", cached, uncached)
44+
}
45+
if same := ComputeDiff("Secret",
46+
cachedHelmReleaseSecret(t, "11", "pending-upgrade", "hook-2"),
47+
cachedHelmReleaseSecret(t, "12", "pending-upgrade", "hook-2")); same != nil {
48+
t.Fatalf("unchanged payload recorded as a change: %+v", same)
49+
}
50+
}
51+
52+
// Every path that hands cached Secrets to a caller serves Helm release
53+
// Secrets without the digests the cache keeps in place of their values.
54+
func TestCachedHelmReleaseSecretServedWithoutValues(t *testing.T) {
55+
t.Cleanup(func() { ResetResourceCache() })
56+
if err := InitTestResourceCache(fake.NewClientset(helmReleaseSecret("10", "deployed", "payload"))); err != nil {
57+
t.Fatal(err)
58+
}
59+
cache := GetResourceCache()
60+
if raw, _ := cache.Secrets().Secrets("apps").Get("sh.helm.release.v1.web.v1"); raw == nil || len(raw.Data["release"]) == 0 || string(raw.Data["release"]) == "payload" {
61+
t.Fatalf("cache should hold a digest, got %+v", raw)
62+
}
63+
64+
check := func(path string, secret *corev1.Secret) {
65+
t.Helper()
66+
value, ok := secret.Data["release"]
67+
if !ok || value != nil {
68+
t.Fatalf("%s: release = %q (present %v), want the key with no value", path, value, ok)
69+
}
70+
}
71+
obj, err := FetchResource(cache, "secrets", "apps", "sh.helm.release.v1.web.v1")
72+
if err != nil {
73+
t.Fatal(err)
74+
}
75+
check("FetchResource", obj.(*corev1.Secret))
76+
objs, err := FetchResourceList(cache, "secrets", nil)
77+
if err != nil || len(objs) != 1 {
78+
t.Fatalf("FetchResourceList: %v %v", objs, err)
79+
}
80+
check("FetchResourceList", objs[0].(*corev1.Secret))
81+
82+
u, err := cache.GetDynamicWithGroup(context.Background(), "secrets", "apps", "sh.helm.release.v1.web.v1", "")
83+
if err != nil {
84+
t.Fatal(err)
85+
}
86+
data, _ := u.Object["data"].(map[string]any)
87+
if value, ok := data["release"]; !ok || value != nil {
88+
t.Fatalf("GetDynamicWithGroup: release = %v (present %v), want the key with no value", value, ok)
89+
}
90+
}
Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
package server
2+
3+
import (
4+
"encoding/json"
5+
"net/http"
6+
"net/http/httptest"
7+
"testing"
8+
9+
corev1 "k8s.io/api/core/v1"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"k8s.io/apimachinery/pkg/types"
12+
"k8s.io/client-go/kubernetes"
13+
"k8s.io/client-go/kubernetes/fake"
14+
"k8s.io/client-go/rest"
15+
16+
"github.com/skyhook-io/radar/internal/auth"
17+
"github.com/skyhook-io/radar/internal/k8s"
18+
"github.com/skyhook-io/radar/pkg/k8score"
19+
)
20+
21+
const helmSecretTestName = "sh.helm.release.v1.web.v3"
22+
23+
func helmReleaseSecretFixture(payload string) *corev1.Secret {
24+
return &corev1.Secret{
25+
TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"},
26+
ObjectMeta: metav1.ObjectMeta{
27+
Name: helmSecretTestName,
28+
Namespace: "default",
29+
UID: "uid-web-v3",
30+
Labels: map[string]string{"owner": "helm", "name": "web", "status": "deployed", "version": "3"},
31+
},
32+
Type: k8score.HelmReleaseSecretType,
33+
Data: map[string][]byte{"release": []byte(payload)},
34+
}
35+
}
36+
37+
type fakeSecretAPI struct {
38+
url string
39+
gets int
40+
impersonate []string // Impersonate-User of each GET
41+
serveUID string // when set, the live Secret has this UID instead
42+
}
43+
44+
// useHelmReleaseSecretCluster caches one Helm release Secret and points the
45+
// live client at a fake API server that answers GETs for it with payload.
46+
func useHelmReleaseSecretCluster(t *testing.T, payload string) *fakeSecretAPI {
47+
t.Helper()
48+
useTestResourceCache(t, fake.NewClientset(
49+
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "default"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}},
50+
helmReleaseSecretFixture(payload),
51+
&corev1.Secret{
52+
ObjectMeta: metav1.ObjectMeta{Name: "app-config", Namespace: "default"},
53+
Type: corev1.SecretTypeOpaque,
54+
Data: map[string][]byte{"password": []byte("hunter2")},
55+
},
56+
))
57+
58+
fakeAPI := &fakeSecretAPI{}
59+
api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
60+
if r.Method != http.MethodGet || r.URL.Path != "/api/v1/namespaces/default/secrets/"+helmSecretTestName {
61+
http.NotFound(w, r)
62+
return
63+
}
64+
fakeAPI.gets++
65+
fakeAPI.impersonate = append(fakeAPI.impersonate, r.Header.Get("Impersonate-User"))
66+
w.Header().Set("Content-Type", "application/json")
67+
live := helmReleaseSecretFixture(payload)
68+
if fakeAPI.serveUID != "" {
69+
live.UID = types.UID(fakeAPI.serveUID)
70+
}
71+
_ = json.NewEncoder(w).Encode(live)
72+
}))
73+
t.Cleanup(api.Close)
74+
fakeAPI.url = api.URL
75+
prev := k8s.SetTestClient(kubernetes.NewForConfigOrDie(&rest.Config{Host: api.URL}))
76+
t.Cleanup(func() { k8s.SetTestClient(prev) })
77+
return fakeAPI
78+
}
79+
80+
type secretRow struct {
81+
Metadata struct {
82+
Name string `json:"name"`
83+
} `json:"metadata"`
84+
Data map[string]*string `json:"data"`
85+
}
86+
87+
// The cache holds a digest in place of each Helm release value; the list
88+
// must serve the key with no value, for one namespace and for all of them
89+
// (the default view).
90+
func TestSecretsList_HelmReleaseServedWithoutValues(t *testing.T) {
91+
useHelmReleaseSecretCluster(t, "H4sIpayload")
92+
93+
for _, path := range []string{"/api/resources/secrets?namespaces=default", "/api/resources/secrets"} {
94+
resp := get(t, path)
95+
if resp.StatusCode != http.StatusOK {
96+
resp.Body.Close()
97+
t.Fatalf("%s: expected 200, got %d", path, resp.StatusCode)
98+
}
99+
var rows []secretRow
100+
err := json.NewDecoder(resp.Body).Decode(&rows)
101+
resp.Body.Close()
102+
if err != nil {
103+
t.Fatalf("%s: decode: %v", path, err)
104+
}
105+
seen := map[string]secretRow{}
106+
for _, row := range rows {
107+
seen[row.Metadata.Name] = row
108+
}
109+
110+
helm, ok := seen[helmSecretTestName]
111+
if !ok {
112+
t.Fatalf("%s: Helm release Secret missing from the list: %v", path, rows)
113+
}
114+
value, hasKey := helm.Data["release"]
115+
if !hasKey {
116+
t.Fatalf("%s: release key dropped; the Keys column would read 0", path)
117+
}
118+
if value != nil {
119+
t.Fatalf("%s: the list served a value for a Helm release Secret: %q", path, *value)
120+
}
121+
if v := seen["app-config"].Data["password"]; v == nil || *v == "" {
122+
t.Fatalf("%s: an ordinary Secret lost its value: %v", path, seen["app-config"].Data)
123+
}
124+
}
125+
}
126+
127+
func TestSecretGet_HelmReleaseServesLivePayload(t *testing.T) {
128+
fakeAPI := useHelmReleaseSecretCluster(t, "H4sIpayload")
129+
130+
resp := get(t, "/api/resources/secrets/default/"+helmSecretTestName)
131+
defer resp.Body.Close()
132+
if resp.StatusCode != http.StatusOK {
133+
t.Fatalf("expected 200, got %d", resp.StatusCode)
134+
}
135+
var body struct {
136+
Resource secretRow `json:"resource"`
137+
}
138+
if err := json.NewDecoder(resp.Body).Decode(&body); err != nil {
139+
t.Fatalf("decode: %v", err)
140+
}
141+
// JSON carries []byte as base64.
142+
if v := body.Resource.Data["release"]; v == nil || *v != "SDRzSXBheWxvYWQ=" {
143+
t.Fatalf("release value = %v, want the live payload", v)
144+
}
145+
if fakeAPI.gets != 1 {
146+
t.Fatalf("live GETs = %d, want 1", fakeAPI.gets)
147+
}
148+
149+
// Ordinary Secrets are still answered from the cache.
150+
resp2 := get(t, "/api/resources/secrets/default/app-config")
151+
defer resp2.Body.Close()
152+
if resp2.StatusCode != http.StatusOK {
153+
t.Fatalf("expected 200, got %d", resp2.StatusCode)
154+
}
155+
if fakeAPI.gets != 1 {
156+
t.Fatalf("an ordinary Secret was read live (GETs = %d)", fakeAPI.gets)
157+
}
158+
}
159+
160+
// The live read can fail where the cached read would not have: the Secret
161+
// was deleted since it was cached, or the API server refuses the caller.
162+
func TestSecretGet_HelmReleaseLiveReadFailures(t *testing.T) {
163+
for _, tc := range []struct {
164+
name string
165+
reason metav1.StatusReason
166+
code int
167+
}{
168+
{"deleted since cached", metav1.StatusReasonNotFound, http.StatusNotFound},
169+
{"forbidden by the API server", metav1.StatusReasonForbidden, http.StatusForbidden},
170+
} {
171+
t.Run(tc.name, func(t *testing.T) {
172+
useHelmReleaseSecretCluster(t, "H4sIpayload")
173+
failing := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
174+
w.Header().Set("Content-Type", "application/json")
175+
w.WriteHeader(tc.code)
176+
_ = json.NewEncoder(w).Encode(metav1.Status{
177+
TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Status"},
178+
Status: metav1.StatusFailure, Reason: tc.reason, Code: int32(tc.code),
179+
})
180+
}))
181+
t.Cleanup(failing.Close)
182+
prev := k8s.SetTestClient(kubernetes.NewForConfigOrDie(&rest.Config{Host: failing.URL}))
183+
t.Cleanup(func() { k8s.SetTestClient(prev) })
184+
185+
resp := get(t, "/api/resources/secrets/default/"+helmSecretTestName)
186+
defer resp.Body.Close()
187+
if resp.StatusCode != tc.code {
188+
t.Fatalf("expected %d, got %d", tc.code, resp.StatusCode)
189+
}
190+
})
191+
}
192+
}
193+
194+
// With auth on, the live read runs as the caller, never as Radar's own
195+
// identity, so the API server applies the caller's RBAC.
196+
func TestSecretGet_HelmReleaseLiveReadImpersonatesCaller(t *testing.T) {
197+
fakeAPI := useHelmReleaseSecretCluster(t, "H4sIpayload")
198+
prevConfig := k8s.SetTestConfig(&rest.Config{Host: fakeAPI.url})
199+
t.Cleanup(func() { k8s.SetTestConfig(prevConfig) })
200+
201+
env := newAuthTestServer(t)
202+
env.srv.permCache.Set("bob", nil, &auth.UserPermissions{AllowedNamespaces: []string{"default"}})
203+
seedServerSecretGetCanI(t, env, "bob", []string{"default"}, nil)
204+
205+
resp := env.authGet(t, "/api/resources/secrets/default/"+helmSecretTestName, "bob", "")
206+
defer resp.Body.Close()
207+
if resp.StatusCode != http.StatusOK {
208+
t.Fatalf("expected 200, got %d", resp.StatusCode)
209+
}
210+
if len(fakeAPI.impersonate) != 1 || fakeAPI.impersonate[0] != "bob" {
211+
t.Fatalf("live read impersonated %q, want bob", fakeAPI.impersonate)
212+
}
213+
}
214+
215+
// The live read must return the object the cache answered for. Another UID
216+
// under the same name means the Secret was recreated, or the cluster context
217+
// switched between the cache lookup and the read; the caller retries.
218+
func TestSecretGet_HelmReleaseLiveReadOfAnotherObjectIsRetryable(t *testing.T) {
219+
fakeAPI := useHelmReleaseSecretCluster(t, "H4sIpayload")
220+
fakeAPI.serveUID = "uid-from-another-cluster"
221+
222+
resp := get(t, "/api/resources/secrets/default/"+helmSecretTestName)
223+
defer resp.Body.Close()
224+
if resp.StatusCode != http.StatusServiceUnavailable {
225+
t.Fatalf("expected 503, got %d", resp.StatusCode)
226+
}
227+
var body struct {
228+
Code string `json:"error_code"`
229+
}
230+
_ = json.NewDecoder(resp.Body).Decode(&body)
231+
if body.Code != "cluster_connecting" {
232+
t.Fatalf("code = %q, want cluster_connecting so the drawer retries", body.Code)
233+
}
234+
}

0 commit comments

Comments
 (0)