Kuadrant / limitador

Rate limiter
Apache License 2.0
59 stars 21 forks source link

Request with `|` (pipe) character in authority header is auto-rejected by Limitador #53

Closed rahulanand16nov closed 9 months ago

rahulanand16nov commented 2 years ago

This issue describes the finding of auto-rejection of requests that contains the pipe character in their :authority header field. I tracked down this problem to a dependency named h2.

Following was from the trace logs:

malformed headers: malformed authority (b"outbound|8081||limitador.rahul-test.svc.cluster.local"): invalid uri character

Why there is a pipe character in the authority header? That format should be familiar if you have seen Istio's cluster config. Still, if not, that format is auto-generated by Istio's control plane by looking at its service registry. So, when you add a Limitador service in the cluster where Istio is also present, the service registry picks up the service and sends that to all the Envoys present in the cluster (gateway or sidecar) as a cluster. And the name of that cluster is of that format as seen above in the log.

As Limitador is used right now, it receives a callout request from an Envoy's RateLimit filter and one particular behavior of envoy is to use the cluster name as :authority header of the callout. Hence why you get the pipe character in the header.

Potential way to solve this problem? I have seen a few comments around updating dependencies but I cannot find them right now. One option is to update the dependency which is already a bit old.

Second, I am going to create an issue in the Istio and Envoy repo to highlight this behavior to them. Possibly Istio can change the cluster format or Envoy can allow overriding authority?

cc @maleck13 @unleashed

unleashed commented 2 years ago

@rahulanand16nov I don't think it is feasible for Istio to change the cluster name format at this stage - it's been well documented for a long time. It might be easier to change Envoy to use the actual authority rather than the cluster name, but still difficult given how this has been around for long - so failing the above, we'll have to work around this case on our side.

rahulanand16nov commented 2 years ago

Good point :thinking:

I'll add steps to reproduce for anyone picking this up before I get to it.

rahulanand16nov commented 2 years ago

cc @alexsnaps

rahulanand16nov commented 2 years ago

I have tried using a DestinationRule which adds the TLS context to the cluster entry with SNI. Envoy swaps the authority header with SNI if present but limitador now rejects the request with a different error: connection error: http2 error: protocol error: unspecific protocol error detected

DestinationRule used:

apiVersion: networking.istio.io/v1beta1
kind: DestinationRule
metadata:
  name: limitador
  namespace: kuadrant-system
spec:
  host: limitador.kuadrant-system.svc.cluster.local
  trafficPolicy:
    portLevelSettings:
    - port:
        number: 8081
      tls:
        mode: SIMPLE
        sni: rate-limit-cluster

There are only two options left: 1) Make a similar change to our dependency: https://github.com/hyperium/h2/pull/487 2) Just keep on using the current fix which is to add a custom cluster entry into Envoy with a name that will pass h2's restriction like rate-limit-cluster.

alexsnaps commented 2 years ago

So that's all hardcoded afaict: https://github.com/istio/istio/blob/32afc27ea91ef68e4e5f0ee3dd47d0aedfba35f1/pilot/pkg/model/service.go#L691-L693

alexsnaps commented 2 years ago

Found this issue that seems to suffer from the same malformed header:

2022-05-27T16:44:03.081190Z     debug   envoy http2     [C449] invalid http2: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [:authority], value: [outbound|4317||otelcol-deployment-collector.opentelemetry.svc.cluster.local]
2022-05-27T16:44:03.081199Z     debug   envoy http2     [C449] invalid frame: Invalid HTTP header field was received on stream 1
rahulanand16nov commented 2 years ago

That's a very similar issue to ours. Added my thoughts there: https://github.com/istio/istio/issues/39196#issuecomment-1144676865

alexsnaps commented 2 years ago

I still can’t figure out where the fix should be tho… wondering if Envoy should somehow address this ¯_(ツ)_/¯

rahulanand16nov commented 2 years ago

Changing Envoy will be a two-step change, first, we change the behavior in Envoy or we add another configurable field to the cluster config and then Istio follows suit to utilize that.

I think Istio should change the pipe to dot because virtually it has no change (or at least I cannot think of any problem).

rahulanand16nov commented 2 years ago

Enabling TLS on the cluster forces Envoy to use SNI which follows the format of :authority header (authorino get's SNI version of the header). Maybe make sure Limitador accepts the TLS since right now it rejects the request with an unknown protocol or similar error can fix the issue.

guicassolato commented 2 years ago

@rahulanand16nov, Envoy gRPC client config allows to overwrite the :authority header (which by default comes from the cluster name, as you said): https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/grpc_service.proto#envoy-v3-api-msg-config-core-v3-grpcservice-envoygrpc.

Can you somehow influence Istio to use that option?

maleck13 commented 9 months ago

@alexsnaps I am thinking we close this, do we expect to do anything about it? I will close and let it be reopened if needed