Remove all string ID fields for Authzs.#8828
Conversation
ezekiel
changed the title
|
|
||
| // Ensure gRPC response is complete. | ||
| if core.IsAnyNilOrZero(authzPB.Id, authzPB.Identifier, authzPB.Status, authzPB.Expires) { | ||
| if core.IsAnyNilOrZero(authzPB.IdInt, authzPB.Identifier, authzPB.Status, authzPB.Expires) { |
There was a problem hiding this comment.
Looking at #8754, stage 1 modified all the SA/RA methods to prefer the int64 Id, but the WFE's gRPC response core.IsAnyNilOrZero() checks still required the string Id to be non-empty. This is the case in three places:
- wfe2/wfe.go:1198 (Challenge)
- wfe2/wfe.go:1397 (postChallenge)
- wfe2/wfe.go:1618 (Authorization)
The problem I'm foreseeing is that during the rollout of stage 2, every stage 1 WFE will answer every authz and challenge request with 500 "Problem getting authorization" as soon as the SA/RA behind it has been updated.
We should probably ship another PR, call it stage 1.5, that swaps authzPB.Id for authzPB.IdInt in these three checks. Deploy that and then land the rest of this PR as stage 2.
| outAuthz3, err = PBToAuthz(pbAuthz3) | ||
| test.AssertNotError(t, err, "PBToAuthz with only int IDInt failed") | ||
| test.AssertDeepEquals(t, inAuthz, outAuthz3) | ||
| } |
There was a problem hiding this comment.
We need a test that sets IdInt = 0 on a PB and asserts PBToAuthz() returns ErrMissingParameters.
| dbAuthzPBIdChecks.IdInt = authzID | ||
| _, err = ra.DeactivateAuthorization(ctx, dbAuthzPBIdChecks) | ||
| test.AssertNotError(t, err, "Could not deactivate authorization") | ||
| deact, err = sa.GetAuthorization2(ctx, &sapb.AuthorizationID2{Id: authzID}) | ||
| test.AssertNotError(t, err, "Could not get deactivated authorization with ID "+dbAuthzPBIdChecks.Id) | ||
| test.AssertNotError(t, err, "Could not get deactivated authorization by ID") | ||
| test.AssertEquals(t, deact.Status, string(core.StatusDeactivated)) |
There was a problem hiding this comment.
I believe this block essentially duplicates the coverage of the block above it.
|
|
||
| // Manipulate authzPB to test marshalling between corepb.Authorization and | ||
| // the SA authz model | ||
| // TODO(#8722): clean up these tests when authz IDs are int-only |
There was a problem hiding this comment.
We need a test that sets IdInt = 0 and asserts authzPBToModel() returns an error that contains "authorization is missing an ID value".
| test.AssertContains(t, err.Error(), "duplicate remote VA perspective \"dadaist\"") | ||
| } | ||
|
|
||
| // TODO(#8722): Remove this whole function when Authz IDs are int-only |
There was a problem hiding this comment.
We need a test that sends a DoDCV() request with Authz.IdInt = 0 and ensures that it errors. Also, we should do the same for DoCAA() in caa_test.go.
Co-authored-by: Samantha Frank <hello@entropy.cat>
Co-authored-by: Samantha Frank <hello@entropy.cat>
3 participants
This is Stage 2 for #8722