-
Notifications
You must be signed in to change notification settings - Fork 465
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-6755] Move the org.wildfly.security:wildfly-elytron-dynamic-ssl artifact into its own module #5947
Conversation
Core -> Full Integration Build 13459 outcome was FAILURE using a merge of 7f57b63 |
Core -> WildFly Preview Integration Build 13519 outcome was FAILURE using a merge of 7f57b63 |
Core -> Full Integration Build 13718 outcome was FAILURE using a merge of 7f57b63 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Hello @Skyllarr , we need a WFLY Jira/PR in place to fix the layers test case in WildFly. We can merge this WFCORE PR as the latest PR for this release, but we need the Jira/PR to run manually the integration Job. An alternative is to hack the WidlFly LayersTestCase to ignore this specific layer, and then remove the hack once Core is bumped on WildFly |
Core -> WildFly Preview Integration Build 13527 outcome was FAILURE using a merge of a218264 Failed tests
|
Thanks @yersan , I've created https://issues.redhat.com/browse/WFLY-19223 and I'm working on fixing the test |
Integration Jobs: https://ci.wildfly.org/buildConfiguration/WF_WildFlyCoreIntegrationExperiments/429188 (Galleon Integration) |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
@Skyllarr The integration test failed, if we really don't want to provision this module when we are trimming the server, the fix in WildFly Full should be to add it at https://github.com/wildfly/wildfly/blob/a53eba00bde46400befd053c4a104e7107da0071/testsuite/shared/src/main/java/org/jboss/as/test/shared/LayersTestBase.java#L45 instead.
But I have doubts whether this is want we want to do, I would expect this feature also when we are trimming the server but we are including the Elytron layer
...esources/modules/system/layers/base/org/wildfly/security/elytron-dynamic-ssl/main/module.xml
Outdated
Show resolved
Hide resolved
@@ -26,6 +26,7 @@ public class LayersTestCase { | |||
// No patching modules in layers | |||
"org.jboss.as.patching", | |||
"org.jboss.as.patching.cli", | |||
"org.wildfly.security.elytron-dynamic-ssl" |
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.
If the intention is to leave the usage of this module when the server is trimmed including the Elytron layer, this change could be fine but we need to add a comment above it explaining the reason why this module is not provided when using layers.
I have doubts we want that, it looks like the intention could be to also provide the capabilities of the dynamic SSL context even when we are using the Elytron layer. If that's the case, I guess you have to register the module with
wildfly-core/controller/src/main/java/org/jboss/as/controller/ResourceDefinition.java
Line 127 in d5456e9
default void registerAdditionalRuntimePackages(final ManagementResourceRegistration resourceRegistration) { |
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.
should be invoked depending on the current Stability level
or as a different way to see it, be invoked when the management resource associated with this Elytron Dynamic SSL is being registered, which I would assume is registered for community only stability level
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.
@yersan yes, this module should be provided when using elytron layer and the community stability. I'll fix this, thank you!
This comment was marked as outdated.
This comment was marked as outdated.
@yersan I've addressed your feedback. I closed the affiliated wildfly PR since these latest changes do not introduce a failure in wildfly testsuite. Thank you! |
Core -> Full Integration Build 13495 outcome was UNKNOWN using a merge of f56300e |
Core -> WildFly Preview Integration Build 13556 outcome was FAILURE using a merge of f56300e Failed tests
|
Core -> Full Integration Build 13757 outcome was UNKNOWN using a merge of f56300e |
This comment was marked as outdated.
This comment was marked as outdated.
bf0a659
to
7bf0715
Compare
Core -> Full Integration Build 13497 outcome was UNKNOWN using a merge of 7bf0715 |
Core -> Full Integration Build 13498 outcome was FAILURE using a merge of 7bf0715 Failed tests
|
Core -> WildFly Preview Integration Build 13557 outcome was FAILURE using a merge of 7bf0715 Failed tests
|
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.
In my opinion, this is completed, although we are waiting for https://issues.redhat.com/browse/WFCORE-6805 to do more testing and see if the approach is the correct one. We are waiting to have the ability to limit the available stability levels based on the provisioning stability level
Well, current failures on integration tests are not expected, so it still requires investigation into what is causing them. |
I see, thanks @yersan ! I will look into the integration tests failures |
…sl artifact into its own module
Core -> Full Integration Build 13532 outcome was FAILURE using a merge of af6dd7c Failed tests
|
Core -> WildFly Preview Integration Build 13587 outcome was FAILURE using a merge of af6dd7c Failed tests
|
@yersan This PR should solve the issues with LayersTestCase wildfly/wildfly#17862 , locally it works for me |
@Skyllarr There is a relevant error on the "Galleon Linux - JDK 11 (Pull Request)" Job:
I don't know exactly what is causing that, but the problem seems to be that I've just taken a look but I don't see the root cause, since Elytron subsystem is advertising to Galleon the requirement of this package at https://github.com/wildfly/wildfly-core/pull/5947/files#diff-263be4d2ab5108fc2de76f44766487703d2444e7bcc70bac0aaa4be6a27a9089R114 @jfdenise Could you shed some light on this? I've just done a quick test and, although the relevant package is correctly advertised with What we are missing here? |
@jfdenise a friendly reminder, do you have any points about why Galleon does not provision the advertised package? |
Hello @jfdenise this is still a blocker for us, could you provide your feedback about why Galleon is not working as we expect for this case? I am not sure if we are missing something here. |
@yersan Is the resource part of the elytron subsystem configuration provided by the 'elytron' layer? Or, less likely, some other layer included in the test-all-layers set? I haven't looked at what the 'elytron' layer configures but I suspect it doesn't add this resource. The resourceRegistration.registerAdditionalRuntimePackages stuff is only relevant if the resource is part of the configuration. It doesn't tell Galleon to provision things that are not in the configuration just in case they user might want to add them later. If that's the case the solution is to tell LayersTestCase not to expect the module to be provisioned. |
The resource is just a subresource of the standard Elytron subsystem, however, it is distributed via an optional module, which, in turn, is never provisioned by any other Galleon Layer.
Ok, thanks for clarifying it. Now I understand the issue is that since not configured by the Elytron subsystem when it is provisioned as a Galleon layer, then the source code will never advertise it requires such a package. However, this subresource does not require any Elytron subsystem configuration, so, I don't know how it can be added to the Elytron layer as an empty feature. I can only think of adding it as a required package for the Elytron layer at: Lines 16 to 20 in bb0806b
But not sure if that is the correct approach. It fixes the problem, since it makes it provisioned always when Elytron is configured. but not sure if that is the approach or even if we add it there, whether we would still need the resourceRegistration.registerAdditionalRuntimePackages stuff |
I think to invert some of the discussions the two times we would expect that this module is NOT provisioned are:
In all other cases this module should be present in the installation |
Superseded by #6066 |
https://issues.redhat.com/browse/WFCORE-6755