Operator CRD with generated ServerConfiguration from refactored config by andreaTP · Pull Request #11929 · keycloak/keycloak

andreaTP · 2022-05-10T18:14:27Z

Initial work to resolve: #10866

A possible plan to get this to main would be:

get this refactoring in (as it doesn't break anything else and prepare the "structure")
port all of the options in a separate PR
use the generated serverConfig in a third follow up PR

vmuzikar

@andreaTP Nice job, I like this approach! 👍 Although I fought a bit with my IDE to make it index the generated class.

Added a few comments but mostly just suggestions for polishing.

The remaining issue we have to tackle is how to handle the exceptions in a nice way – how to make some propety hidden from Quarkus or the Operator.

pedroigor · 2022-05-16T20:14:27Z

I guess this is also another example of how we should be obtaining all available options from the server configuration?

Correct, and it can/will be used by the following modules.

I aim to get this first "example" in, and to bulk move the encoding of all the options in a follow-up PR to help with reviews and split up work.

pedroigor · 2022-05-16T20:23:22Z

Can't we replace this class and Option now we have similar types in from the new module?

We can replace some of this, unfortunately, looks like SPIs configuration is still not covered:

keycloak/docs/maven-plugin/src/main/java/org/keycloak/guides/maven/Options.java

Lines 44 to 64 in 3ff3aeb

for (Spi loadSpi : providerManager.loadSpis().stream().sorted(Comparator.comparing(Spi::getName)).collect(Collectors.toList())) {

for (ProviderFactory providerFactory : providerManager.load(loadSpi).stream().sorted(Comparator.comparing(ProviderFactory::getId)).collect(Collectors.toList())) {

List<ProviderConfigProperty> configMetadata = providerFactory.getConfigMetadata();

if (configMetadata == null) {

continue;

}

String optionPrefix = NS_KEYCLOAK_PREFIX + String.join(OPTION_PART_SEPARATOR, ArrayUtils.insert(0, new String[] {loadSpi.getName(), providerFactory.getId()}, "spi"));

List<Option> options = configMetadata.stream()

.map(m -> new Option(Configuration.toDashCase(optionPrefix.concat("-") + m.getName()), ConfigCategory.GENERAL, false,

m.getHelpText(),

m.getDefaultValue() == null ? "none" : m.getDefaultValue().toString(),

m.getOptions() == null ? (m.getType() == null ? Collections.emptyList() : Collections.singletonList(m.getType())) : m.getOptions()))

.sorted(Comparator.comparing(Option::getKey)).collect(Collectors.toList());

if (!options.isEmpty()) {

providerOptions.computeIfAbsent(toDashCase(loadSpi.getName()), k -> new LinkedHashMap<>()).put(toDashCase(providerFactory.getId()), options);

}

}

}

In this PR I took out the docs bit to be able to perform the change incrementally (i.e. with subsequent non-breaking PRs)
E.g. the docs sub-module will rely on the new configuration only after we move all the configuration keys to the new encoding.

Although we don't have yet a proper support for exposing SPI configuration options, the new module should be able to provide them similarly to what you did to HttpOptions.

The difference here is that these options are dynamic as they come from whatever provider is installed on the server. In the future, and that is related to the extension story, it should also resolve options from custom providers.

If you want, I can look at this closer and try to come up with something to support this.

pedroigor · 2022-05-16T20:33:24Z

@andreaTP Nice work, looks like you are in the right direction :) Looks like we can also refactor the PropertyMapper a bit more such as:

Methods to format an option to CLI and ENV formats should now be available from the Option 8000 . For instance, it should be useful even in the operator to proper validate or document the options that can be set using CLI or ENV.
Some fields in PropertyMapper can probably be removed now that we have the same defined in Option.
Make the PropertyMapper strictly related to Quarkus configuration mapping (e.g.: transformers).

DGuhr

Absolutely the right direction, thanks for that 👍 👍

One thing I'd still love to see (in a further step) is validations in this package. Imo the general validation rules are not dependant on the runtime - they depend on the (way less often changed) standards, e.g. "what is a valid range for a port"? could be answered the same for wildfly, quarkus and pretty much every httpserver out there. As a result, this package should imo be the place to define the validations, e.g. Option defining a Predicate/Function, and perhaps to not impose delays a way to skip the validations in favor of better startup times, bc. they are known to be working.

This would then also enable e.g. the possibility to write an IDE plugin that checks your configuration files for validity even before starting up.

But as said: Nice step in the right direction 👍 rest is for the future.

andreaTP · 2022-05-17T08:09:47Z

@pedroigor I'll look into the further refactoring you proposed, seems good to me!

@DGuhr I agree on the "validation" bits, but this change is going to be invasive enough without them, will follow up as soon as this encoding is more stable (I already have a pretty good idea on how to do it 🙂 ).

Thanks for the feedback everyone!

andreaTP · 2022-05-17T09:18:03Z

@pedroigor I have attempted to follow your suggestions:

Methods to format an option to CLI and ENV formats should now be available from the Option. For instance, it should be useful even in the operator to proper validate or document the options that can be set using CLI or ENV.

Unfortunately found out that the env and cli encodings are dependent on Picocli and Smallrye Config stuff, I would very much prefer to keep the Configuration module agnostic and dependency-free.

Some fields in PropertyMapper can probably be removed now that we have the same defined in Option.

Now PropertyMapper internally uses an Option but still exposes the "full" API, we can clean up the implementation after the next step (e.g. when all configurations are coming from the config module)

andreaTP · 2022-05-17T10:46:43Z

@pedroigor @vmuzikar @DGuhr ready for the next round of review, please consider that this is not the "final" PR and more cleanup will be done in the follow-up PRs to bring the task to completion. 🙏

The goal here is to get the "infrastructure setup" for the new configuration in place, along with one example property already ported over.

vmuzikar

@andreaTP Thanks for the update, really nice work! :)

Added a few comments. Mainly just nitpics to make the compiler happy and/or to align with best practices. The only thing that's blocking merge from my PoV, is the added dependency on commons-text.

vmuzikar · 2022-05-19T13:56:37Z

Again, just a nitpick. My IDE is complaining about using Optional as a field and for a good reason actually.

not sure what to do here, I changed everything to Optional because of a previous comment on this PR ...

I see. I assume you are referring to this: #11929 (comment) (CC @DGuhr).

I'm not fussy on it, we could leave it as is. Just from my PoV the point in the mentioned article:

The class is definitively not intended for use as a property

as well as a note in the official docs:

Optional is primarily intended for use as a method return type where there is a clear need to represent "no result,"

are clear to me that it should not be used as field types.

Personally, I have mixed feelings about it, maybe we can briefly discuss this point in 2/3 weeks and decide what to do with it.

vmuzikar · 2022-05-19T15:06:55Z

Just out of curiosity. Can't we install the Maven plugin and operator in a single step? It'd provide a better UX, IMHO.

I will check, in general, for local development, we need to install the maven-plugin once and iterate with the operator build.

I did a quick test (mvn clean install in operator module with local mvn repo cleaned of any traces of the Operator) and seemed to work fine. Maven seems smart enough to build the plugin dependency first.

andreaTP · 2022-05-23T09:07:34Z

@vmuzikar can you please double-check that I have answered/followed your comments?
There are very few places left, especially when we had diverging comments/opinions by reviewers, but it might already be "good enough" as an iteration and we can fix things on top at a later stage, wdyt?

andreaTP · 2022-05-23T09:23:55Z

apparently some of those changes have broken the build, will fix it by EOD.

vmuzikar

@andreaTP Thanks for the update. LGTM, just found a bug in the camel case method, I believe.

it might already be "good enough" as an iteration and we can fix things on top at a later stage, wdyt?

+1, we just need to agree on the follow-up steps (if there are any) :)

andreaTP · 2022-05-23T13:21:00Z

I finished fixing the open points, and squashed the commits. This should be ready for a final round @pedroigor @vmuzikar

vmuzikar

@andreaTP Thank you for the update!

Just for the visibility, the last thing I'd like to align on is this: #11929 (comment).

andreaTP · 2022-05-24T15:56:28Z

@vmuzikar removed the redundant mvn commands, should be good to go 🙂

vmuzikar

@andreaTP Nice work, LGTM!

pedroigor · 2022-05-25T21:10:16Z

Unfortunately found out that the env and cli encodings are dependent on Picocli and Smallrye Config stuff, I would very much prefer to keep the Configuration module agnostic and dependency-free.

Sorry, if I was not clear. I meant methods like

keycloak/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java

Line 140 in 52d205c

