-
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-5691][Preview] Allow setting timeout for token introspection #6033
base: main
Are you sure you want to change the base?
Conversation
Not sure if this would be a necessary fix that would prevent merging this, but in general model and schema bumps go in an independent commit, and on top of that one, any system change. This is up to the Elytron component leads. CC @fjuma |
@lvydra There is nothing written, but for nondefault stability level, we generally append the stability level at the PR title, e.g "[WFCORE-XYZ][Community] ...." |
This is likely to conflict with #5999 |
Yes, model and schema bumps should be part of a separate commit and should also have a separate WFCORE issue associated with it. Please coordinate with @PrarthonaPaul since both of you will need this. |
@@ -54,8 +54,9 @@ public enum ElytronSubsystemSchema implements PersistentSubsystemSchema<ElytronS | |||
VERSION_17_0(17), | |||
VERSION_18_0(18), | |||
VERSION_18_0_COMMUNITY(18, Stability.COMMUNITY), | |||
VERSION_19_0_PREVIEW(19, Stability.PREVIEW), |
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.
There is no reason to increment the numeric version.
The new schema version would just be 18.0:preview.
final PersistentResourceXMLDescription realmParserPreview_19_0 = decorator(ElytronDescriptionConstants.SECURITY_REALMS) | ||
.addChild(aggregateRealmParser_8_0) | ||
.addChild(customRealmParser) | ||
.addChild(customModifiableRealmParser) | ||
.addChild(identityRealmParser) | ||
.addChild(jdbcRealmParser_14_0) | ||
.addChild(keyStoreRealmParser) | ||
.addChild(propertiesRealmParser_14_0) | ||
.addChild(ldapRealmParser) | ||
.addChild(filesystemRealmParser_16) | ||
.addChild(tokenRealmParserPreview_19_0) | ||
.addChild(cachingRealmParser) | ||
.addChild(distributedRealmParser_18) | ||
.addChild(failoverRealmParser) | ||
.addChild(jaasRealmParser) | ||
.build(); |
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 is not a sustainable way to manage version-specific parsing logic. Why not add children conditionally based on schema version?
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 is the established pattern within the Elytron subsystem and is the correct way to handle this in this pull request.
static final SimpleAttributeDefinition CONNECTION_TIMEOUT = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.CONNECTION_TIMEOUT, ModelType.INT, true) | ||
.setAllowExpression(true) | ||
.setValidator(new IntRangeValidator(0)) | ||
.setRestartAllServices() | ||
.build(); | ||
|
||
static final SimpleAttributeDefinition READ_TIMEOUT = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.READ_TIMEOUT, ModelType.INT, true) | ||
.setAllowExpression(true) | ||
.setValidator(new IntRangeValidator(0)) | ||
.setRestartAllServices() | ||
.build(); | ||
|
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.
Are these attributes only meant to be registered when the server supports PREVIEW features? If so, you'll want to specify this.
e.g.
.setStability(Stability.PREVIEW)
static final SimpleAttributeDefinition CONNECTION_TIMEOUT = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.CONNECTION_TIMEOUT, ModelType.INT, true) | ||
.setAllowExpression(true) | ||
.setValidator(new IntRangeValidator(0)) | ||
.setRestartAllServices() | ||
.build(); | ||
|
||
static final SimpleAttributeDefinition READ_TIMEOUT = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.READ_TIMEOUT, ModelType.INT, true) | ||
.setAllowExpression(true) | ||
.setValidator(new IntRangeValidator(0)) | ||
.setRestartAllServices() | ||
.build(); |
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.
Same as above.
private String readResource(String name) throws IOException { | ||
String namespaceUri = ElytronSubsystemSchema.CURRENT.get(Stability.DEFAULT).getNamespace().getUri(); | ||
String version = namespaceUri.substring(namespaceUri.lastIndexOf(':') + 1); | ||
if (!name.contains(version + ".xml")) { | ||
return ModelTestUtils.readResource(getClass(), name.replace("elytron", "legacy-elytron-subsystem")); | ||
} else { | ||
String previewNamespaceUri = ElytronSubsystemSchema.CURRENT.get(Stability.PREVIEW).getNamespace().getUri(); | ||
String previewVersion = previewNamespaceUri.substring(previewNamespaceUri.lastIndexOf(':') + 1); | ||
if (name.contains(version + ".xml") || name.contains(previewVersion + ".xml")) { | ||
return ModelTestUtils.readResource(getClass(), name.replace("elytron", "elytron-subsystem")); | ||
} else { | ||
return ModelTestUtils.readResource(getClass(), name.replace("elytron", "legacy-elytron-subsystem")); | ||
} | ||
} |
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.
I don't understand the purpose of mockReadResourceWithValidSubsystemTestFilePaths(). If the goal is simply to override the default file names of the subsystem test files, would it not be far simpler to just override the method used to generate them?
e.g.
@Override
protected String getSubsystemXmlPathPattern() {
// Same as super impl, but with "-subsystem-" thrown in
String pattern = (this.schema.getStability() == Stability.DEFAULT) ? "%1$s-subsystem-%2$d.%3$d.xml" : "%1$s-subsystem-%4$s-%2$d.%3$d.xml";
// If not a current schema, prefix with "legacy-"
return !ElytronSubsystemSchema.CURRENT.values().contains(this.schema) ? "legacy-" + pattern : pattern;
}
Here is the commit for the subsystem bump for elytron from community:18.0 to preview:18.0 : 5365664 @lvydra You can cherry-pick that to your PR and add your RFE changes on top. |
…ersion 19.0.0 to 20.0.0
Just one small change on the comment from @PrarthonaPaul - @lvydra please rebase your changes on top of that commit so you both preserve the same SHA - where we use a common commit we are trying to ensure it happens only once. |
Also the Jira issue and this PR seem to be referencing the Community stability level but the code seems to be targeting Preview. I would suggest we continue with Preview as we can then promote to Community or Default in a later step. |
Thanks @PrarthonaPaul, I have rebased my changes on top of your subsystem bump commit. |
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
/retest |
Core -> Full Integration Build 13903 outcome was FAILURE using a merge of d07f14e Failed tests
|
@pferraro Does this look ok now? |
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
https://issues.redhat.com/browse/EAP7-1856
https://issues.redhat.com/browse/WFCORE-5691
https://issues.redhat.com/browse/ELY-2189