Skip to content

Add HostPolicyMatcher #6214

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 10, 2019
Merged

Add HostPolicyMatcher #6214

merged 1 commit into from
Jan 10, 2019

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 1, 2019 •
edited
Loading

Add HostPolicyMatcher + tests

Lets users map endpoints to specific hosts. Supports domain, port, and wildcards.
Will let healthz endpoint to support a specific port like current middleware allows - #5127 (comment)

Should HostAttribute be an attribute or a POCO? Attribute doesn't feel as clean to use in code, and other metadata in this project are POCOs. An attribute would support placing metadata on MVC controllers/actions. An alternative is a POCO in routing and then MVC have an attribute implementing the interface. I made it a POCO to be consistent but I'm on the fence. AuthorizeAttribute is defined in Microsoft.AspNetCore.Authorization. Happy to change it back so it can easily be used on MVC controllers/actions.

Addresses #5957

@JamesNK JamesNK requested a review from rynowak January 1, 2019 22:19
@JamesNK JamesNK mentioned this pull request Jan 4, 2019
Merged
@JamesNK JamesNK force-pushed the jamesnk/hostmatcherpolicy branch from e6db606 to 1c85ac6 Compare January 5, 2019 19:47
@JamesNK JamesNK changed the title [WIP] Add HostPolicyMatcher Add HostPolicyMatcher Jan 5, 2019
@JamesNK
Copy link
Member Author

JamesNK commented Jan 5, 2019

🆙 📅

rynowak
rynowak reviewed Jan 7, 2019
View reviewed changes
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

This is pretty dope

Tratcher
Tratcher reviewed Jan 7, 2019
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

High level item: doc all inputs as expecting Unicode (not punycode).

@JamesNK JamesNK force-pushed the jamesnk/hostmatcherpolicy branch from a59b15d to 7699884 Compare January 8, 2019 01:41
@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2019

🆙 📅

@Tratcher
Copy link
Member

Tratcher commented Jan 8, 2019

Also consider the scenario of missing host: #5909

@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2019 •
edited
Loading

Some missing host tests - f7c3047

@Tratcher What does Kestrel do with unicode host headers? Does it translate Punycode back to unicode in HttpRequest.Host? Edit: Answer was in the HostString xml docs 😄

@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2019

🆙 📅

@JamesNK JamesNK closed this Jan 10, 2019
@JamesNK JamesNK reopened this Jan 10, 2019
@JamesNK JamesNK force-pushed the jamesnk/hostmatcherpolicy branch from fd323c3 to 487b81b Compare January 10, 2019 20:44
@JamesNK JamesNK merged commit 90511e6 into master Jan 10, 2019
@natemcmaster natemcmaster deleted the jamesnk/hostmatcherpolicy branch January 18, 2019 19:17
@VictorioBerra VictorioBerra mentioned this pull request Jun 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynowak rynowak rynowak approved these changes

@Tratcher Tratcher Tratcher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JamesNK @Tratcher @rynowak