xds: NACK missing route specifier server side #4925
Conversation
| case nil: | ||
| // No-op, as no route specifier is a valid configuration on | ||
| // the server side. | ||
| return nil, fmt.Errorf("no RouteSpecifier: %+v", hcm) |
There was a problem hiding this comment.
Was this specified anywhere? In the gRFC or somewhere else?
I'm wondering why we thought it's OK to be nil.
There was a problem hiding this comment.
And it would be OK to delete this case? The default below would then handle nil.
There was a problem hiding this comment.
Actually, we should probably keep that. The client side returns a special error for nil
grpc-go/xds/internal/xdsclient/xds.go
Lines 153 to 154 in 670c133
Change the comment of these two fields to exactly one of RouteConfigName and InlineRouteConfig is set?
There was a problem hiding this comment.
Since the processing of the Route Configuration was added due to RBAC A36 has language on validations once Route Configuration is processed server side. However, there was no explicit language on a nil RouteSpecifier/a missing Route Configuration (usually when RDS fails, which after talking with Eric we have the right behavior which is to ACK resource, but RPC's fail (no calling goodupdate.Fire()). However, all my RBAC code didn't handle the case of a nil RouteSpecifier, which lead to issue #4924. Luckily, Eric pointed to the fact that the RouteSpecifier is in fact required in the Envoy proto: https://github.com/envoyproxy/envoy/blob/56e8c45b1b340c4a4f8f02ec2488354c31806d59/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L317, so this fix fixes the bug in RBAC implementation as well. Changed comment.
There was a problem hiding this comment.
One thing I mentioned in the issue thread I think is important to bring up, is that I think that NACKing a missing RouteSpecifier is fair for users since the addition of RBAC added the addition of a valid route configuration (matching vh + route) for RPC's to proceed normally, and this will communicate to them the delta they need to make in their control plane resources. Especially if we enable RBAC by default.
| case nil: | ||
| // No-op, as no route specifier is a valid configuration on | ||
| // the server side. | ||
| return nil, fmt.Errorf("no RouteSpecifier: %+v", hcm) |
There was a problem hiding this comment.
Since the processing of the Route Configuration was added due to RBAC A36 has language on validations once Route Configuration is processed server side. However, there was no explicit language on a nil RouteSpecifier/a missing Route Configuration (usually when RDS fails, which after talking with Eric we have the right behavior which is to ACK resource, but RPC's fail (no calling goodupdate.Fire()). However, all my RBAC code didn't handle the case of a nil RouteSpecifier, which lead to issue #4924. Luckily, Eric pointed to the fact that the RouteSpecifier is in fact required in the Envoy proto: https://github.com/envoyproxy/envoy/blob/56e8c45b1b340c4a4f8f02ec2488354c31806d59/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L317, so this fix fixes the bug in RBAC implementation as well. Changed comment.
2 participants
This PR NACKs a missing route specifier server side, as it is required in Envoy. https://github.com/envoyproxy/envoy/blob/56e8c45b1b340c4a4f8f02ec2488354c31806d59/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto#L317. A nil route specifier actually caused a bug, as my RBAC HTTP Filter addition didn't have the correct logic when Route Specifier was nil (see #4924). Luckily, this situation is gatekeeped by the requirement that RouteSpecifier be present.
cc: @ejona86, @lidizheng
RELEASE NOTES: None