Yair Zaslavsky has posted comments on this change. Change subject: aaa: Change builtin authenticators and directories initialization ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/25741/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java: Line 116: authConfig.put(ExtensionManager.NAME, "builtin-authn-internal"); Line 117: authConfig.put("ovirt.engine.aaa.authn.profile.name", "internal"); Line 118: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", "builtin-authz-internal"); Line 119: authConfig.put("ovirt.engine.aaa.authn.user", Config.<String> getValue(ConfigValues.AdminUser)); Line 120: authConfig.put("ovirt.engine.aaa.authn.password", Config.<String> getValue(ConfigValues.AdminPassword)); > ok... sorry for the late comment... but we need a new optional extension ma Did you mean an extension property? if not, where do you want this new sensitiveKeys property to be placed, in what configuration? Line 121: results.add(authConfig); Line 122: Line 123: Properties dirConfig = new Properties(); Line 124: dirConfig.put(ExtensionManager.CLASS, "org.ovirt.engine.extensions.aaa.builtin.internal.InternalDirectory"); http://gerrit.ovirt.org/#/c/25741/2/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalDirectory.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalDirectory.java: Line 32 Line 33 Line 34 Line 35 Line 36 > I would have put the above in config as well.... As this ID is hardcoded (not generated during setup) we can put it in the configuration /context of the ExternalAuthenticator , and then obtain it where needed in the authenticator code, similar for example to the way we obtain profile name. http://gerrit.ovirt.org/#/c/25741/2/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionManager.java: Line 136: } Line 137: activate(); Line 138: } Line 139: Line 140: private void load(ExtensionEntry entry) { > bad name that matches public interface. Done Line 141: ExtensionEntry alreadyLoaded = loadedEntries.get(entry.getName()); Line 142: if (alreadyLoaded != null) { Line 143: throw new ConfigurationException(String.format("Could not load the configuration %1$s. %2$s", Line 144: String.format("'%1$s' %2$s", entry.getName(), entry.file != null ? "from file " + entry.file.getAbsolutePath(): ""), Line 174: if (file.getName().endsWith(".properties")) { Line 175: try { Line 176: ExtensionEntry entry = Line 177: new ExtensionEntry(file); Line 178: load(entry); > simpler implementation.... Done Line 179: } catch (IOException exception) { Line 180: throw new ConfigurationException(String.format("Can't load object configuration file '%1$s'", Line 181: file.getAbsolutePath())); Line 182: } Line 233: for (ExtensionEntry entry : loadedEntries.values()) { Line 234: //Engine local config might override the enabled property of the configuration Line 235: // if a proper entry exists at the engine config. Line 236: entry.enabled = config.getBoolean(ENGINE_EXTENSION_ENABLED + entry.getName(), entry.enabled); Line 237: if (entry.enabled && entry.extension == null) { > now we can have race... maybe we should synchronize the load. well, we have one public load method for loading collection of properties. it can be called only after the singleton is initialized ( - that is, after getInstance() is called) - at this point, the call to activate on the private load(directory) method is done. I guess you are referring to the fact that you want to introduce a public load(properties) method (I guess for a single configuration for an extension). In this case - I agree. I also think we should consider removing load(Collection<Properties>) and just call load(properties) the amount of times needed, this will save us from creating the lists at InitOnStartup. What do you think? Line 238: try { Line 239: entry.extension = (Extension) lookupService( Line 240: Extension.class, Line 241: entry.getConfig().getProperty(CLASS), -- To view, visit http://gerrit.ovirt.org/25741 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id8513cb992c5becef7e83c04a8da8bc7f1622348 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
