Skip to content

fix: limit the scope of when a single transaction is used for import #37990

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
shawkins merged 1 commit into from
Mar 24, 2025
+34 −14
Merged

fix: limit the scope of when a single transaction is used for import #37990

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
3 changes: 2 additions & 1 deletion ...torage-services/src/main/java/org/keycloak/exportimport/util/ExportImportSessionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.keycloak.exportimport.util;

import org.keycloak.exportimport.ExportImportConfig;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionTask;

Expand All @@ -40,7 +41,7 @@ public void run(KeycloakSession session) {

@Override
public boolean useExistingSession() {
return true;
return ExportImportConfig.isSingleTransaction();

@vmuzikar vmuzikar Mar 12, 2025

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.

This was already true before the change. We're now calling ExportImportConfig.setSingleTransaction(true); which should have the same effect, no? Are we sure this is the correct fix?

@shawkins shawkins Mar 12, 2025

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.

We're setting it to true now for only one specific import case - when we also need to create the bootstrap admin user.

@vmuzikar vmuzikar Mar 12, 2025

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.

Ah, right. Thanks for the explanation.

}

protected abstract void runExportImportTask(KeycloakSession session) throws IOException;
Expand Down
20 changes: 16 additions & 4 deletions services/src/main/java/org/keycloak/exportimport/ExportImportConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ public class ExportImportConfig {
public static final String ACTION_EXPORT = "export";
public static final String ACTION_IMPORT = "import";

public static final String SINGLE_TRANSACTION = PREFIX + "single-transaction";

public static final String PROVIDER = PREFIX + "provider";
public static final String PROVIDER_DEFAULT = "dir";

Expand Down Expand Up @@ -61,15 +63,15 @@ public class ExportImportConfig {
public static String getAction() {
return System.getProperty(ACTION);
}

public static String getStrategy() {
return System.getProperty(STRATEGY);
}

public static String setStrategy(Strategy strategy) {
return System.setProperty(STRATEGY, strategy.toString());
}

public static Optional<String> getDir() {
return Optional.ofNullable(System.getProperty(DIR));
}
Expand Down Expand Up @@ -106,9 +108,19 @@ public static boolean isReplacePlaceholders() {
public static void setReplacePlaceholders(boolean replacePlaceholders) {
System.setProperty(REPLACE_PLACEHOLDERS, String.valueOf(replacePlaceholders));
}

public static void reset() {
Stream.of(FILE, DIR, ACTION, STRATEGY, REPLACE_PLACEHOLDERS)
.forEach(prop -> System.getProperties().remove(prop));
}

public static void setSingleTransaction(boolean b) {
System.setProperty(SINGLE_TRANSACTION, String.valueOf(b));
}

public static boolean isSingleTransaction() {
return Boolean.getBoolean(SINGLE_TRANSACTION);
}


}
25 changes: 16 additions & 9 deletions services/src/main/java/org/keycloak/services/resources/KeycloakApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.keycloak.Config;
import org.keycloak.common.crypto.CryptoIntegration;
import org.keycloak.config.ConfigProviderFactory;
import org.keycloak.exportimport.ExportImportConfig;
import org.keycloak.exportimport.ExportImportManager;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
Expand Down Expand Up @@ -113,10 +114,11 @@ public void run(KeycloakSession session) {
}

protected void shutdown() {
if (sessionFactory != null)
if (sessionFactory != null) {
sessionFactory.close();
}
}

private static class BootstrapState {
ExportImportManager exportImportManager;
boolean newInstall;
Expand Down Expand Up @@ -151,20 +153,25 @@ public void run(KeycloakSession session) {
var exportImportManager = bootstrapState.exportImportManager = new ExportImportManager(session);
bootstrapState.newInstall = applianceBootstrap.isNewInstall();
if (bootstrapState.newInstall) {
if (!exportImportManager.isImportMasterIncluded()) {
applianceBootstrap.createMasterRealm();
ExportImportConfig.setSingleTransaction(true);
try {
if (!exportImportManager.isImportMasterIncluded()) {
applianceBootstrap.createMasterRealm();
}
// these are also running in the initial bootstrap transaction - if there is a problem, the server won't be initialized at all
exportImportManager.runImport();
createTemporaryAdmin(session);
} finally {
ExportImportConfig.setSingleTransaction(false);
}
// these are also running in the initial bootstrap transaction - if there is a problem, the server won't be initialized at all
exportImportManager.runImport();
createTemporaryAdmin(session);
}
}
}
});

if (!bootstrapState.newInstall) {
bootstrapState.exportImportManager.runImport();
}

importAddUser();

return bootstrapState.exportImportManager;
Expand Down