hubble: Update uint size in flow proto #11161
Conversation
Sizes of some fields in flow.proto is unnecessary large (uint64), whereas for such fields (e.g. endpoint ID or identity) a smaller type is more appropriate. This PR is a clean-up to update the scalar value types of these fields. Includes some changes to related Hubble packages (parsers and filters) and minor test adjustments. Fixes: cilium/hubble#158 Signed-off-by: Matej Gera <matejgera@gmail.com>
|
Please set the appropriate release note label. |
|
test-me-please Edit: Hit https://github.com/cilium/cilium/projects/97#card-33577672 on GKE. |
|
Context for this PR for other reviewers: Back when Hubble was running in its own process, we used to obtain metadata via the Swagger-based Cilium API, which uses Changing the integer sizes of the protobuf definitions does not break backwards-compatibility: https://developers.google.com/protocol-buffers/docs/proto3#updating Protobuf only supports 32 and 64 bit integers, therefore 16 bit values are still represented as 32 bit integers. |
| @@ -280,8 +280,8 @@ func decodeEndpoint(endpoint accesslog.EndpointInfo, namespace, podName string) | |||
| labels := endpoint.Labels | |||
| sort.Strings(labels) | |||
| return &pb.Endpoint{ | |||
| ID: endpoint.ID, | |||
| Identity: endpoint.Identity, | |||
| ID: uint32(endpoint.ID), | |||
There was a problem hiding this comment.
Does anyone know why pkg.proxy.accesslog.EndpointInfo.[ID,Identity] are uint64 ? These type casts don't look safe to me.
There was a problem hiding this comment.
See #11161 (comment) for the rationale - the values will never exceed 32 bit (or 16 bit in this case).
There was a problem hiding this comment.
But these two instances are from Cilium's proxy package hence not something that falls into your description, no?
There was a problem hiding this comment.
True, but they suffered a similar fate: EndpointInfo was intended for JSON serialization (see #964), therefore I assume the type definitions were generated from some JSON-to-Go-struct tool, which picked int64 for the type.
I've checked all initialization of EndpointInfo.ID and they all take the value taken from GetID here (also added by the above PR) which casts a 16 bit integer to 64 bit
cilium/pkg/endpoint/endpoint.go
Line 429 in dd67d2d
So I don't see any safety issue here.
rolinh
added
sig/hubble
There was a problem hiding this comment.
Awesome! Thanks a lot for taking care of this.
We should also review the uses of uint64 for the endpoint id and identiy in other parts of the code, i.e. the getters in https://github.com/cilium/cilium/blob/86045f79ad6c3aeb104a0092d48c3691130b1699/pkg/hubble/parser/getters/getters.go also unnecessarily take uint64 parameters. But let's do that in a follow-up PR.
|
test-gke |
Good call about the getters, wasn't sure whether I should include it or not as it also requires change in daemon. I will open an issue for that + PR. |
This reduces the number of dependencies and pulls in the uint64 -> uint32 change in flow protobuf definition (see [0]). [0]: cilium/cilium#11161 Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
This reduces the number of dependencies and pulls in the uint64 -> uint32 change in flow protobuf definition (see [0]). [0]: cilium/cilium#11161 Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Size of some fields in flow.proto is unnecessarily
large (uint64), whereas for such fields
(e.g. endpoint ID or identity) a smaller type
is more appropriate.
This PR is a clean-up to update the scalar
value types of these fields. Includes some
changes to related Hubble packages (parsers
and filters) and minor test adjustments.
Fixes: cilium/hubble#158 (this is an issue originally filed in Hubble repository, before the code was moved over to Cilium)
Signed-off-by: Matej Gera matejgera@gmail.com