Keep a dataless 2xx from FindApplicationKeyOwner retryable - #42
Keep a dataless 2xx from FindApplicationKeyOwner retryable#42c1-squire-dev[bot] wants to merge 2 commits into
Conversation
FindApplicationKeyOwner joined ErrApplicationKeyOwnerUnknown onto any
response whose Data was nil, alongside the two genuinely ownerless
shapes. But Data == nil is not the provider answering and naming no
owner: the generated ApplicationKeyResponse.UnmarshalJSON decodes into
an anonymous struct and only assigns o.Data from the payload's data key,
returning a nil error when a 200 body simply has no data. A 200 carrying
{}, or a proxy's error envelope, therefore arrives as err == nil with
Data == nil -- a transport anomaly, not a provider answer.
Both Delete call sites in pkg/connector/application_key.go branch on the
sentinel with errors.Is, so that anomaly was converted into a permanent
codes.InvalidArgument refusal. A retryable failure became terminal, and a
live Datadog application key was left un-revokable.
Split the nil-Data case into its own branch ahead of the sentinel guard,
returning a plain wrapped error with no sentinel joined. It falls
through Delete's default arm, keeping its code and staying retryable.
TestApplicationKeyBuilderDeleteKeepsDatalessSuccessBodyRetryable serves
a 200 with a syntactically valid but dataless body. The existing sibling
cannot reach this branch -- its body is syntactically invalid JSON, so
Unmarshal errors and the lookup exits through wrapOfficialClientError
before the guard, landing on codes.Unknown. With this change reverted
the new test fails on codes.InvalidArgument; with it applied it passes.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| require.NotErrorIs(t, err, client.ErrApplicationKeyOwnerUnknown, | ||
| "a dataless 2xx is not the provider naming no owner") |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this NotErrorIs assertion is vacuous — Delete's sentinel arm formats the cause with %v (application_key.go:106), so the sentinel never chains into the returned error and this passes even with the client.go fix reverted. Only the NotEqual(codes.InvalidArgument) check below is discriminating. The sibling ownerless test at line 611-615 already establishes the convention for this: assert at the producer, e.g. _, ownerErr := newLifecycleTestWrapper(server.URL).FindApplicationKeyOwner(context.Background(), "appkey-dataless-1"); require.NotErrorIs(t, ownerErr, client.ErrApplicationKeyOwnerUnknown).
Connector PR Review: Keep a dataless 2xx from FindApplicationKeyOwner retryableBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness: the Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
The NotErrorIs assertion on Delete's return in TestApplicationKeyBuilderDeleteKeepsDatalessSuccessBodyRetryable could not fail. applicationKeyBuilder.Delete's sentinel arm builds its error with status.Errorf and a %v verb for the cause, which does not wrap it, so client.ErrApplicationKeyOwnerUnknown is unreachable through errors.Is from Delete -- the assertion passed even with the client-side bug present, for the wrong reason. Assert the sentinel where it is produced instead, following the convention the sibling ownerless test already uses: FindApplicationKeyOwner is called directly and its error is checked there, where the sentinel really is in the chain. Delete's return keeps the assertions it can actually carry -- the error itself and the surviving gRPC code, whose InvalidArgument flip is what discriminates the fix. Hoist the app key id into a const shared by both calls, and note in the doc comment why the sentinel is asserted at the producer. With the client.go fix reverted and the NotEqual assertion masked, the producer-level NotErrorIs fails on its own -- the sentinel is found in the chain. With the fix applied, both assertions pass. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
1 participant
Summary
FindApplicationKeyOwnertreatedresponse.Data == nilas one of the two genuinely ownerless response shapes, joiningErrApplicationKeyOwnerUnknownonto it. It is not one: it is a transport anomaly, and joining the sentinel made it permanent.This splits the nil-
Datacase into its own branch ahead of the sentinel guard, returning a plain wrapped error with no sentinel joined.The failure mode
Which 2xx bodies reach it.
GetApplicationKeyreturns HTTP 200 with a body that has nodatakey — most simply{}, or an error envelope substituted by an intermediary proxy.Why the generated type returns nil error with nil
Data.ApplicationKeyResponse.UnmarshalJSONin the vendored Datadog client decodes into an anonymous struct and only assignso.Datafrom the payload'sdatakey. When that key is absent it setso.Data = niland returns a nil error — the absence of data is not treated as a failure. So the 200 arrives at the connector aserr == nil, response.Data == nil.What it cost. Both
Deletecall sites inpkg/connector/application_key.gobranch on the sentinel witherrors.Is(err, client.ErrApplicationKeyOwnerUnknown). That branch returned a permanentcodes.InvalidArgument:A retryable transport failure became a terminal refusal, and a live Datadog application key was left un-revokable — no amount of retrying could clear it, because the client had already decided the provider had answered.
The two remaining conditions in the sentinel guard —
Relationships == nilandOwnedBy == nil— stay as they are. Those really are the provider answering successfully and naming no owner, which no retry changes.Test
TestApplicationKeyBuilderDeleteKeepsDatalessSuccessBodyRetryableserves HTTP 200 with a syntactically valid but dataless body ({}) from theGetApplicationKeyendpoint and asserts the error fromDeleteis neithercodes.InvalidArgumentnorerrors.Is(err, client.ErrApplicationKeyOwnerUnknown).The existing sibling
TestApplicationKeyBuilderDeleteKeepsUnmappedTransportFailureRetryablecannot catch this: its response body is syntactically invalid JSON, soUnmarshalerrors and the lookup exits throughwrapOfficialClientErrorbefore ever reaching theData == nilbranch, landing oncodes.Unknown. The new test is what reaches it.Discrimination proof (test added first, then client change reverted to confirm it fails):
client.gochange revertedShould not be: 0x3(codes.InvalidArgument) on the "a transport failure must not be converted into a permanent refusal" assertionNote on the second assertion: the
NotErrorIscheck passes in both directions becauseDelete's sentinel arm formats the cause with%vrather than chaining it — the same reason the neighbouring ownerless test asserts the sentinel at the producer.codes.InvalidArgumentis the discriminating assertion, and it flips as shown above.Gates
go build ./...— cleango vet ./...— cleango test ./... -count=1— all packages passgolangci-lint run— 0 issuesOnly
pkg/client/client.goandpkg/connector/credential_lifecycle_test.gochange;go.mod,go.sum,vendor/, and.versions.yamlare untouched.