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

YARP destination's address configuration only allows URLs starting with http/https #920

Closed
nlyu opened this issue Apr 16, 2021 · 13 comments
Closed
Labels
Milestone

Comments

@nlyu
Copy link

nlyu commented Apr 16, 2021

Problem
In YARP configuration, the destination->address value can only be set to URLs starting with http:// or https://

Any other valid URLs but not starting will https:// or http:// would throw an ArgumentException in YARP during request proxy to the above destination.

How to reproduce
For example, if address is set to a non http/https but valid url(e.g: "fabric:/Abc/Def"), any proxy call to that destination would lead to a System.ArgumentException when YARP is setting the proxyReuqest.RequestUri in TransformRequestAsync: code link

Full error trace:

Error: System.ArgumentException: Only 'http' and 'https' schemes are allowed. (Parameter 'value')
   at System.Net.Http.HttpRequestMessage.set_RequestUri(Uri value)
   at Microsoft.ReverseProxy.Service.RuntimeModel.Transforms.StructuredTransformer.TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, String destinationPrefix)
   at Microsoft.ReverseProxy.Service.Proxy.HttpProxy.CreateRequestMessageAsync(HttpContext context, String destinationPrefix, HttpTransformer transformer, RequestProxyOptions requestOptions, Boolean isStreamingRequest, CancellationToken requestAborted)
   at Microsoft.ReverseProxy.Service.Proxy.HttpProxy.ProxyAsync(HttpContext context, String destinationPrefix, HttpMessageInvoker httpClient, RequestProxyOptions requestOptions, HttpTransformer transformer)
   at Microsoft.ReverseProxy.Middleware.ProxyInvokerMiddleware.Invoke(HttpContext context)
   at Microsoft.ReverseProxy.Middleware.PassiveHealthCheckMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)

Why we need this
We are a team who is building a gateway that is routing to service fabric endpoints. And we leverage the address field in our own httpclient.sendasync() to get our service fabric service Urls when proxying each request. For example: address: fabric:/Abc/Def. It would be very helpful and convenient if YARP allow us to have service fabric service Urls in the address field.

@nlyu nlyu added the Type: Bug Something isn't working label Apr 16, 2021
@Tratcher
Copy link
Member

This is a restriction of HttpRequestMessage, not YARP. You won't be able to use "fabric:" Uris with HttpClient.SendAsync.

@alnikola @davidni the ServiceFabric adapter package we have resolves the fabric addresses to http addresses when loading the config, correct?

@Tratcher
Copy link
Member

Setting up a design meeting...

@alnikola
Copy link
Contributor

Service Fabric adapter uses SF endpoint's URLs for requests and health check. They are defined outside of YARP configuration itself. By default, it considers only https endpoints, but with a manual override it can read any URL. The blocker here, as you correctly pointed out, is HttpRequestMessage.

@karelz karelz added this to the YARP 1.0.0 milestone Apr 22, 2021
@karelz karelz added this to Unscheduled in YARP Planning via automation Apr 22, 2021
@karelz karelz moved this from Unscheduled to 1.0 Backlog in YARP Planning Apr 22, 2021
@karelz
Copy link
Member

karelz commented Apr 22, 2021

Triage: We need to brainstorm -- @Tratcher @davidni @nlyu
It is highly unlikely we will change Runtime behavior + it would be 6.0+ only anyway.

@MihaZupan
Copy link
Member

MihaZupan commented May 28, 2021

May actually be resolved in runtime for (6? +) dotnet/runtime#52836 (comment)

(deferring scheme validation until the SendAsync call on the underlying handler)

@alnikola
Copy link
Contributor

@MihaZupan We need to support it on .Net Core 3.1 and .NET 5 anyways.

@alnikola alnikola assigned alnikola and unassigned alnikola Jun 8, 2021
@mdg215199
Copy link

For what it's worth, i'm getting the same error when having to use transforms. I'm using istio... but I have also tried to just use ingress w/in aks. Everything is fine, up to the point the transformbuilder matches and has to make changes in the url request.

@Tratcher
Copy link
Member

@mdg215199 are you also trying to make fabric:/ urls or something else?

@Tratcher Tratcher changed the title YARP destication's address configuration only allows URLs starting with http/https YARP destination's address configuration only allows URLs starting with http/https Jul 1, 2021
@mdg215199
Copy link

@Tratcher apologies for delay, but i'm i'm not using fabric. I'm using istio (1.10) and aks (k8s 1.19.11). runniny yarp locally and/or if i deploy it to an azure app service, there are no issues. once I try to add requests through istio I get errors mentioned above.

as to why i'm trying to add yarp to aks/istio, most of our request/services are already going through istio/envoy in order to route to our various services. I was hoping to use the same url endpoint and just extend part of the routing in order to follow standards in our service requests. sadly, in this case, I need to do regex look ahead, and based from there, have to make some additional url changes.

I also tried to use istio as an external endpoing using service entry, while also pointing to an app service... but well, same issue.

@Tratcher
Copy link
Member

Tratcher commented Jul 2, 2021

Can you give examples of the urls you're trying to construct?

@mdg215199
Copy link

@Tratcher I need to re-deploy it again. i thought it was still up, but apparently it's not. I was making a https request with a trusted tls cert, and then was getting about same issue about http/https when trying to do the http transform. I'll get it deployed and will send you a link, though might be a day or two before I have a chance to do so. will update the comment once i have it deployed though.

@samsp-msft
Copy link
Contributor

Triage: We don't think that this needs any changes in YARP as the 6.0 runtime postpones the check until later.
Closing. Please re-open if this continues to be a blocking issue.

@mdg215199
Copy link

Sorry, I got side tracked and half forgot about this ticket. I just deployed/tested yarp and there are no issues when running it within istio. On July 2nd, I had stated there was an issue with yarp/istio, though I was wrong and apologize for stating that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants