Documentation and behavior don't match, for HttpProtocolOptions::explicit_http_config

[ravenblackx]

envoy/api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto

Lines 142 to 143 in d34e163

	// To explicitly configure either HTTP/1 or HTTP/2 (but not both!) use ``explicit_http_config``.
	// If the ``explicit_http_config`` is empty, HTTP/1.1 is used.

Actual behavior if explicit_http_config is empty:
Proto constraint validation failed (HttpProtocolOptionsValidationError.ExplicitHttpConfig: embedded message failed validation | caused by field: "protocol_config", reason: is required): explicit_http_config

[ravenblackx]

@alyssawilk maybe?

[ravenblackx]

Or @yanavlasov and @adisuissa as the configuration people.

[adisuissa]

The validation error is somewhat strange, as it is not the explicit_http_config field that is required, but the upstream_protocol_options oneof that surrounds it.

IIUC #14362 made the oneof required, which implies that the doc-string is no longer valid.

[ravenblackx]

The validation error is somewhat strange, as it is not the explicit_http_config field that is required, but the upstream_protocol_options oneof that surrounds it.

It's definitely not the most readable error, but this interpretation isn't what's being complained about either - it's the protocol_config oneof inside the explicit_http_config that's required if the explicit_http_config field is set.

(i.e. the error message is about this one, vs. you're talking about this one - they were both added in the same PR though.)

[ravenblackx]

The error message would be clearer by rephrasing so that explicit_http_config is at the start of the message I think - usually error messages are ordered "high level problem: cause" but this one is ordered "high level problem: cause: rest of high level problem" which is a big part what makes it unclear.

e.g.
Proto constraint validation failed (HttpProtocolOptionsValidationError.ExplicitHttpConfig: embedded message failed validation | caused by field: "protocol_config", reason: is required): explicit_http_config
would be much clearer as
Proto constraint validation for 'explicit_http_config' failed (HttpProtocolOptionsValidationError.ExplicitHttpConfig: embedded message failed validation | caused by field: "protocol_config", reason: is required)

[adisuissa]

I agree that the error message is misleading.
This probably requires changing PGV to provide the correct error.

[adisuissa]

I think I misunderstood the error. Would it be possible to add the config that causes the issue?

[ravenblackx]

Looks like the reordering I suggested could be just here and here, but it would break a lot of test expectations. Putting the thing right at the start of the message (i.e. 'explicit_http_config' Proto constraint validation failed (...)) would also be clearer without breaking [m]any tests. :)

It's awkward for me to show configs because we do it all through generators so it's not yaml, but essentially the problem here is any HttpProtocolOptions message like

typed_extension_protocol_options:
  "@type":"[...].HttpProtocolOptions"
  http_filters: [...]
  explicit_http_config {}

which should be accepted per the current docs, but now has to be (for equivalent behavior to what one would expect from the above)

typed_extension_protocol_options:
  "@type":"[...].HttpProtocolOptions"
  http_filters: [...]
  explicit_http_config:
    http_protocol_options {}

(Forgive yaml syntax errors, I don't do much yaml, not sure of the empty-message syntax off the top of my head.)

[ravenblackx]

Oh, I see why you thought it was the outer one, because the docstring is not super clear either.

If the explicit_http_config is empty, HTTP/1.1 is used.

I believe this is saying if explicit_http_config is set, but to an empty message, which is the case I had with the unexpected error.

The alternative interpretation, if it's not set, is also an error if the other nearby oneof options are also not set, but I don't think that's the case the docstring was referring to. Either way though this fragment of docstring became outdated with #14362. :)