#1672 Custom Default Version Policy

[ibnuda]

Closes #1672

• More Flexible VersionPolicy #1672

New Feature

Configurable HttpRequestMessage.VersionPolicy.

Motivation for New Feature

The following setup:

• an MVC ASP.NET Core as frontend.
• Ocelot gateway
• some microservices

All components above configured to use Kestrel with the following snippet:

var builder = WebApplication.CreateBuilder(args);

builder.WebHost.ConfigureKestrel(opts =>
{
    opts.ConfigureEndpointDefaults(lopts =>
    {
        lopts.Protocols = HttpProtocols.Http2;
    });
});

All components above should only talk to each other using HTTP/2 and no TLS (plain HTTP).

User Scenario

With plain Ocelot 18.0.0, I couldn't make them talk to each other with plain HTTP from frontend to the service.
The services will print out error messages like the following:

HTTP/2 connection error (PROTOCOL_ERROR): Invalid HTTP/2 connection preface

Solution

Found out that I need to make sure that HttpRequestMessage has its VersionPolicy to be set RequestVersionOrHigher.

Changes I made

• namespace: Ocelot.Request.Mapper
  class: RequestMapper
  method: Task<Response<HttpRequestMessage>> Map(HttpRequest request, DownstreamRoute downstreamRoute)
  changes:
  • added VersionPolicy = HttpVersionPolicy.RequestVersionOrHigher, in HttpRequestMessage instantiation.
• namespace: Ocelot.Requester
  class: HttpClientBuilder
  method: IHttpClient Create(DownstreamRoute downstreamRoute)
  changes:
  • added DefaultRequestVersion = downstreamRoute.DownstreamHttpVersion, and DefaultVersionPolicy = HttpVersionPolicy.RequestVersionOrHigher, in HttpClient instantiation.

Then all those components above can talk to each other in HTTP/2.

And I think DefaultVersionPolicy = HttpVersionPolicy.RequestVersionOrHigher could be DefaultVersionPolicy downstreamRoute.DownstreamHttpVersionPolicy.

And I am willing to make a PR if you fine folks think this kind of changes is welcome.

Specifications

• Version: 18.0.0
• Platform: ASP.NET Core 6.0
• Subsystem: ASP.NET 6.0 Docker image. Should be Debian, yes?

[raman-m]

Hi Ibnu!
Thanks for being proactive!

I'll return to your PR back after merging all PRs with lower numbers... Or will find you a mentor 😉

[raman-m]

@ggnaegi You are the mentor of this PR! Assigned! 😉

[ggnaegi]

I have written some comments, you should add a few more test cases and use string constants for the policies.

[ibnuda]

@ggnaegi, sorry for pinging you. i couldn't resolve this conflict. would you please resolve this conflict when you merge this pr? the difference is only whitespace and the intended change.

thanks in advance.

[raman-m]

Hello, Ibnu!
Just do resolving conflicts in Visual Studio, because VS has a perfect merge tool. Also, for src/Ocelot/Configuration/File/FileRoute.cs conflict I would recommend to use the version from develop only, and then re-add your new property.
If no luck, let me know and I'll help you...

[ibnuda]

hello raman. thanks, the conflict is resolved.

and might be off-topic. but in ocelot will support net8, yes?

[raman-m]

@ibnuda commented on Nov 9
hello raman. thanks, the conflict is resolved.

Thanks!

---

and might be off-topic. but in ocelot will support net8, yes?

Yes! Soon! FYI #1787
This feature will be a part of .NET8 version definitely.

[raman-m]

@ggnaegi Is it ready? Or can we add this PR & feature to Nov'23 milestone? How confident are you, guys? 😃

I see only unit tests

• test/Ocelot.UnitTests/Configuration/VersionPolicyCreatorTests.cs

[ggnaegi]

@ggnaegi Is it ready? Or can we add this PR & feature to Nov'23 milestone? How confident are you, guys? 😃

I see only unit tests

• test/Ocelot.UnitTests/Configuration/VersionPolicyCreatorTests.cs

Hello, I think it's safe, we can add it to the november release. Maybe, we could implement some acceptance tests...
@ibnuda do you have a bit of time for some acceptance tests?

[ibnuda]

hello folks, thanks for the answer.
yes, I'm available to write some acceptance tests. just point me where the rest of the acceptance tests are and the general direction then I will push it up.

[ggnaegi]

@ibnuda You should add a new class in test\Ocelot.AcceptanceTests. I have created a template, you can use it if you want, you might have several combinations to test...

• https / http
• DownstreamHttpVersion 1.0, 2.0, 3.0
• DownstreamVersionPolicy "exact", "downgradable", "upgradeable"

I'm just wondering if, instead of using exact, downgradable, upgradeable, you should use the terms
"RequestVersionExact", "RequestVersionOrHigher", "RequestVersionOrLower", I think it's more intuitive for the users...
and you could use nameof for the definition.

using Ocelot.Configuration.File;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Server.Kestrel.Core;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;

namespace Ocelot.AcceptanceTests;

public class DefaultVersionPolicyTests : IDisposable
{
    private readonly Steps _steps;
    private const string Body = "supercalifragilistic";

    public DefaultVersionPolicyTests()
    {
        _steps = new Steps();
    }

    [Fact]
    public async Task should_return_bad_gateway()
    {
        var port = PortFinder.GetRandomPort();

        var configuration = new FileConfiguration
        {
            Routes = new List<FileRoute>
            {
                new()
                {
                    DownstreamPathTemplate = "/",
                    DownstreamHostAndPorts = new List<FileHostAndPort>
                    {
                        new()
                        {
                            Host = "localhost",
                            Port = port,
                        },
                    },
                    DownstreamScheme = "https",
                    DownstreamHttpVersion = "2.0",
                    DownstreamVersionPolicy = "upgradeable",
                    UpstreamPathTemplate = "/",
                    UpstreamHttpMethod = new List<string> { "GET" },
                },
            },
        };

        this.Given(x => x.GivenThereIsAServiceRunningOn($"https://localhost:{port}", HttpProtocols.Http1))
            .And(x => _steps.GivenThereIsAConfiguration(configuration))
            .And(x => _steps.GivenOcelotIsRunning())
            .When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
            .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.BadGateway))
            .BDDfy();
    }

    private void GivenThereIsAServiceRunningOn(string url, HttpProtocols protocols)
    {
        var builder = new WebHostBuilder()
            .UseUrls(url)
            .UseKestrel()
            .ConfigureKestrel(serverOptions =>
            {
                serverOptions.ConfigureEndpointDefaults(listenOptions => { listenOptions.Protocols = protocols; });
            })
            .UseContentRoot(Directory.GetCurrentDirectory())
            .Configure(app =>
            {
                app.Run(async context =>
                {
                    context.Response.StatusCode = (int)HttpStatusCode.OK;
                    await context.Response.WriteAsync(Body);
                });
            })
            .Build();

        builder.Start();
    }

    public void Dispose()
    {
        _steps?.Dispose();
    }
}

[raman-m]

@ggnaegi commented on Nov 20

namespace Ocelot.AcceptanceTests;
public class DefaultVersionPolicyTests : IDisposable
{ }

If we will create all test classes in the root folder then we will make long list of them.
Too many files in root folder! Why not to organize files/namespaces in Acceptance Tests project?
What Ocelot feature will this small feature belong to?

[ggnaegi]

@ggnaegi commented on Nov 20

namespace Ocelot.AcceptanceTests;
public class DefaultVersionPolicyTests : IDisposable
{ }

If we will create all test classes in the root folder then we will make long list of them. Too many files in root folder! Why not to organize files/namespaces in Acceptance Tests project? What Ocelot feature will this small feature belong to?

Yes, you are right, we should reorganize the tests structure, maybe a new PR?

[ibnuda]

hello folks, seems like there's a problem when testing with http2 and https. the test pass just fine in local, but it seems the circle ci setup needs to trust dotnet issued cert.

i made change in the .circleci/config.yaml by adding dotnet dev-certs https but it seems circleci doesn't pick it up.

what can i do to pass the ci?

thanks in advance.

[ggnaegi]

@ibnuda uuupppss... Let me check that
maybe you could try with "DangerousAcceptAnyServerCertificateValidator": true
It might work

[ggnaegi]

Ok, the problem is on ocelot side too... Ok, will have a look later, sorry.

[ibnuda]

Thank, ggnaegi. but please don't feel pressured. please do it in your own pace and.

[raman-m]

Guys, we have the new commits in develop!
Please, rebase feature branch onto develop! ...with conflicts resolving for sure 😉

[raman-m]

@ibnuda commented on Nov 22

What the issue with .circleci/config.yaml and certificates? Some builds have failed?
Show the links to the CircleCI logs plz!
But first, could you rebase the feature branch please?

[raman-m]

@ggnaegi commented:

Yes, it makes sense, you're right

Renamed! See commits 1585061 and 891bb16

[raman-m]

@ibnuda Congrats! 🥳

[raman-m]

Feature commit in develop → 176a4c8