[BUG] Azure.Monitor.OpenTelemetry.Exporter does not honor x-forwarded-for headers on ACA

[onionhammer]

Library name and version

Azure.Monitor.OpenTelemetry.Exporter, 1.3.0-beta.1

Describe the bug

When switching from application insights (Microsoft.ApplicationInsights.AspNetCore) to Aspire's OpenTelemetry model, all requests are logged from the ACA's ingress IP, and the external IP forwarded (x-forwarded-for) by the ingress is no longer honored.

Expected behavior

X-Forwarded-For should contain the end user's actual IP address, and should appear in application insights as the 'Client IP address'

Actual behavior

Every request in app insights on azure container apps now originates from the same IP per ACA cluster

Client IP address	172.169.202.11
City	Des Moines

Reproduction Steps

1. Create an aspire app
2. Configure Azure Monitor open telemetry exporter
3. Deploy to azure container apps

If this is not immediately clear what the issue is (to whichever contributor owns this area) please let me know and I can put together a minimal repro in a separate repository.

Environment

No response

[github-actions]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @cijothomas @rajkumar-rangaraj @reyang @TimothyMothra @vishweshbankwar.

[TimothyMothra]

Adding some extra details for context:

This was accomplished via ClientIpHeaderTelemetryInitializer.
https://github.com/microsoft/ApplicationInsights-dotnet/blob/bd8b0c25712e6f3c5275ae06b0f0e9331340fee4/NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/TelemetryInitializers/ClientIpHeaderTelemetryInitializer.cs#L17

[onionhammer]

@TimothyMothra Would it be doable to workaround this with a telemetry processor? Or should I just wait for a fix

[TimothyMothra]

Hi, sorry for the delay. Our docs aren't up to date on this subject (I will follow up on this) and I went digging for more information.

First, I want to set some context.
Our Exporter doesn't "collect" any data. We only pass along the information provided by the Instrumentation libraries. In this case, that would be OpenTelemetry.Instrumentation.AspNetCore.

That instrumentation library has some customization options, which is documented in their readme.
For your issue specifically, if you set the client.address attribute, our Azure Monitor Exporter will pick up this value.
Your config will look something like this:

builder.Services.Configure<AspNetCoreTraceInstrumentationOptions>(options =>
{
    options.EnrichWithHttpRequest = (activity, httpRequest) =>
    {
        // get the actual IP from the httpRequest object
        activity.SetTag("client.address", "10.10.10.10");
    };
});

As an alternate approach, I was pointed to this article:
https://learn.microsoft.com/aspnet/core/host-and-deploy/proxy-load-balancer#forwarded-headers
If you configure the Forwarded Headers Middleware, this might do the work for you. I haven't personally tested this.

[onionhammer]

Hi @TimothyMothra

If you configure the Forwarded Headers Middleware

This was the first thing I tried, had no effect.

I would encourage the team here to think about the broader picture - I assume we want good defaults that "just work" out of the box, here;

1. User creates a new 'aspire' app, which encourages an opinionated standard way of setting up APIs & telemetry
2. User un-comments / enables the UseAzureMonitor
3. User deploys to Azure Container Apps with azd

This scenario should just work with very little messing around without writing a ton of code that you just kinda "have to know".

@davidfowl

[davidfowl]

I agree with you @onionhammer. In the next preview of Aspire, we automatically turn on the forwarded headers middleware for any web projects with endpoints. That should solve the problem.

We might need to do more default things in service defaults to make this end to end work (if there are more options we should default on).

[onionhammer]

n the next preview of Aspire, we automatically turn on the forwarded headers middleware for any web projects with endpoints.

This library may have to be updated so it cares about that middleware, just adding in the code Damian references (and invoking it) doesn't work.

[davidfowl]

Right it seems like we still need to propagate the client IP to the activity. Is that right @TimothyMothra ?

[TimothyMothra]

Yes, that's correct. But this cannot be done by default as per the specification (link).
In .NET, users can opt-in via the example I shared above.

[onionhammer]

this opt-in does not work, although your other code snippet may work conceptually, it's definitely not ideal.

[onionhammer]

Any update on this? Now that Aspire is GA, this seems like a big miss. @davidfowl

Opting in does not work

[davidfowl]

@sampa-msft can you follow up here?

[davidfowl]

BTW @onionhammer we enable XFF by default in aspire apps during deployment

[onionhammer]

BTW @onionhammer we enable XFF by default in aspire apps during deployment

I saw that; the 'ASPNETCORE_FORWARDEDHEADERS_ENABLED' 'true'? Doesn't seem to do much in my testing

[davidfowl]

What did you test?

[onionhammer]

What did you test?

Adding the above header;

  { name: 'ASPNETCORE_FORWARDEDHEADERS_ENABLED', value: 'true' }

And adding this:

the result:

[davidfowl]

When you enable the forwarded headers, it sets those options by default and enables the middleware.

https://github.com/dotnet/aspnetcore/blob/10517269f40d53eb22cce6b4d520ed27ed1e1b9f/src/DefaultBuilder/src/ForwardedHeadersOptionsSetup.cs#L27

[onionhammer]

Basically I was just trying to throw the kitchen sink at it, but no matter what I do it always just shows the same internal IP address since switching from app insights to otel sdks

Obviously this hack ( #42850 (comment) ) would probably work, but this bug still stands

[davidfowl]

I think the main thing to figure out is where the issue is. If XFF doesn't work, then we should get it fixed. From this issue though it's inclear where the problem is. Is it in the ACA itself or ASP.NET Core?

[onionhammer]

I don't think it's an ACA or and ASP.NET Core issue, since this works OOTB if you use the Application Insights SDK.

[onionhammer]

@sampa-msft can you follow up here?

Bump

The middleware is definitely doing its job, the “context.Connection.RemoteIpAddress” is returning the expected external IP address, but azure log analytics gets this other internal IP. The internal IP does not appear in the request headers by the time it gets to the code that logs the IP + headers

Adding the following code does yield the expected address:

        var ipAddress = context.Connection.RemoteIpAddress?.ToString();
        Activity.Current?.AddTag("client.address", ipAddress);

So… this is definitely a bug or is this a wontfix ?

[samsp-msft]

I think @davidfowl tagged the wrong person, so I didn't get notified.

Just to clarify, I believe this application using OTel SDK with the AzureMonitorExporter, rather than the classic ApplicationInsights SDK or OTLP.
Very few Otel attributes are set automatically in ASP.NET Core. Most are set through the OTel ASP.NET instrumentation package. Doing a quick spelunk of that code, the only time we seem to be setting the client.address attribute is in the case of it being a GRPC request.

The AzureMonitor exporter seems to have additional setting of the client.address here so that is probably where this issue belongs. @vishweshbankwar who changed this code as part of 962ba50.

I suspect that the fix needs to be in the latter.

[onionhammer]

Ok, so inconsistent behavior between gRPC and HTTP?

@TimothyMothra @vishweshbankwar

[vishweshbankwar]

@onionhammer -

The workaround you shared here is correct and also recommended one.

So… this is definitely a bug or is this a wontfix ?

This is by design.

Attributes added by default on activity by the instrumentation libraries is outside the scope of exporter. Exporter simply maps attributes from spans to Application Insights data model fields. There is no change that is needed at the exporter level.

OpenTelemetry.Instrumentation.AspNetCore only sets the required fields from the spec. Any additional attributes that are needed can be added via provided Enrich APIs or processor. There are ongoing discussions about providing specific controls on the optional fields. dotnet/runtime#103302.

gRPC part is still experimental and may change in future based on the final spec

[onionhammer]

OpenTelemetry.Instrumentation.AspNetCore only sets the required fields from the spec.

client.address is one of the fields in the spec though?

[vishweshbankwar]

OpenTelemetry.Instrumentation.AspNetCore only sets the required fields from the spec.

client.address is one of the fields in the spec though?

Yes, but not required. I'd recommend you to open the issue here with your ask. Closing this issue as exporter is working as intended.

[vishweshbankwar]

@onionhammer - I have opened a separate issue for tracking this #44687. The change could also be done in the distro(Azure.Monitor.OpenTelemetry.AspNetCore)

[onionhammer]

@onionhammer - I have opened a separate issue for tracking this #44687. The change could also be done in the distro(Azure.Monitor.OpenTelemetry.AspNetCore)

So should it still be an issue in https://www.nuget.org/packages/OpenTelemetry.Instrumentation.AspNetCore ?

[vishweshbankwar]

@onionhammer - I have opened a separate issue for tracking this #44687. The change could also be done in the distro(Azure.Monitor.OpenTelemetry.AspNetCore)

So should it still be an issue in https://www.nuget.org/packages/OpenTelemetry.Instrumentation.AspNetCore ?

I checked and there is a similar ask already: open-telemetry/opentelemetry-dotnet-contrib#1786.

Clarifying couple things here

When using Azure.Monitor.OpenTelemetry.Exporter along with OpenTelemetry.Instrumentation.AspNetCore, the attribute needs to be added either by default(or via some config) by OpenTelemetry.Instrumentation.AspNetCore or via Enrich/Processor. My previous comments were taking this scenario in consideration(the package mentioned in this issue)

Azure.Monitor.OpenTelemetry.AspNetCore is a Distro that includes Azure.Monitor.OpenTelemetry.Exporter and OpenTelemetry.Instrumentation.AspNetCore. This package enables some settings by default and enabling collection of LocationIP by default could be one of them (#44687 covers this). One possible solution is for the distro to simply use the solution from open-telemetry/opentelemetry-dotnet-contrib#1786 (if implemented).

[onionhammer]

I don't really care about the implementation details; at the end of the day, the Aspire template's default behavior for newly created apps should be that this client.address is set properly to the source IP address without forcing everyone to write custom middleware