public static String toDashCase(String key) {

, or

keycloak/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java

Line 136 in 52d205c

public static String toCliFormat(String key) {

, or

keycloak/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/Configuration.java

Line 132 in 52d205c

public static String toEnvVarFormat(String key) {

. Do they depend on Picocli and Smallrye to badly that we can't bring them to the new module?

I would say that formatting is a must for this new module.

andreaTP · 2022-05-26T07:48:44Z

@pedroigor I have attempted to move those and this is the result:

toDashCase and toCliFormat can be moved
toEnvVarFormat depends on io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores

In general, I believe that this PR is already big enough, do you agree to tackle this improvement separately? Do you wanna open a new Issue for it?

andreaTP · 2022-05-26T07:54:09Z

@vmuzikar needed to rebase because of conflicts, would you mind to re-approve?

pedroigor · 2022-05-26T12:29:41Z

@pedroigor I have attempted to move those and this is the result:

toDashCase and toCliFormat can be moved

toEnvVarFormat depends on io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores

In general, I believe that this PR is already big enough, do you agree to tackle this improvement separately? Do you wanna open a new Issue for it?

It should be fine to borrow that code from SmallRye and have it on our side.

But yeah, you have a good baseline, I'll look at these points later and collaborate with some changes.

pedroigor · 2022-05-25T21:03:35Z

Although we don't have yet a proper support for exposing SPI configuration options, the new module should be able to provide them similarly to what you did to HttpOptions.

The difference here is that these options are dynamic as they come from whatever provider is installed on the server. In the future, and that is related to the extension story, it should also resolve options from custom providers.

If you want, I can look at this closer and try to come up with something to support this.

vmuzikar self-requested a review May 12, 2022 06:33

vmuzikar reviewed May 13, 2022

View reviewed changes

andreaTP mentioned this pull request May 16, 2022

[DRAFT] Demo operator CRD with generated ServerConfiguration #11806

Closed

andreaTP force-pushed the shared-config branch 3 times, most recently from cc7d17e to 7649588 Compare May 16, 2022 15:18

andreaTP marked this pull request as ready for review May 16, 2022 15:21

andreaTP requested review from DGuhr and pedroigor May 16, 2022 15:21

andreaTP changed the title ~~[DRAFT] Demo operator CRD with generated ServerConfiguration 2~~ Operator CRD with generated ServerConfiguration from refactored config May 16, 2022

pedroigor reviewed May 16, 2022

View reviewed changes

DGuhr reviewed May 17, 2022

View reviewed changes

Comment thread dist-config/src/main/java/org/keycloak/config/Option.java Resolved

DGuhr reviewed May 17, 2022

View reviewed changes

Comment thread dist-config/src/main/java/org/keycloak/config/OptionBuilder.java Resolved

DGuhr reviewed May 17, 2022

View reviewed changes

hmlnarik reviewed May 17, 2022

View reviewed changes

Comment thread pom.xml Resolved

andreaTP force-pushed the shared-config branch from 6f1236c to 92cb5fe Compare May 19, 2022 08:48

DGuhr reviewed May 19, 2022

View reviewed changes

Comment thread dist-config/pom.xml Resolved

DGuhr reviewed May 19, 2022

View reviewed changes

Comment thread operator/maven-plugin/src/main/java/org/keycloak/operator/maven/ServerConfigGen.java Resolved

DGuhr reviewed May 19, 2022

View reviewed changes

Comment thread pom.xml Resolved

vmuzikar reviewed May 19, 2022

View reviewed changes

andreaTP force-pushed the shared-config branch from 15c8eb6 to 5c916ff Compare May 23, 2022 09:02

vmuzikar requested changes May 23, 2022

View reviewed changes

Comment thread operator/maven-plugin/src/main/java/org/keycloak/operator/maven/ServerConfigGen.java Resolved

Comment thread ...runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java Resolved

andreaTP force-pushed the shared-config branch from c317350 to f6d5dde Compare May 23, 2022 13:20

vmuzikar reviewed May 24, 2022

View reviewed changes

andreaTP force-pushed the shared-config branch from 0e7826b to f799d24 Compare May 24, 2022 12:33

andreaTP mentioned this pull request May 24, 2022

Build operator dependencies first before testing it #11642

Merged

vmuzikar previously approved these changes May 25, 2022

View reviewed changes

vmuzikar requested a review from pedroigor May 25, 2022 07:00

andreaTP dismissed vmuzikar’s stale review via b82c7d7 May 26, 2022 07:52

andreaTP force-pushed the shared-config branch from f799d24 to b82c7d7 Compare May 26, 2022 07:52

Refactor dist config to a common module

c4a0a01

andreaTP force-pushed the shared-config branch from b82c7d7 to c4a0a01 Compare May 26, 2022 08:18

pedroigor approved these changes May 26, 2022

View reviewed changes

pedroigor merged commit d667102 into keycloak:main May 26, 2022

	for (Spi loadSpi : providerManager.loadSpis().stream().sorted(Comparator.comparing(Spi::getName)).collect(Collectors.toList())) {
	for (ProviderFactory providerFactory : providerManager.load(loadSpi).stream().sorted(Comparator.comparing(ProviderFactory::getId)).collect(Collectors.toList())) {
	List<ProviderConfigProperty> configMetadata = providerFactory.getConfigMetadata();

	if (configMetadata == null) {
	continue;
	}

	String optionPrefix = NS_KEYCLOAK_PREFIX + String.join(OPTION_PART_SEPARATOR, ArrayUtils.insert(0, new String[] {loadSpi.getName(), providerFactory.getId()}, "spi"));
	List<Option> options = configMetadata.stream()
	.map(m -> new Option(Configuration.toDashCase(optionPrefix.concat("-") + m.getName()), ConfigCategory.GENERAL, false,
	m.getHelpText(),
	m.getDefaultValue() == null ? "none" : m.getDefaultValue().toString(),
	m.getOptions() == null ? (m.getType() == null ? Collections.emptyList() : Collections.singletonList(m.getType())) : m.getOptions()))
	.sorted(Comparator.comparing(Option::getKey)).collect(Collectors.toList());

	if (!options.isEmpty()) {
	providerOptions.computeIfAbsent(toDashCase(loadSpi.getName()), k -> new LinkedHashMap<>()).put(toDashCase(providerFactory.getId()), options);
	}
	}
	}

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!