-
Notifications
You must be signed in to change notification settings - Fork 464
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.
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
[WFCORE-6834] wildfly-elytron-integration jar duplicated in server modules #6014
Conversation
Core -> WildFly Preview Integration Build 13623 outcome was FAILURE using a merge of 28eeaef Failed tests
|
Core -> Full Integration Build 13824 outcome was FAILURE using a merge of 28eeaef Failed tests
|
@ivassile The failure seems related
|
Yes. I'm aware that we probably need changes in WF. Since I don't know all the detailed requirements for the original implementation which causes this issue, I asked @fjuma to review and provide feedback on the changes we need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this case, I realised that org.wildfly.extension.elytron.jaas-realm
is not really an extension, it is just a JBoss Modules module that exposes org/wildfly/extension/elytron/JaasCustomSecurityRealmWrapper.
My naive understanding of why it was implemented this way is that Elytron expects the use of a JBoss Modules module to create a custom realm. @fjuma, correct me if I am wrong here.
In such a case, it seems that naming it starting with org.wildfly.extension.
was not correct at all, as it makes it to look like a regular WildFly extension instead of a login module that can be used to provide a custom realm. @fjuma, are there any other reasons for naming it with the org.wildfly.extension.
prefix? Is there a requirement that forces us to keep the elytron integration artifact as a resource instead of a dependency?
@ivassile In any case, org.wildfly.extension.elytron.jaas-realm
is not an extension so we should remove it from HostExcludesTestCase
and host exclusions. HostExcludesTestCase
could be adapted to understand more these cases since what it does is to search for JBoss Modules modules that supplies a META-INF/services/org.jboss.as.controller.Extension
resource. It assumes if a JBoss Modules module supplies this resource, then the module is supplying an extension, but that's not the case for org.wildfly.extension.elytron.jaas-realm
module.
I've just opened https://issues.redhat.com/browse/WFLY-19378 to adress this.
@yersan @ivassile @fjuma The org.wildfly.extension.elytron.jaas-realm module is jboss.api=deprecated, which means we are a long time away from being able to rename it. So, while I agree that the name is unfortunate and it would be nice to rename it, I think the best we can do is make sure there's an issue to eventually remove the module, and even having that JIRA shouldn't block getting any fix done. Removing and renaming are both breaking changes so there's not much point doing two breaking changes when one will suffice. |
@bstansberry @fjuma @ivassile
I also want to correct this, I missinterpreted it. This module is not just exporting the org/wildfly/extension/elytron/JaasCustomSecurityRealmWrapper (there is an exclude). It looks like this configuration is exporting everything under If this is correct, I'm wondering if the correct approach would be to also export the moved resource as a dependency and add a filter to that dependency that excludes the aforementioned resource. |
@@ -48,5 +44,6 @@ | |||
<module name="org.jboss.logmanager"/> | |||
<module name="org.wildfly.common"/> | |||
<module name="org.wildfly.security.elytron-private" export="true"/> | |||
<module name="org.wildfly.extension.elytron"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most equivalent configuration would be by re-exporting org.wildfly.extension.elytron
from this module. We would like to limit it to the org.wildfly.extension.elytron
package only, so use the following:
<module name="org.wildfly.extension.elytron" export="true">
<exports>
<exclude path="org/wildfly/extension/elytron/*"/>
</exports>
</module>
@@ -33,10 +33,6 @@ | |||
<exclude path="org/wildfly/extension/elytron/JaasCustomSecurityRealmWrapper"/> | |||
</exports> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This export was not excluding JaasCustomSecurityRealmWrapper
as seems to be the intention, exports/exclude works only with paths and not with specific class resources, but JaasCustomSecurityRealmWrapper is not under a specific package so we cannot easily exclude it from this module export. We can safely remove it from here.
Hello @ivassile FYI, take a look at Zulip where we have been discussing these points |
Issue: https://issues.redhat.com/browse/WFCORE-6834
Requires WFLY-19378 wildfly/wildfly#17928 to pass CI.