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

More Flexible VersionPolicy #1672

Closed
ibnuda opened this issue Jun 27, 2023 · 6 comments · Fixed by #1673
Closed

More Flexible VersionPolicy #1672

ibnuda opened this issue Jun 27, 2023 · 6 comments · Fixed by #1673
Assignees
Labels
Configuration Ocelot feature: Configuration feature A new feature merged Issue has been merged to dev and is waiting for the next release Spring'24 Spring 2024 release
Milestone

Comments

@ibnuda
Copy link
Contributor

ibnuda commented Jun 27, 2023

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
Copy link
Member

raman-m commented Jun 28, 2023

Dear Ibnu,
Why do you use v18 if the v19 is available?

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

I'm afraid Ocelot team doesn't support outdated versions and releases. We have no separate release branch for v18 because of the lack of resources. It is pretty hard to support even the current release version because of Tom's inactivity!
So, we have an intention to support current release version (v19) only and you need to make all tests and design your solution using the latest Ocelot version which is v19.

Finally,
What about upgrading to v19.0.2 ?

Pay attention that v18 (.NET 6) and v19 (.NET 7) target different .NET SDKs which can lead to different behavior of objects from System.Net namespace, the behavior of HttpClient and HttpRequest!

@raman-m raman-m added question Initially seen a question could become a new feature or bug or closed ;) waiting Waiting for answer to question or feedback from issue raiser labels Jun 28, 2023
@ibnuda
Copy link
Contributor Author

ibnuda commented Jun 29, 2023

hi, raman.

we're currently waiting for dotnet 8 to be released before we upgrade all services, including the gateway that uses ocelot.

So, we have an intention to support current release version (v19) only and you need to make all tests and design your solution using the latest Ocelot version which is v19.

that's also our plan. i failed to mention it in my first post.
and i will write the pr as soon as i have my computer within my reach. please expect the pr around friday this week or next monday at the latest.

thanks for the attention and cooperation in advance.

@ibnuda ibnuda closed this as completed Jun 29, 2023
@raman-m
Copy link
Member

raman-m commented Jun 29, 2023

Why did you close the issue?

With 99% probability the bug exists in .NET 7 release aka v19.0.2.
We can backport the fix from current develop branch after delivery, to the some feature branch with old v18 commits. This is the question of feature management/delivery.

So, I believe we can still collaborate on your bug/issue for v18 to apply the final fix for both versions: v18 and v19.
What do you think?

But sure, you need to understand, there will be no additional v18 release because I'm not owner to manage Ocelot efficiently.
So, there will be no NuGet package v.18.0.1 with the fix, current .NET 6 release is v18.0.0
You will have to use the code from the bug fix branch and compile the Ocelot project manually targeting .NET 6...

Please, reopen the issue if you have an intention to contribute soon!

@ibnuda
Copy link
Contributor Author

ibnuda commented Jun 30, 2023

sorry, i closed this issue because i thought we reached an agreement that:

  • this feature is welcome
  • i will make a pr for this feature
  • i will base the pr on develop branch.
  • there's no need to backport this feature to v18 branch.

personally, i don't mind using the latest version of Ocelot.

and re-opening this issue.

@ibnuda ibnuda reopened this Jun 30, 2023
ibnuda added a commit to ibnuda/Ocelot that referenced this issue Jun 30, 2023
@raman-m raman-m added needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance accepted Bug or feature would be accepted as a PR or is being worked on and removed question Initially seen a question could become a new feature or bug or closed ;) waiting Waiting for answer to question or feedback from issue raiser labels Jun 30, 2023
@raman-m
Copy link
Member

raman-m commented Jun 30, 2023

+ Accepted

A solution is ready in PR #1673

@raman-m raman-m added feature A new feature and removed needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance labels Jun 30, 2023
@raman-m raman-m added the 2023 Annual 2023 release label Jan 7, 2024
@raman-m raman-m added this to the Annual 2023 milestone Jan 7, 2024
raman-m pushed a commit to ibnuda/Ocelot that referenced this issue Feb 20, 2024
raman-m added a commit that referenced this issue Apr 3, 2024
* version 8.21.0

* New 8.23 version with `dotnet dev-certs` command

* Publish `latest` and `8.23.2` tags
ggnaegi added a commit that referenced this issue Apr 19, 2024
* feature written, tests passed

* actualy passes almost all the test.

* resolve conflict, hopefully.

* please.

* let it cook.

* uses constants instead of string for version policies.

* conflict res

* swapped downstream method and version.

* #1731 Read the Docs configuration file v2 (#1733)

* fixing the documentation, using Release/20.0 as base branch

* using latest conf.py, created with sphinx-quickstart, fixing the warnings during documentation generation

* Update .readthedocs.yaml

* switching to threemammals.org for copyright

* adding requirements file, updating readthedocs.yaml, adding formats pdf / epub and config for requirements file

* fixing code block in websockets.rst

* ok, now it should be fine...

* Update kubernetes.rst: Review and fix markup code

* Update websockets.rst: Review and fix markup

* Update conf.py: Update release, author and copyright

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* * When using the QoS option "ExceptionsAllowedBeforeBreaking" the circuit breaker never opens the circuit.

* merge issue, PortFinder

* some code improvements, using httpresponsemessage status codes as a base for circuit breaker

* Adding more unit tests, and trying to mitigate the test issues with the method "GivenThereIsAPossiblyBrokenServiceRunningOn"

* fixing some test issues

* setting timeout value to 5000 to avoid side effects

* again timing issues

* timing issues again

* ok, first one ok

* Revert "ok, first one ok"

This reverts commit 2e4a673.

* inline method

* putting back logging for http request exception

* removing logger configuration, back to default

* adding a bit more tests to check the policy wrap

* Removing TimeoutStrategy from parameters, it's set by default to pessimistic, at least one policy will be returned, so using First() in circuit breaker and removing the branch Policy == null from delegating handler.

* Fix StyleCop warnings

* Format parameters

* Sort usings

* since we might have two policies wrapped,  timeout and circuit breaker, we can't use the name CircuitBreaker for polly qos provider, it's not right. Using PollyPolicyWrapper and AsnycPollyPolicy instead.

* modifying circuit breaker delegating handler name, usin Polly policies instead

* renaming CircuitBreakerFactory to PolicyWrapperFactory in tests

* DRY for FileConfiguration, using FileConfigurationFactory

* Add copy constructor

* Refactor setup

* Use expression body for method

* Fix acceptance test

* IDE1006 Naming rule violation: These words must begin with upper case characters

* CA1816 Change ReturnsErrorTests.Dispose() to call GC.SuppressFinalize(object)

* Sort usings

* Use expression body for method

* Return back named arguments

---------

Co-authored-by: raman-m <dotnet044@gmail.com>

* feature written, tests passed

* actualy passes almost all the test.

* resolve conflict, hopefully.

* missed this one.

* please.

* come on...

* let it build.

* let it cook.

* copied from main branch.

* conflict res

* resolving conflicts.

* another attempt.

* lf

* re-incorporate downstream version policy.

* renamed the version policies and added acceptance tests.

* trust the dotnet dev cert.

* accepts cert from dotnet.

* Fix compiling errors

* Refactor tests

* a bit of code cleanup, removing some usings

* a bit more cleanup in fileroute

* try and error with the tests

* "Yahoo!...", said @ibnuda :)

* FileRoute: let it go...
Binary copy! :LoL:

* FileRoute: let it cook...
Re-add sweet props

* `dotnet dev-certs` for the `build` job

* Recover `kubernetes.rst`

* docs/make.bat original version

* OcelotBuilderExtensions

* original src/Ocelot.Provider.Polly/v7/PollyPolicyWrapper.cs

* `IVersionPolicyCreator` XML docs

* Code review by @raman-m (part 1)

* RequestMapper : care about diff

* Code review by @raman-m (part 2)

* Fix Should_return_OK_status_and_multiline_indented_json_response_with_json_options_for_custom_builder

* Update configuration.rst

Add DownstreamVersionPolicy section

* Update docs

* Rename `DownstreamVersionPolicy` to `DownstreamHttpVersionPolicy`

* update docs after prop renaming

* Sort props

---------

Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

Implemented by PR #1673
Commit: 140f9b5

@raman-m raman-m closed this as completed Apr 19, 2024
@raman-m raman-m removed the accepted Bug or feature would be accepted as a PR or is being worked on label Apr 19, 2024
@raman-m raman-m added the merged Issue has been merged to dev and is waiting for the next release label Apr 19, 2024
@raman-m raman-m added Spring'24 Spring 2024 release and removed 2023 Annual 2023 release labels May 22, 2024
@raman-m raman-m modified the milestones: Annual 2023, Spring'24 May 22, 2024
raman-m added a commit that referenced this issue May 23, 2024
* feature written, tests passed

* actualy passes almost all the test.

* resolve conflict, hopefully.

* please.

* let it cook.

* uses constants instead of string for version policies.

* conflict res

* swapped downstream method and version.

* #1731 Read the Docs configuration file v2 (#1733)

* fixing the documentation, using Release/20.0 as base branch

* using latest conf.py, created with sphinx-quickstart, fixing the warnings during documentation generation

* Update .readthedocs.yaml

* switching to threemammals.org for copyright

* adding requirements file, updating readthedocs.yaml, adding formats pdf / epub and config for requirements file

* fixing code block in websockets.rst

* ok, now it should be fine...

* Update kubernetes.rst: Review and fix markup code

* Update websockets.rst: Review and fix markup

* Update conf.py: Update release, author and copyright

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* * When using the QoS option "ExceptionsAllowedBeforeBreaking" the circuit breaker never opens the circuit.

* merge issue, PortFinder

* some code improvements, using httpresponsemessage status codes as a base for circuit breaker

* Adding more unit tests, and trying to mitigate the test issues with the method "GivenThereIsAPossiblyBrokenServiceRunningOn"

* fixing some test issues

* setting timeout value to 5000 to avoid side effects

* again timing issues

* timing issues again

* ok, first one ok

* Revert "ok, first one ok"

This reverts commit 2e4a673.

* inline method

* putting back logging for http request exception

* removing logger configuration, back to default

* adding a bit more tests to check the policy wrap

* Removing TimeoutStrategy from parameters, it's set by default to pessimistic, at least one policy will be returned, so using First() in circuit breaker and removing the branch Policy == null from delegating handler.

* Fix StyleCop warnings

* Format parameters

* Sort usings

* since we might have two policies wrapped,  timeout and circuit breaker, we can't use the name CircuitBreaker for polly qos provider, it's not right. Using PollyPolicyWrapper and AsnycPollyPolicy instead.

* modifying circuit breaker delegating handler name, usin Polly policies instead

* renaming CircuitBreakerFactory to PolicyWrapperFactory in tests

* DRY for FileConfiguration, using FileConfigurationFactory

* Add copy constructor

* Refactor setup

* Use expression body for method

* Fix acceptance test

* IDE1006 Naming rule violation: These words must begin with upper case characters

* CA1816 Change ReturnsErrorTests.Dispose() to call GC.SuppressFinalize(object)

* Sort usings

* Use expression body for method

* Return back named arguments

---------

Co-authored-by: raman-m <dotnet044@gmail.com>

* feature written, tests passed

* actualy passes almost all the test.

* resolve conflict, hopefully.

* missed this one.

* please.

* come on...

* let it build.

* let it cook.

* copied from main branch.

* conflict res

* resolving conflicts.

* another attempt.

* lf

* re-incorporate downstream version policy.

* renamed the version policies and added acceptance tests.

* trust the dotnet dev cert.

* accepts cert from dotnet.

* Fix compiling errors

* Refactor tests

* a bit of code cleanup, removing some usings

* a bit more cleanup in fileroute

* try and error with the tests

* "Yahoo!...", said @ibnuda :)

* FileRoute: let it go...
Binary copy! :LoL:

* FileRoute: let it cook...
Re-add sweet props

* `dotnet dev-certs` for the `build` job

* Recover `kubernetes.rst`

* docs/make.bat original version

* OcelotBuilderExtensions

* original src/Ocelot.Provider.Polly/v7/PollyPolicyWrapper.cs

* `IVersionPolicyCreator` XML docs

* Code review by @raman-m (part 1)

* RequestMapper : care about diff

* Code review by @raman-m (part 2)

* Fix Should_return_OK_status_and_multiline_indented_json_response_with_json_options_for_custom_builder

* Update configuration.rst

Add DownstreamVersionPolicy section

* Update docs

* Rename `DownstreamVersionPolicy` to `DownstreamHttpVersionPolicy`

* update docs after prop renaming

* Sort props

---------

Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m raman-m added the Configuration Ocelot feature: Configuration label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration feature A new feature merged Issue has been merged to dev and is waiting for the next release Spring'24 Spring 2024 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants