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

Allow certain response headers coming from AzurePipelinesCredential to be logged #5992

Closed
ahsonkhan opened this issue Sep 16, 2024 · 3 comments · Fixed by #6001
Closed

Allow certain response headers coming from AzurePipelinesCredential to be logged #5992

ahsonkhan opened this issue Sep 16, 2024 · 3 comments · Fixed by #6001
Assignees
Milestone

Comments

@ahsonkhan
Copy link
Member

ahsonkhan commented Sep 16, 2024

These two response headers are useful for the service team to debug and diagnose issues, and hence it is requested that we do log them. For example, the e2e id is needed to understand the behavior observed in #5786 / #5875

  • x-vss-e2eid
  • x-msedge-ref

We decided, other than logging them, to also add these header values to the exception message that is thrown, so they are always visible, even with logging disabled.

Given our logged headers are based on an allow list, currently these two headers are REDACTED.

CaseInsensitiveSet const Policies::_detail::g_defaultAllowedHttpHeaders
= {"Accept",
"Accept-Ranges",
"Cache-Control",
"Connection",
"Content-Length",
"Content-Range",
"Content-Type",
"Date",
"ETag",
"Expires",
"If-Match",
"If-Modified-Since",
"If-None-Match",
"If-Unmodified-Since",
"Last-Modified",
"Pragma",
"Range",
"Request-Id",
"Retry-After",
"Server",
"traceparent",
"tracestate",
"Transfer-Encoding",
"User-Agent",
"WWW-Authenticate",
"x-ms-client-request-id",
"x-ms-date",
"x-ms-error-code",
"x-ms-range",
"x-ms-request-id",
"x-ms-return-client-request-id",
"x-ms-version"};

The open questions we're diving into with the service team, are:

  1. Which headers are safe to log? x-vss-e2eid, activityid, x-msedge-ref, x-tfs-session
  2. Which headers do the service team need and would benefit from being logged for troubleshooting and diagnostics? x-vss-e2eid, x-msedge-ref
  3. Are these headers unique/specific to Azure Pipelines, and only used by them, or are they used by any other services. For example, are all x-vss-* headers safe to log and ones only used/needed by them? x-vss-e2eid is used by ADO, but x-msedge-ref and activityid might be used by others.

Tracking issues across languages:
Azure/azure-sdk-for-net#45993
Azure/azure-sdk-for-java#41871
Azure/azure-sdk-for-go#23444
Azure/azure-sdk-for-js#31130
Azure/azure-sdk-for-python#37412

cc @manolerazvan, @geekzter, @kboom

@ahsonkhan ahsonkhan added this to the 2024-10 milestone Sep 16, 2024
@ahsonkhan ahsonkhan self-assigned this Sep 16, 2024
@LarryOsterman
Copy link
Member

QQ: Which customers are asking for this and what are the semantics of these two headers?

If this is only a single customer, would it be better to allow the customer to add them to the allow list themselves rather than making a global change which affects all customers?

@ahsonkhan
Copy link
Member Author

ahsonkhan commented Sep 16, 2024

This is coming from the Azure Pipelines team, since that info is useful for them to track down and debug service issues or questions that customers might report.

Most logging of the SDK is there to provide sufficient information to service teams and CSAs to help troubleshoot issues that customers might report, so such requests don't typically come directly from end customers.

In this case, we don't expect or want end users to have to enable logging of certain headers that are necessary for troubleshooting, after an issue has occurred, retry repro-ing, and submitting more info. This isn't a customer facing customization. For headers that are safe to log, and are necessary for troubleshooting, it is a good idea to log them pre-emptively.

The service team can better answer the question about what the header semantics are, but I don't think that detail is necessary for our purposes (and could vary from service to service).

The open questions we're diving into with the service team, are:

  1. Which headers are safe to log?
  2. Which headers do the service team need and would benefit from being logged for troubleshooting and diagnostics?
  3. Are these headers unique/specific to Azure Pipelines, and only used by them, or are they used by any other services. For example, are all x-vss-* headers safe to log and ones only used/needed by them?

That said, we could consider allowing these headers to be logged only in identity, or only for this credential type by having the internal implementation add to the loggable headers, rather than adding them to the global list of headers that are logged by all SDK clients.

@LarryOsterman
Copy link
Member

LarryOsterman commented Sep 16, 2024

In general, I'm quite leery of adding service-specific headers to the set of global inclusions.

This is explicitly why we allow service clients (and identity clients fall into this category) to specify service client specific exceptions. Storage does this extensively example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants