search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.32k stars 1.63k forks source link

#1672 Custom Default Version Policy #1673

Closed ibnuda closed 5 months ago

ibnuda commented 1 year ago

Closes #1672

New Feature

Configurable HttpRequestMessage.VersionPolicy.

Motivation for New Feature

The following setup:

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

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

raman-m commented 10 months ago

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 commented 10 months ago

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

ibnuda commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

hello raman. thanks, the conflict is resolved.

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

raman-m commented 10 months ago

@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 commented 10 months ago

@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

ggnaegi commented 10 months ago

@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

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 commented 10 months ago

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 commented 10 months ago

@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...

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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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

ggnaegi commented 10 months ago

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

ibnuda commented 10 months ago

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

raman-m commented 10 months ago

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

raman-m commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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

ggnaegi commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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

ggnaegi commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

raman-m commented 7 months ago

@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!

raman-m commented 7 months ago

Hi @ggnaegi @ibnuda ! I'm experiencing this Docker build error image Any ideas? Have you tried to build the docker file locally in terminal? Maybe something changed in last 3 months?

Please run local build and let me know your building results plz 👉 

docker build --platform linux/amd64 -t ocelot2/circleci-build -f Dockerfile.base .
raman-m commented 7 months ago

I've downloaded https://dl-cdn.alpinelinux.org/alpine/v3.19/community/x86_64/APKINDEX.tar.gz archive and unpacked it, Opened file APKINDEX in text editor and my search for libssl1.1 was failed. image

raman-m commented 7 months ago

Browsing to this "Official images for the .NET SDK" I'd say that 8.0-alpine tags for "Linux amd64 Tags" still exist but they are very fresh being updated in February, a week ago! So, I expect that something has really changed and our old build script doesn't work now. So, usage of 8.0-alpine tags forces to alpine/v3.19 downloading (the latest version). I will try to use old OS and tags, downgrading version and try Ubuntu base image aka 8.0-jammy tags...

I'm surprised that 8.0-alpine tags don't work now, but they did 3 months ago, on Nov 20, 2023, when I've made the latest build and uploaded to ocelor2 account.

Please, test with me this @ggnaegi build script more!

raman-m commented 7 months ago

I've tried to build using Ubuntu 8.0-jammy tags... And the situation is even worst: no apk app

Terminal Screenshot ![image](https://github.com/ThreeMammals/Ocelot/assets/10501504/f225660a-3908-4c21-a58e-640cf59017d0)
The Log ```sh $ docker build --platform linux/amd64 -t ocelot2/circleci-build -f Dockerfile.base . [+] Building 339.5s (5/10) docker:default => [internal] load .dockerignore 0.1s => => transferring context: 2B 0.0s => [internal] load build definition from Dockerfile.base 0.1s => => transferring dockerfile: 810B 0.0s => [internal] load metadata for mcr.microsoft.com/dotnet/sdk:8.0-jammy 3.5s => [1/7] FROM mcr.microsoft.com/dotnet/sdk:8.0-jammy@sha256:dc273e23006f85ef4bf154844a3147325c50adc4b5dac9191238bed4931743ac 334.1s => => resolve mcr.microsoft.com/dotnet/sdk:8.0-jammy@sha256:dc273e23006f85ef4bf154844a3147325c50adc4b5dac9191238bed4931743ac 0.0s => => sha256:dc273e23006f85ef4bf154844a3147325c50adc4b5dac9191238bed4931743ac 1.08kB / 1.08kB 0.0s => => sha256:cef43d7a511b5e8be73ae808387eb44b10cbecbceb803d51815f349cd44798e5 6.31kB / 6.31kB 0.0s => => sha256:d6da2f781aef9761cb3a5fc1dd3c8ab381d756ad9e63dc062ebb3e94c479ec70 2.22kB / 2.22kB 0.0s => => sha256:01007420e9b005dc14a8c8b0f996a2ad8e0d4af6c3d01e62f123be14fe48eec7 29.54MB / 29.54MB 22.0s => => sha256:f43949b92c055645d77181e9c69d43afd59f8dbae9240fbcc49aa6a6ecfd7fa6 16.71MB / 16.71MB 38.4s => => extracting sha256:01007420e9b005dc14a8c8b0f996a2ad8e0d4af6c3d01e62f123be14fe48eec7 7.1s => => extracting sha256:f43949b92c055645d77181e9c69d43afd59f8dbae9240fbcc49aa6a6ecfd7fa6 3.8s => => sha256:806af25bb63722bda0c992837991f30eab0d87822c726ed73182b809f8cbccb9 3.52kB / 3.52kB 39.2s => => sha256:0fd2412a534941d36163f7c1538c77dd4f25f20ddb4be86ca815b013a7be5618 32.22MB / 32.22MB 68.0s => => extracting sha256:806af25bb63722bda0c992837991f30eab0d87822c726ed73182b809f8cbccb9 0.0s => => sha256:57465db633926d8119ce5f512478092bc9f7369026d1a8e20bdb5ce3ab43478d 154B / 154B 68.6s => => extracting sha256:0fd2412a534941d36163f7c1538c77dd4f25f20ddb4be86ca815b013a7be5618 1.5s => => sha256:3516fa6f6787ebc34c55d511cdc7558a8cc2a9a3680f7f437cca29db56e2cc79 11.01MB / 11.01MB 84.1s => => extracting sha256:57465db633926d8119ce5f512478092bc9f7369026d1a8e20bdb5ce3ab43478d 0.0s => => extracting sha256:3516fa6f6787ebc34c55d511cdc7558a8cc2a9a3680f7f437cca29db56e2cc79 0.6s => => sha256:9f028a886f322a75a6471fde8d633361dd27ccf9e5c0c80bada82122369eaa21 20.77MB / 20.77MB 101.7s => => extracting sha256:9f028a886f322a75a6471fde8d633361dd27ccf9e5c0c80bada82122369eaa21 10.1s => => sha256:4900eda947318b2d47f46d2708a1e294d7bcae947d4a69739642177086e01655 187.60MB / 187.60MB 315.7s => => extracting sha256:4900eda947318b2d47f46d2708a1e294d7bcae947d4a69739642177086e01655 16.4s => => sha256:323602a23faff668fd9419ab692662b1df98bc9c580187e8b2ab2c28cfd622bd 15.71MB / 15.71MB 329.5s => => extracting sha256:323602a23faff668fd9419ab692662b1df98bc9c580187e8b2ab2c28cfd622bd 1.4s => ERROR [2/7] RUN apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client 1.7s ------ > [2/7] RUN apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client: 0.954 /bin/sh: 1: apk: not found ------ Dockerfile.base:3 -------------------- 1 | FROM mcr.microsoft.com/dotnet/sdk:8.0-jammy 2 | 3 | >>> RUN apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client 4 | 5 | RUN curl -L --output ./dotnet-install.sh https://dot.net/v1/dotnet-install.sh -------------------- ERROR: failed to solve: process "/bin/sh -c apk add bash icu-libs krb5-libs libgcc libintl libssl1.1 libstdc++ zlib git openssh-client" did not complete successfully: exit code: 127 ```

Well... Rolling back to 8.0-alpine tags...

raman-m commented 7 months ago

Anyway, this 3rd line of the script must be reviewed

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

Also you can get some ideas in alpine3.19 Dockerfile which is the latest version and current default base Docker file of 8.0-alpine tag.

ggnaegi commented 7 months ago

Anyway, this 3rd line of the script must be reviewed

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

Also you can get some ideas in alpine3.19 Dockerfile which is the latest version and current default base Docker file of 8.0-alpine tag.

I might just have written that example without checking it @raman-m

raman-m commented 7 months ago

I've rebased feature branch and resolve conflicts! The rest of gaps in diff I'll fix during next code review... Can't help you guys more... 😫

raman-m commented 7 months ago

@ggnaegi I used this branch to play with Docker build I'll create PR after your thorough testing of Docker script.

ggnaegi commented 5 months ago

@raman-m Ok, done, could you please build the new images?

raman-m commented 5 months ago

Rebased once again!

raman-m commented 5 months ago

@ggnaegi @ibnuda My congratulations! 🎉 The latest build is successful! ✅ This means that the new Docker container is functioning properly on CircleCI, and new SSL tests are passing successfully. Good job! 👍

raman-m commented 5 months ago

@ggnaegi @RaynaldM So, you both approved the PR... But if you don't mind, let's give a time to the author to fix issues from my code review

raman-m commented 5 months ago

We have DownstreamHttpVersion in docs And new section DownstreamVersionPolicy without Http prefix.

Why not to rename from DownstreamVersionPolicy to DownstreamHttpVersionPolicy❓

ggnaegi commented 5 months ago

We have DownstreamHttpVersion in docs And new section DownstreamVersionPolicy without Http prefix.

Why not to rename from DownstreamVersionPolicy to DownstreamHttpVersionPolicy❓

Yes, it makes sense, you're right

raman-m commented 5 months ago

🤣 LoL conflicts! 🤯

raman-m commented 5 months ago

@ggnaegi commented:

Yes, it makes sense, you're right

Renamed! See commits https://github.com/ThreeMammals/Ocelot/pull/1673/commits/15850610971304e852adb6dffab07816bb32f721 and https://github.com/ThreeMammals/Ocelot/pull/1673/commits/891bb164321450a60e14666135fafba1493f5a4f

raman-m commented 5 months ago

@ibnuda Congrats! 🥳

raman-m commented 4 months ago

Feature commit in develop → https://github.com/ThreeMammals/Ocelot/commit/176a4c8b2b84bb9b4bcacb03be112eeb1e8dfdc0