Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature request] Remove (make configurable) x-request-id request header sanitizing #6050

Closed
defat opened this issue Feb 25, 2019 · 21 comments · Fixed by adobe/envoy#1 or #7140
Closed
Labels
design proposal Needs design doc/proposal before implementation help wanted Needs help!

Comments

@defat
Copy link

defat commented Feb 25, 2019

We are using x-request-id header for distibuted logging in the backend powered with istio.

Now request id is generated on the server side (envoy edge server) and returned to client as a response header. We are using request id reported from clients for issues tracking.

The problem is - we are unable to trace timed out request.

To solve this issue we want to generate request id on the client side, but envoy sanitizes this header and replaces it. There are several solutions like using own custom header beside istio-generated x-request-id, however maintaining several ids does not seem the right way.

Is it possible to make headers sanitizing configurable or remove x-request-id sanitizing with proper validation for it to be a valid UUID?

@mattklein123 mattklein123 added the design proposal Needs design doc/proposal before implementation label Feb 25, 2019
@mattklein123
Copy link
Member

Is it possible to make headers sanitizing configurable or remove x-request-id sanitizing with proper validation for it to be a valid UUID?

I think it's reasonable to make this configurable.

@stale
Copy link

stale bot commented Mar 27, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 27, 2019
@stale
Copy link

stale bot commented Apr 3, 2019

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

@stale stale bot closed this as completed Apr 3, 2019
@cdmurph32
Copy link

Can we please re-open this? This would also solve istio/istio#10875

@mattklein123 mattklein123 reopened this May 20, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 20, 2019
@mattklein123 mattklein123 added the help wanted Needs help! label May 20, 2019
@atrifan
Copy link
Contributor

atrifan commented May 27, 2019

I would propose the following fix. Feedback would be very much appreciated before actual PR.

Just changing the 208 line code from source/common/http/conn_manager_utility.cc

 if (config.generateRequestId() && (edge_request || !request_headers.RequestId()))

with

if (config.generateRequestId() && !request_headers.RequestId()) 

Is there any reason why you would want to reset requestId header for edge_requests? Is there any concern in this fix - should backward compatibility be maintained.

My thoughts would be that in a mesh to mesh communication reseting the x-request-id would loose traceability across multiple components. Is this behaviour indeed wanted?

@mattklein123
Copy link
Member

Is there any reason why you would want to reset requestId header for edge_requests? Is there any concern in this fix - should backward compatibility be maintained.

Yes, we need to maintain back compat and make this configurable. The edge check is to prevent an untrusted client from spoofing an identical request ID, for example.

ravilr added a commit to ravilr/contour that referenced this issue Jun 5, 2019
revert this when envoyproxy/envoy#6050 lands in envoy-v1.11.0 release,
which does the right thing of being able to configure it to be generated, only when not present.
@rzachariah
Copy link

We're attempting to incrementally adopt Envoy. Some of our services are deployed with Envoy sidecars, but most are not. Prior to using Envoy, we were already using x-request-id headers to track activity across our microservice architecture.

We're noticing that Envoy is sanitizing a client generated x-request-id that looks like

xxxxxxxx-xxxx-4xxx-xxxx-xxxxxxxxxxxx

and mutating it to

xxxxxxxx-xxxx-9xxx-xxxx-xxxxxxxxxxxx

This makes it hard for us to trace requests in and out of the Envoy mesh.

Plus one this feature request.

@a-sharma11
Copy link

+1
We have the same problem making it impossible to adopt envoy incrementally. It breaks all our existing tracing capabilities.

htuch pushed a commit that referenced this issue Jun 12, 2019
Add edge_accept_request_id property for the envoy.http_connection_manager filter. Field added to resolve #6050 and also maintain backward compatibility

Risk: Low - small feature disabled by default and maintaining backward compatibility

Testing: Added 2 additional integration tests in test/common/http/conn_manager_utility_test.c to validate behaviour for:

1. edge request - activated edge_accept_request_id set to true but no x-request-id header sent - expected to generate a new one
2. edge request - activated edge_accept_request_id set to true and sent x-request-id header sent - expected to keep the old one.
3. all previous tests regarding edge requests resetting the x-request-id should still pass

Fixes #6050

Signed-off-by: trifan <trifan@adobe.com>
@a-sharma11
Copy link

Thank you @htuch for closing this. Is this available to use with latest?

@htuch
Copy link
Member

htuch commented Jun 24, 2019

@a-sharma11 this should be available on master @ HEAD.

@a-sharma11
Copy link

thank you @htuch ! Any documentation on how to use it? If i can follow the pr correctly seems like a new setting is added.

@htuch
Copy link
Member

htuch commented Jun 24, 2019

See

// Whether the connection manager will keep the :ref:`x-request-id
.

@a-sharma11
Copy link

a-sharma11 commented Jul 12, 2019

@htuch I am using the 1.11 which got released yesterday. I have a front proxy with preserve_external_request_id set to true. I am still seeing the behavior where it changes the version number of the uuid. Here is a screenshot. /trace/1 incoming x-request-id is "8cda17a5-eb41-4ced-9843-acc826f95c8c" but the outgoing from front proxy is "8cda17a5-eb41-9ced-9843-acc826f95c8c"

xxxxxxxx-xxxx-4xxx-xxxx-xxxxxxxxxxxx

and mutating it to

xxxxxxxx-xxxx-9xxx-xxxx-xxxxxxxxxxxx

Config:

static_resources:
  listeners:
  - address:
      socket_address:
        address: 0.0.0.0
        port_value: 80
    filter_chains:
    - filters:
      - name: envoy.http_connection_manager
        typed_config:
          "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
          tracing:
            operation_name: egress
          codec_type: auto
          preserve_external_request_id: true
          stat_prefix: ingress_http
          route_config:
            name: local_route
            virtual_hosts:
            - name: backend
              domains:
              - "*"
              routes:
              - match:
                  prefix: "/"
                route:
                  cluster: service1
                decorator:
                  operation: Enter-Trade
          http_filters:
          - name: envoy.router
            typed_config: {}
  clusters:
  - name: service1
    connect_timeout: 0.250s
    type: strict_dns
    lb_policy: round_robin
    http2_protocol_options: {}
    load_assignment:
      cluster_name: service1
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: service1
                port_value: 80
  - name: jaeger
    connect_timeout: 1s
    type: strict_dns
    lb_policy: round_robin
    load_assignment:
      cluster_name: jaeger
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: jaeger
                port_value: 80
tracing:
  http:
    name: envoy.zipkin
    typed_config:
      "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig
      collector_cluster: jaeger
      collector_endpoint: "/api/v1/spans"
      shared_span_context: false
admin:
  access_log_path: "/dev/null"
  address:
    socket_address:
      address: 0.0.0.0
      port_value: 8001

Dockerfile:

FROM envoyproxy/envoy-alpine:v1.11.0

COPY front-proxy.yaml /etc/front-envoy.yaml

CMD ls /etc/front-envoy.yaml && /usr/local/bin/envoy -l debug -c /etc/front-envoy.yaml

Screen Shot 2019-07-12 at 5 11 14 PM

@atrifan
Copy link
Contributor

atrifan commented Jul 12, 2019 via email

@a-sharma11
Copy link

@athampy Thanks for quick response! I created this github demo project which is equivalent to my use case. It includes the setup and the curl command to run.
https://github.com/a-sharma11/envoy_demo

@a-sharma11
Copy link

a-sharma11 commented Jul 15, 2019

@athampy - also its very surprising that its changing just the version number digit of the guid to an invalid value. 8cda17a5-eb41-9ced-9843-acc826f95c8c is actually an invalid guid as per http://sqa.fyicenter.com/1000330_UUID_GUID_Validator.html. Possible value for this digit should be 5 or under.

@atrifan
Copy link
Contributor

atrifan commented Jul 15, 2019

Don’t want to be disrespectful or anything but it’s the second time you get my mention wrong - it’s @atrifan unless you want to talk to someone else. I am going to provide a resolution in 24 hours if that’s ok with you. Thanks, Alex

@a-sharma11
Copy link

Ops @atrifan very sorry about that! ty for your quick response.

@atrifan
Copy link
Contributor

atrifan commented Jul 15, 2019

Ok found a little time to investigate the issue and came up with a fast update: the feature is working - test with a request-id like: curl http://localhost:8000/trace/1 -H "X-Request-Id: myRequestId" -i you will then see that envoy preserves the requestId.

For your particular issue though - envoy preservers the requestId (2nd 3rd and 4th service) 1st service modifies it - i see that you grab your x-request-id from envoy examples readme. I would say that the x-request-id 8cda17a5-eb41-4ced-9843-acc826f95c8c is considered invalid by envoy and replaced - tried with a different one from their tests 125a4afb-6f55-a4ba-ad80-413f09f48a28 and it works just fine.

To fast summarize, the conclusion is that the preservation of the x-request-id works just fine - that particular x-request-id that you provided in your example does not work - I didn't jump into the implementation details of why envoy thinks the requestId provided by you is corrupted - if you feel it should not be that way please raise a bug/issue and you might be able to get additional info on that matter.

P.S: you can even test with their replacement of x-request-id: 8cda17a5-eb41-9ced-9843-acc826f95c8c and it works.

Hope that helps.

Thanks,
Alex

@a-sharma11
Copy link

Thats very interesting. Thanks for looking into it @atrifan ! The values i am using are valid guid. I will report it as new bug.

@a-sharma11
Copy link

Based on your analysis above, i also performed a test by encapsulating the guid into quotes and that seems to be working. This command is working as expected and propagating it properly.

curl 'http://localhost:8000/trace/1' -H 'x-request-id: "14c62072-28aa-4517-9f86-b5f9c84f20c7"'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design proposal Needs design doc/proposal before implementation help wanted Needs help!
Projects
None yet
7 participants