Add unauthorized boolean the accesslog.

[saumoh]

Title
In the proxy we've added an Unauthorized response flag. This PR adds the same to filter access logs.
Once this PR is merged it will be possible to set the flag in source/common/access_log/grpc_access_log_impl.cc
See also, comment in envoyproxy/envoy#2415

cc: @htuch
Signed-off-by: Saurabh Mohan saurabh+github@tigera.io

[wora]

Is this actually better than an error code or an enum? A request can be denied for many reasons. A boolean field typically leads to a dead ended design, e.g. impossible to extend. We should avoid using boolean unless the concept is extremely stable and unlikely to grow for next 10 years.

[saumoh]

@wora fair point. I mentioned previously I was trying to address the feedback in a Envoy PR.
will close this.

[mattklein123]

@saumoh please reopen this PR. The original feedback still stands and we need to have parity between the textual access log and gRPC access log. The field doesn't necessarily have to be a boolean in the gRPC world. It could be an enum, etc. Thanks!

[saumoh]

@wora It isn't clear to me from your comment how to address the concern that you raised.
ResponseFlag is currently setup as a set of booleans and this pr is trying to add to it.
Are you suggesting that we change to way ResponseFlag is currently setup to be an enum instead? And then we support a bitwise-OR on it so that we can set multiple flags? Or something else.
Any examples that you could share would help. Thanks!
cc @mattklein123

[mattklein123]

@saumoh I think there are 2 options:

1. Just do the PR as-is, and if we want more info on access denied reasons we can add another field later.
2. Make this field an enum for the type of access denied, and for now just populate with a single generic default value. Then just populate that enum in code based on the existing ResponseFlag code. Then we can more easily add a more specific reason later.

I don't really have a very strong opinion either way.

[saumoh]

@mattklein123 Appreciate the guidance and suggestion(s) on how to address this.
I've pushed a change to get some feedback on if the new way will give us the extensible format.
When we get some consensus I'll clean it up for another parse

[mattklein123]

In general this approach looks great to me. I would move forward with a complete PR.

[mattklein123]

This bool seems redundant to me?

[saumoh]

@mattklein123 Thanks for the feedback. I've update the PR. I kept the Unauthorized message as I felt that it will give us flexibility in case we ever want to add more information in the future. But I also don't feel too strongly about keeping it in case we want to boil it down to just the enum.

[mattklein123]

@saumoh in general this LGTM. I will defer to @htuch @wora for further review. Thank you for following up on this!

[htuch]

Should this be under the Unauthorized message below? That way, there is no need to have a permitted default, it can just have for default TYPE_NAME_UNSPECIFIED = 0 as per https://github.com/envoyproxy/data-plane-api/blob/master/STYLE.md?

[htuch]

I would rename this to Reason (it's already qualified as Unauthorized by the outer scope).

[htuch]

I think maybe unauthorized_details might be a clearer name.

[saumoh]

@htuch Thanks for the comments...indeed the naming you suggested is better! :-)

[htuch]

Thanks!