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

Enhances for http access log #9842

Closed
dvdlevanon opened this issue Nov 16, 2022 · 4 comments · May be fixed by linkerd/linkerd2-proxy#2420
Closed

Enhances for http access log #9842

dvdlevanon opened this issue Nov 16, 2022 · 4 comments · May be fixed by linkerd/linkerd2-proxy#2420

Comments

@dvdlevanon
Copy link

dvdlevanon commented Nov 16, 2022

What problem are you trying to solve?

I have an issue with Access Log regarding those fields:

  • request_bytes - Its zero if Content-Length header is not set on the request.
  • response_bytes - Its zero if Content-Length header is not set on the response.
  • client_addr = It doesn't provide a way to use X-Forwarded-For header - when a Load Balancer involve, it would be the same address for all requests.

How should the problem be solved?

The fix should enhance the http-access-log which implemented in this file:
https://github.com/linkerd/linkerd2-proxy/blob/main/linkerd/http-access-log/src/lib.rs#L121

  • For request_bytes - if Content-Length header is missing, fallback to actual input stream length (or at least, amount of bytes read from the stream).
  • For response_bytes - if Content-Length header is missing, fallback to actual output stream length.
  • client_addr - Provide a way to specify which client_addr to take.

Any alternatives you've considered?

As an alternative, instead of changing current fields, new fields can be added:

  • input_bytes - actual input stream length
  • output_bytes - actual output stream length
  • x_forward_for - the value of x_forward_for header.

How would users interact with this feature?

With this feature, system admins and devops can easily track the payload sizes of both request and response, without asking devs to set the Content-Length header which is not set by default.

System admins and devops can also know where requests came from by checking the client_addr and get the actual IP instead of the load balancer IP.

Would you like to work on this feature?

maybe

@hawkw
Copy link
Contributor

hawkw commented Nov 17, 2022

I think these are definitely reasonable changes to make. Recording the actual request/response body length when a Content-Length header is not present is definitely desirable, and it would be nice to do that.

Regarding the client_addr field, that field is currently defined as the IP address of the client that opened the connection to the Linkerd proxy that emitted the access log. I don't think this field should be overridden by the value of an X-Forwarded-For (or Forwarded) header if one is present, since that would mean we are no longer recording the client IP address at all. Instead, I think it would be fine to add a new field to the access log for X-Forwarded-For with the value of that header, if the header is present. That way, we record the header value as well as the actual client IP address.

@dvdlevanon
Copy link
Author

I completely agree regarding the client_addr @hawkw

@stale
Copy link

stale bot commented Feb 17, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Feb 17, 2023
@stale
Copy link

stale bot commented May 24, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 24, 2023
@stale stale bot closed this as completed Jun 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants