6c2dcb6f
test(wire): a proto field number cannot change unnoticed
a73x 2026-08-12 18:35
Commit message
internal/transport/fieldnumbers_test.go
| Old | New | ||
|---|---|---|---|
| @@ -0,0 +1,348 @@ | |||
| 1 | package transport | ||
| 2 | |||
| 3 | import ( | ||
| 4 | "fmt" | ||
| 5 | "testing" | ||
| 6 | |||
| 7 | "github.com/a73x/eitri/internal/pb" | ||
| 8 | "github.com/stretchr/testify/assert" | ||
| 9 | "github.com/stretchr/testify/require" | ||
| 10 | "google.golang.org/protobuf/proto" | ||
| 11 | "google.golang.org/protobuf/reflect/protoreflect" | ||
| 12 | "google.golang.org/protobuf/reflect/protoregistry" | ||
| 13 | ) | ||
| 14 | |||
| 15 | // A proto field's NUMBER is its wire identity: it is what a peer that predates a | ||
| 16 | // field reads past, and what an agent one release behind matches its known | ||
| 17 | // fields against. Renaming a field is a source change; RENUMBERING one is a wire | ||
| 18 | // change that no compiler and no `make proto-check` can see — the code builds, | ||
| 19 | // the generated Go regenerates cleanly, and every fielded agent silently reads | ||
| 20 | // the moved field as absent (see contract_test.go for what "absent" then means: | ||
| 21 | // a dropped exposure set, an uncertified host key, a guest stranded ephemeral). | ||
| 22 | // | ||
| 23 | // These two tests make a renumber loud. The first LOCKS every field number in | ||
| 24 | // the wire schema against a written-down table, so no number can move — or field | ||
| 25 | // appear or vanish — without this table and a human moving with it. The second | ||
| 26 | // proves the table is not lying by exercising every field through a real | ||
| 27 | // round-trip, so a field the wire never actually carries cannot hide behind a | ||
| 28 | // table entry. | ||
| 29 | |||
| 30 | // wireSchema is the committed truth: every message in proto/eitri/v1/sync.proto, | ||
| 31 | // every field, and the number it is pinned to. Reserved numbers are deliberately | ||
| 32 | // absent — a reserved slot carries nothing, and re-using one is exactly the | ||
| 33 | // mistake this table exists to catch (a new field on an old number reads, on a | ||
| 34 | // fielded agent, as whatever used to live there). Update this table ONLY in the | ||
| 35 | // same change that edits the .proto, and only ever by adding — never by moving. | ||
| 36 | var wireSchema = map[string]map[string]protoreflect.FieldNumber{ | ||
| 37 | "AgentMessage": { | ||
| 38 | "hello": 1, | ||
| 39 | "report": 2, | ||
| 40 | "console_opened": 3, | ||
| 41 | "tcp_opened": 4, | ||
| 42 | }, | ||
| 43 | "ServerMessage": { | ||
| 44 | "snapshot": 1, | ||
| 45 | "console_open": 2, | ||
| 46 | "tcp_open": 3, | ||
| 47 | }, | ||
| 48 | "Hello": { | ||
| 49 | "host_id": 1, | ||
| 50 | "hostname": 2, | ||
| 51 | "os": 3, | ||
| 52 | "arch": 4, | ||
| 53 | "provisioner": 5, | ||
| 54 | "last_seen_epoch": 7, | ||
| 55 | "capacity": 8, | ||
| 56 | "credential": 9, | ||
| 57 | "facts": 10, | ||
| 58 | }, | ||
| 59 | "Capacity": { | ||
| 60 | "vcpus": 1, | ||
| 61 | "mem_mb": 2, | ||
| 62 | "disk_gb": 3, | ||
| 63 | }, | ||
| 64 | "HostFacts": { | ||
| 65 | "os_id": 1, | ||
| 66 | "os_pretty": 2, | ||
| 67 | "os_version": 3, | ||
| 68 | "kernel": 4, | ||
| 69 | "cpu_model": 5, | ||
| 70 | "virt": 6, | ||
| 71 | "agent_version": 7, | ||
| 72 | }, | ||
| 73 | "HostMetrics": { | ||
| 74 | "uptime_s": 1, | ||
| 75 | "mem_used_mb": 2, | ||
| 76 | "mem_available_mb": 3, | ||
| 77 | "load1": 4, | ||
| 78 | "load5": 5, | ||
| 79 | "load15": 6, | ||
| 80 | "disk_used_gb": 7, | ||
| 81 | "disk_free_gb": 8, | ||
| 82 | }, | ||
| 83 | "ActualVM": { | ||
| 84 | "vm_id": 1, | ||
| 85 | "power": 2, | ||
| 86 | "phase": 3, | ||
| 87 | "ip": 4, | ||
| 88 | "last_error": 5, | ||
| 89 | "ssh_host_pubkey": 6, | ||
| 90 | "status_detail": 7, | ||
| 91 | }, | ||
| 92 | "QuarantinedVM": { | ||
| 93 | "vm_id": 1, | ||
| 94 | "name": 2, | ||
| 95 | "vmspec_json": 3, | ||
| 96 | "destroy_at_unix": 4, | ||
| 97 | }, | ||
| 98 | "ActualStateReport": { | ||
| 99 | "vms": 1, | ||
| 100 | "destroyed": 2, | ||
| 101 | "quarantined": 3, | ||
| 102 | "capacity": 4, | ||
| 103 | "fence_violation": 5, | ||
| 104 | "last_seen_epoch": 6, | ||
| 105 | "metrics": 7, | ||
| 106 | "guest_cidr": 8, | ||
| 107 | "exposures": 9, | ||
| 108 | "host_uplink_addr": 10, | ||
| 109 | }, | ||
| 110 | "VMDesired": { | ||
| 111 | "vm_id": 1, | ||
| 112 | "name": 2, | ||
| 113 | "image_url": 3, | ||
| 114 | "image_sha256": 4, | ||
| 115 | "cloud_init": 5, | ||
| 116 | "vcpus": 6, | ||
| 117 | "mem_mb": 7, | ||
| 118 | "disk_gb": 8, | ||
| 119 | "persistent": 9, | ||
| 120 | "power_state": 10, | ||
| 121 | "tombstoned": 11, | ||
| 122 | "ssh_authorized_key": 12, | ||
| 123 | "ssh_host_cert": 17, | ||
| 124 | "ssh_user_ca_authorized_keys": 18, | ||
| 125 | "host_cert_required": 19, | ||
| 126 | }, | ||
| 127 | "DesiredStateSnapshot": { | ||
| 128 | "epoch": 1, | ||
| 129 | "vms": 2, | ||
| 130 | "agent_upgrade": 3, | ||
| 131 | "exposures": 4, | ||
| 132 | }, | ||
| 133 | "AgentUpgrade": { | ||
| 134 | "version": 1, | ||
| 135 | "url": 2, | ||
| 136 | "sha256": 3, | ||
| 137 | }, | ||
| 138 | "ConsoleOpen": { | ||
| 139 | "vm_id": 1, | ||
| 140 | }, | ||
| 141 | "ConsoleOpened": { | ||
| 142 | "ok": 1, | ||
| 143 | "error": 2, | ||
| 144 | }, | ||
| 145 | "TCPOpen": { | ||
| 146 | "vm_id": 1, | ||
| 147 | "port": 2, | ||
| 148 | }, | ||
| 149 | "TCPOpened": { | ||
| 150 | "ok": 1, | ||
| 151 | "error": 2, | ||
| 152 | }, | ||
| 153 | "ExposureDesired": { | ||
| 154 | "id": 1, | ||
| 155 | "vm_id": 2, | ||
| 156 | "guest_port": 3, | ||
| 157 | "host_port": 4, | ||
| 158 | "protocol": 5, | ||
| 159 | }, | ||
| 160 | "ExposureActual": { | ||
| 161 | "id": 1, | ||
| 162 | "state": 2, | ||
| 163 | "reason": 3, | ||
| 164 | "sessions": 4, | ||
| 165 | }, | ||
| 166 | "ExposureSessions": { | ||
| 167 | "active": 1, | ||
| 168 | "refused": 2, | ||
| 169 | "dropped": 3, | ||
| 170 | }, | ||
| 171 | } | ||
| 172 | |||
| 173 | // TestWireFieldNumbersAreLocked walks every message in the compiled wire schema | ||
| 174 | // and asserts its field numbers, exactly, against wireSchema. It fails three | ||
| 175 | // ways, each the tripwire it is meant to be: a field whose number moved, a field | ||
| 176 | // added to the .proto without a table entry, and a table entry for a field the | ||
| 177 | // .proto no longer has. Because it enumerates the descriptors rather than the | ||
| 178 | // table, no message and no field can slip the check by being left out. | ||
| 179 | func TestWireFieldNumbersAreLocked(t *testing.T) { | ||
| 180 | fd := pb.File_proto_eitri_v1_sync_proto | ||
| 181 | require.NotNil(t, fd, "wire file descriptor is not registered") | ||
| 182 | |||
| 183 | msgs := fd.Messages() | ||
| 184 | seen := make(map[string]bool, msgs.Len()) | ||
| 185 | |||
| 186 | for i := 0; i < msgs.Len(); i++ { | ||
| 187 | md := msgs.Get(i) | ||
| 188 | name := string(md.Name()) | ||
| 189 | seen[name] = true | ||
| 190 | |||
| 191 | want, ok := wireSchema[name] | ||
| 192 | if !assert.Truef(t, ok, "message %s is in the wire schema but not in wireSchema — add its field numbers to the table", name) { | ||
| 193 | continue | ||
| 194 | } | ||
| 195 | |||
| 196 | fields := md.Fields() | ||
| 197 | got := make(map[string]protoreflect.FieldNumber, fields.Len()) | ||
| 198 | for j := 0; j < fields.Len(); j++ { | ||
| 199 | f := fields.Get(j) | ||
| 200 | got[string(f.Name())] = f.Number() | ||
| 201 | } | ||
| 202 | |||
| 203 | for fname, wantNum := range want { | ||
| 204 | gotNum, present := got[fname] | ||
| 205 | if !assert.Truef(t, present, "%s.%s is in wireSchema but not in the .proto", name, fname) { | ||
| 206 | continue | ||
| 207 | } | ||
| 208 | assert.Equalf(t, wantNum, gotNum, "%s.%s field number moved: schema says %d, wire says %d", name, fname, wantNum, gotNum) | ||
| 209 | } | ||
| 210 | for fname := range got { | ||
| 211 | assert.Containsf(t, want, fname, "%s.%s is a new field with no wireSchema entry — pin its number in the table", name, fname) | ||
| 212 | } | ||
| 213 | } | ||
| 214 | |||
| 215 | for name := range wireSchema { | ||
| 216 | assert.Truef(t, seen[name], "wireSchema has message %s that the .proto no longer defines", name) | ||
| 217 | } | ||
| 218 | } | ||
| 219 | |||
| 220 | // TestEveryFieldSurvivesARoundTrip is the completeness half: it proves wireSchema | ||
| 221 | // is not a comfortable fiction. contract_test.go round-trips hand-written | ||
| 222 | // literals, so a field simply left out of a literal round-trips trivially and is | ||
| 223 | // never tested — the very gap that let a renumber pass unseen. Here, for every | ||
| 224 | // message and every field, we set that ONE field to a distinct non-zero sentinel | ||
| 225 | // by reflection, marshal it through the real wire codec, read it back, and assert | ||
| 226 | // the field came back set and equal. A field wired to a number no peer expects | ||
| 227 | // would decode as absent and fail proto.Equal; a field the schema forgot cannot | ||
| 228 | // be forgotten here because we iterate the descriptors, not a list. | ||
| 229 | // | ||
| 230 | // One field at a time (rather than every field at once) is deliberate: it sets | ||
| 231 | // oneof members without them evicting each other, it needs no per-message | ||
| 232 | // knowledge of which fields conflict, and it pins each number independently, so | ||
| 233 | // two same-typed fields swapping numbers is caught by distinct sentinels rather | ||
| 234 | // than masked by a shared value. | ||
| 235 | func TestEveryFieldSurvivesARoundTrip(t *testing.T) { | ||
| 236 | fd := pb.File_proto_eitri_v1_sync_proto | ||
| 237 | msgs := fd.Messages() | ||
| 238 | |||
| 239 | for i := 0; i < msgs.Len(); i++ { | ||
| 240 | md := msgs.Get(i) | ||
| 241 | fields := md.Fields() | ||
| 242 | for j := 0; j < fields.Len(); j++ { | ||
| 243 | f := fields.Get(j) | ||
| 244 | t.Run(fmt.Sprintf("%s/%s", md.Name(), f.Name()), func(t *testing.T) { | ||
| 245 | in := newMessage(t, md) | ||
| 246 | setSentinel(t, in, f) | ||
| 247 | |||
| 248 | out := newMessage(t, md) | ||
| 249 | roundTrip(t, in.Interface(), out.Interface()) | ||
| 250 | |||
| 251 | assert.Truef(t, out.Has(f), | ||
| 252 | "%s.%s did not survive the round-trip — its wire number is not the one the codec reads", | ||
| 253 | md.Name(), f.Name()) | ||
| 254 | assert.Truef(t, proto.Equal(in.Interface(), out.Interface()), | ||
| 255 | "%s.%s round-tripped to a different value:\n in=%v\nout=%v", | ||
| 256 | md.Name(), f.Name(), in.Interface(), out.Interface()) | ||
| 257 | }) | ||
| 258 | } | ||
| 259 | } | ||
| 260 | } | ||
| 261 | |||
| 262 | // newMessage makes a fresh, empty message for a descriptor, using its registered | ||
| 263 | // Go type so proto.Marshal/Unmarshal and proto.Equal all operate on the real | ||
| 264 | // generated types rather than a dynamic stand-in. | ||
| 265 | // | ||
| 266 | //nolint:ireturn // protoreflect.Message IS the interface the reflection API returns; there is no concrete type here. | ||
| 267 | func newMessage(t *testing.T, md protoreflect.MessageDescriptor) protoreflect.Message { | ||
| 268 | t.Helper() | ||
| 269 | mt, err := protoregistry.GlobalTypes.FindMessageByName(md.FullName()) | ||
| 270 | require.NoErrorf(t, err, "no registered Go type for %s", md.FullName()) | ||
| 271 | return mt.New() | ||
| 272 | } | ||
| 273 | |||
| 274 | // setSentinel sets exactly field f of m to a distinct, non-zero value. For a | ||
| 275 | // message or repeated field it builds a minimally-populated value so the field | ||
| 276 | // is present on the wire; the point is that the NUMBER carries, not that nested | ||
| 277 | // content is exhaustive (each nested message is exercised in full as its own | ||
| 278 | // top-level case). | ||
| 279 | func setSentinel(t *testing.T, m protoreflect.Message, f protoreflect.FieldDescriptor) { | ||
| 280 | t.Helper() | ||
| 281 | switch { | ||
| 282 | case f.IsList(): | ||
| 283 | list := m.NewField(f).List() | ||
| 284 | list.Append(scalarSentinel(t, f)) | ||
| 285 | m.Set(f, protoreflect.ValueOfList(list)) | ||
| 286 | case f.IsMap(): | ||
| 287 | // No map fields exist in this schema; fail loudly if one is added so | ||
| 288 | // this guard is extended rather than silently skipping the field. | ||
| 289 | t.Fatalf("%s is a map field — extend setSentinel to cover maps", f.FullName()) | ||
| 290 | default: | ||
| 291 | m.Set(f, scalarSentinel(t, f)) | ||
| 292 | } | ||
| 293 | } | ||
| 294 | |||
| 295 | // scalarSentinel returns a distinct non-zero value for one (non-list) element of | ||
| 296 | // f's element type. | ||
| 297 | func scalarSentinel(t *testing.T, f protoreflect.FieldDescriptor) protoreflect.Value { | ||
| 298 | t.Helper() | ||
| 299 | switch f.Kind() { | ||
| 300 | case protoreflect.BoolKind: | ||
| 301 | return protoreflect.ValueOfBool(true) | ||
| 302 | case protoreflect.Int32Kind, protoreflect.Sint32Kind, protoreflect.Sfixed32Kind: | ||
| 303 | return protoreflect.ValueOfInt32(int32(f.Number()) + 1) | ||
| 304 | case protoreflect.Int64Kind, protoreflect.Sint64Kind, protoreflect.Sfixed64Kind: | ||
| 305 | return protoreflect.ValueOfInt64(int64(f.Number()) + 1) | ||
| 306 | case protoreflect.Uint32Kind, protoreflect.Fixed32Kind: | ||
| 307 | return protoreflect.ValueOfUint32(uint32(f.Number()) + 1) | ||
| 308 | case protoreflect.Uint64Kind, protoreflect.Fixed64Kind: | ||
| 309 | return protoreflect.ValueOfUint64(uint64(f.Number()) + 1) | ||
| 310 | case protoreflect.FloatKind: | ||
| 311 | return protoreflect.ValueOfFloat32(float32(f.Number()) + 1.5) | ||
| 312 | case protoreflect.DoubleKind: | ||
| 313 | return protoreflect.ValueOfFloat64(float64(f.Number()) + 1.5) | ||
| 314 | case protoreflect.StringKind: | ||
| 315 | return protoreflect.ValueOfString(fmt.Sprintf("sentinel-%s-%d", f.Name(), f.Number())) | ||
| 316 | case protoreflect.BytesKind: | ||
| 317 | return protoreflect.ValueOfBytes([]byte(fmt.Sprintf("sentinel-%s-%d", f.Name(), f.Number()))) | ||
| 318 | case protoreflect.EnumKind: | ||
| 319 | // No enums in this schema; pick the first non-zero value if one is ever | ||
| 320 | // added, else fail so the guard is extended deliberately. | ||
| 321 | vals := f.Enum().Values() | ||
| 322 | if vals.Len() < 2 { | ||
| 323 | t.Fatalf("%s is an enum with no non-zero value — extend scalarSentinel", f.FullName()) | ||
| 324 | } | ||
| 325 | return protoreflect.ValueOfEnum(vals.Get(1).Number()) | ||
| 326 | case protoreflect.MessageKind, protoreflect.GroupKind: | ||
| 327 | nested := newMessage(t, f.Message()) | ||
| 328 | populateOne(t, nested) | ||
| 329 | return protoreflect.ValueOfMessage(nested) | ||
| 330 | default: | ||
| 331 | t.Fatalf("%s has unhandled kind %v — extend scalarSentinel", f.FullName(), f.Kind()) | ||
| 332 | return protoreflect.Value{} | ||
| 333 | } | ||
| 334 | } | ||
| 335 | |||
| 336 | // populateOne sets a single non-zero field on a nested message so that the | ||
| 337 | // message is non-empty on the wire (an all-zero nested message would still marshal | ||
| 338 | // to a present-but-empty field, but a populated one is a stronger witness). It is | ||
| 339 | // intentionally shallow: full field coverage of every message comes from that | ||
| 340 | // message's own top-level cases. | ||
| 341 | func populateOne(t *testing.T, m protoreflect.Message) { | ||
| 342 | t.Helper() | ||
| 343 | fields := m.Descriptor().Fields() | ||
| 344 | if fields.Len() == 0 { | ||
| 345 | return | ||
| 346 | } | ||
| 347 | setSentinel(t, m, fields.Get(0)) | ||
| 348 | } | ||