Source server variables from IIS when running ANCM in-proc #10022

mderriey · 2019-05-07T01:45:34Z

Source server variables from IIS when running ANCM in-proc

Addresses #6013

mderriey · 2019-05-07T01:48:07Z

This is the first version to get something out there before asking too many questions.

Right now it doesn't honor the UriMatchPart parameter and returns IIS variables as-is; do we want to support that to not break existing code too much?
Also, do we want to update the error message "Rules using the '{0}' server variable are not supported" and give a hint that it would work if hosted in-proc? See https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/Rewrite/src/Properties/Resources.Designer.cs#L205-L219

Let me know if we'd like to support that or if you see anything else.

analogrelay

Looks OK to me. Minor suggestion around using a real type instead of a mock. Also not sure about adding the dependency. Now that it's one big happy <FrameworkReference>, it's probably not a big deal, but having to depend on Servers.IIS from Rewrite feels a little off.

src/Middleware/Rewrite/ref/Microsoft.AspNetCore.Rewrite.csproj

src/Middleware/Rewrite/test/IISUrlRewrite/ServerVariableTests.cs

analogrelay

Looks OK to me. Minor suggestion around using a real type instead of a mock. Also not sure about adding the dependency. Now that it's one big happy <FrameworkReference>, it's probably not a big deal, but having to depend on Servers.IIS from Rewrite feels a little off.

src/Middleware/Rewrite/src/Internal/PatternSegments/IISServerVariableSegment.cs

src/Middleware/Rewrite/src/Internal/IISUrlRewrite/ServerVariables.cs

src/Middleware/Rewrite/src/IISUrlRewriteOptionsExtensions.cs

davidfowl · 2019-05-10T06:28:38Z

