[module-sdk] fix: prevent CRD update churn and prune unknown extensions

pkg/crd-installer/installer.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"maps"
 	"os"
 	"slices"
 	"sync"
@@ -22,6 +23,7 @@ import (
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/util/retry"
 
+	"github.com/deckhouse/module-sdk/pkg/crd-installer/openapi"
 	"github.com/deckhouse/module-sdk/pkg/utils"
 )
 
@@ -164,14 +166,18 @@ func (cp *CRDsInstaller) processCRD(ctx context.Context, crdFilePath string) err
 
 	crdReader := apimachineryYaml.NewDocumentDecoder(crdFileReader)
 
+	var errs error
+
 	for {
 		n, err := crdReader.Read(cp.buffer)
 		if err != nil {
 			if err == io.EOF {
 				break
 			}
 
-			return err
+			// the documents read so far may already have failed: reporting only the read
+			// error would hide them
+			return errors.Join(errs, err)
 		}
 
 		data := cp.buffer[:n]
@@ -182,18 +188,19 @@ func (cp *CRDsInstaller) processCRD(ctx context.Context, crdFilePath string) err
 
 		rd := bytes.NewReader(data)
 
-		err = cp.putCRDToCluster(ctx, rd, n)
-		if err != nil {
-			return err
+		// one bad document must not skip the rest of the file: the documents after it are
+		// unrelated CRDs, and skipping them silently leaves the module without them
+		if err := cp.putCRDToCluster(ctx, rd, n); err != nil {
+			errs = errors.Join(errs, err)
 		}
 	}
 
-	return nil
+	return errs
 }
 
 func (cp *CRDsInstaller) putCRDToCluster(ctx context.Context, crdReader io.Reader, bufferSize int) error {
-	// Decode into unstructured so vendor schema extensions (x-kubernetes-*, x-ui-*, ...)
-	// survive verbatim; the typed struct below is used only to read metadata.
+	// Decode into unstructured first: the typed struct below cannot hold
+	// x-kubernetes-sensitive-data, and sanitize needs the original document to recover it.
 	desired := &unstructured.Unstructured{}
 	err := apimachineryYaml.NewYAMLOrJSONDecoder(crdReader, bufferSize).Decode(&desired)
 	if err != nil {
@@ -213,6 +220,19 @@ func (cp *CRDsInstaller) putCRDToCluster(ctx context.Context, crdReader io.Reade
 		return fmt.Errorf("invalid CRD document apiversion/kind: '%s/%s'", crd.APIVersion, crd.Kind)
 	}
 
+	if err := applyServerDefaults(crd, desired); err != nil {
+		return fmt.Errorf("default %s: %w", crd.Name, err)
+	}
+
+	// a schema this build cannot decode must not keep the CRD out of the cluster: the
+	// document is queued as it came — what the installer did before it pruned anything —
+	// and the error is reported afterwards. The apiserver prunes the key it does not
+	// understand, while a missing CRD takes every custom resource of that kind with it.
+	sanitizeErr := sanitize(desired)
+	if sanitizeErr != nil {
+		sanitizeErr = fmt.Errorf("sanitize %s: %w", crd.Name, sanitizeErr)
+	}
+
 	cp.k8sTasks.Go(func() error {
 		err := cp.updateOrInsertCRD(ctx, crd, desired)
 		if err == nil {
@@ -251,15 +271,120 @@ func (cp *CRDsInstaller) putCRDToCluster(ctx context.Context, crdReader io.Reade
 		return err
 	})
 
+	return sanitizeErr
+}
+
+// sanitize drops the schema keys the apiserver does not know from the CRD document.
+//
+// x-doc-examples and friends, plus keys that look official but are not, like
+// x-kubernetes-immutable, are removed here instead of being sent. The apiserver prunes
+// them anyway, after logging one "unknown field" warning per occurrence, and because it
+// prunes them the stored spec could never equal the desired one, so every run issued a
+// pointless Update.
+//
+// Only the version schemas are rewritten; the rest of the document is passed through
+// untouched. Rebuilding it from apiextensionsv1.CustomResourceDefinition instead would
+// pin the installer to the CRD fields of the apiextensions-apiserver it was compiled
+// against, and silently strip anything a newer or patched apiserver understands.
+func sanitize(desired *unstructured.Unstructured) error {
+	versions, ok := nestedValue(desired.Object, "spec", "versions").([]any)
+	if !ok {
+		// no versions, or not a list: let the apiserver reject the document
+		return nil
+	}
+
+	for _, version := range versions {
+		versionMap, ok := version.(map[string]any)
+		if !ok {
+			continue
+		}
+
+		schema, ok := versionMap["schema"].(map[string]any)
+		if !ok {
+			continue
+		}
+
+		rawSchema, ok := schema["openAPIV3Schema"].(map[string]any)
+		if !ok {
+			continue
+		}
+
+		cleanSchema, err := openapi.Prune(rawSchema)
+		if err != nil {
+			name, _, _ := unstructured.NestedString(versionMap, "name")
+
+			return fmt.Errorf("version %q schema: %w", name, err)
+		}
+
+		schema["openAPIV3Schema"] = cleanSchema
+	}
+
+	return nil
+}
+
+// nestedValue returns the value at the given path without copying it, or nil if the path
+// does not lead to one. Errors are the same as absence for every caller here.
+func nestedValue(obj map[string]any, fields ...string) any {
+	value, found, err := unstructured.NestedFieldNoCopy(obj, fields...)
+	if err != nil || !found {
+		return nil
+	}
+
+	return value
+}
+
+// applyServerDefaults writes the .spec fields the apiserver fills in itself into the
+// document, so a manifest that omits them does not differ from the stored object on
+// every single reconcile.
+//
+// ponytail: only the .spec fields known to churn are written here; if a CRD still updates
+// on every reconcile, diff the stored object against the manifest and add the field that
+// differs.
+func applyServerDefaults(crd *apiextensionsv1.CustomResourceDefinition, desired *unstructured.Unstructured) error {
+	apiextensionsv1.SetDefaults_CustomResourceDefinitionSpec(&crd.Spec)
+
+	// served and storage have no omitempty upstream, so the stored object always carries
+	// both on every version — a manifest that omits either would differ from it forever
+	versions, _ := nestedValue(desired.Object, "spec", "versions").([]any)
+	for _, version := range versions {
+		versionMap, ok := version.(map[string]any)
+		if !ok {
+			continue
+		}
+
+		for _, field := range []string{"served", "storage"} {
+			if _, ok := versionMap[field]; !ok {
+				versionMap[field] = false
+			}
+		}
+	}
+
+	// spec.conversion is defaulted too, but updateOrInsertCRD always takes the in-cluster
+	// one, which the apiserver has already defaulted
+	names := map[string]string{
+		"singular": crd.Spec.Names.Singular,
+		"listKind": crd.Spec.Names.ListKind,
+	}
+
+	for field, value := range names {
+		if value == "" {
+			continue
+		}
+
+		if err := unstructured.SetNestedField(desired.Object, value, "spec", "names", field); err != nil {
+			return fmt.Errorf("set spec.names.%s: %w", field, err)
+		}
+	}
+
 	return nil
 }
 
 func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensionsv1.CustomResourceDefinition, desired *unstructured.Unstructured) error {
 	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		desired.SetLabels(overlay(desired.GetLabels(), cp.crdExtraLabels))
+
 		existing, err := cp.k8sClient.Resource(crdGVR).Get(ctx, crd.GetName(), apimachineryv1.GetOptions{})
 		if apierrors.IsNotFound(err) {
-			mergeLabels(desired, cp.crdExtraLabels)
-
 			_, err = cp.k8sClient.Resource(crdGVR).Create(ctx, desired, apimachineryv1.CreateOptions{})
 			if err != nil {
 				return fmt.Errorf("create crd: %w", err)
@@ -317,8 +442,6 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio
 			}
 		}
 
-		mergeLabels(desired, cp.crdExtraLabels)
-
 		desiredSpec, _, err := unstructured.NestedMap(desired.Object, "spec")
 		if err != nil {
 			return fmt.Errorf("read desired spec: %w", err)
@@ -329,12 +452,27 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio
 			return fmt.Errorf("read existing spec: %w", err)
 		}
 
-		// diff on lossless unstructured specs so vendor extensions are not silently dropped
-		// ponytail: apiserver-defaulted .spec fields may differ from the manifest and cause
-		// reconcile churn; the update stays idempotent, tighten the diff here if it ever churns.
+		// labels and annotations belong to whoever wrote them: dropping
+		// app.kubernetes.io/managed-by or meta.helm.sh/release-name breaks the next helm
+		// upgrade of the chart that installed the CRD, so both maps are overlaid, never
+		// replaced. The result is what the object must end up holding, which is also what
+		// makes the comparison below exact.
+		//
+		// ponytail: overlaying means a key the manifest stops declaring stays in the
+		// cluster — retracting one needs the field ownership the apiserver keeps for
+		// server-side apply; switch this whole update to Apply if that ever matters.
+		labels := overlay(existing.GetLabels(), desired.GetLabels())
+		annotations := overlay(existing.GetAnnotations(), desired.GetAnnotations())
+
+		// The specs are compared, not merged: the desired one is pruned by sanitize and the
+		// existing one by the apiserver, so a manifest applied over the state the apiserver
+		// derived from it is a no-op. A .spec key this cluster's apiserver does not know is
+		// the exception — it prunes the key while the desired document keeps it, so that CRD
+		// is updated on every reconcile. Deliberate: the key is sent so an apiserver that
+		// does know it gets it.
 		if cmp.Equal(existingSpec, desiredSpec) &&
-			cmp.Equal(existing.GetLabels(), desired.GetLabels()) &&
-			cmp.Equal(existing.GetAnnotations(), desired.GetAnnotations()) {
+			cmp.Equal(existing.GetLabels(), labels) &&
+			cmp.Equal(existing.GetAnnotations(), annotations) {
 			return nil
 		}
 
@@ -343,8 +481,8 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio
 		if err := unstructured.SetNestedMap(existing.Object, desiredSpec, "spec"); err != nil {
 			return fmt.Errorf("set spec: %w", err)
 		}
-		existing.SetLabels(desired.GetLabels())
-		existing.SetAnnotations(desired.GetAnnotations())
+		existing.SetLabels(labels)
+		existing.SetAnnotations(annotations)
 		existing.SetResourceVersion(resourceVersion)
 
 		_, err = cp.k8sClient.Resource(crdGVR).Update(ctx, existing, apimachineryv1.UpdateOptions{})
@@ -356,17 +494,22 @@ func (cp *CRDsInstaller) updateOrInsertCRD(ctx context.Context, crd *apiextensio
 	})
 }
 
-func mergeLabels(u *unstructured.Unstructured, extra map[string]string) {
-	labels := u.GetLabels()
-	if labels == nil {
-		labels = make(map[string]string, len(extra))
+// overlay returns base with over's keys written on top of it. A key base holds and over
+// does not is kept.
+//
+// nil in, nil out: the apiserver returns no map at all for empty labels or annotations, and
+// an empty one written back would never compare equal to that — SetLabels/SetAnnotations
+// remove the field for nil and set metadata.labels: {} for an empty map.
+func overlay(base, over map[string]string) map[string]string {
+	if len(over) == 0 {
+		return base
 	}
 
-	for k, v := range extra {
-		labels[k] = v
-	}
+	out := make(map[string]string, len(base)+len(over))
+	maps.Copy(out, base)
+	maps.Copy(out, over)
 
-	u.SetLabels(labels)
+	return out
 }
 
 func (cp *CRDsInstaller) GetCRDFromCluster(ctx context.Context, crdName string) (*apiextensionsv1.CustomResourceDefinition, error) {