Add PEM config info to Kestrel doc by JamesNK · Pull Request #20913 · dotnet/AspNetCore.Docs

JamesNK · 2020-12-10T00:53:49Z

Fixes #19113
Fixes dotnet/aspnetcore#26240

JamesNK · 2020-12-10T01:04:36Z

My change currently adds PEM to just .NET Core 3 docs. Should be changed to just .NET 5 docs. I don't know the right way to do that in this file. There seems like A LOT of duplication. I think this file needs to be refactored to support versioning without some much duplication.

halter73 · 2020-12-10T20:36:28Z

Review link: https://review.docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel?view=aspnetcore-5.0&branch=pr-en-us-20913

Does something like this fix the moniker ranges for now? https://gist.github.com/halter73/464ad810feab0c3857e1b49252fdb8b9 I'm not sure how to test the output without submitting a PR.

I completely agree that kestrel.md is in severe need of refactoring. I hope to fix this properly and remove most of the unnecessary duplication when I update the docs to include other .NET 5 Kestrel config changes like SNI. If you want to do some deduping this as part of this PR, I'd be super thankful though.

guardrex · 2020-12-11T19:03:02Z

FYI on the duplication: It's by-design. We call it "whole-topic versioning." It was required back in the 2.x days, when there were three supported versions of Kestrel content under wildly differing framework feature sets. The versioning tags (moniker ranges) were being flipped into and out of every couple of lines or paragraphs ... it was 😵 insanely hard to maintain 😵. We had to do it back then. However, that was then ... and this is now ... so hopefully you'll be successful. I'm just letting you know that it had to be that way back then. We had no choice on it ... the doc was a 👹 BeAsT 👹 that ate authors for lunch! 😨

JamesNK · 2020-12-14T02:32:55Z

What should happen here to support these .NET 5 specific properties? More duplicate?

guardrex · 2020-12-14T11:12:23Z

Yes ... an entire duplicate of the doc (the 3.x version) becomes the new 5.x doc/version with added 5.x sample(s), if needed. This new moniker-range versioned content goes at the top above the existing 3.x content.

At some point (perhaps now), the 2.x block of content and any 2.x samples that exist get the chop 🔪. There are two maintenance benefits of whole topic versioning ... first was what I was saying about not flipping in and out of versions ... this is the second benefit ... we can just chop all of a version's content in a few quick edits.

You see the major downside ... multiple updates to content are required when something changes across versions.

In the future if we get version by file, we'd use the same process as we use for whole topic versioning, except the 3.x content would be duplicated to a new 5.x file, then the new file would be updated. Scott said he'd look further into version by file for the repo in 2021.

I hope we get version by file because we've been hitting section header build warnings and cross-linking problems with our whole topic versioning approach. The build system flakes out over duplicate headers in a markdown file, and cross-links to content (bookmarks) can break. Whole topic versioning isn't what doc system managers and engineers envisioned or currently support for moniker range versioning. I've avoided problems in two Blazor docs by using explicit ids on explicit heading tags. For an example, see the headers in ...

https://github.com/dotnet/AspNetCore.Docs/edit/master/aspnetcore/blazor/blazor-server-ef-core.md

... compare Line 23 to Line 154.

I don't care for having to do it, but it works. No build warnings that way. I avoid broken bookmarks by not linking directly to sections. In a few spots where needed, I tell the reader, '... see the *{SECTION HEADING TITLE}* of the <xref:{UID}> article.'

IIRC, @Rick-Anderson has another approach, possibly better than what I'm doing.

Anyway ... with version by file, there would be no dup heading problem. It's designed to solve our flipping in and out of versions and our 🔪 content when a version drops scenarios.

I'll cc: @scottaddie here to correct any mistakes in my remarks and/or advise further.

Rick-Anderson · 2020-12-14T20:07:39Z

What should happen here to support these .NET 5 specific properties? More duplicate?

I prefer to not duplicate IF you can keep the moniker pairs to 6 or under. That's not a hard number either. Once you get lots of moniker pairs, it becomes too fragile and error prone. At that point, resort to our old duplicate versioning:

::: moniker range=">= aspnetcore-5.0"
// copy of 3.x version, updated to 5.x
::: moniker-end

::: moniker range=">= aspnetcore-3.0 < aspnetcore-5.0"
// 3.1 version
::: moniker-end

cc @wadepickett @scottaddie

blowdart · 2021-01-04T15:31:09Z

-*Kestrel support for SNI*
+#### Kestrel support for SNI

 [Server Name Indication (SNI)](https://tools.ietf.org/html/rfc6066#section-3) can be used to host multiple domains on the same IP address and port. For SNI to function, the client sends the host name for the secure session to the server during the TLS handshake so that the server can provide the correct certificate. The client uses the furnished certificate for encrypted communication with the server during the secure session that follows the TLS handshake.


Provided rather than furnished? Easier to understand

blowdart · 2021-01-04T15:33:15Z

+  * **Path**&ndash;**Password**
+  * **Path**&ndash;**KeyPath**&dash;**Password**
+  * **Subject**&ndash;**Store**
+* Any number of endpoints may be defined in this way so long as they don't cause port conflicts.


port conflicts needs explaining.

blowdart · 2021-01-04T15:34:04Z

+
+* Endpoints names are case-insensitive. For example, `HTTPS` and `Https` are valid.
+* The `Url` parameter is required for each endpoint. The format for this parameter is the same as the top-level `Urls` configuration parameter except that it's limited to a single value.
+* These endpoints replace those defined in the top-level `Urls` configuration rather than adding to them. Endpoints defined in code via `Listen` are cumulative with the endpoints defined in the configuration section.


Needs an example. I know what it means, but would all readers?

Just checking, is it literally Urls or is it the plural of Url? This needs to be a literal and you can't make literals plural by tacking on a s

blowdart · 2021-01-04T15:34:43Z

+Schema notes:
+
+* Endpoints names are case-insensitive. For example, `HTTPS` and `Https` are valid.
+* The `Url` parameter is required for each endpoint. The format for this parameter is the same as the top-level `Urls` configuration parameter except that it's limited to a single value.


Can you link back to the Urls parameter so people can easily see the format? If not, duplicate the format here.

blowdart · 2021-01-04T15:35:12Z

+* Endpoints names are case-insensitive. For example, `HTTPS` and `Https` are valid.
+* The `Url` parameter is required for each endpoint. The format for this parameter is the same as the top-level `Urls` configuration parameter except that it's limited to a single value.
+* These endpoints replace those defined in the top-level `Urls` configuration rather than adding to them. Endpoints defined in code via `Listen` are cumulative with the endpoints defined in the configuration section.
+* The `Certificate` section is optional. If the `Certificate` section isn't specified, the defaults defined in earlier scenarios are used. If no defaults are available, the server throws an exception and fails to start.


What earlier scenarios? Specify them or link to them. Also what exception?

What is an earlier scenario? The style guide prefers previous, but you need to be specific, such as

If the Certificate section isn't specified, the defaults, defined previously, are used.

blowdart

Some small nit picks.

Rick-Anderson · 2021-01-04T20:35:26Z

+
+* Endpoints names are case-insensitive. For example, `HTTPS` and `Https` are valid.
+* The `Url` parameter is required for each endpoint. The format for this parameter is the same as the top-level `Urls` configuration parameter except that it's limited to a single value.
+* These endpoints replace those defined in the top-level `Urls` configuration rather than adding to them. Endpoints defined in code via `Listen` are cumulative with the endpoints defined in the configuration section.


Just checking, is it literally Urls or is it the plural of Url? This needs to be a literal and you can't make literals plural by tacking on a s

Rick-Anderson · 2021-01-04T20:38:04Z

+* Endpoints names are case-insensitive. For example, `HTTPS` and `Https` are valid.
+* The `Url` parameter is required for each endpoint. The format for this parameter is the same as the top-level `Urls` configuration parameter except that it's limited to a single value.
+* These endpoints replace those defined in the top-level `Urls` configuration rather than adding to them. Endpoints defined in code via `Listen` are cumulative with the endpoints defined in the configuration section.
+* The `Certificate` section is optional. If the `Certificate` section isn't specified, the defaults defined in earlier scenarios are used. If no defaults are available, the server throws an exception and fails to start.


What is an earlier scenario? The style guide prefers previous, but you need to be specific, such as

If the Certificate section isn't specified, the defaults, defined previously, are used.

Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>

JamesNK · 2021-01-04T21:46:05Z

I'm going to wait until #21091 is merged and rebase before making anymore changes.

JamesNK added 2 commits December 10, 2020 13:39

Add PEM config info to Kestrel doc

f04d248

Update

e618e2c

JamesNK requested review from blowdart, halter73 and javiercn December 10, 2020 00:53

Update

ad77613

pem or crt files

ed4684f

blowdart reviewed Jan 4, 2021

View reviewed changes

blowdart requested changes Jan 4, 2021

View reviewed changes

Rick-Anderson approved these changes Jan 4, 2021

View reviewed changes

Apply suggestions from code review

ba7b8cf

Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>

JamesNK added the blocked label Jan 4, 2021

JamesNK mentioned this pull request Jan 16, 2021

Add PEM config info to Kestrel doc #21235

Merged

JamesNK closed this Jan 16, 2021

Rick-Anderson deleted the jamesnk/kestrel-pem branch May 7, 2021 21:54

Conversation

JamesNK commented Dec 10, 2020

Uh oh!

JamesNK commented Dec 10, 2020

Uh oh!

halter73 commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guardrex commented Dec 11, 2020

Uh oh!

JamesNK commented Dec 14, 2020

Uh oh!

guardrex commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rick-Anderson commented Dec 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blowdart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

halter73 commented Dec 10, 2020 •

edited

Loading

guardrex commented Dec 14, 2020 •

edited

Loading