fix(secret): make transformer idempotent

internal/workload/secret/dataloader.go

@@ -59,9 +59,14 @@ func transformSecret(in any) (any, error) {
 		return nil, fmt.Errorf("expected *unstructured.Unstructured, got %T", in)
 	}
 
-	// Extract key names from data before removing it
-	data, _, _ := unstructured.NestedMap(secret.Object, "data")
-	keys := slices.Sorted(maps.Keys(data))
+	// Extract key names from data before removing it.
+	// We must check whether the data/stringData fields actually exist because
+	// the WatchList code path in client-go can run the transformer twice on the
+	// same object (once in a temporary store, once in DeltaFIFO.Replace).
+	// On the second run the data fields are already removed, so we must preserve
+	// the annotation written by the first run.
+	data, dataFound, _ := unstructured.NestedMap(secret.Object, "data")
+	_, stringDataFound, _ := unstructured.NestedMap(secret.Object, "stringData")
 
 	// Remove the actual secret data
 	unstructured.RemoveNestedField(secret.Object, "data")
@@ -73,15 +78,20 @@ func transformSecret(in any) (any, error) {
 		annotations = make(map[string]string)
 	}
 
-	// Store keys as comma-separated list (empty string if no keys)
-	var keyList strings.Builder
-	for i, k := range keys {
-		if i > 0 {
-			keyList.WriteString(",")
+	// Only overwrite the annotation if we actually found data/stringData fields.
+	// If neither field existed, this is a second transform run and the annotation
+	// is already correct from the first run.
+	if dataFound || stringDataFound {
+		keys := slices.Sorted(maps.Keys(data))
+		var keyList strings.Builder
+		for i, k := range keys {
+			if i > 0 {
+				keyList.WriteString(",")
+			}
+			keyList.WriteString(k)
 		}
-		keyList.WriteString(k)
+		annotations[annotationSecretKeys] = keyList.String()
 	}
-	annotations[annotationSecretKeys] = keyList.String()
 	secret.SetAnnotations(annotations)
 
 	// Remove other unnecessary fields to reduce memory usage

internal/workload/secret/dataloader_test.go

@@ -0,0 +1,171 @@
+package secret
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+// TestTransformSecretIdempotent verifies that transformSecret is idempotent.
+// The Kubernetes informer's WatchList code path can run the transformer twice
+// on the same object: once in a temporary store, and again in DeltaFIFO.Replace().
+// If transformSecret is not idempotent, the second run would see no "data" field
+// (already removed by the first run) and overwrite the cached-secret-keys
+// annotation with an empty string, causing keys to disappear.
+func TestTransformSecretIdempotent(t *testing.T) {
+	obj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "my-secret",
+				"namespace": "my-team",
+				"labels": map[string]interface{}{
+					"nais.io/managed-by": "console",
+				},
+				"annotations": map[string]interface{}{
+					"console.nais.io/last-modified-at": "2024-10-18T12:44:57Z",
+					"console.nais.io/last-modified-by": "user@example.com",
+				},
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{
+				"DATABASE_URL": "cG9zdGdyZXM6Ly9sb2NhbGhvc3QvbXlkYg==",
+				"API_KEY":      "c2VjcmV0LWtleQ==",
+			},
+		},
+	}
+
+	// First transform: should extract keys and remove data
+	result1, err := transformSecret(obj)
+	if err != nil {
+		t.Fatalf("first transformSecret call failed: %v", err)
+	}
+
+	secret1 := result1.(*unstructured.Unstructured)
+	keys1 := secret1.GetAnnotations()[annotationSecretKeys]
+	if keys1 != "API_KEY,DATABASE_URL" {
+		t.Fatalf("after first transform: expected keys annotation %q, got %q", "API_KEY,DATABASE_URL", keys1)
+	}
+
+	// data should be removed
+	if _, exists, _ := unstructured.NestedMap(secret1.Object, "data"); exists {
+		t.Fatal("after first transform: data field should have been removed")
+	}
+
+	// Second transform (simulates WatchList double-transform): keys must be preserved
+	result2, err := transformSecret(secret1)
+	if err != nil {
+		t.Fatalf("second transformSecret call failed: %v", err)
+	}
+
+	secret2 := result2.(*unstructured.Unstructured)
+	keys2 := secret2.GetAnnotations()[annotationSecretKeys]
+	if keys2 != keys1 {
+		t.Fatalf("after second transform: keys annotation changed from %q to %q (transformer is not idempotent)", keys1, keys2)
+	}
+
+	// Verify the full round-trip through toGraphSecret still works
+	graphSecret, ok := toGraphSecret(secret2, "dev")
+	if !ok {
+		t.Fatal("toGraphSecret returned false after double transform")
+	}
+
+	if len(graphSecret.Keys) != 2 {
+		t.Fatalf("expected 2 keys, got %d: %v", len(graphSecret.Keys), graphSecret.Keys)
+	}
+	if graphSecret.Keys[0] != "API_KEY" || graphSecret.Keys[1] != "DATABASE_URL" {
+		t.Fatalf("expected keys [API_KEY, DATABASE_URL], got %v", graphSecret.Keys)
+	}
+}
+
+// TestTransformSecretEmptyData verifies that a secret with no data keys
+// is handled correctly through double-transform.
+func TestTransformSecretEmptyDataIdempotent(t *testing.T) {
+	obj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "empty-secret",
+				"namespace": "my-team",
+				"labels": map[string]interface{}{
+					"nais.io/managed-by": "console",
+				},
+			},
+			"type": "Opaque",
+			"data": map[string]interface{}{},
+		},
+	}
+
+	result1, err := transformSecret(obj)
+	if err != nil {
+		t.Fatalf("first transform failed: %v", err)
+	}
+
+	secret1 := result1.(*unstructured.Unstructured)
+	keys1 := secret1.GetAnnotations()[annotationSecretKeys]
+	if keys1 != "" {
+		t.Fatalf("expected empty keys annotation for empty secret, got %q", keys1)
+	}
+
+	// Second transform should not break anything
+	result2, err := transformSecret(secret1)
+	if err != nil {
+		t.Fatalf("second transform failed: %v", err)
+	}
+
+	secret2 := result2.(*unstructured.Unstructured)
+	keys2 := secret2.GetAnnotations()[annotationSecretKeys]
+	if keys2 != "" {
+		t.Fatalf("expected empty keys annotation after second transform, got %q", keys2)
+	}
+
+	graphSecret, ok := toGraphSecret(secret2, "dev")
+	if !ok {
+		t.Fatal("toGraphSecret returned false")
+	}
+	if len(graphSecret.Keys) != 0 {
+		t.Fatalf("expected 0 keys, got %d: %v", len(graphSecret.Keys), graphSecret.Keys)
+	}
+}
+
+// TestTransformSecretNoDataField verifies that a secret without any data
+// field at all (e.g. freshly created) is handled correctly.
+func TestTransformSecretNoDataFieldIdempotent(t *testing.T) {
+	obj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Secret",
+			"metadata": map[string]interface{}{
+				"name":      "new-secret",
+				"namespace": "my-team",
+				"labels": map[string]interface{}{
+					"nais.io/managed-by": "console",
+				},
+			},
+			"type": "Opaque",
+		},
+	}
+
+	result1, err := transformSecret(obj)
+	if err != nil {
+		t.Fatalf("first transform failed: %v", err)
+	}
+
+	secret1 := result1.(*unstructured.Unstructured)
+
+	result2, err := transformSecret(secret1)
+	if err != nil {
+		t.Fatalf("second transform failed: %v", err)
+	}
+
+	secret2 := result2.(*unstructured.Unstructured)
+	graphSecret, ok := toGraphSecret(secret2, "dev")
+	if !ok {
+		t.Fatal("toGraphSecret returned false")
+	}
+	if len(graphSecret.Keys) != 0 {
+		t.Fatalf("expected 0 keys, got %d: %v", len(graphSecret.Keys), graphSecret.Keys)
+	}
+}