Adds IngressController to Yarp #842
Conversation
|
What's the content in /samples/KuberenetesIngress for? Is that apps that you would publish to k8 behind the new ingress? General structure suggestions:
|
Yes |
Tratcher
left a comment
Tratcher
left a comment
There was a problem hiding this comment.
I want to talk to you about the object model mapping in YarpParser. Everything else is code cleanup suggestions. I don't expect to review OperatorFramework in this PR, we can follow up on that seperately.
|
|
||
| namespace Yarp.ReverseProxy.Kubernetes.Ingress.Services | ||
| { | ||
| public class Receiver : BackgroundHostedService |
There was a problem hiding this comment.
The Receiver looks like standard equipment, people wouldn't be customizing this right? Can it move over to the Protocol package and be included in LoadFromMessages too?
Startup is the main content I expect in the Ingress app, that's where people would do most customization.
|
|
||
| try | ||
| { | ||
| using var client = new HttpClient(); |
There was a problem hiding this comment.
Why is the client inside the outer while loop? Move up to line ~49?
| var request = new HttpRequestMessage(); | ||
| request.RequestUri = new Uri(_options.ControllerUrl); | ||
| request.Method = HttpMethod.Get; | ||
| using var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken); | ||
| using var stream = await response.Content.ReadAsStreamAsync(); |
There was a problem hiding this comment.
| var request = new HttpRequestMessage(); | |
| request.RequestUri = new Uri(_options.ControllerUrl); | |
| request.Method = HttpMethod.Get; | |
| using var response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken); | |
| using var stream = await response.Content.ReadAsStreamAsync(); | |
| using var stream = await client.GetStreamAsync(_options.ControllerUrl, cancellationToken); |
| services.Configure<ReceiverOptions>(_configuration.Bind); | ||
| services.AddHostedService<Receiver>(); | ||
| services.AddReverseProxy() | ||
| .LoadFromMessages(); |
There was a problem hiding this comment.
| .LoadFromMessages(); | |
| .LoadFromMessages(receiverOptions => _configuration.Bind(receiverOptions)); |
|
|
||
| namespace Yarp.ReverseProxy.Kubernetes.Protocol | ||
| { | ||
| public class MessageConfigProvider : IProxyConfigProvider, IUpdateConfig |
There was a problem hiding this comment.
Receiver and MessageConfigProvider can be combine, and IUpdateConfig removed.
| _config = new MessageConfig(null, null); | ||
| } | ||
|
|
||
| public IProxyConfig GetConfig() => _config; |
There was a problem hiding this comment.
It's recommended that GetConfig block if it's called before the first config becomes available. That will prevent the ingress from fully starting and accepting requests until the config is available. Otherwise you may 404 the first requests.
There was a problem hiding this comment.
Hm, what if there is no k8s ingress available? Are you saying that we should still wait on an empty config before accepting requests?
Otherwise I think that makes sense.
There was a problem hiding this comment.
You mean if the controller isn't available? At that point it might make sense for the ingress to fail in some way. Throwing from here would be the most blunt solution.
| } | ||
|
|
||
| var protocol = context.Options.Https ? "https" : "http"; | ||
| var uri = $"{protocol}://{address.Ip}:{port.Port}"; |
There was a problem hiding this comment.
IPv6? it will need square brackets.
There was a problem hiding this comment.
It's a follow up I have, but very low priority. K8s doens't use IPv6 unless you opt in and it's extremely uncommon.
|
|
||
| var ip = address.Ip; | ||
| var cluster = clusters[service.Name]; | ||
| cluster.ClusterId = ip; |
There was a problem hiding this comment.
The cluster id is the destination ip?
In theory there should be many routes -> few clusters -> many destinations.
This looks more like one route -> one cluster -> one destination?
|
|
||
| var protocol = context.Options.Https ? "https" : "http"; | ||
| var uri = $"{protocol}://{address.Ip}:{port.Port}"; | ||
| cluster.Destinations[uri] = new Destination() |
There was a problem hiding this comment.
nit: don't recreate destination each time.
|
|
||
| foreach (var port in subset.Ports ?? Enumerable.Empty<V1EndpointPort>()) | ||
| { | ||
| if (!MatchesPort(port, service?.Port)) |
There was a problem hiding this comment.
Different ports for a route to the same IP need to be in different clusters (all destinations in a cluster are equal).
| { | ||
| foreach (var address in subset.Addresses ?? Enumerable.Empty<V1EndpointAddress>()) | ||
| { | ||
| if (!clusters.ContainsKey(service?.Name)) |
There was a problem hiding this comment.
name should be cluster:port
|
Looking pretty good, and only one minor build error left. Let me know when you're ready for a final review. |
|
Go ahead. |
src/ReverseProxy.Kubernetes.Protocol/Yarp.ReverseProxy.Kubernetes.Protocol.csproj
Outdated
Show resolved
Hide resolved
|
@jkotalik awesome work! Quick question... Is the Operator Framework something that is planned to live as a standalone repo published as a Nuget so people can create operators based on it? Or is it something built purposefuly for Yarp? I'm building an operator for Orleans right now and would love to use this if that is reusable. Thanks! |
Long term the goal is to make it standalone in some capacity (separate repo, package, etc). It isn't specific to YARP. We don't have a timeline on when it would be available on nuget, but we aim to do so. |
|
Ok understood. Hopefuly it won't be long! 😄 For now will use it as a submodule. Thanks! |
|
@jkotalik Just to give you a feedback. I was able to create an operator for Microsoft Orleans and it worked just fine for what we wanted. Unfortunately, we will not be able to release it until we have the Operator Framework out as a NuGet (at least in preview). Will keep an eye on it, thanks! |
|
@jkotalik @Tratcher Is there any documentation on using Yarp as a K8s Ingress Controller? I found the following README. But I found nothing here https://microsoft.github.io/reverse-proxy/index.html |
|
@StevenTCramer it's not in the official docs yet because we've never released a package for it. It will be added to the docs when released. |
Fixes #200
This is a basic implementation of a Kubernetes ingress and ingress controller.
Relevant subfolders are mostly in src/ReverseProxy.KuberenetesController and src/ReverseProxy.WebApp.
I left most of the OperatorFramework subfolder structure unchanged as we want to reorganize it.
Key follow ups: