[cozystack-controller] Ancestor tracking webhook - #1400

pkg/lineage/mapper.go (1)

16-40: Clarify stub-only mapping and avoid drift with the real mapper.

Document or gate stubMapper behind a build tag to ensure it’s not used in production and doesn’t diverge from the webhook/controller mapping logic.

Example doc:

 type stubMapper struct{}
 
+// NOTE: stubMapper is intended for local dev/tests only. Production mapping is
+// provided by the webhook/controller. Keep entries in sync or remove before release.
 var stubMapperMap = map[string]string{

pkg/lineage/lineage.go (3)

87-91: Use structured logging instead of stderr prints.

Library code should not write to stderr directly; use the request-scoped logger.

-		if err != nil {
-			fmt.Fprintf(os.Stderr, "Could not fetch owner %s/%s (%s): %v\n", obj.GetNamespace(), owner.Name, owner.Kind, err)
-			continue
-		}
+		if err != nil {
+			l.Error(err, "could not fetch owner",
+				"namespace", obj.GetNamespace(), "name", owner.Name, "kind", owner.Kind)
+			continue
+		}

---

71-77: Clarify variadic-args error message.

Current log miscounts and confuses total vs variadic args.

 	if len(memory) != 0 && len(memory) != 1 {
 		l.Error(
 			fmt.Errorf("invalid argument count"), "could not parse variadic arguments to WalkOwnershipGraph",
-			"args passed", len(memory)+5, "expected args", "4|5",
+			"memory args", len(memory), "expected memory args", "0|1",
 		)
 		return out
 	}

---

101-130: Flatten the single-iteration loop for readability.

Replace the for { ... break } pattern with early returns/guards to simplify control flow.

cmd/cozystack-controller/main.go (1)

41-41: Nit: clearer alias would help readability.

Consider aliasing as lineagewebhook or lineage for clarity over lcw.

internal/lineagecontrollerwebhook/types.go (1)

14-21: Use a pointer decoder to follow controller-runtime patterns and avoid unnecessary copies.

Change decoder to a pointer; InjectDecoder can then assign directly.

 type LineageControllerWebhook struct {
   Scheme *runtime.Scheme
   client.Client
-  decoder   admission.Decoder
+  decoder   *admission.Decoder
   dynClient dynamic.Interface
   mapper    meta.RESTMapper
   config    atomic.Value
 }

internal/lineagecontrollerwebhook/config.go (1)

21-24: Key may collide across namespaces/kinds; consider including SourceRef.Kind/Namespace.

Using only SourceRef.Name and Chart risks collisions. Consider a key like "///" and update the controller to match.

internal/lineagecontrollerwebhook/controller.go (2)

31-31: Prefer keyed struct literal for runtimeConfig

Prevents accidental field-order bugs if struct changes.

-	c.config.Store(&runtimeConfig{newConfig})
+	c.config.Store(&runtimeConfig{chartAppMap: newConfig})

---

22-22: Hard-coded namespace “cozy-system”

If CRDs ever move or are cluster-scoped, this will silently miss them.

Make the namespace configurable (env/flag) or handle cluster-scoped CRDs by omitting Namespace in ListOptions.

internal/lineagecontrollerwebhook/webhook.go (4)

66-71: No-op for non-CREATE operations (unless configured otherwise)

PR text says “on creation”; guard here in case the webhook config isn’t restricted to CREATE.

 	warn := make(admission.Warnings, 0)
 
+	if req.Operation != admissionv1.Create {
+		return admission.Allowed("no-op for non-CREATE operations")
+	}

Add import:

-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	admissionv1 "k8s.io/api/admission/v1"

---

73-89: Simplify error handling flow

The for-loop is harder to read; a switch is clearer.

-	labels, err := h.computeLabels(ctx, obj)
-	for {
-		if err != nil && errors.Is(err, NoAncestors) {
-			return admission.Allowed("object not managed by app")
-		}
-		if err != nil && errors.Is(err, AncestryAmbiguous) {
-			warn = append(warn, "object ancestry ambiguous, using first ancestor found")
-			break
-		}
-		if err != nil {
-			return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err))
-		}
-		if err == nil {
-			break
-		}
-	}
+	labels, err := h.computeLabels(ctx, obj)
+	switch {
+	case err == nil:
+	case errors.Is(err, NoAncestors):
+		return admission.Allowed("object not managed by app")
+	case errors.Is(err, AncestryAmbiguous):
+		warn = append(warn, "object ancestry ambiguous, using first ancestor found")
+	default:
+		return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err))
+	}

---

92-97: Generalize log/error text (“pod” → “object”)

This webhook handles many kinds, not just Pods.

-	mutated, err := json.Marshal(obj)
-	if err != nil {
-		return admission.Errored(500, fmt.Errorf("marshal mutated pod: %w", err))
-	}
-	logger.V(1).Info("mutated pod", "namespace", obj.GetNamespace(), "name", obj.GetName())
+	mutated, err := json.Marshal(obj)
+	if err != nil {
+		return admission.Errored(500, fmt.Errorf("marshal mutated object: %w", err))
+	}
+	logger.V(1).Info("mutated object", "gvk", obj.GroupVersionKind().String(), "namespace", obj.GetNamespace(), "name", obj.GetName())

---

131-140: Label overwrite policy

applyLabels unconditionally overwrites existing values. If these keys are reserved for this webhook, fine; otherwise consider not overriding.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/lineagecontrollerwebhook/webhook.go (1)

43-49: Initialize runtime config before registering the webhook

Prevent nil deref in Map() during early requests by seeding an empty config before registration.

Apply this diff:

 cachedDisco := memory.NewMemCacheClient(discoClient)
 h.mapper = restmapper.NewDeferredDiscoveryRESTMapper(cachedDisco)

+// Safe default: empty mapping to avoid nil deref before the controller populates it.
+// Adjust the concrete type/field names if they differ.
+h.config.Store(&runtimeConfig{chartAppMap: map[string]string{}})
+
 // Register HTTP path -> handler.
 mgr.GetWebhookServer().Register("/mutate-lineage", &admission.Webhook{Handler: h})

internal/lineagecontrollerwebhook/webhook.go (2)

73-88: Simplify error handling; the for-loop is unnecessary

The loop executes at most once. A switch makes intent clear.

Apply this diff:

-	labels, err := h.computeLabels(ctx, obj)
-	for {
-		if err != nil && errors.Is(err, NoAncestors) {
-			return admission.Allowed("object not managed by app")
-		}
-		if err != nil && errors.Is(err, AncestryAmbiguous) {
-			warn = append(warn, "object ancestry ambiguous, using first ancestor found")
-			break
-		}
-		if err != nil {
-			return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err))
-		}
-		if err == nil {
-			break
-		}
-	}
+	labels, err := h.computeLabels(ctx, obj)
+	switch {
+	case err == nil:
+		// proceed
+	case errors.Is(err, NoAncestors):
+		return admission.Allowed("object not managed by app")
+	case errors.Is(err, AncestryAmbiguous):
+		warn = append(warn, "object ancestry ambiguous, using first ancestor found")
+	default:
+		return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err))
+	}

---

92-97: Fix log/message wording: this webhook mutates generic objects, not just pods

Apply this diff:

-	if err != nil {
-		return admission.Errored(500, fmt.Errorf("marshal mutated pod: %w", err))
-	}
-	logger.V(1).Info("mutated pod", "namespace", obj.GetNamespace(), "name", obj.GetName())
+	if err != nil {
+		return admission.Errored(500, fmt.Errorf("marshal mutated object: %w", err))
+	}
+	logger.V(1).Info("mutated object", "namespace", obj.GetNamespace(), "name", obj.GetName())

packages/system/cozystack-controller/templates/service.yaml (1)

10-13: Optional: mark port as HTTPS for clarity

Set appProtocol for better intent and tooling hints.

Apply this diff:

   ports:
     - port: 443
       targetPort: 9443
       protocol: TCP
       name: webhook
+      appProtocol: https

packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml (2)

18-23: Expand resources to match PR intent and fix style nit

PR mentions labeling “pods, secrets, PVCs, etc.” The rules currently miss secrets. Also fix spacing after comma.

-        resources: ["pods","persistentvolumeclaims"]
+        resources: ["pods", "persistentvolumeclaims", "secrets"]

---

24-33: Namespace scoping looks good; confirm release ns exclusion

You exclude cozystack.io/system=true and kube-system. Ensure the release namespace is labeled cozystack.io/system=true so webhook won’t mutate its own certs/pods. If not guaranteed, add an explicit NotIn for the release namespace.

packages/system/cozystack-controller/templates/certmanager.yaml (1)

4-7: Template resource names to avoid cross-install collisions

If this chart can be installed per-tenant or multiple times, hardcoded Issuer/Certificate names may collide. Prefer fullname-based names.

Example pattern:

-  name: cozystack-controller-webhook-selfsigned
+  name: {{ include "cozystack-controller.fullname" . }}-webhook-selfsigned
...
-  name: cozystack-controller-webhook-ca
+  name: {{ include "cozystack-controller.fullname" . }}-webhook-ca
...
-  secretName: cozystack-controller-webhook-ca
+  secretName: {{ include "cozystack-controller.fullname" . }}-webhook-ca
...
-  name: cozystack-controller-webhook
+  name: {{ include "cozystack-controller.fullname" . }}-webhook
-  secretName: cozystack-controller-webhook-cert
+  secretName: {{ include "cozystack-controller.fullname" . }}-webhook-cert

If you adopt this, also update the inject-ca-from(-secret) and Service tls.secret mounts accordingly in other templates.

Also applies to: 12-20, 25-29, 34-38

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: lllamnyp
PR: cozystack/cozystack#1380
File: internal/controller/kubeovnplunger/metrics.go:291-296
Timestamp: 2025-09-10T22:02:47.707Z
Learning: In the kubeovnplunger controller, r.lastLeader map is initialized in SetupWithManager method (line 151) with make(map[string]string) before any reconcile operations begin, making defensive nil checks unnecessary in the reconcile methods.

Learnt from: lllamnyp
PR: cozystack/cozystack#1380
File: internal/controller/kubeovnplunger/metrics.go:291-296
Timestamp: 2025-09-10T22:02:47.707Z
Learning: In the kubeovnplunger controller, r.lastLeader map is initialized in SetupWithManager method before any reconcile operations begin, making defensive nil checks unnecessary in the reconcile methods.

packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml (2)

23-23: Set failurePolicy to Ignore to avoid cluster-wide CREATE outages if the webhook is down.

-    failurePolicy: Fail
+    failurePolicy: Ignore

---

4-9: Fix CA injection key and quote the value; scope the MWC name to the release (also resolves YAML parse error).

• Use cert-manager’s secret-based injector and reference the actual secret (likely ...-webhook-cert).
• Quote Helm expressions to satisfy yamllint and templating.
• Give the cluster-scoped MutatingWebhookConfiguration a release-scoped name.

 metadata:
-  name: lineage
+  name: {{ include "cozystack-controller.fullname" . }}-lineage
   annotations:
-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/cozystack-controller-webhook
+    cert-manager.io/inject-ca-from-secret: '{{ .Release.Namespace }}/cozystack-controller-webhook-cert'
   labels:
     app: cozystack-controller

internal/lineagecontrollerwebhook/types.go (1)

14-14: Align kubebuilder marker with chart manifest: create-only and Ignore failures.

Prevents noisy updates and blocking behavior; keeps generator output consistent with the Helm manifest.

-// +kubebuilder:webhook:path=/mutate-lineage,mutating=true,failurePolicy=Fail,sideEffects=None,groups="",resources=pods,secrets,services,persistentvolumeclaims,verbs=create;update,versions=v1,name=mlineage.cozystack.io,admissionReviewVersions={v1}
+// +kubebuilder:webhook:path=/mutate-lineage,mutating=true,failurePolicy=Ignore,sideEffects=None,groups="",resources=pods,secrets,services,persistentvolumeclaims,verbs=create,versions=v1,name=mlineage.cozystack.io,admissionReviewVersions={v1}

internal/lineagecontrollerwebhook/config.go (1)

32-39: Guard against nil config to avoid panics on first requests.

initConfig isn’t called before Map; Load() can be nil leading to a nil deref.

 func (l *LineageControllerWebhook) Map(hr *helmv2.HelmRelease) (string, string, string, error) {
-	cfg := l.config.Load().(*runtimeConfig).chartAppMap
+	l.initConfig()
+	rc := l.config.Load().(*runtimeConfig)
+	cfg := rc.chartAppMap
 	s := &hr.Spec.Chart.Spec
 	val, ok := cfg[chartRef{s.SourceRef.Name, s.Chart}]
 	if !ok {
 		return "", "", "", fmt.Errorf("cannot map helm release %s/%s to dynamic app", hr.Namespace, hr.Name)
 	}
 	return val.groupVersion, val.kind, val.prefix, nil
 }

pkg/lineage/lineage.go (1)

132-139: Guard against empty HR label; avoid Get(..., "").

If the label exists but is empty, this still calls Get with an empty name.

Apply:

-	name, ok := labels[HRLabel]
-	if !ok {
+	name, ok := labels[HRLabel]
+	if !ok || name == "" {
 		return
 	}

packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml (2)

11-17: Add reinvocation policy and a shorter timeout.

Improves correctness when earlier webhooks mutate objects and prevents long API-server hangs.

     admissionReviewVersions: ["v1"]
     sideEffects: None
+    reinvocationPolicy: IfNeeded
+    timeoutSeconds: 5
     clientConfig:
       service:
         name: cozystack-controller
         namespace: {{ .Release.Namespace }}
         path: /mutate-lineage

---

22-22: Tidy list formatting flagged by yamllint.

-        resources: ["pods","secrets", "services", "persistentvolumeclaims"]
+        resources: ["pods", "secrets", "services", "persistentvolumeclaims"]

pkg/lineage/lineage.go (8)

96-100: Allow HR-label fallback when owners exist but none could be resolved.

The early return prevents falling back to the HR label if all owner fetches failed (RBAC, NotFound, unknown GVK). Return only when you actually appended results from owners.

Apply:

-	if len(ownerRefs) > 0 {
-		return
-	}
+	if len(ownerRefs) > 0 && len(out) > 0 {
+		return
+	}

---

85-94: Prefer controller owner when present.

Follow the OwnerReference with controller=true first; otherwise fall back to all owners. This reduces branching and avoids non-controller refs.

Apply:

-	ownerRefs := obj.GetOwnerReferences()
-	for _, owner := range ownerRefs {
+	ownerRefs := obj.GetOwnerReferences()
+	// prefer controller owner if present
+	var toVisit []metav1.OwnerReference
+	for i := range ownerRefs {
+		if ownerRefs[i].Controller != nil && *ownerRefs[i].Controller {
+			toVisit = []metav1.OwnerReference{ownerRefs[i]}
+			break
+		}
+	}
+	if len(toVisit) == 0 {
+		toVisit = ownerRefs
+	}
+	for _, owner := range toVisit {
 		ownerObj, err := getUnstructuredObject(ctx, client, mapper, owner.APIVersion, owner.Kind, obj.GetNamespace(), owner.Name)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "Could not fetch owner %s/%s (%s): %v\n", obj.GetNamespace(), owner.Name, owner.Kind, err)
+			l.Error(err, "Could not fetch owner", "namespace", obj.GetNamespace(), "name", owner.Name, "kind", owner.Kind)
 			continue
 		}
 
 		out = append(out, WalkOwnershipGraph(ctx, client, mapper, appMapper, ownerObj, visited)...)
 	}

---

6-6: Use structured logging; drop unused os import.

Keep logs consistent with controller-runtime.

Apply:

-import "os"

-			fmt.Fprintf(os.Stderr, "Could not fetch owner %s/%s (%s): %v\n", obj.GetNamespace(), owner.Name, owner.Kind, err)
+			l.Error(err, "Could not fetch owner", "namespace", obj.GetNamespace(), "name", owner.Name, "kind", owner.Kind)

Also applies to: 89-91

---

112-121: Validate prefix before TrimPrefix to avoid accidental name truncation.

Only strip when HR name actually has the returned prefix.

Apply:

-		a, k, p, err := appMapper.Map(hr)
+		a, k, p, err := appMapper.Map(hr)
 		if err != nil {
 			break
 		}
-		ownerObj, err := getUnstructuredObject(ctx, client, mapper, a, k, obj.GetNamespace(), strings.TrimPrefix(obj.GetName(), p))
+		ownerName := obj.GetName()
+		if p != "" {
+			if !strings.HasPrefix(ownerName, p) {
+				break
+			}
+			ownerName = strings.TrimPrefix(ownerName, p)
+		}
+		ownerObj, err := getUnstructuredObject(ctx, client, mapper, a, k, obj.GetNamespace(), ownerName)

---

71-77: Fix log message: expected arg count is 5|6.

Apply:

-			"args passed", len(memory)+5, "expected args", "4|5",
+			"args passed", len(memory)+5, "expected args", "5|6",

---

49-50: Consider using UID in visit-key to harden cycle detection.

APIVersion/Kind/Namespace/Name can be reused after deletion; including UID removes ambiguity in edge cases.

If acceptable, extend ObjectID with UID and set it from obj.GetUID() when computing id. I can draft a minimal patch if you want.

Also applies to: 79-84

---

146-183: Optional: cache REST mappings to cut discovery churn.

getUnstructuredObject resolves a RESTMapping for every hop. If this runs per admission, consider a RESTMapper with caching or a small GVK→Resource map in-memory.

---

40-48: Add tests for failure/fallback paths.

Please add unit tests covering: (1) owners present but all fetches fail → HR-label fallback triggers; (2) HR label empty → no Get call; (3) HR Map prefix mismatch → break without panic; (4) controller vs non-controller owners.

I can generate table-driven tests for these cases—want me to open a follow-up?

Also applies to: 132-144

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml (3)

10-10: Rename webhook to a stable, descriptive DNS name

Past feedback still applies; prefer lineage-webhook.cozystack.io for clarity.

Apply:

-  - name: lineage.cozystack.io
+  - name: lineage-webhook.cozystack.io

---

23-23: Avoid blocking creates if webhook is down

Since this webhook only adds labels, prefer failurePolicy: Ignore to prevent outages.

Apply:

-    failurePolicy: Fail
+    failurePolicy: Ignore

---

6-6: Broken CA injection annotation and templating; also target the secret created by cert-manager

The value is unquoted Helm+literal causing YAML parse errors, and it points to a non-existent object. certmanager.yaml creates cozystack-controller-webhook-cert (a Secret). Use inject-ca-from-secret and quote.

Apply:

-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/cozystack-controller-webhook
+    cert-manager.io/inject-ca-from-secret: '{{ .Release.Namespace }}/cozystack-controller-webhook-cert'

pkg/lineage/lineage_test.go (2)

29-42: Don’t crash in init; handle config and client errors defensively

Using GetConfigOrDie and ignoring errors can crash tests. Initialize conditionally and skip when cluster config isn’t available.

Apply:

-func init() {
-	cfg := config.GetConfigOrDie()
-
-	dynClient, _ = dynamic.NewForConfig(cfg)
-
-	discoClient, _ := discovery.NewDiscoveryClientForConfig(cfg)
-
-	cachedDisco := memory.NewMemCacheClient(discoClient)
-	mapper = restmapper.NewDeferredDiscoveryRESTMapper(cachedDisco)
-
-	zapLogger, _ := zap.NewDevelopment()
-	l = zapr.NewLogger(zapLogger)
-	ctx = logr.NewContext(context.Background(), l)
-}
+func TestMain(m *testing.M) {
+	cfg, err := config.GetConfig()
+	if err != nil {
+		// No cluster; skip all tests gracefully.
+		os.Exit(0)
+	}
+	if dynClient, err = dynamic.NewForConfig(cfg); err != nil {
+		os.Exit(0)
+	}
+	discoClient, err := discovery.NewDiscoveryClientForConfig(cfg)
+	if err != nil {
+		os.Exit(0)
+	}
+	cachedDisco := memory.NewMemCacheClient(discoClient)
+	mapper = restmapper.NewDeferredDiscoveryRESTMapper(cachedDisco)
+	zapLogger, err := zap.NewDevelopment()
+	if err != nil {
+		os.Exit(0)
+	}
+	l = zapr.NewLogger(zapLogger)
+	ctx = logr.NewContext(context.Background(), l)
+	os.Exit(m.Run())
+}

---

44-53: Fix signature mismatch; avoid os.Args; gate as opt-in integration test

Call WalkOwnershipGraph with an AppMapper, read namespace/name from env (or flags), and log via t.Logf.

Apply:

-func TestWalkingOwnershipGraph(t *testing.T) {
-	obj, err := dynClient.Resource(schema.GroupVersionResource{"", "v1", "pods"}).Namespace(os.Args[1]).Get(ctx, os.Args[2], metav1.GetOptions{})
-	if err != nil {
-		t.Fatal(err)
-	}
-	nodes := WalkOwnershipGraph(ctx, dynClient, mapper, obj)
-	for _, node := range nodes {
-		fmt.Printf("%#v\n", node)
-	}
-}
+func TestWalkingOwnershipGraph(t *testing.T) {
+	if dynClient == nil || mapper == nil {
+		t.Skip("dynamic client/mapper not configured")
+	}
+	if os.Getenv("RUN_LINEAGE_INTEGRATION") != "true" {
+		t.Skip("opt-in via RUN_LINEAGE_INTEGRATION=true")
+	}
+	ns := os.Getenv("E2E_NS")
+	name := os.Getenv("E2E_NAME")
+	if ns == "" || name == "" {
+		t.Skip("set E2E_NS and E2E_NAME")
+	}
+	gvr := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}
+	obj, err := dynClient.Resource(gvr).Namespace(ns).Get(ctx, name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	nodes := WalkOwnershipGraph(ctx, dynClient, mapper, noopMapper{}, obj)
+	for _, node := range nodes {
+		t.Logf("%#v", node)
+	}
+}

Add this helper in the file:

// Place near the imports.
import helmv2 "github.com/fluxcd/helm-controller/api/v2"

// Place above TestWalkingOwnershipGraph.
type noopMapper struct{}

func (noopMapper) Map(*helmv2.HelmRelease) (string, string, string, error) {
	return "", "", "", fmt.Errorf("no mapping")
}

cmd/cozystack-controller/main.go (1)

218-231: Registering both controller and webhook is fine; consider a feature flag

Looks good as a dual registrar. Consider a cmd flag (e.g., --enable-lineage-webhook) to allow disabling during staged rollouts.

packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml (3)

4-4: Scope the object name to this chart/release

Bare “lineage” is too generic and risks collisions. Use a chart-scoped name.

Apply either:

-  name: lineage
+  name: cozystack-controller-lineage

or (if you have a fullname helper):

-  name: lineage
+  name: {{ include "cozystack-controller.fullname" . }}-lineage

---

11-17: Harden admission settings (reinvocation, timeout, match policy)

Add defensive defaults to avoid long API-server stalls and enable reinvocation.

Apply:

   - name: lineage.cozystack.io
     admissionReviewVersions: ["v1"]
     sideEffects: None
+    reinvocationPolicy: IfNeeded
+    timeoutSeconds: 5
+    matchPolicy: Equivalent
     clientConfig:

---

22-22: Style nit: add spaces after commas

Minor YAMLlint warning.

Apply:

-        resources: ["pods","secrets", "services", "persistentvolumeclaims"]
+        resources: ["pods", "secrets", "services", "persistentvolumeclaims"]

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro