DNM: Migrate off gogo protobuf - #3247
Draft
vvoland wants to merge 23 commits into
Draft
Conversation
protoc-gen-gogoswarm was built on gogo's generator framework, which has no counterpart in google.golang.org/protobuf. Replace it with protoc-gen-swarm, built on google.golang.org/protobuf/compiler/protogen, and port the storeobject, raftproxy and authenticatedwrapper plugins onto it. The deepcopy plugin is gone. protoc-gen-go-vtproto emits CloneVT, which does the same deep copy, so the generated Copy method is now a one-line wrapper around it and roughly ten thousand lines of hand-rolled copy code go away. CopyFrom is kept, implemented with proto.Reset and proto.Merge, so the CopierFrom interface still means something. Generated servers now have to satisfy the unexported guard method that protoc-gen-go-grpc puts on every service interface. The proxies emit it explicitly rather than embedding Unimplemented<Svc>Server: embedding would let a method the generator failed to emit compile fine and then silently resolve to a stub that bypasses authorization. containerd/protobuild drove generation before and is gogo-oriented, so hack/generate-protos.sh replaces it, invoking protoc with protoc-gen-go, protoc-gen-go-grpc, protoc-gen-go-vtproto and protoc-gen-swarm. It also emits api/api.pb.txt, which protobuild used to produce, using protoc alone. This commit does not build on its own: api/raft.proto embeds raftpb.Message, which welds the dependency update, the regeneration and the call-site changes into one unit. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
None of the gogoproto options survive the move to protoc-gen-go, and dropping
them changes the generated Go API:
- customname is gone, so identifiers take their stock spelling: ID becomes
Id, NodeID becomes NodeId, CAConfig becomes CaConfig. Enum values gain
their type prefix, so TaskStateRunning becomes TaskState_RUNNING.
- nullable=false is gone. The protobuf-go runtime cannot represent an
embedded message as a value, so all 62 such fields become pointers.
- stdduration fields become *durationpb.Duration, and the os.FileMode
customtype becomes a plain uint32.
None of it changes the encoding. The struct tags, and with them the field
numbers and wire types, come out identical, which is what the regression test
added later in this series pins down.
go_package options replace the -M mappings protobuild used to pass. The import
paths keep their historical github.com/docker/swarmkit prefix so the generated
descriptors stay compatible with released versions; hack/generate-protos.sh
stages a tree that resolves it.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
go.etcd.io/raft/v3 v3.6.0 and go.etcd.io/etcd/... v3.6.5 generate their protobuf types with protoc-gen-gogo. api/raft.proto embeds raftpb.Message, and protoc-gen-go cannot generate a message whose field type does not implement protoreflect, so swarmkit could not move off gogo while pinned to those versions. raft v3.7.0 and etcd v3.7.1 are generated with protoc-gen-go and no longer depend on gogo at all. Their generated API changes with the move: raftpb is proto2, so its scalar fields are pointers and have to be read through the getters, and the wal and snap entry points take pointers. Both releases require Go 1.26, hence the language version bump here and in the Dockerfile. github.com/gogo/protobuf drops out of the root module entirely, taking 180k lines of vendored code with it. It survives in swarmd as an indirect dependency of certificate-transparency-go and an old prometheus/common, which is outside swarmkit's control. full diff: etcd-io/raft@v3.6.0...v3.7.0 full diff: etcd-io/etcd@v3.6.5...v3.7.1 Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Output of hack/generate-protos.sh. Each definition now produces four files
instead of one:
x.pb.go messages and enums, from protoc-gen-go
x_grpc.pb.go service stubs, from protoc-gen-go-grpc
x_vtproto.pb.go Marshal/Unmarshal/Size/Clone/Equal, from vtprotobuf
x_swarm.pb.go Copy, store objects, raft proxies, authenticated wrappers
vtprotobuf replaces the hand-written marshallers gogo used to generate, so the
hot paths keep their generated codecs rather than falling back to reflection.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Mostly mechanical, but six classes of change are not. Embedded messages that were values are pointers now, so an equality check that used to compare contents compares identity. The store's optimistic concurrency check in tx.update was the one that mattered; it compares the version index explicitly. Every other == and != on a former value message is either gone or a compile error, since protobuf messages are no longer comparable and cannot be map keys: portallocator, remotes, the scheduler and the docker executor all grew a small comparable key struct. Assigning a message that used to be copied by value now shares a pointer. The store copies metadata before handing it back so a later mutation cannot reach into memdb, the template getters copy an expanded spec rather than aliasing the caller's, and expandMounts expands into a copy. gogotypes.DurationFromProto and TimestampFromProto returned an error; AsDuration and AsTime cannot fail and return the zero value for a nil message. Every caller that had a fallback on the error path now tests IsValid instead, so a missing field still takes the fallback rather than reading as zero. Reads of what used to be a non-nullable field go through the generated getters for the whole chain rather than only its first hop, since cluster.Spec.GetCaConfig().ExternalCas dereferences a nil CAConfig just as the original did. A cluster spec that omits ca_config or dispatcher passes validation, so this is reachable from the control API and would take the manager down. Where such a field is written rather than read, it is allocated first. reflect.DeepEqual is not valid on these types and every use is replaced with the generated EqualVT. Two identical messages compare equal until one of them is marshalled, after which protoimpl's sizeCache makes them differ: UpdateVolume and UpdateService would have started rejecting legitimate updates as attempts to change an immutable field, the updater would have cancelled and restarted in-flight rolling updates on every pass, and the agent's status reporter would have re-sent statuses it had already delivered. For the same reason a handful of tests that asserted on String() output now assert on the fields they meant to check, as the official String() renders bytes fields as text where gogo rendered them as a []byte literal. swarm-rafttool built its walpb.Snapshot without an index or term when there was no snapshot on disk. walpb is proto2, so those come out absent rather than zero, and etcd 3.7 rejects such a record; decrypt failed on a state directory with no snapshot yet. Also: deepcopy.Copy dispatches on CopierFrom rather than leaving it as dead API, defaults drops a hand-rolled duration clone for durationpb.New, and CopyFrom gets test coverage, since the generated test that used to exercise it went away with gogo's testgen and Copy and CopyFrom no longer share an implementation. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Encodes what the migration had to preserve: payloads produced by the old gogo marshaller for a Node, a Task and a Service, covering non-nullable embedded messages, repeated non-nullable messages, map ordering, stdduration and the os.FileMode customtype. The test decodes each one, checks the fields came through, and requires that re-encoding produces byte-identical output. Getting any of this wrong would not fail to compile. It would break rolling upgrades and make existing raft logs and snapshots unreadable. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
statusReporter.run removes an entry from the pending map while it is being sent, and puts it back if the send fails. It only put it back when nothing else had arrived in the meantime, on the assumption that whatever arrived is newer. That assumption does not hold. UpdateTaskStatus orders by state, not by arrival: a status that arrives late but is older is dropped there. The retry path had no such rule, so a COMPLETE whose send failed was discarded in favour of, say, a READY that merely landed while it was in flight. The reporter then had nothing left to send above the state the manager already knew about, and the task's final state was never reported at all. Order the retry by state as well. This is a pre-existing bug, but it used to need reflect.DeepEqual to report two identical statuses as different before it could bite. Comparing them properly makes it about half again as likely: TestReporter hangs on roughly a third of runs before this change and on none of 300 after it. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3247 +/- ##
==========================================
- Coverage 14.73% 13.33% -1.41%
==========================================
Files 200 235 +35
Lines 93077 105023 +11946
==========================================
+ Hits 13712 14001 +289
- Misses 78019 89749 +11730
+ Partials 1346 1273 -73 🚀 New features to boost your workflow:
|
The proto2 scalars in etcd's walpb and raftpb need a pointer, and the
migration reached for the protobuf helpers to build one. Go 1.26 allocates
and initialises in one step, so the helper buys nothing:
manager/state/raft/raft.go:629:19: newexpr: call of Uint64(x) can be
simplified to new(x) (modernize)
This drops the last use of the proto package from three of the files.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Member
LOL both versions are still a net "less LOC" 
But ... also much more lines changed now 😂 
|
Deepcopy tests did not detect aliases left by CopyFrom and omitted several message and oneof shapes. The raft decryption test always supplied a snapshot and left its WAL reader open. Cover every generated message shape, mutate copied reference fields, and verify protobuf round trips. Add a no-snapshot WAL case and close WAL readers after reading so temporary-directory cleanup is reliable. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
The initial manager node and the nodes created by certificate issuance
left NodeSpec.Annotations and Node.Status unset. Both fields were
declared with (gogoproto.nullable) = false before the migration to the
standard protobuf runtime, so every node object used to carry them and
API consumers read them without nil checks.
This made dockerd panic while processing the first node update during
swarm initialization:
panic: runtime error: invalid memory address or nil pointer dereference
github.com/moby/moby/v2/daemon.(*Daemon).logNodeEvent
/usr/src/moby/daemon/events.go:152
Initialize both fields to their empty values, which is also exactly
what gogo used to emit on the wire for them, and add regression tests
asserting the invariant on nodes created by both constructors.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Every submessage of ClusterSpec was declared with (gogoproto.nullable) = false before the migration to the standard protobuf runtime: a spec missing one on the wire decoded as an all-zero value, so stored cluster objects always carried all of them, and API consumers (dockerd's swarm inspect conversion among others) dereference them directly. After the migration the default cluster object leaves AcceptancePolicy and TaskDefaults nil, and UpdateCluster stores the client-provided spec wholesale while validateClusterSpec reads the submessages only through nil-safe getters. A client built against the new pointer API can therefore persist a spec whose Raft field is nil, which replicates to every manager and crash-loops them at startup when Manager.Run reads the election tick from it. Backfill absent submessages with their empty values when accepting a spec in UpdateCluster, matching the old decoding semantics, and initialize the two missing ones in defaultClusterObject. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
validateNodeSpec only checks that the spec itself is present, and UpdateNode stores the client-provided spec wholesale. A spec without Annotations therefore reintroduces the nil field that node creation now always initializes, and API consumers dereference it directly. Backfill an empty Annotations before storing, which is what decoding the request produced before the migration to the standard protobuf runtime. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
The migration to the standard protobuf runtime added a nil fallback for the formerly non-nullable ClusterSpec.Raft in (*Node).getCurrentRaftConfig, but missed the same read in Manager.Run, which dereferences the config for the election and heartbeat tick comparison. Guard it the same way. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Task.Annotations and Task.ServiceAnnotations were declared with (gogoproto.nullable) = false before the migration to the standard protobuf runtime, so every stored task carried both and gogo emitted them on the wire even when empty. NewTask left Annotations nil, and the network-attachment task created by AttachNetwork left both nil, exposing valid partial objects to API consumers that read them without nil checks. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
The handler dereferenced request.Config before any validation, so a node sending an AttachNetworkRequest without a config could crash the manager. Return InvalidArgument instead, mirroring DetachNetwork. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
PublishLogs dereferenced LogMessage.Context to verify the sender's node ID. The field was non-nullable before the migration to the standard protobuf runtime, so it can now legally arrive as nil, and a rogue or buggy agent could crash the manager with a single publish message. Read it through the nil-safe getters, so that such messages are rejected with PermissionDenied like any other spoofed node ID. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
processUpdates logged an unpublished volume that is no longer in the store, but then went on to dereference it anyway. Return from the batch callback instead. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Meta.Version became a pointer in the migration to the standard protobuf runtime, so the inequality started comparing object identity instead of the version index: the node read back from the store never aliases the node info's copy, and an up-to-date node could be misclassified as stale. Compare the contents with EqualVT, as the orchestrator already does for spec versions. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
A ControllerServiceCapability whose type oneof is unset yields a nil Rpc message; the generated getter is nil-safe but the Type field access on its result is not, so a CSI plugin returning an empty capability entry crashed the manager. Skip such capabilities, mirroring the agent-side plugin code. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
The response's Version field was non-nullable before the migration to the standard protobuf runtime, and clients dereference it. During manager bootstrap the cluster object does not exist yet; the old code panicked server-side on the nil cluster, while the getter chain introduced by the migration silently returns an empty response with a nil Version instead. Return Unavailable for this window, which getKEKUpdate already retries on, and read the version in getKEKUpdate through nil-safe getters for robustness against unfixed servers. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Several commands dereference spec submessages that were non-nullable before the migration to the standard protobuf runtime: service create builds a ServiceSpec without Annotations and Merge writes the name and labels through it, node update and cluster update do the same with specs received from the server, and service logs reads the log context of received messages. Backfill the submessages before writing through them, and use nil-safe getters for the reads. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
The tool inspects arbitrary state directories, where submessages that were non-nullable before the migration to the standard protobuf runtime can legally be absent: a snapshot without membership or store data and store actions without specs crashed dump, and renewcert indexed the snapshot's cluster list without checking it. Treat an absent store as empty, mirroring the raft restore path, and skip redaction of absent specs. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completely vibecoded slop, just to get an idea of what it would look like