Skip to content

Commit 881591d

Browse files
lexfreilexfrei
committed
fix(securitygroup): log swallowed errors in the membership reconciler
The reconciler intentionally treats a malformed attachments annotation and a failed pod-mapping List as empty / no-op to stay resilient, but it swallowed both errors silently. That hides real defects: a bad annotation writer, or a transient API error that drops a pod update until the next periodic resync with no trace. Log both while preserving the resilient return behavior. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 11ed2a9 commit 881591d

2 files changed

Lines changed: 37 additions & 5 deletions

File tree

‎internal/securitygroupcontroller/reconciler.go‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,14 +109,17 @@ func appLabels(ref sdnv1alpha1.ApplicationReference) map[string]string {
109109
}
110110
}
111111

112-
// decodeAttachments parses the attachments annotation. A missing or malformed
113-
// value yields nil.
114-
func decodeAttachments(s string) []sdnv1alpha1.ApplicationReference {
112+
// decodeAttachments parses the attachments annotation. A missing value yields
113+
// nil; a malformed value is treated as empty (so a bad annotation cannot wedge
114+
// the reconciler) but is logged, since silently swallowing it hides a real
115+
// defect in whatever wrote the annotation.
116+
func decodeAttachments(ctx context.Context, s string) []sdnv1alpha1.ApplicationReference {
115117
if s == "" {
116118
return nil
117119
}
118120
var refs []sdnv1alpha1.ApplicationReference
119121
if err := json.Unmarshal([]byte(s), &refs); err != nil {
122+
log.FromContext(ctx).Error(err, "ignoring malformed attachments annotation", "annotation", attachmentsAnnotation)
120123
return nil
121124
}
122125
return refs
@@ -173,7 +176,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
173176
// directly — is the boundary that stops a SecurityGroup from labeling pods a
174177
// tenant could not otherwise select.
175178
desired := map[string]struct{}{}
176-
for _, app := range decodeAttachments(cnp.Annotations[attachmentsAnnotation]) {
179+
for _, app := range decodeAttachments(ctx, cnp.Annotations[attachmentsAnnotation]) {
177180
pods := &corev1.PodList{}
178181
if err := r.List(ctx, pods, client.InNamespace(ns), client.MatchingLabels(appLabels(app))); err != nil {
179182
return ctrl.Result{}, err
@@ -288,12 +291,16 @@ func (r *Reconciler) mapPodToSGs(ctx context.Context, obj client.Object) []recon
288291

289292
cnps := &CiliumNetworkPolicyList{}
290293
if err := r.List(ctx, cnps, client.InNamespace(pod.Namespace), client.MatchingLabels{sgLabelKey: sgLabelValue}); err != nil {
294+
// Returning no requests is the right signal here, but a transient List
295+
// error would otherwise be invisible — a pod update could be missed
296+
// until the next periodic resync with no trace. Log it.
297+
log.FromContext(ctx).Error(err, "failed to list SecurityGroup policies for pod mapping", "pod", pod.Name, "namespace", pod.Namespace)
291298
return nil
292299
}
293300

294301
var reqs []reconcile.Request
295302
for i := range cnps.Items {
296-
for _, app := range decodeAttachments(cnps.Items[i].Annotations[attachmentsAnnotation]) {
303+
for _, app := range decodeAttachments(ctx, cnps.Items[i].Annotations[attachmentsAnnotation]) {
297304
if appMatchesPod(app, pod.Labels) {
298305
reqs = append(reqs, reconcile.Request{NamespacedName: types.NamespacedName{
299306
Namespace: cnps.Items[i].Namespace,

‎internal/securitygroupcontroller/reconciler_test.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,28 @@ func TestMapPodToSGs(t *testing.T) {
294294
t.Fatalf("mapPodToSGs(bare) = %+v, want none", reqs)
295295
}
296296
}
297+
298+
func TestDecodeAttachments(t *testing.T) {
299+
ctx := context.Background()
300+
301+
if got := decodeAttachments(ctx, ""); got != nil {
302+
t.Fatalf("decodeAttachments(empty) = %+v, want nil", got)
303+
}
304+
305+
// A malformed annotation must be treated as empty (nil), not crash the
306+
// reconciler. The error is logged, not returned.
307+
if got := decodeAttachments(ctx, "{not json"); got != nil {
308+
t.Fatalf("decodeAttachments(malformed) = %+v, want nil", got)
309+
}
310+
311+
// A well-formed annotation round-trips.
312+
refs := []sdnv1alpha1.ApplicationReference{appRef("Postgres", "db")}
313+
encoded, err := json.Marshal(refs)
314+
if err != nil {
315+
t.Fatalf("marshal: %v", err)
316+
}
317+
got := decodeAttachments(ctx, string(encoded))
318+
if len(got) != 1 || got[0].Kind != "Postgres" || got[0].Name != "db" {
319+
t.Fatalf("decodeAttachments(valid) = %+v, want one Postgres/db ref", got)
320+
}
321+
}

0 commit comments

Comments
 (0)