unleash: Create dereferences a nil response when bifrost returns any non-201

[Starefossen]

Summary

unleash.Create never checks bifrost's HTTP status. The generated client returns err only for transport failures, so any non-201 response leaves resp.JSON201 nil, and the nil flows into toUnleashInstance, which dereferences it.

Severity: low today, rising — see "Why this has not bitten yet"
Component: internal/unleash/queries.go (Create), internal/unleash/models.go:221 and the toUnleashInstance below it

Mechanism

resp, err := client.CreateInstance(ctx, req)
if err != nil {
    return nil, err          // transport errors only
}
unleashInstance := bifrostUnleashToK8s(resp.JSON201)   // nil for any non-201
…
return toUnleashInstance(unleashInstance), nil          // dereferences u.Spec

bifrostUnleashToK8s guards nil and returns nil (models.go:222-224), but toUnleashInstance starts with for _, env := range u.Spec.ExtraEnvVars with no guard.

Reachable bifrost responses today: 400 validation_failed, 400 no_version_source, 500 creation_failed.

Why this has not bitten yet

Loki, 14 days of bifrost traffic: 2985 GET /v1/releasechannels, 15 PUT /v1/unleash/{name}, and zero POST /v1/unleash. Instance creation is essentially never exercised, so the path has not been hit. No matching panic appears in nais-api's logs over the same window.

Why it is worth fixing now

nais/bifrost#549 adds two new create responses — 409 already_exists when the instance already exists, and 503 when the existence check is inconclusive. The 409 in particular is raised on a duplicate POST, which is the one non-exotic way to hit a non-201. It converts a rare data-loss path in bifrost into a nil dereference here.

Proposed fix

Check the status before converting, and map bifrost's error responses to sensible GraphQL errors — at minimum 409 should surface as "an Unleash instance already exists for this team" rather than an internal error. A nil guard in toUnleashInstance is worth having regardless, as defence in depth.

Acceptance criteria

• A non-201 from bifrost returns an error, not a nil dereference.
• 409 surfaces a distinguishable "already exists" message.
• toUnleashInstance tolerates nil.
• Test covering at least the 409 and 500 responses.

[Starefossen]

Correction — the nil dereference cannot happen

I was wrong. Retracting the mechanism in the description.

fromContext(ctx).bifrostClient is of type BifrostClient (internal/unleash/dataloader.go:58), the hand-written wrapper — not the generated client. The wrapper already checks the status:

func (b *bifrostClientImpl) CreateInstance(...) (*bifrostclient.CreateInstanceResponse, error) {
	resp, err := b.client.CreateInstanceWithResponse(ctx, req)
	if err != nil {
		return nil, b.logError(err, "CreateInstance", "calling bifrost")
	}
	if resp.JSON201 != nil {
		return resp, nil
	}
	return nil, b.handleErrorResponse(resp.HTTPResponse, resp.JSON400, resp.JSON500, "CreateInstance")
}

So a non-201 returns (nil, err), queries.go returns early on the error, and resp.JSON201 is non-nil on every path that reaches bifrostUnleashToK8s. GetInstance and UpdateInstance guard the same way.

I read queries.go in isolation and inferred the consequence without checking what type the client actually was.

What is actually true, and much milder

For a 409 or 503 — the two responses nais/bifrost#549 adds — both JSON400 and JSON500 are nil, so handleErrorResponse falls through to its generic branch:

return fmt.Errorf("bifrost %s returned %s", operation, resp.Status)

The user gets "bifrost CreateInstance returned 409 Conflict" instead of "an Unleash instance already exists for this team". That is a message-quality issue, not a crash, and there is no nil dereference to guard against.

Revised scope

• Map 409 to a clear "already exists" message on create.
• Decide whether 503 (bifrost could not determine whether the instance exists) should be retryable from the caller's perspective or surfaced as-is.
• A nil guard in toUnleashInstance is still cheap defence in depth, but it is not fixing a live defect.

Downgrading severity accordingly. Sorry for the noise.