Envoy: Proxy protocol listener uses exceptions on the data plane

[htuch]

As part of our effort to remove data plane exceptions (@asraa @yanavlasov), we probably should fix the explicit usage of exceptions in this extension.

[htuch]

See https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc for source

[arctdav]

I'm interested

[asraa]

Thanks @arctdav! Please tag me on a PR, would love to see.

We are using absl::Status, but other filters use their own state class like https://github.com/envoyproxy/envoy/blob/78eaac391b519a69900ebcdccea668f690593e9a/source/extensions/filters/listener/tls_inspector/tls_inspector.h#L39

[arctdav]

Thanks, I will. I am a true beginner and need a bit of time to refresh my memory of C++ again. I will let you know when I'm certain this issue is out of my scope to handle

[arctdav]

@htuch, by fix the explicit usage of exceptions in this extension, do I change throw EnvoyException("failed to read proxy protocol"); into something? Like "try catch"? Sorry C++ isn't my strong suit.

[mrSeoD]

Hi, I would love to help fixing this problem as well. @htuch I would appreciate if you could be more specific on what you mean by fixing "the explicit usage of exceptions." Do you want us to use another way of catching on error other than exceptions? Thank you!

Hi @arctdav I believe you use try-catch when you are testing the exceptions on the driver file.

[asraa]

Hey @mrSeoDavid @arctdav I might be able to offer a suggestion on where to start!

The exceptions are caught in a try/catch block in Filter::onRead() here https://github.com/envoyproxy/envoy/blob/f0ebefc2a36746dbf12301d3c258e2f1b66cfe82/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc#L44

You could instead have onReadWorker() return a status (which you could define yourself, or you can use absl::Status), and handle an error status like exceptions are handled in the catch block. This would mean replacing throw EnvoyException(...) to return Status::Error. In pseudo-code, something like:

void Filter::onRead() {
  Filter::Status status = onReadWorker();
  if (status == Filter::Status::Error) {
    config_->stats_.downstream_cx_proxy_proto_error_.inc();
    cb_->continueFilterChain(false);
  }
}

and void Filter::onReadWorker() would change to Filter::Status Filter::onReadWorker(). This status class you define/extend may or may not need to hold more information, I'm not sure how many distinct exceptions are in the filter code or how many distinct error messages there are.

You can see this L4 filter for an example state. It's methods return ParseState, and one of the parse states is an Error, which is handled https://github.com/envoyproxy/envoy/blob/f0ebefc2a36746dbf12301d3c258e2f1b66cfe82/source/extensions/filters/listener/tls_inspector/tls_inspector.h#L39

[arctdav]

@mrSeoDavid please go ahead to handle this issue.

[kapilsharma432001]

@htuch I wanna work on this issue. Please assign this to me and since i am new contributor so please guide me through this.

[mrSeoD]

@asraa Thank you so much for your help! I understood everything, I'll get on the work.

[mrSeoD]

@asraa Quick question, for the onReadWorker() method, I was able to make it return a state instead of the throwing exceptions.

However, for all the other methods who are throwing exceptions as well, do I need just need to change these throwing exceptions as just returning the state? I don't think this will work for the methods that are already assigned to return other types like "lenV2Address" method as it is assigned to return size_t values in the first place.

Then, am I supposed to make new methods that will tests individual methods just like how the "onRead()" method is testing "onReadWorker()" method? Thanks a lot!

[asraa]

I don't think this will work for the methods that are already assigned to return other types like "lenV2Address" method as it is assigned to return size_t values in the first place.

Right, I see. We encountered this as well from exception removal proof of concepts, and ended up using absl::StatusOr<size_t> that either returns an error status or a size_t if there is no error. I don't know if this is the best case for this scenario. Here are some general options: maybe you return absl::optional<int> with an int if success, nullopt if failed; or return -1 for an error; or decide to use statusor (but this is would mean mapping error states to statusor errors); or as maybe just another option, you can use a union type?

I suggest tackling this piece-meal. Looking at bool ReadProxyHeader, I think exception messages could be changes to trace log statements, and false could be returned. Looking at size_t lenV2Address, maybe an absl::optional<int>?

I would caution -- remember that throw will immediately stop any following execution, so when you replace something with throw, remember things need to be returned up to the try/catch.

[mrSeoD]

@asraa thank you so much for your detailed suggestions and explanation. This is what I have done so far: for all of the methods that had void return type in the first place, I've created an enum class in the .h file and made these methods return enum values that represent a failure.

For the methods that were originally designated to return boolean, I've followed your suggestion and changed the exception messages to trace log statements and returned false. However, I did not know how to write the trace log statement, so after googling, I wrote this way: "TRACE(_T(an exception message))" Is this the right way to do it? If so, do I have to #include anything in this file?

For the lenV2Address, don't we need the method's header to be "absl::optional Filter::lenV2Address(char* but)"? not absl::optional as we are returning a size_t value or a std::nullopt?

Lastly, if I made methods like parseV2Header and parseV1Header return values of enum class, don't I need to change the code for other methods or class that use this method in detecting an error? Just like how the onRead() method's try-catch code changed to an if-statement code like you've suggested in the https://github.com/envoyproxy/envoy/issues/11044#issuecomment-624623421.

Once again, thank you so much for your help.

[asraa]

However, I did not know how to write the trace log statement, so after googling, I wrote this way: "TRACE(_T(an exception message))"

Envoy has log message macros. You can use format strings in them too. https://github.com/envoyproxy/envoy/blob/10c755e9d9b8acd7cf1702a4f49dbcbdf0696198/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc#L30

For the lenV2Address, don't we need the method's header to be "absl::optional Filter::lenV2Address(char* but)"?

Yes

don't I need to change the code for other methods or class that use this method in detecting an error?

Yes you will need to propagate the error back through the methods that call them. I think readProxyHeader calls them. So you'll need to return early if readProxyHeader fails with the previous exceptions, and it looks like it already returns early if the value is false, so replacing a parse exception with false and propagating false back will be ok.
https://github.com/envoyproxy/envoy/blob/10c755e9d9b8acd7cf1702a4f49dbcbdf0696198/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc#L56

Feel free to ping me, it might just be easier to make a draft PR I can comment on the diffs

[asraa]

/cc @arthuryan-k