Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change int64 nanoseconds to timestamppb in proto messages #7060

Closed
aarongable opened this issue Aug 29, 2023 · 0 comments · Fixed by #7142
Closed

Change int64 nanoseconds to timestamppb in proto messages #7060

aarongable opened this issue Aug 29, 2023 · 0 comments · Fixed by #7142
Assignees

Comments

@aarongable
Copy link
Contributor

We have lots of proto messages with fields like this:

boulder/sa/proto/sa.proto

Lines 197 to 202 in 66a4c11

message AddSerialRequest {
int64 regID = 1;
string serial = 2;
int64 created = 3; // Unix timestamp (nanoseconds)
int64 expires = 4; // Unix timestamp (nanoseconds)
}

Those int64s are nanoseconds since the unix epoch, which is so unclear that we have to comment it in every single proto file. Clients have to produce them with

nowNanos := ca.clk.Now().UnixNano()

and servers have to parse them with
Created: time.Unix(0, req.Created),

And of course, just using an integer for this leads to fun times if you mess up, because the go zero value for time.Time{} is not the same thing as 0 nanoseconds since the unix epoch.

Thankfully, gRPC offers an alternative: timestamppb. We use it in a couple places:

boulder/sa/proto/sa.proto

Lines 354 to 359 in 66a4c11

message LeaseCRLShardRequest {
int64 issuerNameID = 1;
int64 minShardIdx = 2;
int64 maxShardIdx = 3;
google.protobuf.Timestamp until = 4;
}

Client code is much clearer:
Until: timestamppb.New(deadline.Add(-time.Second)),

as is accessing the value as a server:

boulder/sa/sa.go

Line 1004 in 66a4c11

req.Until.AsTime(),

So it would be nice to change all of our usages of int64 nanoseconds into timestamppb instead. I think the process will look like this:

  1. Rename all of our current int64 fields (e.g. notBefore) to their name plus "NS" for nanoseconds (e.g. notBeforeNS). This change should be safe and deployable because gRPC field names are not sent across the wire, they're process local, so no client or server cares what other clients or servers name the message fields.
  2. Add new timestamppb fields to all of our messages, using the old (good) names, and populate them at the same time as we populate the existing nanosecond fields. This change should be safe because adding new fields to gRPC messages is always safe.
  3. Switch to reading values from the new fields, completely ignoring the old fields. This change will be safe once the second change has fully deployed.
  4. Stop populating and fully remove the old fields. This change will be safe once the third change has fully deployed.
@aarongable aarongable added this to the Sprint 2023-09-05 milestone Sep 5, 2023
pgporada added a commit that referenced this issue Sep 15, 2023
Rename all of int64 timestamp fields to `<fieldname>NS` to indicate they
are Unix nanosecond timestamps.

Part 1 of 4 related to
#7060
pgporada added a commit that referenced this issue Oct 11, 2023
* Adds new `google.protobuf.Timestamp` fields to each .proto file where
we had been using `int64` fields as a timestamp.
* Updates relevant gRPC messages to populate the new
`google.protobuf.Timestamp` fields in addition to the old `int64`
timestamp fields.
* Added tests for each `<x>ToPB` and `PBto<x>` functions to ensure that
new fields passed into a gRPC message arrive as intended.
* Removed an unused error return from `PBToCert` and `PBToCertStatus`
and cleaned up each call site.

Built on-top of #7069
Part 2 of 4 related to
#7060
pgporada added a commit that referenced this issue Oct 30, 2023
…#7121)

Switch to reading grpc timestamp values from the new timestamppb
protofbuf fields, completely ignoring the old int64 fields.

Part 3 of 4 for #7060
jsha pushed a commit that referenced this issue Nov 27, 2023
This is a cleanup PR finishing the migration from int64 timestamps to
protobuf `*timestamppb.Timestamps` by removing all usage of the old
int64 fields. In the previous PR
#7121 all fields were
switched to read from the protobuf timestamppb fields.

Adds a new case to `core.IsAnyNilOrZero` to check various properties of
a `*timestamppb.Timestamp` reducing the visual complexity for receivers.

Fixes #7060
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants