Add int-type Authz ID proto fields alongside any string-type proto fields#8754
Add int-type Authz ID proto fields alongside any string-type proto fields#8754ezekiel wants to merge 12 commits into
Conversation
This is stage one of normalizing Authz ID to be consistently int64-type.
Fixes to Load Generator incoming.
Co-authored-by: Aaron Gable <aaron@letsencrypt.org>
AuthorizationID and AuthorizationIDs in sa.proto where unreferenced and unused. Apparent remnants from before Authz 2.
|
Still adding real fixes for load-generator. |
this instead of overloading a core.Authorization.ID
| return core.Authorization{}, ErrInvalidParameters | ||
| } | ||
| authzIDInt = parsed | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { | |
| return core.Authorization{}, ErrMissingParameters | |
| } |
There was a problem hiding this comment.
This particular spot is very interesting - it did not historically have any protection against zero-values. When added, it causes other test failures. I'll re-add the error return here and see if there's something obvious to fix because I'd rather have the protection than not.
There was a problem hiding this comment.
This test file probably needs more updates than this: there should be tests for PBToAuthz with a string input, an int input, both, and neither.
| return nil, berrors.MalformedError("Unable to parse Authz ID %q as integer: %v", req.AuthzID, err) | ||
| } | ||
| authzIDInt = parsed | ||
| } |
| "sync" | ||
| "time" | ||
|
|
||
| corepb "github.com/letsencrypt/boulder/core/proto" |
There was a problem hiding this comment.
Imports of github.com/letsencrypt/boulder packages should always be grouped below.
There was a problem hiding this comment.
Same comment here: this file is going to want test cases for AuthzMeta containing just a string ID, just an int ID, both, and neither.
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| dbAuthzPB := getAuthorization(t, authzPB.Id, sa) | ||
| // TODO(#8722): Remove this when Authz ID is int64-only |
There was a problem hiding this comment.
There shouldn't be any need to do this logic in the unit tests. This logic is necessary for prod because each service doesn't know what version of the other services its talking to over gRPC -- a deploy might be in progress, or might be stuck halfway, or any number of other scenarios. But here in the unit tests, this code knows that we're using the latest version of the RA code. No need for these safety rails, just use the int.
| test.AssertEquals(t, err.Error(), "certificate public key must be different than account key") | ||
| } | ||
|
|
||
| func TestDeactivateAuthorization(t *testing.T) { |
There was a problem hiding this comment.
Add cases to this test for string authz ID, int ID, both, and neither. You can probably do this by manipulating the dbAuthzPB returned by getAuthorization.
| if req.Authz.Id == "" && req.Authz.IdInt == 0 { | ||
| return nil, errIncompleteGRPCRequest |
There was a problem hiding this comment.
No need for this, PBToAuthz will return an error if neither is populated. But keep the TODO, so we remember to add it back to the IsAnyNilOrZero check in the followup.
| var ( | ||
| errIncompleteGRPCRequest = errors.New("incomplete gRPC request message") | ||
| errIncompleteGRPCResponse = errors.New("incomplete gRPC response message") | ||
| errMalformedGRPCRequest = errors.New("malformed gRPC request message field") |
There was a problem hiding this comment.
This error message is only returned in once place. No need to predeclare it like this; the others are only up here for the sake of consistency, because they're returned from dozens of spots.
2 participants
This is stage one of normalizing Authz ID to be consistently int64-type (#8722).