Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report use of unknown fields #5967

Closed
htuch opened this issue Mar 29, 2019 · 13 comments
Closed

Report use of unknown fields #5967

htuch opened this issue Mar 29, 2019 · 13 comments
Assignees
Labels
enhancement inactive Denotes the issue/PR has not seen activity in the last 90 days.

Comments

@htuch
Copy link
Contributor

htuch commented Mar 29, 2019

What language does this apply to?

C++ (possibly others)

Describe the problem you are trying to solve.

We would like to allow unknown fields during proto parsing from wire (and formats such as JSON/YAML), but to report the details for the purposes of logging. Our use case is Envoy (https://github.com/envoyproxy/envoy), where configuration with unknown fields can fall into two categories:

  1. Intentional delivery of a newer field from a server with a newer API proto than the client. Client should just ignore.

  2. Accidental misconfiguration of protos (typically the JSON/YAML representation, but also text proto) by users.

We would be confident enabling unknown fields if we could track their use to support (1) and still detect (2).

Describe the solution you'd like

A callback during proto parsing when an unknown field is encountered.

Describe alternatives you've considered

Today, we only allow a binary choice by the user. We encourage them to allow unknown fields if the protos are machine generated and to disallow them if not. This is often too coarse, with a mixture of configuration proto provenance not uncommon.

CC @mattklein123

@BSBandme
Copy link
Contributor

BSBandme commented Apr 2, 2019

What are the inputs and return value for the callback you would like to proposal? I think it's some kind of hard to just providing one callback to fully support all users concerns.

@BSBandme BSBandme assigned ghost Apr 2, 2019
@htuch
Copy link
Contributor Author

htuch commented Apr 2, 2019

It would really depend on what is most idiomatic for protobuf, but the effect that we want is for C++ code calling into functions like JsonStringToMessage() or message.ParseFromString() to be able to learn which unknown fields were encountered during the parsing operations. Any thoughts?

Also CC @lizan

@ghost
Copy link

ghost commented Apr 3, 2019

JsonStringToMessage has an option for ignore_unknown_fields. If you set it to false for (1) and set it to true for (2), would that give you what you need? The parse failure message should inform you approximately where the error occurs in JSON.

@htuch
Copy link
Contributor Author

htuch commented Apr 4, 2019

We need to programatically obtain a list of unknown fields to use for more detailed logging and stats. We want to accept the unknown fields, but provide an indicator in our own access logs of all accepted unknown fields, and scraping error strings for this seems a bit fragile.

@lizan
Copy link
Contributor

lizan commented Apr 4, 2019

The converter::ErrorListener is for this purpose, though it is in internal header so ideally we want to make it as a part of public API.

@ghost
Copy link

ghost commented Apr 9, 2019

What kind of information would you like to log? Do you need to know the JSON key that is unknown + the full hierarchy to get there? Do you need to know the line number + column number of that unknown field?

Random idea: if I give you a traverser that can traverse the entire JSON, and for each JSON field that I visit, I'll give invoke your visitor with the JSON key + JSON value + the corresponding FieldDescriptor for that JSON (or absent of FieldDescriptor for unknown field), would that be sufficient for your logging/stats purpose?

@htuch
Copy link
Contributor Author

htuch commented Apr 9, 2019

I think we just want to know the field names for JSON and the field number for proto, that should be sufficient for logging.

The idea you present would definitely work for JSON, is there a way of also tackling this for proto?

@ghost
Copy link

ghost commented Apr 11, 2019

For regular proto message, you can do access unknown fields using:
const proto2::Reflection* reflection = message.GetReflection();
reflection->GetUnknownFields(message);

@htuch
Copy link
Contributor Author

htuch commented Apr 12, 2019

@haon4 ack; in that case, what you suggest in #5967 (comment) sounds great. Do you think this can be added to the protobuf C++ libs?

@ghost ghost assigned BSBandme and unassigned ghost Apr 26, 2019
@ghost
Copy link

ghost commented Apr 26, 2019

I had a chat with Yilun about this. We have some ideas, but not enough free cycles :( Once Yilun finishes up his current project he might be able to look designing this.

@htuch
Copy link
Contributor Author

htuch commented Apr 29, 2019

Great. Meanwhile, we will add support in Envoy for handling the proto3 ingestion path using reflection and then we can consume the JSON support for that path as it becomes available.

Copy link

github-actions bot commented May 2, 2024

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label May 2, 2024
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement inactive Denotes the issue/PR has not seen activity in the last 90 days.
Projects
None yet
Development

No branches or pull requests

3 participants