Log and count unknown proto fields #6818
Comments
mergeconflict
changed the title
|
See also protocolbuffers/protobuf#5967, where we track the necessary work on protobuf side to support this for JSON/YAML. We can only do this for proto configuration files today, which is only a partial solution. |
|
Would it be useful to add these counts as stats? NBD, but generally it would be difficult search through logs for a fleet of envoys |
|
I think both stats and warning-level logs would be good. I agree that we need stats for monitoring to alert on. |
|
I think I can address this in the context of #6651 |
This is the plumbing to support envoyproxy#6651 and envoyproxy#6818, where we remove the global control over message validation type and put it under the control over a new injectable interface. It's pretty horrible IMHO that there is this much churn; the closest major context object that we could associate with is Api, but it's not part of the filter API as such and we will want to later inject different validators at different points, depending on whether we are reading the static bootstrap or on xDS. No new behavior should be added in this PR. Risk level: Low. Testing: bazel test //test/... Signed-off-by: Harvey Tuch <[email protected]>
This is the plumbing to support #6651 and #6818, where we remove the global control over message validation type and put it under the control of a new injectable interface. It's pretty horrible IMHO that there is this much churn; the closest major context object that we could associate with is Api. The main from the new dependency is that we can: 1. Inject different validators, e.g. one for static config when ingesting bootstrap resources, another for xDS. 2. Validation state, so that we can reach back to the server stats for the purpose of tracking unknown fields. This PR introduces a visitor pattern for the validator and starts to do the threading needed for dependency injection here. It's possible that we could extend this visitor for deprecated fields in the future to provide finer grained control over these alongside unknown, if there is a use case. No new behavior should be added in this PR. Risk level: Low. Testing: bazel test //test/... Signed-off-by: Harvey Tuch <[email protected]>
|
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions. |
stale
bot
added
the
stale
|
This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions. |
stale
bot
removed
the
stale
…/dynamic config. (#7857) As per #6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown field validation. The defaults are the same for static as today (strict) and for dynamic we are by default permissive. This permits easy rollout of new API minor versions, including those related to security fixes. Fixes a regression that occurred in #7200 where strict/permissive checking CLI options were inverted. As per #6818, added stats/warning for any unknown fields encountered. Risk level: Low (strictly more permissive by default) Testing: additional unit and integration tests added, exercising both permissive/strict checking over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc). Fixes #6651 Fixed #6818 Signed-off-by: Harvey Tuch <[email protected]>
Deprecated proto fields are currently logged as a warning or an error in
MessageUtil::checkForDeprecation, and counted inSnapshotImpl::deprecatedFeatureEnabled. We should probably do the same thing for unknown proto fields inMessageUtil::checkUnknownFieldswhen not in strict mode.The text was updated successfully, but these errors were encountered: