fix(proto): nest TypeMeta in common Unknown per upstream wire format#59
Open
indyjonesnl wants to merge 2 commits into
Open
fix(proto): nest TypeMeta in common Unknown per upstream wire format#59indyjonesnl wants to merge 2 commits into
indyjonesnl wants to merge 2 commits into
Conversation
Port a small-scale version of upstream apimachinery's
`pkg/api/apitesting/roundtrip/roundtrip.go` (the randfill-driven
codec roundtrip harness) to rusternetes. For each of six
representative resources — Pod, Deployment, Service, ConfigMap,
Secret, Event — generate 50 random instances per case and verify:
1. `decode(encode(decode(x)))` is stable as `serde_json::Value`.
2. The `k8s\0`-prefixed protobuf `Unknown` envelope round-trips
and recovers the original `apiVersion` / `kind` plus the
wrapped object.
Strategies cap collection sizes at 2 and use a deterministic
`make_object_meta` helper so values stay reproducible across runs.
The Event strategy uses whole-second `metav1.Time` and exact-
microsecond `metav1.MicroTime` values to dodge sub-microsecond
truncation; that limitation is documented in the file header.
Adds `proptest = "1"` to `crates/common` dev-dependencies. All 14
tests run in <1s on a developer laptop, well under the 30s budget.
(cherry picked from commit 0d8f773)
`runtime.Unknown` in k8s.io/apimachinery is:
message Unknown {
optional TypeMeta typeMeta = 1; // nested message
optional bytes raw = 2;
optional string contentEncoding = 3;
optional string contentType = 4;
}
but the prost `Unknown` in `common::protobuf` flattens TypeMeta in at
tags 1+2 (apiVersion, kind) and shifts raw/contentEncoding/contentType
to 3-5. Its own encoder and decoder agree, so the round-trip tests pass
— but any upstream client (kubectl, client-go) decoding the envelope
parses the apiVersion string bytes as a nested TypeMeta message and hits
`proto: illegal wireType 6` on byte 'v'=0x76.
The api-server serving path dodges this today by hand-rolling the
correct nested layout in `middleware.rs::wrap_json_in_protobuf_with_type_meta`
instead of using this struct — its own comment notes the prost struct is
wrong. That leaves two parallel encoders and a latent landmine: anything
that routes serialization through `common::encode_protobuf` emits broken
bytes.
Align the prost struct with the real wire format (nested `UnknownTypeMeta`
at field 1, raw at 2, contentEncoding 3, contentType 4), update
encode/decode/extract_type_meta accordingly, and add a wire-shape test
that fails fast if field 1 ever stops being an embedded message.
Co-Authored-By: Claude Code <noreply@anthropic.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
1 participant
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.
What
Align the prost
runtime.Unknowndefinition incommon::protobufwith thereal Kubernetes wire format, and add a proptest fuzz harness covering the
JSON↔protobuf round-trip.
k8s.io/apimachinery/pkg/runtime.Unknownis:Today the prost struct flattens
TypeMetain at tags 1+2 (apiVersion,kind) and shiftsraw/contentEncoding/contentTypeto 3–5. Its ownencoder and decoder agree, so the round-trip tests pass — but any upstream
client (kubectl, client-go) that decodes the envelope parses the
apiVersionstring bytes as a nested
TypeMetamessage and hitsproto: illegal wireType 6on byte'v'=0x76.Why this isn't already breaking serving (and why it still matters)
The api-server's HTTP serving path does not use this struct — it
hand-rolls the correct nested layout in
middleware.rs::wrap_json_in_protobuf_with_type_meta, whose own commentnotes the prost struct is wrong and is deliberately bypassed. So clients are
fine against the live server today.
But that leaves two parallel encoders and a latent landmine: anything that
serializes through
common::encode_protobufemits broken bytes. The struct iscurrently test-only in this repo, which is exactly why the bug has stayed
invisible. This PR makes the prost struct the single correct source of truth.
Follow-up (proposed)
With the struct fixed,
middleware.rscould route through the now-correctcommon::Unknownand delete the duplicate hand-rolled varint encoder, leavingone wire-format definition instead of two. Happy to do that in a follow-up if
you'd take it.
Changes
common/src/protobuf.rs: nestedUnknownTypeMetaat field 1,rawat 2,contentEncoding3,contentType4; updateencode_protobuf/decode_protobuf/extract_type_meta; add a wire-shape test that failsfast if field 1 ever stops being an embedded message.
common/tests/fuzz_roundtrip_jsonproto_test.rs: proptest harness (portedsmall-scale from apimachinery's
roundtrip.go) — 6 resources × 50 randominstances, asserting JSON stability and
k8s\0Unknown-envelope recovery.Tests
cargo test -p rusternetes-common— fuzz 14/14, protobuf lib 7/7 (incl newwire-shape test).
cargo test -p rusternetes-api-server --test protobuf_test2/2.
cargo fmt --all -- --checkclean.🤖 Generated with Claude Code