Annotate proto messages with version and detect etcd version generated the wal by serathius · Pull Request #13216 · etcd-io/etcd

serathius · 2021-07-14T09:07:20Z

Implements annotations as part of #13168

This PR introduces custom proto annotations etcd_version_msg, etcd_version_field, etcd_version_enum, etcd_version_enum_value.

They will be used to indicate which version a specific field/message/enum etc was introduced, and will be used during downgrade to check compatibility. For example when downgrading storage to v3.5 we should make sure wal doesn't include any message or sets any field from v3.6+. As result we can be sure that WAL will be correctly interpreted by older version.

Notes to reviewer:

For now I used unassigned proto field numbers, those should be replaced with official ones, by registering them in https://github.com/protocolbuffers/protobuf/blob/master/docs/options.md
After a day of trying I gave up on reading Extensions to read annotation values. gogo/protobuf registers custom options in their own register that is not compatible with google/protobuf. As so I cannot use protoreflect to analyse proto in a convenient way. As a workaround I dump the options to string and parse them myself. Background GetExtension on FileOptions error: proto: not an extendable proto gogo/protobuf#347

serathius · 2021-07-15T13:27:04Z

cc @lilic

lilic

Couple of initial questions haven't looked at the wal code yet.

lilic · 2021-07-15T13:39:22Z

api/etcdserverpb/raft_internal.proto

-  membershippb.ClusterVersionSetRequest cluster_version_set = 1300;
-  membershippb.ClusterMemberAttrSetRequest cluster_member_attr_set = 1301;
-  membershippb.DowngradeInfoSetRequest  downgrade_info_set = 1302;
+  membershippb.ClusterVersionSetRequest cluster_version_set = 1300 [(versionpb.etcd_version_field) = "3.5"];


Just trying to understand, will these changes need to be backported to 3.5 release as well? Would this create any compatibility issues?

No, storage downgrade is done by older binaries. It's enough to have those annotations in v3.6, so it can verify the WAL and mark it as v3.5 compatible. v3.5 doesn't need to now about annotations.

lilic · 2021-07-15T13:42:34Z

api/etcdserverpb/raft_internal.proto

  AuthEnableRequest auth_enable = 1000;
  AuthDisableRequest auth_disable = 1011;
-  AuthStatusRequest auth_status = 1013;
+  AuthStatusRequest auth_status = 1013 [(versionpb.etcd_version_field) = "3.5"];


Did you have to manually write these versions, if so will they change in future release will we have to keep bumping them?

No need to bump them, those annotations indicate etcd version that introduced the specific field/message. However any new message/field added after this change should include those annotations.

Thanks for that.

However any new message/field added after this change should include those annotations.

is this enforced in any way? e.g. with tests or failure of proto generation?

+1 to enforcing.
If we had no 'message level annotations', it would be easier to enforce no fields without an annotation
in the transitive proto-descriptor.

Enforcing is definitely in the plan. It might be a little harder to implement with message and enum annotations, but it should be possible if we introduce allowlisting (enforcer will require always message version set, but for fields only if they are not in allow list maintained by enforcer).

Decided on splitting message vs field and enum vs enum value based on:

Enum default value (equal 0) will never be considered set, so annotating it doesn't make sense. It can be also confusing as someone might think that setting this enum value will mean that required etcd version is changed, but it's not.

Messages with no fields, for example AuthStatusRequest. We technically can determine the version based on field annotation that uses this message, but I think it might become confusing when we will add some fields to this message. Decided that messages should be self sufficient to define their etcd version.

Readability, having this annotation on each and every field makes the proto much less readable. I would prefer to keep the proto definition cleaner and even if the enforces will be more complicated.

Enforcing of the version is not done to all the proto files, like lease.proto doesn't have it, so how do we determine going forward which require a version field in a tested way?

For storage versioning we just need to ensure that protos used in WAL have annotations. This means that we should look for all submessages from raftpb.Entry as a root. We might consider expanding this to all proto if that would useful for other purposes.

server/etcdserver/version/wal.go

api/versionpb/version.proto

ptabor · 2021-07-15T14:14:19Z

api/etcdserverpb/raft_internal.proto

  AuthEnableRequest auth_enable = 1000;
  AuthDisableRequest auth_disable = 1011;
-  AuthStatusRequest auth_status = 1013;
+  AuthStatusRequest auth_status = 1013 [(versionpb.etcd_version_field) = "3.5"];


+1 to enforcing.
If we had no 'message level annotations', it would be easier to enforce no fields without an annotation
in the transitive proto-descriptor.

api/versionpb/version.proto

serathius · 2021-07-19T15:27:25Z

Addressed/responded on all issues. PTAL

lilic

Mainly reviewed as I was re-reading the storage versioning design doc.

raft/raftpb/raft.proto

server/storage/schema/wal.go

lilic · 2021-07-29T12:22:30Z

api/etcdserverpb/raft_internal.proto

  AuthEnableRequest auth_enable = 1000;
  AuthDisableRequest auth_disable = 1011;
-  AuthStatusRequest auth_status = 1013;
+  AuthStatusRequest auth_status = 1013 [(versionpb.etcd_version_field) = "3.5"];


Enforcing of the version is not done to all the proto files, like lease.proto doesn't have it, so how do we determine going forward which require a version field in a tested way?

raft/raftpb/raft.proto

api/etcdserverpb/rpc.proto

…enerated wal Co-authored-by: Lili Cosic <cosiclili@gmail.com>

alxn · 2025-07-08T03:47:39Z

For now I used unassigned proto field numbers, those should be replaced with official ones

Any update on this ?

serathius force-pushed the wal branch 2 times, most recently from 22b26c1 to b52a31d Compare July 14, 2021 11:07

serathius changed the title ~~Wal~~ Annotate proto messages with version and detect etcd version generated the wal Jul 14, 2021

serathius mentioned this pull request Jul 14, 2021

Implement storage versioning #13168

Closed

13 tasks

serathius force-pushed the wal branch 3 times, most recently from 3d661c6 to 3917301 Compare July 14, 2021 13:50

serathius requested a review from ptabor July 14, 2021 13:51

lilic reviewed Jul 15, 2021

View reviewed changes

ptabor reviewed Jul 15, 2021

View reviewed changes

serathius force-pushed the wal branch 2 times, most recently from da1f4e1 to bdaebb0 Compare July 16, 2021 12:17

serathius mentioned this pull request Jul 26, 2021

Explore using vtprotobuf #13107

Closed

lilic reviewed Jul 29, 2021

View reviewed changes

serathius commented Jul 29, 2021

View reviewed changes

raft/raftpb/raft.proto Outdated Show resolved Hide resolved

serathius commented Jul 29, 2021

View reviewed changes

raft/raftpb/raft.proto Outdated Show resolved Hide resolved

serathius force-pushed the wal branch 3 times, most recently from de6d681 to 5d3d98c Compare July 30, 2021 12:08

ptabor reviewed Aug 2, 2021

View reviewed changes

api/etcdserverpb/rpc.proto Outdated Show resolved Hide resolved

ptabor approved these changes Aug 2, 2021

View reviewed changes

serathius force-pushed the wal branch 4 times, most recently from bf11d4f to f36eec0 Compare August 4, 2021 11:29

api: Annotate proto messages and use it to detect etcd version that g…

1b4e54c

…enerated wal Co-authored-by: Lili Cosic <cosiclili@gmail.com>

serathius force-pushed the wal branch from f36eec0 to 1b4e54c Compare August 4, 2021 11:36

ptabor merged commit 873f369 into etcd-io:main Aug 6, 2021

serathius deleted the wal branch June 15, 2023 20:36

alxn mentioned this pull request Jul 8, 2025

etcd 3rd party proto library is causing a proto namespace conflict with in-house applications. #20311

Closed

4 tasks

ds8088 mentioned this pull request Dec 26, 2025

logger: conflict in protobuf field option IDs due to usage of reserved ID range livekit/protocol#1350

Open

Conversation

serathius commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serathius commented Jul 15, 2021

Uh oh!

lilic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serathius commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alxn commented Jul 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

serathius commented Jul 14, 2021 •

edited

Loading

serathius commented Jul 19, 2021 •

edited

Loading