Skip to content

Conversation

@prochnowc
Copy link
Contributor

@prochnowc prochnowc commented Sep 4, 2024

Allow specifying the HTTP protocol version and version policy

Allow specifying the HTTP protocol version and version policy when retrieving documents from eg. discovery endpoints.

Description

The PR allows users of HttpDocumentRetriever (for example the ConfigurationManager) to specify the HTTP version and version policy used when sending HTTP requests.

The version and policy can bei either specified explicitly via properties on HttpDocumentRetrieve or implicitly via HttpClient.

When using .NET 6 and no explicit values have been set, the default version and policy from the HttpClient is used.
This allows users of OpenID connect authentication to easily setup the HTTP version and policy by configuring the HttpClient of the Backchannel.

Fixes #2808

@prochnowc prochnowc requested a review from a team as a code owner September 4, 2024 09:05
@prochnowc
Copy link
Contributor Author

@microsoft-github-policy-service agree

@prochnowc
Copy link
Contributor Author

Also fixes #1980

@pmaytak pmaytak linked an issue Sep 23, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this feature needed?
What are the pain points? What are the scenarios?

@pmaytak pmaytak self-requested a review January 1, 2025 06:10
Copy link
Contributor

@pmaytak pmaytak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the configuration with HTTP 2 doesn't seem to work and returns this error:

System.Net.Http.HttpRequestException: 'Requesting HTTP version 2.0 with version policy RequestVersionExact while unable to establish HTTP/2 connection.'

I used this test case and set version to 2.0 and policy to RequestVersionExact.

@pmaytak pmaytak self-requested a review January 1, 2025 06:10
@prochnowc
Copy link
Contributor Author

@pmaytak

Getting the configuration with HTTP 2 doesn't seem to work and returns this error:

System.Net.Http.HttpRequestException: 'Requesting HTTP version 2.0 with version policy RequestVersionExact while unable to establish HTTP/2 connection.'

I used this test case and set version to 2.0 and policy to RequestVersionExact.

I added

                #if NET6_0_OR_GREATER
                documentRetriever.HttpVersionPolicy = HttpVersionPolicy.RequestVersionExact;
                documentRetriever.HttpVersion = new Version(2,0);
                #endif

after Line 90 and all tests are green for me.

@pmaytak
Copy link
Contributor

pmaytak commented Jan 3, 2025

@pmaytak

Getting the configuration with HTTP 2 doesn't seem to work and returns this error:

System.Net.Http.HttpRequestException: 'Requesting HTTP version 2.0 with version policy RequestVersionExact while unable to establish HTTP/2 connection.'

I used this test case and set version to 2.0 and policy to RequestVersionExact.

I added

                #if NET6_0_OR_GREATER
                documentRetriever.HttpVersionPolicy = HttpVersionPolicy.RequestVersionExact;
                documentRetriever.HttpVersion = new Version(2,0);
                #endif

after Line 90 and all tests are green for me.

Hmm. GetMetadataTest fails for me on NET 6/8/9 with the above error; succeeds on NET 462/472. However, HttpVersionTest fails on NET 462/472 because HttpResponseMesssage.Content is null; succeeds on NET6/8/9 because Content defaults to EmptyContent. I ran the tests through VS Test Explorer and dotnet test. Did you run the tests on all frameworks?

@prochnowc
Copy link
Contributor Author

prochnowc commented Jan 3, 2025

I fixed HttpVersionTest.

About the HTTP/2 exception, are you behind a corporate firewall which disallows HTTP/2 connections?
Can you try Invoke-WebRequest -HttpVersion 2 https://login.microsoftonline.com/common/.well-known/openid-configuration in PowerShell and see if RawContent contains HTTP/2.0 200 OK ?

@prochnowc
Copy link
Contributor Author

I fixed HttpVersionTest.

About the HTTP/2 exception, are you behind a corporate firewall which disallows HTTP/2 connections? Can you try Invoke-WebRequest -HttpVersion 2 https://login.microsoftonline.com/common/.well-known/openid-configuration in PowerShell and see if RawContent contains HTTP/2.0 200 OK ?

Any news @pmaytak ?

@pmaytak
Copy link
Contributor

pmaytak commented Jan 14, 2025

@prochnowc Can you please provide details for jmprieur's quesitons:

#2809 (review)

@prochnowc
Copy link
Contributor Author

Why is this feature needed? What are the pain points? What are the scenarios?

This feature allows using HTTP/2 for discovery endpoints. HTTP/2's multiplexing allows multiple metadata requests (e.g., for .well-known/openid-configuration or JSON Web Keys (JWKs)) to be handled concurrently without needing multiple connections. It also reduces the size of the HTTP headers (because of compression) used in OIDC requests and responses, leading to faster exchanges.

These optimizations minimize redundant network traffic, reduce the number of required connections, and compress transmitted data.

@prochnowc
Copy link
Contributor Author

@pmaytak Any issues left?

@pmaytak
Copy link
Contributor

pmaytak commented Jan 28, 2025

I fixed HttpVersionTest.

About the HTTP/2 exception, are you behind a corporate firewall which disallows HTTP/2 connections? Can you try Invoke-WebRequest -HttpVersion 2 https://login.microsoftonline.com/common/.well-known/openid-configuration in PowerShell and see if RawContent contains HTTP/2.0 200 OK ?

The tests worked on my local machine. On my VM the PowerShell command was successful but the tests still failed. Not sure why.

@prochnowc prochnowc requested a review from jmprieur January 28, 2025 09:59
/// <summary>
/// If set specifies the protocol version to use when sending HTTP requests.
/// </summary>
public Version HttpVersion { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need more documentation on this? ex in the wiki?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could create a separate PR for the Wiki and add a page explaining how to enable HTTP/2. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whichever works for you. We can also take the content in a comment here and copy it over for you

Copy link
Contributor

@pmaytak pmaytak Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can create a PR for a wiki?

If you paste the content here (how to use this feature), I can create a wiki page for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Enable using HTTP/2 for discovery endpoints [Bug] Can not call discovery and retrieve metadata via HTTP/2 any more

5 participants