Replace custom multi-error joining with errors.Join

[Manolo Mancebo (rh-returners)]

Which issue this PR addresses:

Fixes ARO-15179

What this PR does / why we need it:

Replaces custom multi-error aggregation patterns (collecting []string and joining with strings.Join) with the standard errors.Join introduced in Go 1.20, across four packages: pkg/cluster, pkg/operator/controllers/checkers/internetchecker, pkg/operator/controllers/subnets, and pkg/util/deployer.
Additionally upgrades %v to %w in error format strings where errors were previously being collected, preserving the full wrapping chain for errors.Is/errors.As callers.

As part of review, two related fixes were folded in:

• pkg/cluster/loadbalancerprofile.go - both deleteUnusedManagedIPs and createPublicIPAddresses originally folded their header text into the errors.Join slice as a fake peer error, which would have made it indistinguishable from real sub-error to any errors.Is/errors.As caller. Both now wrap the join instead.
• pgk/operator/controllers/subnets/subnets_controller.go 's reconcileSubnets previously collected raw Azure SDK errors with no subnet identity, so multi-subnet failure gave no indication of which subnet caused which error. Each sub-error is now tagged with its subnet name at the collection site.

Test plan for issue:

• Existing unit tests in pkg/cluster (TestReconcileLoadBalancerProfile) cover the changed error paths. The test assertion was also improved: the previous assert.Contains(t, tt.expectedErr, err) compares elements via reflect.DeepEqual (testify's ObjectsAreEqual), which requires equal concrete types, not just equal values. Once the returned error's concrete type changed to errors.joinError (or *fmt.wrapError where %w is used), it could never be DeepEqual to the *error.errorString values in tt.expectedErr, regardless of message content so the assertion would have failed indiscriminately rather than validating message correctness it now compares .Error() strings directly. making it sensitive to message format. All tests pass after the changes (make unit-test-go).
• No new failure modes were introduced. Most error messages are unchanged, two categories intentionally changes as a result of the header-wrap and subnet-identity fixes above.

Is there any documentation that needs to be updated for this PR?

No — this is a tech debt cleanup replacing a pre-Go-1.20 workaround with the idiomatic standard library equivalent. No user-facing behaviour or API contract changes.

How do you know this will function as expected in production?

The error messages surfaced in logs are identical to those produced before this change. The only behavioural difference is that errors are now proper wrapped multi-errors, meaning errors.Is/errors.As can now unwrap individual sub-errors a strict improvement. These errors are internal to the RP control plane and operator and are not returned directly in CloudError responses to callers.

[Copilot]

🟢 Approval recommended

The changes are a mechanical refactor to standard error-joining/wrapping with a targeted test assertion improvement, and the updated code paths preserve message formatting while improving unwrap behavior.

Pull request overview

This PR modernizes multi-error aggregation across the RP and operator code by replacing ad-hoc string-joining patterns with standard-library errors.Join, and updates error formatting to use %w so callers can unwrap sub-errors via errors.Is/errors.As while preserving the rendered error messages.

Changes:

• Replace []string + strings.Join multi-error construction with errors.Join in deployer, operator subnet reconciliation, and internet connectivity checker code.
• Improve error wrapping in pkg/cluster/loadbalancerprofile.go by switching relevant %v/string formatting to %w and joining per-item failures via errors.Join.
• Fix and harden the TestReconcileLoadBalancerProfile assertion to compare error messages rather than (non-comparable) error identities.

File summaries

File	Description
pkg/util/deployer/deployer.go	Wraps aggregated removal errors using errors.Join (with an outer contextual message).
pkg/operator/controllers/subnets/subnets_controller.go	Aggregates per-subnet reconcile errors as []error and returns a joined multi-error.
pkg/operator/controllers/checkers/internetchecker/checker.go	Returns a joined multi-error from concurrent URL checks (nil when none fail).
pkg/cluster/loadbalancerprofile.go	Converts cleanup/create IP failure aggregation to errors.Join and upgrades relevant formatting to %w.
pkg/cluster/loadbalancerprofile_test.go	Updates the test to assert on err.Error() against expected message strings.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Anton Asserzon (aasserzo)]

LGTM 👍

[Copilot]

🟡 Changes recommended

The internet checker does not yet wrap underlying errors with %w, so error-chain preservation remains incomplete.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Four moderate review comments request coverage proving joined and wrapped errors preserve their underlying causes.

Review details

Suppressed comments (4)

pkg/cluster/loadbalancerprofile_test.go:1141

• This assertion still verifies only Error() text, so a regression from the new %w/errors.Join chain back to string aggregation would pass. Add sentinel failures and assert they are discoverable through the returned error tree with errors.Is/errors.As (the repository's test/util/error.AssertErrorMatchesAll helper covers joined errors).

				expectedErrMsgs := make([]string, len(tt.expectedErr))
				for i, e := range tt.expectedErr {
					expectedErrMsgs[i] = e.Error()
				}
				assert.Contains(t, expectedErrMsgs, err.Error(), "Unexpected error exception")

pkg/operator/controllers/checkers/internetchecker/checker.go:69

• The checker tests cover only one URL and compare only Error() text, so they do not verify that the new errors.Join result preserves underlying errors or aggregates multiple failures. Add a multi-URL case with sentinel errors and assert them with errors.Is/errors.As.

	return errors.Join(errsAll...)

pkg/operator/controllers/subnets/subnets_controller.go:146

• There is no failure case in the reconcile manager tests that exercises this new joined-error path. Add a case where both subnet operations fail and assert the returned error contains both underlying failures, not just its formatted message.

		return errors.Join(combinedErrors...)

pkg/util/deployer/deployer.go:131

• The new errors.Join wrapping is not exercised by TestDeployDeleteFailure, which checks only the rendered message. Add a sentinel deletion error and assert errors.Is on the returned error so this %w contract cannot regress to the previous string-only aggregation.

		return fmt.Errorf("error removing resource:\n%w", errors.Join(errs...))

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Hevellyn (ehvs)]

LGTM

[Andrew Denton (ventifus)]

Overall this is a clean and complete migration — all six Jira-listed sites are converted, the TODO comment in internetchecker is properly retired, and the %w upgrades improve the error chain. A few things worth addressing:

Test assertion rationale: the PR description says the old assert.Contains(t, tt.expectedErr, err) "compared error objects by identity". That's not quite right — testify uses reflect.DeepEqual, so it was comparing by value. The real reason the assertion needed changing is a type mismatch: errors.Join returns *errors.joinError (an unexported stdlib type), which is not DeepEqual to the *errors.errorString elements in tt.expectedErr even when the .Error() strings are identical. The fix itself is correct and necessary; the stated reason just misidentifies why.

Inconsistent header patterns in loadbalancerprofile: see inline comments on the two errors.Join(append(...)...) sites.

Missing subnet identity in subnets_controller: see inline comment.

[Manolo Mancebo (rh-returners)]

Test assertion rationale: the PR description says the old assert.Contains(t, tt.expectedErr, err) "compared error objects by identity". That's not quite right — testify uses reflect.DeepEqual, so it was comparing by value. The real reason the assertion needed changing is a type mismatch: errors.Join returns *errors.joinError (an unexported stdlib type), which is not DeepEqual to the *errors.errorString elements in tt.expectedErr even when the .Error() strings are identical. The fix itself is correct and necessary; the stated reason just misidentifies why.

Andrew Denton (@ventifus) , Thanks for your honest review, I have re-written PR description to match with what you mentioned.

[Copilot]

🟡 Changes recommended

Moderate message-format changes remain, and tests do not fully verify joined error causes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

pkg/cluster/loadbalancerprofile.go:65

• The updated test still compares only err.Error(), so it would pass if any of the new %w verbs regressed to %v. Since preserving individual causes is the purpose of this change, use sentinel inputs and assert errors.Is (for example via test/util/error.AssertErrorMatchesAll) for the create and cleanup errors.

		return fmt.Errorf("multiple errors occurred while updating outbound-rule-v4\n%w\n%w", err, cleanupError)

pkg/cluster/loadbalancerprofile_test.go:1141

• The updated assertion still checks only the rendered string. It does not verify the new %w/errors.Join contract, so the test would pass if the individual create or cleanup causes were converted back to text; add sentinel failures and assert errors.Is on the returned aggregate.

				require.Error(t, err, "Expected an error but got none")
				expectedErrMsgs := make([]string, len(tt.expectedErr))
				for i, e := range tt.expectedErr {
					expectedErrMsgs[i] = e.Error()
				}
				assert.Contains(t, expectedErrMsgs, err.Error(), "Unexpected error exception")

pkg/operator/controllers/subnets/subnets_controller.go:149

• No test case assigns wantErr in TestReconcileManager, so this new aggregation branch is not exercised. Add a case where both subnet helpers fail and assert the joined message plus errors.Is for each sentinel error.

		return fmt.Errorf("failed to reconcile subnets: %w", errors.Join(combinedErrors...))

pkg/util/deployer/deployer.go:131

• Only a single failure is tested here, and the test checks only Error(), so the new errors.Join path is not verified for multiple removal failures or errors.Is. Add a case with two sentinel errors and use test/util/error.AssertErrorMatchesAll or AssertErrorIs to verify both causes survive.

		return fmt.Errorf("error removing resource:\n%w", errors.Join(errs...))

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

[Miguel Abad Perez (tiguelu)]

Looks good, left some minor nit comments.

[Copilot]

🟢 Approval recommended

No blocking issues were identified in the reviewed changes.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

Only a minor test-coverage nit remains; no blocking issues were identified.

Review details

Suppressed comments (1)

pkg/operator/controllers/subnets/subnets_controller.go:137

• The new aggregation path is not exercised by TestReconcileManager: every table case succeeds or skips not-found subnets, and no case sets wantErr. A regression could therefore remove the subnet identity or stop preserving the wrapped cause unnoticed; add a failing NSG or service-endpoint case that asserts the subnet-tagged message and errors.Is for the sentinel cause.

				combinedErrors = append(combinedErrors, fmt.Errorf("failed to reconcile subnet: %s: %w", subnetName, err))

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite