[API] Breaking change with RFC header constraints

[asraa]

This PR #10093 introduces potentially breaking RFC header validations on header-related fields.

Previously, Envoy code uses debug ASSERT(valid()) on these fields to guard only against \r\n\0 characters in headers. However, previously API valid configs may have header related fields that didn't have these characters but were not RFC compliant, These would now be broken because of this.

This is a tracking issue for the work:

• add an Envoy specific header validation to PGV mimicing valid()
• replace the validations in the PR to that validation
• mark fields as deprecated and add a replacement field that is validated.

[alyssawilk]

@asraa thoughts on assigning this your way vs help wanted vs leaving as is?

[asraa]

Assigned to myself, thanks! I'm on my way working on it :)

[stale]

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.

[asraa]

@antoniovicente @htuch The last point on moving forward is still outstanding -- what do you think? Keep header validations as-is? Do a giant deprecation/replacement to RFC compliant HeaderStrings?

[htuch]

My $0.02 is that we do a giant deprecation. The way I think this could work is that we introduce a runtime option (default existing behavior) and we add a recursive traversal via reflection (similar to checkForUnknownFields) at config ingest. This would enforce the stricter requirement beyond what PGV does when it sees a weaker PGV header annotation. Then we can replace this with pure PGV after the deprecation period.

These reflection based traversals can be expensive in terms of CPU cost, but since this is only for a bounded period of time and we have so much other protobuf overhead going on, it might not be too bad from a marginal overhead perspective.

[antoniovicente]

Can we figure out a way to runtime guard operations done by PGV? Or have a way to instantiate the validator in 2 different ways, so we can select weak or strict based on a runtime guard?

Possibly related: #11058

[htuch]

How general do we need to make this? If it's just for the headers, I think we can have Envoy do some reflection based weakening/strengthening of the constraints on config ingest, and use the internal runtime guards for this. If we foresee a general need for this, then, sure, let's design this into PGV, but that's a reasonable project and we should file something at https://github.com/envoyproxy/protoc-gen-validate/issues.

[asraa]

If i understand the intent of #11058 correctly, the annotations appear only as documentation, and not as actual validation, so I don't think that we have any need beyond headers. If so, field traversal with reflection is probably the way to go.

I can tackle after exception removal/when there's some spare cycles

[antoniovicente]

We may want #11058 to extend to validation eventually(requiring some fields be set, or using different defaults), that's why I mention it.

[stale]

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]

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.