I think we should move this feature to the features assembly in Http/*

mderriey · 2019-05-10T10:04:13Z

Hey @davidfowl thanks for jumping in.

What feature are you referring to exactly?

Tratcher · 2019-05-10T15:17:29Z

@mderriey he's talking about moving IServerVariablesFeature so we can avoid the IIS server dependency. We'd also need to move GetIISServerVariable, or not use it in this code path.

analogrelay · 2019-05-10T16:27:12Z

That would be a breaking change, but I think I'm OK with it. It seems reasonable. Server Variables aren't actually an IIS-specific concept anyway. They date back to CGI so it seems reasonable that other servers may want to provide them.

If we do this, I'd consider renaming the extension method to GetServerVariable instead of GetIISServerVariable.

davidfowl · 2019-05-10T16:50:49Z

That would be a breaking change, but I think I'm OK with it. It seems reasonable. Server Variables aren't actually an IIS-specific concept anyway. They date back to CGI so it seems reasonable that other servers may want to provide them.

Exactly!

If we do this, I'd consider renaming the extension method to GetServerVariable instead of GetIISServerVariable.

Right! I think that's what we want to do for 3.0. Lets not hold up this PR as a result tho. File an issue and refactor this since most of these changes are related to the rewrite middleware.

mderriey · 2019-05-10T21:28:39Z

Thanks for the explanation/context.

I'd be happy to try and implement the final solution to expand my knowledge of the codebase.

Could you please tag me in the resulting issue when it's created?

analogrelay · 2019-05-10T22:33:15Z

Filing a bug now :)

analogrelay · 2019-05-10T22:35:43Z

#10167

Feel free to move discussion on the subject of moving/renaming the feature to that issue.

mderriey · 2019-05-11T04:52:48Z

Great, thanks. I'll wait until this PR is merged to start the move.

Thanks again.

analogrelay · 2019-05-11T15:53:10Z

Code Check failed but I can't get AzDO to load. Usually this means you've got reference assemblies that are out of date. See https://github.com/aspnet/AspNetCore/blob/master/docs/ReferenceAssemblies.md for instructions on updating.

analogrelay · 2019-05-11T15:53:56Z

Got it to load, yep, just need to update the refasms.

error: Generated code is not up to date in src/Middleware/Rewrite/ref/Microsoft.AspNetCore.Rewrite.netcoreapp3.0.cs. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md)

Fixes dotnet#6013

jkotalik

Nits at this point 😄

Looks good, we can handle moving the feature to Http in a second PR.

I also want to do some E2E verification that this works when running inprocess.

jkotalik · 2019-05-13T18:37:42Z

src/Middleware/Rewrite/src/IISUrlRewriteOptionsExtensions.cs

@@ -19,7 +19,8 @@ public static class IISUrlRewriteOptionsExtensions
        /// <param name="options">The <see cref="RewriteOptions"/></param>
        /// <param name="fileProvider">The <see cref="IFileProvider"/> </param>
        /// <param name="filePath">The path to the file containing UrlRewrite rules.</param>
-        public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath)
+        /// <param name="useNativeIISServerVariables">Server variables are sourced natively from IIS if the application uses the in-process hosting model; use <c>false</c> to disable that behavior</param>
+        public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath, bool useNativeIISServerVariables = true)


Suggested change

public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath, bool useNativeIISServerVariables = true)

public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath, bool alwaysUseManagedServerVariables = false)

Generally we prefer bool defaults to be false.

No worries, thanks for pointing that out. I pushed extra commits to address that.

jkotalik · 2019-05-13T18:39:22Z

src/Middleware/Rewrite/test/IISUrlRewrite/TestServerVariablesFeature.cs

@@ -0,0 +1,21 @@
+using System.Collections.Generic;


nit copyright.

jkotalik · 2019-05-14T18:15:30Z

There are still test failures and I believe you need to re update references. That's the reason we are working on making things internal.

We inverted the meaning of the new boolean parameter

mderriey · 2019-05-14T22:24:16Z

Hey @jkotalik, sorry about that. I forgot to invert the boolean values after we changed the meaning of the parameter. I also regenerated reference assemblies.

Sorry that this one drags out so much, I keep forgetting about the ref assemblies.

Thanks for your time, I'll keep an eye on the outcomes of the builds.

jkotalik · 2019-05-15T15:26:52Z

Thanks @mderriey

mderriey requested review from analogrelay, jkotalik and Tratcher as code owners May 7, 2019 01:45

Eilon added the area-servers label May 7, 2019

analogrelay reviewed May 7, 2019

View reviewed changes

src/Middleware/Rewrite/ref/Microsoft.AspNetCore.Rewrite.csproj Show resolved Hide resolved

src/Middleware/Rewrite/test/IISUrlRewrite/ServerVariableTests.cs Outdated Show resolved Hide resolved

analogrelay reviewed May 7, 2019

View reviewed changes

analogrelay added this to the 3.0.0-preview6 milestone May 7, 2019

jkotalik self-assigned this May 7, 2019

jkotalik reviewed May 7, 2019

View reviewed changes

src/Middleware/Rewrite/src/Internal/PatternSegments/IISServerVariableSegment.cs Outdated Show resolved Hide resolved

jkotalik reviewed May 7, 2019

View reviewed changes

src/Middleware/Rewrite/src/Internal/IISUrlRewrite/ServerVariables.cs Outdated Show resolved Hide resolved

mderriey force-pushed the 6013-source-server-variables-from-iis-when-running-ancm-in-proc branch from 4b997b1 to 2e90d8f Compare May 8, 2019 02:21

jkotalik reviewed May 8, 2019

View reviewed changes

src/Middleware/Rewrite/src/IISUrlRewriteOptionsExtensions.cs Outdated Show resolved Hide resolved

mderriey force-pushed the 6013-source-server-variables-from-iis-when-running-ancm-in-proc branch from 9fedfd5 to 0333d78 Compare May 9, 2019 10:17

mderriey mentioned this pull request May 11, 2019

Move IServerVariables feature and friends to Http.Abstractions #10167

Closed

mderriey added 4 commits May 12, 2019 13:57

Source server variables from IIS when running ANCM in-proc

304e41c

Fixes dotnet#6013

Update project references for Microsoft.AspNetCore.Rewrite

71dcfee

Address initial PR feedback

0a0694c

Add a parameter to opt out of native IIS server variables

8ed9634

mderriey added 2 commits May 12, 2019 13:57

Enable native IIS server variables by default

ab27657

Update reference assemblies

3845ca8

mderriey force-pushed the 6013-source-server-variables-from-iis-when-running-ancm-in-proc branch from 0333d78 to 3845ca8 Compare May 12, 2019 04:18

jkotalik approved these changes May 13, 2019

View reviewed changes

mderriey added 2 commits May 14, 2019 08:04

Add copyright

f8ec5b2

Make boolean parameter false by default

a9b9ef8

mderriey added 2 commits May 15, 2019 08:19

Generate reference assemblies

20aad11

Fix failing tests

ac49cf1

We inverted the meaning of the new boolean parameter

jkotalik merged commit 4a2bd2a into dotnet:master May 15, 2019

analogrelay mentioned this pull request May 15, 2019

Have url_rewrite middleware resolve server variables from IIS when running ANCM in-proc #6013

Closed

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 6, 2023

	public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath, bool useNativeIISServerVariables = true)
	public static RewriteOptions AddIISUrlRewrite(this RewriteOptions options, IFileProvider fileProvider, string filePath, bool alwaysUseManagedServerVariables = false)

Source server variables from IIS when running ANCM in-proc #10022

Source server variables from IIS when running ANCM in-proc #10022

Uh oh!

Conversation

mderriey commented May 7, 2019

Uh oh!

mderriey commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl commented May 10, 2019

Uh oh!

mderriey commented May 10, 2019

Uh oh!

Tratcher commented May 10, 2019 • edited by analogrelay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay commented May 10, 2019

Uh oh!

davidfowl commented May 10, 2019

Uh oh!

mderriey commented May 10, 2019

Uh oh!

analogrelay commented May 10, 2019

Uh oh!

analogrelay commented May 10, 2019

Uh oh!

mderriey commented May 11, 2019

Uh oh!

analogrelay commented May 11, 2019

Uh oh!

analogrelay commented May 11, 2019

Uh oh!

jkotalik left a comment

Choose a reason for hiding this comment

Uh oh!

jkotalik May 13, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik May 13, 2019

Choose a reason for hiding this comment

Uh oh!

mderriey May 13, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik May 13, 2019

Choose a reason for hiding this comment

Uh oh!

mderriey May 13, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik commented May 14, 2019

Uh oh!

mderriey commented May 14, 2019

Uh oh!

jkotalik commented May 15, 2019

Uh oh!

Uh oh!

mderriey commented May 7, 2019 •

edited

Loading

Tratcher commented May 10, 2019 •

edited by analogrelay

Loading