[WFCORE-4407] Configure Elytron domain services with LAZY initial mode #3729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Core - Full Integration Build 8474 outcome was UNKNOWN using a merge of c89d68f |
|
I forgot rebasing with the latest in master, hence the two previous errors, hopefully, CI will start over again |
yersan
commented
Apr 8, 2019
elytron/src/test/java/org/wildfly/extension/elytron/DomainTestCase.java
Outdated
Show resolved
Hide resolved
yersan
commented
Apr 8, 2019
elytron/src/test/resources/org/wildfly/extension/elytron/domain-test.xml
Outdated
Show resolved
Hide resolved
wildfly-ci
added
the
deps-ok
|
The changes in this PR are still incomplete. SaslAuthenticationFactory and HttpAuthenticationFactory services must be down to be able to configure Elytron, so more services need to be changed from ACTIVE to LAZY |
|
I am just wondering if the authentication factories could be problematic - I think they have a runtime attribute to show the authentication mechanisms they support. |
|
Core - Full Integration Build 8497 outcome was UNKNOWN using a merge of 8dc61f7 |
yersan
force-pushed
the
bugs/WFCORE-4407
branch
from
8dc61f7
to
2bd408e
Compare
|
Core - Full Integration Build 8498 outcome was FAILURE using a merge of 2bd408e Failed tests |
|
@darranl yes, there is one runtime attribute, I'm looking for alternatives and getting familiar with the behavior. Obviously, CI errors are related to this PR, you could add the hold label again :( |
|
One option to consider could be changing the start up mode for services with runtime attributes and operations based on the running mode of the server - so for embedded admin keep the factorieslazy but leave them active for the rest. The other services can remain lazy as they will be started only when required. |
yersan
force-pushed
the
bugs/WFCORE-4407
branch
from
2bd408e
to
e5676cb
Compare
|
@darranl I followed your idea. Maybe there could be more child services under SecurityDomain that could be changed to LAZY as well, but since there are dependencies between all of them and other services, I was not sure if we could have all of them LAZY too, change them affects to the test suite and I wanted to minimize the changes, so I focused only on the parent services: authentication factories (http/sals) and ssl context. Let's see what CI says. |
yersan
commented
Apr 12, 2019
elytron/src/main/java/org/wildfly/extension/elytron/TrivialAddHandler.java
Outdated
Show resolved
Hide resolved
yersan
force-pushed
the
bugs/WFCORE-4407
branch
from
e5676cb
to
682084f
Compare
|
@yersan is there any reason to hold this any longer or can we go ahead and merge? We have some subsystem enhancements on the way through so these changes risk causing a conflict if we hold them all to the last minute. |
|
Hello @darranl ; sorry for the delay. I'm still not comfortable with the changes included in this PR. I tested again this morning with the latest branches and I cannot use this sequence in an embedded server to configure Elytron and legacy security domain: Should I close it and open it when it works or it is fine to keep the hold label? |
|
Hello @yersan I think if it is acknowledged that a PR needs more than minor changes before being merged I think it is better to close it down and reopen once ready to leave us with a cleaner queue. Having said that I think the changes you have made so far even though they do not completely address your problem could be merged and you use a new Jira issue to continue working on the specific problem you are addressing. Overall your investigations do identify we have too many active services in this subsystem. |
|
Hi @darranl, the problem I had was in the CLI script generated to test this issue. I've just corrected it and now this branch works as I expected. Would you mind checking this feature again and reviewing the hold label?, sorry for all the mess. Please keep in mind there could be even more services that can be changed to LAZY, I focussed only on in these: authentication factories (http/sals) and ssl context. Maybe we can create a new issue to address the remainder. Just in case, this is my corrected script: |
darranl
approved these changes
May 15, 2019
|
I have kicked off CI again to double check with recent changes but I think this is ready to merge. |
jmesnil
added
the
ready-for-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change the initial mode of some of the Security Domain service graph as LAZY to allow configuring them in an embedded server running in admin-mode.
Jira issue: https://issues.jboss.org/browse/WFCORE-4407
CC: @darranl