#1672 Custom Default Version Policy

[ibnuda]

Closes #1672

• #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! 😉

[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?

[ibnuda]

@raman-m here's the link https://app.circleci.com/pipelines/github/ThreeMammals/Ocelot/1401/workflows/fed691a9-ee47-4ecf-8408-58ecdb2802cb/jobs/2652

i will rebase it asap.

[raman-m]

@ibnuda Do you need a help from mentor to rebase feature branch?

[ggnaegi]

@ibnuda Ok, so, the tests are now passing, but it wasn't dead easy. https://app.circleci.com/pipelines/circleci/GZQVatYXRpPzhVdjSqemdz/Ub5zkLQTdd3gUF5FDU1Htb/30/workflows/efb521ef-7721-421c-a4e7-9bc1bdacb220/jobs/58 The changes are included mainly in the CI Pipeline image Dockerfile

FROM mcr.microsoft.com/dotnet/sdk:7.0-alpine

RUN apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client

# Generate and export the development certificate
RUN dotnet dev-certs https -ep /certs/cert.pem -p '' && \
    chmod 644 /certs/cert.pem

ENV ASPNETCORE_URLS="https://+;http://+" 
ENV ASPNETCORE_HTTPS_PORT=443 
ENV ASPNETCORE_Kestrel__Certificates__Default__Password="" 
ENV ASPNETCORE_Kestrel__Certificates__Default__Path=/certs/cert.pem

Still, a simple improvement here:

  public class VersionPolicies
  {
      public const string RequestVersionExact = nameof(HttpVersionPolicy.RequestVersionExact);
      public const string RequestVersionOrLower = nameof(HttpVersionPolicy.RequestVersionOrLower);
      public const string RequestVersionOrHigher = nameof(HttpVersionPolicy.RequestVersionOrHigher);
  }

But you should first rebase and then we will be able to proceed... From your feature branch you should do git rebase develop and follow the instructions.

@raman-m we should update the docker images for these tests.

[raman-m]

@ggnaegi commented on Nov 27

Are you blocked in development because of required new Docker image for builds? What is the readiness of this PR to plan to include PR in future milestones?

[raman-m]

@ibnuda Please 🙏 resolve merge conflicts via rebasing feature branch onto develop.

[ggnaegi]

@raman-m could you update the image on docker and update the references in the circle ci yaml file on the develop branch?

FROM mcr.microsoft.com/dotnet/sdk:8.0-alpine

RUN apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client

RUN curl -L --output ./dotnet-install.sh https://dot.net/v1/dotnet-install.sh

RUN chmod u+x ./dotnet-install.sh 

# Install .NET 7 SDK
RUN ./dotnet-install.sh -c 7.0 -i /usr/share/dotnet

# Install .NET 6 SDK
RUN ./dotnet-install.sh -c 6.0 -i /usr/share/dotnet

# Generate and export the development certificate
RUN dotnet dev-certs https -ep /certs/cert.pem -p '' && \
    chmod 644 /certs/cert.pem

ENV ASPNETCORE_URLS="https://+;http://+" 
ENV ASPNETCORE_HTTPS_PORT=443 
ENV ASPNETCORE_Kestrel__Certificates__Default__Password="" 
ENV ASPNETCORE_Kestrel__Certificates__Default__Path=/certs/cert.pem

[raman-m]

@ggnaegi commented on Nov 30

I could... but...

Did you test this Docker script in your ggnaegi Docker Hub account? Did you make an image and test it? We will switch to ocelot2 account in .circleci/config.yml during merging... Now you need to test it.

Today is late... I will make test Docker image in future after your confirmation it is tested. Right before merging this PR I will make an image and upload to ocelot2 account. OK?

[raman-m]

@ggnaegi Pay attention that we use new - image: ocelot2/circleci-build:8.21.0 in develop! We go away from mijitt0m account! The author or you have to resolve merge conflicts. Then create test image in your account please as I said in prev message!

[raman-m]

@ibnuda @ggnaegi Who will resolve merge conflicts? 😉 Please merge develop! Also I'm curious, Is it possible to rebase onto develop? 🤣 see a lot of merge commits...

[raman-m]

@ggnaegi commented on Nov 30, 2023

I will try to find some time these days.... Seems both you and the author are blocked by Docker image upgrading.

[ggnaegi]

@raman-m I could create the images, but I don't have the needed permissions on ocelot2.

[raman-m]

@ggnaegi You want too much! :wink: My answer: https://ocelot-2.slack.com/archives/C0667CH7RPD/p1708412345257399 The quote:

Personal account doesn't allow to add Collaborators, unfortunately. Also, see Docker Pricing: https://www.docker.com/pricing/ Conclusion: To work as a team we must upgrade to Team subscription! And sure we have to convert current ocelot2 account to Organization one.

Please Note! Team subscription costs $11 per user monthly!