[Bug] assert stops polling on any non-NotFound API error, and reports a stale mismatch instead of the real error #2758

chainsaw version

v0.2.15, and the same code is in main.

Description

assert treats every API error except NotFound as terminal, and then hides it. Two separate problems, and the second one is worse than the first.

The poll in pkg/engine/operations/assert/operation.go:

if candidates, err := internal.Read(ctx, &obj, o.client); err != nil {
    if kerrors.IsNotFound(err) {
        errs = append(errs, errors.New("actual resource not found"))
        return false, nil
    }
    return false, err
}

internal.Read is a single client.Get when the name is set, and client.PollInterval is 50ms, so a one minute wait is about 1200 GETs. One failed GET ends the operation, whatever timeout was configured for it.

The second problem is the reporting. The terminating error is dropped:

if err == nil { return nil }
if len(lastErrs) != 0 { return multierr.Combine(lastErrs...) }
return err

lastErrs is recorded in the callback only when err == nil, so what gets printed is the mismatch from an earlier poll, and the error that actually stopped the loop is gone. This one stands on its own even if you keep the current retry policy: every transient API error is reported as a field mismatch against a stale snapshot, so the reader is sent to the controller under test instead of to the API call that failed. The report was read three times, and each time the conclusion was that the controller had never touched the object.

How it looked in our CI: two of three apiservers logged rpctypes.EtcdError{code:0xe, desc:"etcdserver: leader changed"}, chainsaw's ASSERT ERROR is stamped 9ms later, and the resource became ready 1m52s after that, inside the assert's 6m timeout. The printed actual was an object snapshot from the first second of the wait.

pkg/engine/operations/error/operation.go has both properties too.

One more thing that made this hard to read: prune() in pkg/utils/diff/diff.go drops every key of the actual that has no literal counterpart in the expected tree, so an expression key like (conditions[?type == 'Ready']) never matches the literal conditions and the printed actual is status: {} for any condition assert.

I looked for an existing report first: the titles of the last 800 issues and PRs, bodies not read. #2755, #2597 and #2757 are the closest by wording and none of them is this.

Steps to reproduce

1. assert on a resource that needs about two minutes, timeout: 6m.
2. Make one GET fail with something other than NotFound while it polls. Forcing an etcd leader election does it, so does a proxy in front of the apiserver that injects a single 500.

Expected

Retriable API errors keep the poll running to the operation timeout: IsInternalError, IsServiceUnavailable, IsTimeout, IsServerTimeout, IsTooManyRequests, and 500s carrying a non-Status body. Forbidden, Unauthorized and malformed requests stay terminal.

When the poll does end on an error, print that error. The stale mismatch can stay if it is useful, but it needs a label saying which is which.

I can send a PR for both of these if you need.