[idebeijer]

Description

The post-renderer made two assumptions about every rendered document: that it holds a single resource, and that anything at spec.template is a pod template. Neither always holds, and each one fails the deploy of a chart that is valid for Helm and kubectl alike.

It made the manifest invalid. The zarf package label went onto the document's metadata, but a list's metadata is a ListMeta, which has no labels field, so the API server rejected the deploy before anything in the list was created:

ValidationError(ConfigMapList.metadata): unknown field "labels" in
io.k8s.apimachinery.pkg.apis.meta.v1.ListMeta

A Service inside a list was never registered as a connect string, since the document's kind is ConfigMapList or List, never Service.

A namespace named only by list items was never tracked, since GetNamespace() on the wrapper reads a ListMeta and returns "". Zarf creates namespaces before Helm applies, so a chart whose list items point at a namespace that doesn't exist yet failed outright:

server-side apply failed for object list-kinds-elsewhere/list-elsewhere
/v1, Kind=ConfigMap: namespaces "list-kinds-elsewhere" not found

A Namespace inside a list stayed owned by Helm, so it never got AdoptZarfManagedLabels and Helm deleted it on uninstall along with whatever else lived in it.

Rather than teach each of those to walk into items, flattenHelmResources replaces a list document with the resources it held before any of them run, so the rest of the post-renderer only ever sees one resource per manifest and none of it changes. Helm flattens a list into its items before applying it anyway, so the objects that reach the cluster are the same either way.

Detection is obj.IsList(), the same check Helm's kube client uses via meta.IsListType() when it flattens, so Zarf and Helm agree on which documents hold more than one resource.

Which lists actually reach the post-renderer

Only typed ones. Before running a post-renderer, Helm merges the rendered files with annotateAndMerge, which pipes every document through kio.ParseAll (https://github.com/helm/helm/blob/v4.2.0/pkg/action/action.go#L206). That expands a generic kind: List into its items, so a chart using one never arrives here as a list. Typed list kinds (ConfigMapList, SecretList, etc) are left intact and are the ones that broke. The generic case is covered anyway, since that expansion is a kustomize dependency inside Helm rather than a documented contract.

Related Issue

Fixes #5343

Checklist before merging

• Test, docs, adr added or updated as needed
• Contributor Guide Steps followed

[netlify]

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	2ca186c
🔍 Latest deploy log	https://app.netlify.com/projects/zarf-docs/deploys/6aa981ae3ee37d0008cdcb7d

[AustinAbro321]

Thank you for the PR! A few comments, but overall it's a reasonable change

[codecov]

Codecov Report

❌ Patch coverage is 75.67568% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/internal/packager/helm/post-render.go	75.67%	6 Missing and 3 partials ⚠️

Files with missing lines	Coverage Δ
src/internal/packager/helm/post-render.go	43.46% <75.67%> (+30.86%)	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[idebeijer]

9973550 adds a fix to another issue I encountered, further described in the PR description under heading spec.template is not always a pod template.

[AustinAbro321]

A few more comments, appreciate the cycles

[AustinAbro321]

We end up with a problem here where namespaces in a NamespaceList get into the final manifest and therefore removed on package removal.

I think a cleaner solution would be to have a function like this called at the top of EditHelmResources. This way the rest of our logic doesn't have to worry about resource lists

// flattenHelmResources expands list documents into their leaf resources before Zarf assigns ownership
func flattenHelmResources(resources []releaseutil.Manifest) ([]releaseutil.Manifest, error) {
	flattened := make([]releaseutil.Manifest, 0, len(resources))
	for _, resource := range resources {
		_, rawData, err := processManifestContent(resource.Content, nil)
		if err != nil {
			return nil, err
		}
		if len(rawData.Object) == 0 || !rawData.IsList() {
			flattened = append(flattened, resource)
			continue
		}

		err = flattenListResource(rawData, func(obj *unstructured.Unstructured) error {
			content, err := yaml.Marshal(obj.Object)
			if err != nil {
				return fmt.Errorf("failed to marshal list item: %w", err)
			}
			leaf := resource
			leaf.Content = string(content)
			flattened = append(flattened, leaf)
			return nil
		})
		if err != nil {
			return nil, err
		}
	}
	return flattened, nil
}

func flattenListResource(obj *unstructured.Unstructured, addResource func(*unstructured.Unstructured) error) error {
	if !obj.IsList() {
		return addResource(obj)
	}
	return obj.EachListItem(func(item runtime.Object) error {
		listItem, ok := item.(*unstructured.Unstructured)
		if !ok {
			return fmt.Errorf("unexpected list item type %T", item)
		}
		return flattenListResource(listItem, addResource)
	})
}

[idebeijer]

Thanks, yea that's a bunch better. Taken as suggested.

One side effect though. Helm tracks which file a document came from with a postrenderer.helm.sh/postrender-filename annotation it stamps before calling post-renderer. And on a list that lands on the wrapper rather than the items, so flattening drops it. This is helm get manifest on the e2e fixture:

# Source: generated-by-postrender-1.yaml           item of a ConfigMapList
# Source: generated-by-postrender-2.yaml           item of a ConfigMapList
# Source: list-kinds/templates/generic-list.yaml    item of a generic List

I think nothing downstream reads it, and the annotation never reaches the cluster. It is only that the generic list keeps its name, since annotateAndMerge (check PR description why this is) expanded that one before stamping, so two things that should look the same no longer do. If you want, I can copy the annotation onto each item while flattening if you would rather.

[AustinAbro321]

Good callout. From my evaluation it's fine if the annotation is dropped. Looks easy enough to add, but I wouldn't want to have to update this if a future version of Helm to drop or change the annotation

[AustinAbro321]

lgtm thanks!

[brandtkeller]

Looking at the two loops (in order) we have flattenHelmResources -> editHelmResources and each call processManifestContent. So unconditionally this now adds a full decode/encode round trip and the shape of each is pretty similar - negligible on small charts but a code smell.

[idebeijer]

Good point, I'll see if I can add optimize this later.

[brandtkeller]

lgtm - I think we can iterate on optimizing as needed but this solves a real problem as is.

[idebeijer]

lgtm - I think we can iterate on optimizing as needed but this solves a real problem as is.

Thanks, I hadn't taken the time to optimize yet. I'll see if I still can in a later PR, because I did run a benchmark after your feedback and it wasn't great.