8000 fix: makes the db setting require an explicit setting in prod by shawkins · Pull Request #33760 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: makes the db setting require an explicit setting in prod #33760

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
Closed
wants to merge 1 commit into from
Closed

fix: makes the db setting require an explicit setting in prod #33760

wants to merge 1 commit into from

Conversation

shawkins
Copy link
Contributor
@shawkins shawkins commented Oct 9, 2024 •
edited
Loading

Also various cleanups related to the configuration - there is some tech debt, and a few things that are probably wrong. Some of this could be separated from the pr, but we do need to change how we detect auto-builds are needed to understand the absense of the db option. See also #33902

I had initially tried to go the direction of mapping the the database from the profile property - but that revealed some additional compleity that is needed to handle that case (so that the other values mapped from db could work). So instead this uses a delayed default value, which looks at the profile.

These changes also bring up what is dev vs prod means with the non-server commands. As it stands the changes will force users to specify --db on import, export, etc. just like start if --optimized is not specified. The alternative to this behavior is to default to the persisted db instead - but only if what's persisted is in the prod profile.

WDYT? @mabartos @vmuzikar @pedroigor

closes: #23805

@shawkins shawkins force-pushed the iss23805 branch 11 times, most recently from e22a572 to 0297f84 Compare October 13, 2024 21:02
@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Oct 13, 2024
Closed
@shawkins shawkins mentioned this pull request Oct 14, 2024
Closed
2 tasks
@shawkins shawkins force-pushed the iss23805 branch 5 times, most recently from 2dc7a07 to f9f28fb Compare October 15, 2024 11:57
@shawkins
Copy link
Contributor Author
shawkins commented Oct 15, 2024 •
edited
Loading

@vmuzikar @pedroigor @mabartos @Pepo48 just wanted to get some opinions on how to proceed with this pr as there are quite a few things going on there. Let me know what you want separated out:

  • db should be required in non-dev profiles - this affects a lot of tests. I changed the quarkus tests individually, but baked the setting into the arquillian tests. We may want to decide upon some initial backwards compatibility - either recommending that users who don't like the change update their keycloak.conf, or go down the path of a feature flag. In making this change, which is based upon the value being null in the prod profile, it was clear that quite a bit of our option handling logic should be refined. Initially the problem was that the change detection logic does consider the case of a missing build option, but there were quite a few other cases as well. My main goal was to make the logic between KeycloakProcessor and Picocli symmetrical. Some of the changes there:

    • The change detection and property persisting now use the same method to build the properties set. We still have special handling, but only for the optimized and profile properties.
    • We will only deal with fully resolved values, so routines dealing with the profile aren't needed, but this also requires initializing the profile in Picocli sooner than we were doing before.
    • Previously we were persisting all quarkus.properties (including things like passwords), but checking for build time changes in only quarkus datasource related properties - I've been hit by it before with some testing where I was setting some other quarkus build time property, but the autobuild was not happening. The changes here embrace that we don't know which of the properties are build time (quarkus only provides limited facilities for this), so we'll just rebuild whenever any of these change.
    • To have a better exception message, we need a specific check for whether the db property is set because our PropertyMapper validation doesn't consider this case - we can of course refine this however.

  • Some of our checks around dev profile behavior were specific to start, but any of the commands that can trigger autobuilds are affected as well. So this required moving around where those checks were performed and adding a couple of new tests https://github.com/keycloak/keycloak/pull/33760/files#diff-b1a9fed0083e8f87c45bbf63ce53ba9abe396b0aabb664a728234366ae07afbeR225 and https://github.com/keycloak/keycloak/pull/33760/files#diff-a27b6a53a3974903a2e610cfdf2df1e9494f3d7c79afe34ac45759f0b51c8c4cR92

  • Refined some of the ConfigValue ordinal handling with additional comments to make our expectations explicit.

    • Differentiated between the quarkus.properties in the classpath and the filesystem
    • Make the PropertyMapper transformValue explcitly set the ordinal to 0
    • Add a constanst for our lowest level of options that should be set by the user. Obviously they can unintentionally include a provider that shadows the classpath location for one of our built-in files, but we call this out in the docs as something they shouldn't do.

  • there's a bug in the windows script with --debug handling https://github.com/keycloak/keycloak/pull/33760/files#diff-743aef43b0717e3b41db7c269c0a8460de315f6a5b2cde028fbbcb5c1fc41370L45 (actually my fix doesn't seem to work here yet)
     

I would also like to move more of the testing into unit tests, but I want to pick up the changes from #33648 merged first.

@shawkins shawkins mentioned this pull request Oct 15, 2024
Closed
2 tasks
@shawkins shawkins force-pushed the iss23805 branch 3 times, most recently from ff98f5d to ae048a8 Compare October 15, 2024 19:49
@shawkins shawkins mentioned this pull request Oct 16, 2024
Closed
@shawkins shawkins mentioned this pull request Oct 23, 2024
Merged
vmuzikar
vmuzikar requested changes Nov 11, 2024
View reviewed changes
Copy link
Contributor
@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

Thinking about this further, I'm afraid we can't make --db required in prod mode. Even though H2 is considered a dev only DB, it's still a breaking change and as such would need to wait for 27.

WDYT @keycloak/cloud-native @stianst?

@shawkins
Copy link
Contributor Author

Thinking about this further, I'm afraid we can't make --db required in prod mode. Even though H2 is considered a dev only DB, it's still a breaking change and as such would need to wait for 27.

I would not view it as breaking because it's already documented to not use dev-file in production - and they would have a straight-forward workaround of putting db=dev-file in their keycloak.conf.

I'll clean up this pr now that a lot of it has been merged.

@shawkins shawkins requested a review from vmuzikar November 21, 2024 22:43
@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Nov 21, 2024
Closed
@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Nov 22, 2024
Closed
@shawkins shawkins marked this pull request as ready for review November 22, 2024 19:23
@shawkins shawkins requested review from a team as code owners November 22, 2024 19:23
@shawkins shawkins requested a review from mabartos November 22, 2024 19:24
@shawkins
fix: makes the db setting require an explicit setting in prod
762aa3d 
closes: keycloak#23805

Signed-off-by: Steve Hawkins <shawkins@redhat.com>
@shawkins
Copy link
Contributor Author
shawkins commented Dec 5, 2024

Switching to a new pr with a deprecation instead: #35674

@shawkins shawkins marked this pull request as draft December 5, 2024 18:36
This was referenced Dec 5, 2024
Merged
Open
@shawkins shawkins closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6469
Reviewers

@vmuzikar vmuzikar Awaiting requested review from vmuzikar

@mabartos mabartos Awaiting requested review from mabartos

Assignees
No one assigned
Labels
flaky-test team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

H2 Database should be opt-in and well-documented
2 participants
@shawkins @vmuzikar
0