Skip to content

fix: ensuring placeholders can be used with --import-realm #33589

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

Merged
vmuzikar merged 1 commit into from
Oct 4, 2024
+2 −2
Merged

fix: ensuring placeholders can be used with --import-realm #33589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ImportDistTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.keycloak.it.junit5.extension.CLIResult;
import org.keycloak.it.junit5.extension.DistributionTest;
import org.keycloak.it.junit5.extension.RawDistOnly;
import org.keycloak.it.junit5.extension.WithEnvVars;
import org.keycloak.it.utils.KeycloakDistribution;

import com.fasterxml.jackson.databind.ObjectMapper;
Expand Down
2 changes: 1 addition & 1 deletion quarkus/tests/integration/src/test/resources/quickstart-realm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"realm": "quickstart-realm",
"enabled": true,
"enabled": ${TEST_REALM_ENABLED:true},
Copy link
Contributor

@vmuzikar vmuzikar Oct 4, 2024

Choose a reason for hiding this comment

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

We're not explicitly testing if the realm is enabled in the ImportAtStartupDistTest but it should be fine as the import would fail if placeholders were disabled.

"accessTokenLifespan": 60,
"accessCodeLifespan": 60,
"accessCodeLifespanUserAction": 300,
Expand Down
1 change: 1 addition & 0 deletions services/src/main/java/org/keycloak/exportimport/ExportImportManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public boolean isImportMasterIncludedAtStartup(String dir) {
if (dir == null) {
throw new IllegalStateException("Import not enabled");
}
ExportImportConfig.setReplacePlaceholders(true);
Copy link
Contributor

@vmuzikar vmuzikar Oct 4, 2024

Choose a reason for hiding this comment

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

This looks like an odd place to set configuration. Can't this be moved somewhere else (maybe the ExportImportManager)?

Copy link
Contributor Author

@shawkins shawkins Oct 4, 2024

Choose a reason for hiding this comment

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

This looks like an odd place to set configuration.

Do you mean you want to get rid of the ExportImportConfig class? That has the responsibility currently of handling all of these system properties.

Copy link
Contributor Author

@shawkins shawkins Oct 4, 2024

Choose a reason for hiding this comment

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

Another alternative is to default to always replacing placeholders. I think we didn't do that before just to limit the scope of the changes, but as long as the core folks agree then we can remove both of these calls.

Copy link
Contributor

@vmuzikar vmuzikar Oct 4, 2024

Choose a reason for hiding this comment

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

Do you mean you want to get rid of the ExportImportConfig class? That has the responsibility currently of handling all of these system properties.

No, I meant that I don't think we should be calling ExportImportConfig.setReplacePlaceholders(true); from isImportMasterIncludedAtStartup which is essentially just a getter/checker, it should not modify the configuration IMHO.

Another alternative is to default to always replacing placeholders. I think we didn't do that before just to limit the scope of the changes, but as long as the core folks agree then we can remove both of these calls.

That might be a bit out of scope for now.

Copy link
Contributor Author

@shawkins shawkins Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, I meant that I don't think we should be calling ExportImportConfig.setReplacePlaceholders(true); from isImportMasterIncludedAtStartup which is essentially just a getter/checker, it should not modify the configuration IMHO.

No, it's not great. It's similar to the issues raised in the TODO about undoing the static state changes. We can move it up, but it's not ideal in KeycloakApplication either.

Copy link
Contributor

@vmuzikar vmuzikar Oct 4, 2024

Choose a reason for hiding this comment

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

TBH, it makes more sense to me if it was in KeycloakApplication than in isImportMasterIncludedAtStartup. Feels cleaner.

Copy link
Contributor Author

@shawkins shawkins Oct 4, 2024

Choose a reason for hiding this comment

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

Let's just resolve this in a follow-up: #33596 - ideally we should be encapsulating as much as possible the usage of static ExportImportConfig logic.

Copy link
Contributor

@vmuzikar vmuzikar Oct 4, 2024

Choose a reason for hiding this comment

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

Ok, let's do it as a follow-up.


return getStartupImportProviders(dir).map(Supplier::get).anyMatch(provider -> {
try {
Expand Down