Skip to content
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.

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

Keycloak.x troubleshooting #240

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pedroigor
Copy link
Contributor

No description provided.

@pedroigor pedroigor assigned stianst and unassigned stianst Feb 19, 2021
Copy link
Contributor

@slaskawi slaskawi left a comment

Choose a reason for hiding this comment

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

LGTM. Would it feasible to add a paragraph with attaching a debugger?

Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@pedroigor Thanks, I am approving the PR, but added few comments inline with some consideration for the usability. Leaving to you whether/how to apply them or not. Those are just some ideas from the long-term and not a blocker for this PR IMO.

+1 for @slaskawi note about debugging

* Runtime properties
* For each property indicate their source (persisted, CLI, system property, environment vars, etc)
* Features enabled
* List of installed providers
Copy link
Contributor

Choose a reason for hiding this comment

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

From the long term perspective, it can be useful if there is a difference between the default providers (+ configuration options for providers), which have "default" values from the system and between the configuration options, which were provided/overriden by the administrator. So that those, which have non-default values are highlighted.

For example, if administrator overrides the "userProvider" to use "map" instead of default "jpa", then the output can be something like this:

keycloak.realm.provider=jpa (BuiltIn)
keycloak.user.provider=map (ProvidedByUser)

The providers, which were overriden can be also highlighted somehow (maybe different color?) as those are probably most important for the administrator to notice.

In addition to "BuiltIn", "ProvidedByUser", it may be also useful to have 3rd value like "ProvidedByUserProp" or something like that, which means that particular provider option is indirectly configured with the 1st level option. For example by set the 1st level runtime property kc.db to mysql, I indirectly set the provider properties keycloak.connectionsJpa.default.url , keycloak.connectionsJpa.default.driver etc. So this can be also somehow highlighted in the report

This is probably not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point.

However, I'm not sure that showing the built-in and the active provider makes sense. The reason being that you want to list the active providers at runtime and not necessarily what I'm overriding. In fact, if you choose to use map, for instance, is because you do not want jpa.

Your comment also made me think about listing the "available" providers, not only those actually active?

Perhaps, we should also introduce the idea of "active" providers because "installed" is related to those that are active or not?

Btw, we currently keep a list of all default providers at build-time, so we can do whatever we want with this information.

I like the idea of expanding a top-level property and expand what it has impacted.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for "available" and "active" providers and show them somehow.

In relation to that, it worth to mention that we have 2 basic type of SPI/providers.

  1. Those, which require to have only single implementation per server, which means that default provider needs to be provided. These can be obtained with session.getProvider(Provider.class) . Example is "realm" provider
  2. Those, which allow to have many implementations available. Like Authenticator or ProtocolMapper SPI for example.

Not sure if we should differentiate between those two in the report as well? Usually the providers of type (2) are not configured globally at server level at all (Typically they are created in the admin console etc). But maybe there are cases when they are configured globally as well?

```
Current Profile: none
Runtime Properties:
kc.cluster = local (PersistedConfigSource)
Copy link
Contributor

Choose a reason for hiding this comment

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

From the usability perspective, it can be also useful if at the end of "show-config" page, there is some glossary with the explanation what means PersistedConfigSource, what means SysPropConfigSource etc.

Copy link
Contributor Author

@pedroigor pedroigor Feb 22, 2021

Choose a reason for hiding this comment

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

Or perhaps change the output to show from server image and from system properties ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is good point. Question is, if it would be 100% clear for typical administrator what exactly it means? But maybe we can have both? Like the nicer names (from system properties) and also have the glossary at the end of the page? Not sure what is best for usability TBH...

@pedroigor
Copy link
Contributor Author

@slaskawi Added a section for debugging.

@@ -620,6 +620,112 @@ keep it up to date with the latest release.
For SPIs and Providers we will extend the ProviderFactory to add an option method to return information on what
properties it supports. This will allow including SPI/provider specific configuration in the documentation.

## Troubleshooting

Keycloak should provide the appropriate leve of information and also tools for troubleshooting. It should help users to:

Choose a reason for hiding this comment

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

A minor typo here: s/leve/level


Keycloak should provide the appropriate leve of information and also tools for troubleshooting. It should help users to:

* Identity the problem through clear and meaningful messages

Choose a reason for hiding this comment

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

s/Identity/Identify (I believe that was the original intention)

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.

5 participants