feat(helm): support allowable errors for helm chart retries by brandtkeller · Pull Request #5287 · zarf-dev/zarf

brandtkeller · 2026-08-26T23:54:48Z

Description

Adds support for a retry on helm chart installations when the failure isn't deterministic. There may be many but the first recorded one is the presence of an admission/mutation webhook and the inability to send requests as the endpoint may not be entirely reconciled by kubernetes networking when attempting to deploy resources that then call that webhook.

I looked into whether the 500 error returned might include a Retry-After header that we look for on 429 responses but it was not there (and probably rightfully so).

Related Issue

Fixes #5286

Checklist before merging

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

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

netlify · 2026-08-26T23:54:54Z

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	`f2f3ef3`
🔍 Latest deploy log	https://app.netlify.com/projects/zarf-docs/deploys/6a92e5bd4344450008e95c4d

codecov · 2026-08-27T00:01:56Z

Codecov Report

❌ Patch coverage is 59.42029% with 28 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/internal/packager/helm/chart.go	61.19%	23 Missing and 3 partials ⚠️
src/pkg/packager/deploy.go	0.00%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/pkg/packager/deploy.go	`24.81% <0.00%> (-0.08%)`	⬇️
src/internal/packager/helm/chart.go	`11.19% <61.19%> (+10.05%)`	⬆️

... and 5 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.

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

soltysh · 2026-08-28T13:48:19Z

+		return false
+	}
+
+	if wrapped, ok := err.(interface{ Unwrap() []error }); ok {


There are several problems in this entire block, and the next function:

The rules for errors are such that you'd need to match both signatures Unwrap() []error and Unwrap() error, see https://pkg.go.dev/errors#pkg-overview. I've tried with using fmt.Errorf("...%w...", ...err...) and that matched only Unwrap() error.

It's easier to just use AsType instead, which does the entire work for you. Here it will simplify this and the next method to look like so:

apiStatus, ok = errors.AsType[*apierrors.APIStatus](err) if !ok { return false } status := apiStatus.Status() return status.Reason == metav1.StatusReasonInternalError && strings.Contains(status.Message, "failed calling webhook")

Agree on covering both Unwrap() []error and Unwrap() error - I added an additional test case for coverage with one nested through fmt.Errorf("%w") - otherwise I think we have coverage on the other cases.

My understanding was that errors.As / errors.AsType returns the first matching node and the policy I was looking to implement was more closely aligned with "all leaves must be allowed". AsType would report a match for a webhookFailure that also happens to be joined with another terminal failure.

Certainly open to thoughts - my goal was to ensure we're conservative with what is allowed and then loosen/add as required.

Certainly open to thoughts - my goal was to ensure we're conservative with what is allowed and then loosen/add as required.

I believe here, the same rule applies you've mentioned in the other comment. Whenever errors are wrapped like NewInternalError the double wrapping will not happen, since the internal error will be printed as a string in the details of the message. So I believe, here, it'll be simpler for us to just find the first and only errors.APIStatus. It will also make the code simpler to read 😉

This can be done in a followup.

soltysh · 2026-08-28T13:50:31Z

+		return false
+	}
+	status := apiStatus.Status()
+	return status.Reason == metav1.StatusReasonInternalError && strings.Contains(status.Message, "failed calling webhook")


I'm somehow worried about these string matching errors, they are not reliable enough, imo. Looking deeper, how about instead of matching apistatus trying to match ErrCallingWebhook instead? Or both?

I was digging through this some more. If I understand correctly ErrCallingWebhook would be better if it survived to the client, but it looks like the api-server might wrap that internal type with apierrors.NewInternalError before serializing the response. Are you aware of a typed signal for this path I might have missed?

You're right, it is only wrapped under apierrors.NewInternalError sadly.

soltysh · 2026-08-28T14:07:56Z

-	if errors.Is(histErr, driver.ErrReleaseNotFound) {
-		// No prior release, try to install it.
-		l.Info("performing Helm install", "chart", zarfChart.Name)
+		if errors.Is(histErr, driver.ErrReleaseNotFound) {


I'm not a helm expert, but codex suggest that a failed, retried releases might not return an error, but rather a failed release instead, which means this code will not go the install path, but rather upgrade one. This can potentially skip some install-related steps. Should we also look at the release status and check if it's reporting StatusFailed?

I think this is a tricky balance - if we fail during install then the next retry will attempt upgrade, skipping the install specific steps. But if we implement a replace for install on retry then we could retry already executed install steps.

GIven we're narrowing when a retry can occur (webhook failed) - my inclination is that the current semantics should generally be okay. If we were retrying chart installs regardless of error type then we might have to think on this further.

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

soltysh

lgtm

soltysh · 2026-08-31T14:48:29Z

+		return false
+	}
+	status := apiStatus.Status()
+	return status.Reason == metav1.StatusReasonInternalError && strings.Contains(status.Message, "failed calling webhook")


You're right, it is only wrapped under apierrors.NewInternalError sadly.

soltysh · 2026-08-31T14:51:06Z

+		return false
+	}
+
+	if wrapped, ok := err.(interface{ Unwrap() []error }); ok {


Certainly open to thoughts - my goal was to ensure we're conservative with what is allowed and then loosen/add as required.

I believe here, the same rule applies you've mentioned in the other comment. Whenever errors are wrapped like NewInternalError the double wrapping will not happen, since the internal error will be printed as a string in the details of the message. So I believe, here, it'll be simpler for us to just find the first and only errors.APIStatus. It will also make the code simpler to read 😉

soltysh · 2026-08-31T14:51:26Z

+		return false
+	}
+
+	if wrapped, ok := err.(interface{ Unwrap() []error }); ok {


This can be done in a followup.

…#5287) Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>

brandtkeller added 3 commits August 26, 2026 23:41

feat: support retryable errors for helm installs

2ad7a67

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

Merge branch 'main' of github.com:zarf-dev/zarf into 5286_helm_retry

d433087

fix: simplify logic handling

7cef81e

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

brandtkeller self-assigned this Aug 26, 2026

github-project-automation Bot added this to Zarf Aug 26, 2026

brandtkeller and others added 3 commits August 27, 2026 18:18

fix: update language

1bd6b27

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

fix: docs generation and linting

00bd64f

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

Merge branch 'main' into 5286_helm_retry

12c7741

brandtkeller marked this pull request as ready for review August 27, 2026 21:46

brandtkeller requested review from a team as code owners August 27, 2026 21:46

soltysh reviewed Aug 28, 2026

View reviewed changes

fix: update unit tests for join and leaves

f2f3ef3

Signed-off-by: Brandt Keller <brandt.keller@defenseunicorns.com>

brandtkeller requested a review from soltysh August 29, 2026 13:59

soltysh approved these changes Aug 31, 2026

View reviewed changes

soltysh added this pull request to the merge queue Aug 31, 2026

Merged via the queue into main with commit cdb212b Aug 31, 2026
42 checks passed

soltysh deleted the 5286_helm_retry branch August 31, 2026 15:16

github-project-automation Bot moved this to Done in Zarf Aug 31, 2026

zarf-release-please Bot mentioned this pull request Aug 28, 2026

chore(main): release 0.85.0 #5263

Merged

Uh oh!

Conversation

brandtkeller commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Checklist before merging

Uh oh!

netlify Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for zarf-docs canceled.

Uh oh!

codecov Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brandtkeller commented Aug 26, 2026 •

edited

Loading

netlify Bot commented Aug 26, 2026 •

edited

Loading

codecov Bot commented Aug 27, 2026 •

edited

Loading