Skip to content

[DRAFT] Improvement on 'Hostname' guide by adding recommended settings by scenarios/examples#20973

Closed
andre-nascimento6791 wants to merge 2 commits intokeycloak:mainfrom
andre-nascimento6791:issue-20931
Closed

[DRAFT] Improvement on 'Hostname' guide by adding recommended settings by scenarios/examples#20973
andre-nascimento6791 wants to merge 2 commits intokeycloak:mainfrom
andre-nascimento6791:issue-20931

Conversation

@andre-nascimento6791
Copy link
Copy Markdown
Contributor

Considering the analysis done in Issue #20571 on each issue mentioned in the umbrella Issue #14666, it was decided that the first action to take is to improve the documentation/guides which are related to Hostname, Frontend and Admin Console and Proxy settings.

Closes #20931

@andre-nascimento6791 andre-nascimento6791 force-pushed the issue-20931 branch 2 times, most recently from 3cb56a2 to 7c09f44 Compare June 13, 2023 16:27
@abstractj abstractj requested review from Pepo48 and mabartos June 13, 2023 21:50
@abstractj
Copy link
Copy Markdown
Contributor

@Pepo48 @mabartos could you please review?

Copy link
Copy Markdown
Contributor

@Pepo48 Pepo48 left a comment

Choose a reason for hiding this comment

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

hey @andre-nascimento-rh, thanks for the PR. I added couple of suggestions. Right now I'm not really sure about the table format.

I guess we should try to make the bash commands examples in a way that a user can click on the play button in his IDE and it will automatically start the server for him (at least in development mode). I think that eventually we can get rid of the tables completely and use a simple section format with a description and an example command.

But it's just my preference after I saw the compiled result. We should definitely discuss it within the team.

Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
@andre-nascimento6791
Copy link
Copy Markdown
Contributor Author

@pedroigor ,

As you are the most experienced guy in Hostname options/settings, please, could you review this suggestion of improvements on documentation/guide?

My idea is to provide some use-case driven scenarios, starting from the simplest case and progressing to more complex cases.

Please evaluate:

  • If that approach is suitable in your view and experience, given that we are trying to clarify this topic to Keycloak beginners and to Keycloak advanced users/developers;

  • If each of those scenarios makes sense and/or it is relevant to be mentioned there;

  • If the command options are correct and If they work properly (I tested all of them locally against the Keycloak nightly version and a simple Nginx proxy).

Your comments and suggestions are welcome.

Thanks in advance.

Copy link
Copy Markdown
Contributor

@Pepo48 Pepo48 left a comment

Choose a reason for hiding this comment

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

@andre-nascimento-rh thanks! I appreciate the progress. I added a couple of suggestions again. PTAL and let me know.

Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment on lines 103 to 111
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just nitpicks, but I think that there is still room for simplifying the structure and improving the readability.
I'm suggesting the following:

  • remove the slash from every place, where it doesn't fulfil the role of an alternative - scenario/example; recommended settings/keycloak configuration; beginner/quick - all have very similar meaning in this context,
  • on the other hand, let's add more precise, more detailed descriptions; use rather full sentence instead of collocations separated by commas,
  • remove the bullet points - if you need, break the line instead,
  • use either bold or italic text (not both at the same time).

Here's what I mean by that:

Suggested change
=== Scenario 01 - Beginner/Quick Test in Localhost with HTTP only
* __**Description**__:
** The most basic option to run, in localhost, all default values.
* __**Recommended Settings / Keycloak configuration**__:
<@kc.startdev parameters="" />
* __**Expected Behavior**__:
** Both Frontend/Backend and Admin Console at: http://localhost:8080
---
=== Scenario 01 - Exposing the server on localhost using HTTP
.Description:
Running the server on localhost using HTTP with the default settings.
.Keycloak configuration:
<@kc.startdev parameters="" />
All 3 endpoint categories are available at: http://localhost:8080
---

Regardless, I'm not sure, whether it's worth to mention this particular scenario, as it doesn't provide any additional information regarding the hostname setup (compared to others bellow). But it's up to a discussion, I used it as a reference example of how the format could be potentially improved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok. In the regard of simplifying the format, suggestions accepted and applied. Thanks.

  • remove the slash from every place, where it doesn't fulfil the role of an alternative - scenario/example; recommended settings/keycloak configuration; beginner/quick - all have very similar meaning in this context,

    • Done. Please review again If it is better this way.
  • on the other hand, let's add more precise, more detailed descriptions; use rather full sentence instead of collocations separated by commas

    • What is your suggestion of full sentence, where do you think it is not good?
  • remove the bullet points - if you need, break the line instead

    • Done.
  • use either bold or italic text (not both at the same time).

    • Done.
  • Regardless, I'm not sure, whether it's worth to mention this particular scenario, as it doesn't provide any additional information regarding the hostname setup (compared to others bellow). But it's up to a discussion, I used it as a reference example of how the format could be potentially improved.

    • Exactly. In my view, the most important points of this review are: Evaluate which scenarios make sense to mention in this Guide and If the kc start command options are right for each scenario. And I need your help in this effort.

Thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is your suggestion of full sentence, where do you think it is not good?

I meant it rather like a reminder that we should avoid sentence structures like the one you use initially:
The most basic option to run, in localhost, all default values.

Anyway, I think that right now the rest is ok. Thanks!

Comment thread docs/guides/server/hostname.adoc Outdated
Copy link
Copy Markdown
Contributor

@Pepo48 Pepo48 left a comment

Choose a reason for hiding this comment

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

@andre-nascimento-rh I added multiple comments/suggestions. Some of the suggestions applies for the multiple scenarios. I deliberately didn't comment every single occurrence.

Multiple walkthroughs are seemingly required anyway. We just need to improve the consistency and syntax/semantics at some places as well. Of course, in the final iterations Andy can help us with these as well. But I feel like there is still a room for an improvement that we can do on our own.

Please, if you find time, try to read through it on your own too. I feel like some of these adjustments can be easily spotted (especially the typos like the period in headings). I will do the same in the following days.

Thanks for your time and effort!

Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@andre-nascimento-rh I asked about this above, in my earlier comments - is there a reason to capitalize the keywords like Proxy, Hostname, Edge etc? Unless they're the part of a heading, I would use lowercase as in normal sentence.

Also, there is lot of inconsistency in using pre-formated text (text enclosed into the backticks). Sometimes you use it, sometimes you don't. We need to be more consistent and I would suggest to not to overuse it.

Moreover, in this specific case the description consists of two sentences, which are synonymous. Please, rephrase it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Double-checked and changed the leaving capitalized words.

Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that you didn't fully accepted my previous suggestion - that's ok, just please be careful about the consistency. even when in the headings. Check the existing examples in the Hostname Configuration Guide.
So I would do at least these adjustments:

Suggested change
=== Scenario 01 - Development Test in Localhost with HTTP only
=== Scenario 01 - Development test on localhost over HTTP

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but it exactly the opposite: I accepted almost all of your suggestions. I applied the change at the adoc file and pushed it. I understood that using capitalized words in titles there wasn't an issue for you. But now I'm changing all the titles. No problem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@andre-nascimento-rh I meant this specific suggestion that covered just the fist scenario. In the suggestion I deliberately changed the heading in a way that it doesn't contain uppercase letters (and also the wording is little bit different). I noticed that you didn't apply it (which is totally fine), but we need to stay consistent.

I don't even have strong preference, whether the first letter of a word in the heading should be uppercase or not (it was my natural suggestion to stay consistent with what we already have but in the end it's up to your own style), just the consistency hasn't been there yet. Therefore I reminded it. 😊

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I applied what you suggested. Are you seeing the last version?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Explained and resolved offline.

Comment thread docs/guides/server/hostname.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are mentioning the necessity to setup TLS for HTTPS scenarios (for the production mode) right at the beginning. Is it necessary to repeat the same in the descriptions of the particular scenarios?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe it is clearer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, then we should at least correct the capital letter in "Certificates".

Comment thread docs/guides/server/hostname.adoc Outdated
Comment thread docs/guides/server/hostname.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Your `reverse proxy` doesn't have HTTPS/TLS termination and trust in Keycloak. And you provide access in HTTP only to Keycloak behind your `proxy` (Not recommended at all!).
Reverse proxy doesn't have HTTPS/TLS termination and trust in Keycloak. The access to your Keycloak instance behind the proxy is then provided over HTTP only, which is not recommended.

Btw, this is a good example where I would use an additional formatting to highlight "not recommended part".

Copy link
Copy Markdown
Contributor Author

@andre-nascimento6791 andre-nascimento6791 Jun 20, 2023

Choose a reason for hiding this comment

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

Done. Review it If that way suits your expectation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I meant by this suggestion was to get rid of the bracket part (the capital letter and exclamation mark specifically).

And during the process I thought also about minimizing the usage of second-person pronouns (e.g. you) and possessive pronouns (e.g. your). But I would rather leave this opened for someone with better linguistic knowledge.

Comment thread docs/guides/server/hostname.adoc Outdated
@jonkoops jonkoops requested a review from a team June 20, 2023 12:40
@abstractj abstractj requested a review from Pepo48 June 20, 2023 17:03
@abstractj
Copy link
Copy Markdown
Contributor

@andre-nascimento-rh please do not mark the conversation as resolved and leave that to the reviewers to resolve.

@Pepo48
Copy link
Copy Markdown
Contributor

Pepo48 commented Jun 21, 2023

@andre-nascimento-rh do you think that we could break the draft PR into the smaller pieces (logically related chunks) and try to merge them one by one? Let's for example start with those about which we are confident enough.

So I would say let's be conservative and take just two scenarios from this draft - 05 and 12. In my opinion both of them are providing something, which is not yet fully covered in the current version of the guide, both are HTTPS examples (I think we should predominantly demonstrate examples that are production ready) and on top of that we can take some information from the other examples (reencrypt, passthrough) and squash them under these two as well.

The PR would be much smaller, easier to digest and review. Wdyt?

@Pepo48
Copy link
Copy Markdown
Contributor

Pepo48 commented Jun 21, 2023

cc: @mabartos ^^

@andre-nascimento6791
Copy link
Copy Markdown
Contributor Author

@andre-nascimento-rh do you think that we could break the draft PR into the smaller pieces (logically related chunks) and try to merge them one by one? Let's for example start with those about which we are confident enough.

The PR would be much smaller, easier to digest and review. Wdyt?

@Pepo48, I agree with the suggestion of breaking the draft PR into the smaller pieces. Let's go for it from the next push.

So I would say let's be conservative and take just two scenarios from this draft - 05 and 12. In my opinion both of them are providing something, which is not yet fully covered in the current version of the guide, both are HTTPS examples (I think we should predominantly demonstrate examples that are production ready)...

Even I'm not a big fan of this approach, still there are people interested in making Keycloak responding only over HTTP, and some of them stuck in how to achieve that. Therefore I would still keep or add some scenarios to run only over HTTP. WDYT?

@Pepo48
Copy link
Copy Markdown
Contributor

Pepo48 commented Jul 20, 2023

This PR is superseded by #21846.

@Pepo48 Pepo48 closed this Jul 20, 2023
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.

Improvements on Documentation/Guides for "Hostname/Proxy/Admin Console" Settings

3 participants