WFLY-19078 [Preview] - Update the elytron-oidc-client subsystem parser to enumerate schema versions #17705

PrarthonaPaul · 2024-03-11T18:35:42Z

Issue: https://issues.redhat.com/browse/WFLY-19078

More information about the wildfly-bot[bot]

pferraro · 2024-03-12T17:39:26Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+public enum ElytronOidcSubsystemSchema implements PersistentSubsystemSchema<ElytronOidcSubsystemSchema> {
+    VERSION_1_0(1),
+    VERSION_2_0(2),
+    VERSION_3_0_COMMUNITY(3, 0, Stability.COMMUNITY),


Since there is no community-specific namespace for 2.0, you don't need to increment the version.
e.g.

VERSION_2_0_COMMUNITY(2, 0, Stability.COMMUNITY)

I have changed the community version to version 2 and updated the schema and test files.
Quick question, when attributes in 2.0 community move to default, will they be part of subsystem version 3.0 or 2.0?

Since 2.0 (default) already exists, you would increment to 3.0.

So, would i go back to VERSION_3_0_COMMUNITY(3, 0, Stability.COMMUNITY),?

You would only introduce community:3.0 if you are adding new features to the existing community:2.0.

The model version of a subsystem identifies a set of potential features exposed by a subsystem, independent of whether or not they are enabled (or indeed enablable) by a user's server configuration. For a given version of WildFly, the ModelVersion of a subsystem is fixed. This value is merely descriptive, and is largely invisible to users. In general, a subsystem increments this version when the set of potential features has changed since the previous WildFly release.

That said, for this PR, you want to increment your subsystem model version to 3.0.0.

In contrast, a subsystem's XML schema describes what can be configured for a given subsystem. A given version of WildFly can boot using XML that complies with any schema version of a given subsystem. However, for a given subsystem, what can be configured is constrained by the stability level of the server process. That we version these using numbers is merely a convention (albeit a useful one), as is the inclusion of the stability level to the namespace uri - the uniqueness of the namespace URI is all that actually matters.

In general, we need to create a new schema namespace if the current namespace was already published by a previous WildFly release.
That said, for this PR, we definitely need a new schema namespace.

Technically, the only requirement for a new schema namespace is that its URI should be distinct from any existing one. In other words, you could name these however you wanted, so long as you take care to only register schemas supported by the current server process.

That said, requiring subsystems to perform conditional schema registration depending on the stability level of the server is something we want to avoid. To that effect, the ExtensionParsingContext, i.e. the thing with which you register your subsystem parsers, will introspect the Stability associated with a given SubsystemSchema and respond appropriately.

If you used "wildfly:elytron-oicd-client:3.0" for the new namespace, the default logic of SubsystemSchema.getStability() would return Stability.DEFAULT. This would otherwise cause ExtensionParsingContext to treat this as a valid parser for a server started using --stability=default, which would be incorrect. Technically, you could override the value return by this method, but that would likely introduce confusion, as it would not be clear to which stability level the schema applies.

That leaves 2 obvious options:

"wildfly:elytron-oicd-client:community:2.0"

"wildfly:elytron-oicd:client:community:3.0"

Either will technically work with the existing infrastructure as they are distinct from the existing "wildfly:elytron-oicd-client:2.0".

Let's say the next release of WildFly adds a new preview feature to this subsystem. For a server running in preview, the new schema includes changes from this PR. Using the first convention, the next schema becomes "wildfly:elytron-oicd-client:preview:2.0". However, using the 2nd convention, the next schema becomes "wildfly:elytron-oicd:client:preview:4.0".

Now consider a subsequent release of WildFly adding a new default feature to this subsystem. Both cases require a version bump to avoid namespace collisions.
If we follow the conventions from the first option, the new names become:
"wildfly:elytron-oicd:client:preview:3.0"
"wildfly:elytron-oicd:client:community:3.0"
"wildfly:elytron-oicd:client:3.0"
If we follow the conventions from the second option, the new names become:
"wildfly:elytron-oicd:client:preview:5.0"
"wildfly:elytron-oicd:client:community:5.0"
"wildfly:elytron-oicd:client:5.0"

From a technical perspective, both conventions are problem free. The first convention results in more compact versions, while the second leaves gaps.
I find the first convention to be a bit more intuitive since, it result in more compact version numbers, while the second convention always results in version gaps for the default stability level, e.g. both "wildfly:elytron-oicd:client:3.0" and "wildfly:elytron-oicd:client:4.0" never existed.

That was a bit of a ramble, but I hope this was more helpful than not.

Are you missing two namespaces in the last option @pferraro

FYI I like "wildfly:elytron-oicd-client:community:2.0" as that seems to be what @Skyllarr did in her PR ;-)

+1, I agree that going with 2.0 makes sense and matches what Diana did as well.

Thanks @fjuma, @pferraro and @darranl for your input.
I have added a new commit to convert the schema to preview:2.0 since we have decided to move WFLY-17143 and WFLY-16532 to preview as well. I have also switched back to not incrementing the model version to 3.0.0.

@PrarthonaPaul The model version still needs to be incremented to 3.0.

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcExtension.java

pferraro · 2024-03-12T17:47:10Z

...client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemModel.java

+enum ElytronOidcClientSubsystemModel implements SubsystemModel {
+    VERSION_1_0_0(1, 0, 0),
+    VERSION_2_0_0(2, 0, 0),
+    VERSION_3_0_0_COMMUNITY(3, 0, 0),


A ModelVersion has no stability-level component, so this should just be VERSION_3_0_0.

I will change it to just version 1 and 2.
In that case, should I remove the following function?

wildfly/elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

Lines 47 to 51 in 8988494

private static void from3(ChainedTransformationDescriptionBuilder chainedBuilder) {

ResourceTransformationDescriptionBuilder builder = chainedBuilder.createBuilder(VERSION_3_0_0_COMMUNITY.getVersion(), VERSION_2_0_0.getVersion());

builder.addChildResource(PathElement.pathElement(SECURE_SERVER))

.getAttributeBuilder();

}

I have removed it for now

If the management model (for any stability level) changes, you still need to increment the model version.

I have incremented the model version to 3 and included a from3(...) method

pferraro · 2024-03-12T17:51:09Z

...nt/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemRegistrar.java

+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ */
+
+public class ElytronOidcClientSubsystemRegistrar implements SubsystemResourceDefinitionRegistrar {


Where is this class used?
I can't seem to find anything that calls this object's register(...) method.
Did you mean to reference this from your Extension?

I have removed the register(...) method.

fjuma · 2024-03-21T21:58:32Z

@pferraro When you get a chance, please take a look at @PrarthonaPaul's updates for this one. Thanks!

fjuma · 2024-03-22T19:08:06Z

...nt/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemRegistrar.java

+
+public class ElytronOidcClientSubsystemRegistrar {
+
+    static final String NAME = "elytron-oidc-client";


Is this variable needed? Can ElytronOidcExtension.SUBSYSTEM_NAME be used?

removed NAME and using ElytronOidcExtension.SUBSYSTEM_NAME instead

fjuma · 2024-03-22T19:12:55Z

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

        builder.rejectChildResource(PathElement.pathElement(SECURE_SERVER));
    }
+    private static void from3(ChainedTransformationDescriptionBuilder chainedBuilder) {


I don't think transformers are needed for preview changes.

@fjuma @PrarthonaPaul They aren't needed. But....

In the draft process doc I encourage writing them, under the theory that waiting to do so may result in problems not being caught until we try to promote a feature to 'default'. But that was before we'd done any actual non-default resources or attributes, so we haven't really thought through what this means in practice.

I'm interested in what @yersan plus you, @darranl and @pferraro and (and everybody really) thinks about this.

Prerequisite: I'm assuming adding transformation stuff for resources/attributes that don't exist at 'default' stability works at all; i.e. the kernel transformation code doesn't blow up when processing non-existent stuff. If that assumption is wrong, then we should go with Option 1 below.

Some options:

Drop the suggestion to write transformers from the process doc. Require PRs that add them to remove them.

Keep the suggestion but ask people who add them to comment out the functional bits; just leave the 'framing' in place.

Allow them, and require that they are correct if present. (I don't think what's here is correct.)

By correct I mean:

3.a) if the resources/attributes being transformed were at 'default' then the transformation is correct.
3.b) while the resources/attributes are not at default, the transformation does no harm.

I think 3.b) is the tricky one, but in most cases it should be fine. If my memory is correct and a secondary HC can't register unless the DC is running at 'default' and the secondary is too, then the common case of 'reject this new thing' if present will be fine, as 'this new thing' cannot be part of the config.

If we go with option 3, for cases where 3.b) is an issue then we can ask that the problematic part is commented out.

Options 1 and 2 are also valid. In the end we need to examine the transformation rules when the feature moves to default, even if the code already exists and is just starting to take effect. Maybe we'll be more likely to check the transformers if every PR moving resources/attributes to default is required to have a transformer section; i.e. it can never rely on one that was previously written.

If my memory is correct and a secondary HC can't register unless the DC is running at 'default' and the secondary is too, then the common case of 'reject this new thing' if present will be fine, as 'this new thing' cannot be part of the config.

That was what I had in my mind, but weeks ago I learnt that's not true, and there is a reason. At this moment we are only allowing domain mode for paired stability levels. E.g DC community -- Secondary HC community, preview -- preview, default -- default ... and so on. If we do not allow it, and we restrict it to default -- X, then users would be forced to run Domain Mode always in default stability level, however, community is the default stability level for WildFly. This situation "forces" us to always create transformers. Notice we are not taking into account whether we are running a mixed domain (different host controller versions).

Another point is we have always discouraged mixed domain usage in WildFly, correct me if I am wrong. Basically, because our mixed domain integration testsuite is limited to only certain versions. This is also a bit more problematic now since we are not forced to bump the schema of any new attribute/operation added at the community stability level and lower levels. Maybe should we?

Having said that, if we want to continue "allowing" mixed domains in WildFly we are forced to write always transformers and, I understand we are also forced to bump the schema model on each feature that involves management resource changes.

Another alternative would be to restrict the HC registration to only on the default stability level for mixed domains only. That would leverage the requirements of writing transformers only when we are promoting to default, but we could add them if we want for testing purposes and to prepare the changes for the future promotion of the feature to default stability level. @bstansberry I guess that would make your option 3) to fit well.

Am I missing something? Maybe this should be moved to Zulip.

@yersan Yes, we should discuss this in Zulip. We should see if we can figure out how to have different rules for mixed domain as opposed to domain mode overall.

In any case what Prarthona has here is fine for 32 Beta, and really for 32 Final. I don't think we want to support mixed domains involving preview stability level HCs.

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

fjuma · 2024-03-22T19:16:53Z

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ProviderDefinition.java

@@ -28,6 +30,7 @@
 */
 class ProviderDefinition extends SimpleResourceDefinition {

+    static final ResourceRegistration PATH = ResourceRegistration.of(PathElement.pathElement(ElytronOidcDescriptionConstants.PROVIDER), Stability.DEFAULT);


Just to check, where does this get used?

Here:

wildfly/elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

Line 100 in 1983491

.addChild(factory.builder(ProviderDefinition.PATH)

In that case, you should reuse this constant in your constructor, rather than define the PathElement twice.

bstansberry · 2024-03-24T18:21:10Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.RESOURCE;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.TOKEN_MINIMUM_TIME_TO_LIVE;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.TURN_OFF_CHANGE_SESSION_ID_ON_LOGIN;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.USE_RESOURCE_ROLE_MAPPINGS;


Static imports come first.

@bstansberry This is something we should enforce via checkstyle rules.

moved to the top

@bstansberry This is something we should enforce via checkstyle rules.

+1. I have no idea why this wasn't done in, say, 2011.

Answering my own question -- it wasn't done because by the time we had a solid 'style' we already had too much code to fix if we turned checkstyle onto it.

true - whoever enables it would need to be willing to fix the errors!

@bstansberry @pferraro
Would you like me to create an unassigned WFLY issue for the time being to track this?

bstansberry · 2024-03-24T18:21:25Z

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

+import static org.wildfly.extension.elytron.oidc.ElytronOidcClientSubsystemModel.VERSION_2_0_0;
+import static org.wildfly.extension.elytron.oidc.ElytronOidcClientSubsystemModel.VERSION_3_0_0;
+import static org.wildfly.extension.elytron.oidc.ElytronOidcDescriptionConstants.SECURE_DEPLOYMENT;
+import static org.wildfly.extension.elytron.oidc.ElytronOidcDescriptionConstants.SECURE_SERVER;


Static imports comes first.

moved to the top

bstansberry · 2024-03-24T18:22:48Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+import org.jboss.staxmapper.IntVersion;
+
+import java.util.EnumSet;
+import java.util.Map;


java and javax packages come before other packages, after static imports

moved to the top, after static imports

bstansberry · 2024-03-24T18:23:38Z

...ent/src/test/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemTestCase.java

+import org.junit.runners.Parameterized.Parameters;
+
+import java.util.EnumSet;
+import java.util.Properties;


java and javax packages come before others, after static imports

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/RealmDefinition.java

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java

pferraro · 2024-03-27T14:37:01Z

...nt/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemRegistrar.java

+ * @author <a href="mailto:prpaul@redhat.com">Prarthona Paul</a>
+ */
+
+public class ElytronOidcClientSubsystemRegistrar {


What is the intended behavior of this class? It is named "Registrar", but does not register anything. Did you intend to implement SubsystemResourceDefinitionRegistrar?

I have removed this class. I think initially I was trying to implement SubsystemResourceDefinitionRegistrar, but did not need it.
Thanks for catching it

pferraro · 2024-03-27T14:37:17Z

...nt/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemRegistrar.java

+    static final AttributeParser SIMPLE_ATTRIBUTE_PARSER = new AttributeElementParser();
+    static final AttributeMarshaller SIMPLE_ATTRIBUTE_MARSHALLER = new AttributeElementMarshaller();
+
+     static class AttributeElementMarshaller extends AttributeMarshaller.AttributeElementMarshaller {


Nit: the formatting is off here.

removed this class

pferraro · 2024-03-27T14:46:24Z

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcExtension.java

+        context.setSubsystemXmlMapping(SUBSYSTEM_NAME, ElytronOidcSubsystemSchema.VERSION_1_0.getNamespace().toString(), ElytronOidcSubsystemSchema.VERSION_1_0);
+        context.setSubsystemXmlMapping(SUBSYSTEM_NAME, ElytronOidcSubsystemSchema.VERSION_2_0.getNamespace().toString(), ElytronOidcSubsystemSchema.VERSION_2_0);
+        context.setSubsystemXmlMapping(SUBSYSTEM_NAME, ElytronOidcSubsystemSchema.VERSION_2_0_PREVIEW.getNamespace().toString(), ElytronOidcSubsystemSchema.VERSION_2_0_PREVIEW);


Take advantage of your enumeration, so that you don't need to touch this method again.

context.setSubsystemXmlMappings(SUBSYSTEM_NAME, EnumSet.allOf(ElytronOidcSubsystemSchema.class));

...-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemDefinition.java

pferraro · 2024-03-27T14:48:04Z

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ProviderDefinition.java

@@ -28,6 +30,7 @@
 */
 class ProviderDefinition extends SimpleResourceDefinition {

+    static final ResourceRegistration PATH = ResourceRegistration.of(PathElement.pathElement(ElytronOidcDescriptionConstants.PROVIDER), Stability.DEFAULT);


In that case, you should reuse this constant in your constructor, rather than define the PathElement twice.

pferraro · 2024-03-27T14:55:08Z

elytron-oidc-client/src/test/java/org/wildfly/extension/elytron/oidc/OidcTestCase.java

        super(ElytronOidcExtension.SUBSYSTEM_NAME, new ElytronOidcExtension());
    }

    @Before
    public void prepare() throws Throwable {
        if (services != null) return;
        String subsystemXml = "oidc.xml";
-        services = super.createKernelServicesBuilder(new DefaultInitializer()).setSubsystemXmlResource(subsystemXml).build();
+        services = super.createKernelServicesBuilder(new DefaultInitializer(Stability.PREVIEW)).setSubsystemXmlResource(subsystemXml).build();


This should use the stability associated with the currently tested schema.

...client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemModel.java

pferraro · 2024-03-27T15:23:04Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.RESOURCE;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.TOKEN_MINIMUM_TIME_TO_LIVE;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.TURN_OFF_CHANGE_SESSION_ID_ON_LOGIN;
+import static org.wildfly.extension.elytron.oidc.SecureDeploymentDefinition.USE_RESOURCE_ROLE_MAPPINGS;


true - whoever enables it would need to be willing to fix the errors!

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

pferraro · 2024-03-27T15:34:36Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+                        .addAttribute(ProviderAttributeDefinitions.ALLOW_ANY_HOSTNAME, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.ALWAYS_REFRESH_TOKEN, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.AUTH_SERVER_URL, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.AUTODETECT_BEARER_ONLY, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CLIENT_KEY_PASSWORD, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CLIENT_KEYSTORE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CLIENT_KEYSTORE_PASSWORD, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CONFIDENTIAL_PORT, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CONNECTION_POOL_SIZE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CONNECTION_TIMEOUT_MILLIS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CONNECTION_TTL_MILLIS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CORS_ALLOWED_HEADERS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CORS_ALLOWED_METHODS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CORS_EXPOSED_HEADERS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.CORS_MAX_AGE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.DISABLE_TRUST_MANAGER, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.ENABLE_CORS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.EXPOSE_TOKEN, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.IGNORE_OAUTH_QUERY_PARAMETER, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.PRINCIPAL_ATTRIBUTE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.PROVIDER_URL, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.PROXY_URL, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.REALM_PUBLIC_KEY, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.REGISTER_NODE_AT_STARTUP, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.REGISTER_NODE_PERIOD, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.SOCKET_TIMEOUT_MILLIS, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.SSL_REQUIRED, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.TOKEN_SIGNATURE_ALGORITHM, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.TOKEN_STORE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.TRUSTSTORE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.TRUSTSTORE_PASSWORD, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)
+                        .addAttribute(ProviderAttributeDefinitions.VERIFY_TOKEN_AUDIENCE, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER)


Why not ... ?

.addAttributes(Stream.of(ProviderAttributeDefinitions.ATTRIBUTES))

(same for other resources)

We need to specify the parser and marshaller specifically, because otherwise when I added it as above, I got a lot of parser errors. Adding the attributes one at a time was the only way I could fix it.
Thanks @fjuma and @pferraro for the comments.
I have added the changes in a separate commit to be squashed later.

This is because these attributes do not override their parser/marshaller, such that AttributeDefinition.getParser() and getAttributeMarshaller() does not return the expected SIMPLE_ATTRIBUTE_PARSER and SIMPLE_ATTRIBUTE_MARSHALLER, respectively.
That said, I think it was a poor design choice that XML behavior was ever hardwired into the AttributeDefinition, so I do think it is preferable to specify custom parsing/marshalling behavior in your PersistentResourceXMLDescription.
To accommodate this, I've opened https://issues.redhat.com/browse/WFCORE-6763 to add a PersistentResourceXMLDescription.Builder.addAttributes(Stream<AttributeDefinition>, AttributeParser, AttributeMarshaller) method.
In the meantime, you can use:

Stream.of(ProviderAttributeDefinitions.ATTRIBUTES).forEach(attribute -> builder.addAttribute(attribute, SIMPLE_ATTRIBUTE_PARSER, SIMPLE_ATTRIBUTE_MARSHALLER));

or similar.

Okay, perfect!
I believe the reason why we are using a custom marshaller and parser is for easy migration from the legacy keycloak adapter. The elytron team wanted to have the structure of the new subsystem be as close to the older one as possible.
I will update this and can create an issue to update it to use PersistentResourceXMLDescription.Builder.addAttributes(Stream<AttributeDefinition>, AttributeParser, AttributeMarshaller) in the future once WFCORE-6763 is merged.
Thank you!

I have made these updates. I could not add SecureDeploymentDefinition.ALL_ATTRIBUTES directly because CredentialDefinition.CREDENTIAL and RedirectRewriteRuleDefinition.REDIRECT_REWRITE_RULE were added to SecureDeploymentDefinition.ALL_ATTRIBUTES and were clashing with when I added them as children.
So, I had to create an array first including all the attributes minus credentials and redirect-rewrite-rule

Thanks @pferraro
Please let me know is this PR looks good (or if there are any changes I should make). Once this is good to go, I can squash the commits and add it to WFLY-16532.

I have created https://issues.redhat.com/browse/WFLY-19177 to track the changes needed to add attributes using a custom parser and marshaller.

PrarthonaPaul · 2024-03-27T20:10:39Z

elytron-oidc-client/src/test/java/org/wildfly/extension/elytron/oidc/OidcTestCase.java


 /**
 * Subsystem parsing test case.
 *
 * <a href="mailto:fjuma@redhat.com">Farah Juma</a>
 */
-@RunWith(Parameterized.class)
-public class OidcTestCase extends AbstractSubsystemTest {
+public class OidcTestCase extends AbstractSubsystemSchemaTest<ElytronOidcSubsystemSchema> {


In order to make sure we could specify the stability and schema version this test runs with, I had to change this to extend AbstractSubsystemSchemaTest instead.
Same with ExpressionsTestCase

darranl · 2024-04-02T10:40:53Z

Just added the labels for 32.x so we can continue to evaluate today.

…ser to enumerate schema versions

bstansberry · 2024-04-02T18:29:22Z

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java

+import org.jboss.staxmapper.XMLExtendedStreamReader;
+
+import javax.xml.stream.XMLStreamException;
+import javax.xml.stream.XMLStreamWriter;


@PrarthonaPaul This shouldn't block merging this, but these should move to immediately after the java.* ones.

Good catch, I've created a follow-up issue to track this (https://issues.redhat.com/browse/WFLY-19196).

I'm dismissing my review as my main checkstyle concerns have been addressed. Farah and Paul are doing the more substantive review.

fjuma

Adding my approval on this one since follow up issues have been created to address the remaining items (these have also been linked from WFLY-19078).

fjuma · 2024-04-02T21:15:35Z

@PrarthonaPaul Just noticed, there are failures in MigrateOperationTestCase that do look related:

java.lang.AssertionError: WFLYCTL0158: Operation handler failed: java.lang.NoClassDefFoundError: Could not initialize class org.wildfly.extension.elytron.oidc.ElytronOidcSubsystemDefinition
  at org.junit.Assert.fail(Assert.java:89)
  at org.junit.Assert.assertTrue(Assert.java:42)
  at org.jboss.as.model.test.ModelTestUtils.checkOutcome(ModelTestUtils.java:119)
  at org.jboss.as.subsystem.test.AbstractSubsystemTest.checkOutcome(AbstractSubsystemTest.java:161)
  at org.keycloak.subsystem.adapter.extension.MigrateOperationTestCase.testMigrateDefaultKeycloakConfig(MigrateOperationTestCase.java:73)

fjuma · 2024-04-02T21:36:02Z

@PrarthonaPaul Just noticed, there are failures in MigrateOperationTestCase that do look related:

java.lang.AssertionError: WFLYCTL0158: Operation handler failed: java.lang.NoClassDefFoundError: Could not initialize class org.wildfly.extension.elytron.oidc.ElytronOidcSubsystemDefinition
  at org.junit.Assert.fail(Assert.java:89)
  at org.junit.Assert.assertTrue(Assert.java:42)
  at org.jboss.as.model.test.ModelTestUtils.checkOutcome(ModelTestUtils.java:119)
  at org.jboss.as.subsystem.test.AbstractSubsystemTest.checkOutcome(AbstractSubsystemTest.java:161)
  at org.keycloak.subsystem.adapter.extension.MigrateOperationTestCase.testMigrateDefaultKeycloakConfig(MigrateOperationTestCase.java:73)

This test does pass for me locally though with your scope PR that includes the changes here so perhaps there was some other change needed that you already included there.

PrarthonaPaul · 2024-04-04T19:05:21Z

Closing this PR since this issue is closed by #17790

wildfly-bot bot requested a review from fjuma March 11, 2024 18:35

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Mar 11, 2024

PrarthonaPaul mentioned this pull request Mar 11, 2024

[WFLY-16532][Preview] add ability to configure additional scope for authentication request #17018

Closed

PrarthonaPaul force-pushed the WFLY-19078 branch 3 times, most recently from 736b2ea to 8988494 Compare March 12, 2024 13:36

pferraro reviewed Mar 12, 2024

View reviewed changes

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcExtension.java Outdated Show resolved Hide resolved

pferraro reviewed Mar 12, 2024

View reviewed changes

PrarthonaPaul force-pushed the WFLY-19078 branch 4 times, most recently from 979d371 to 7426070 Compare March 19, 2024 14:10

PrarthonaPaul force-pushed the WFLY-19078 branch 2 times, most recently from 46c84b9 to 1983491 Compare March 22, 2024 17:09

fjuma changed the title ~~WFLY-19078 [Community] - Update the elytron-oidc-client subsystem parser to enumerate schema versions~~ WFLY-19078 [Preview] - Update the elytron-oidc-client subsystem parser to enumerate schema versions Mar 22, 2024

fjuma reviewed Mar 22, 2024

View reviewed changes

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 22, 2024

View reviewed changes

fjuma requested a review from pferraro March 22, 2024 19:25

bstansberry previously requested changes Mar 24, 2024

View reviewed changes

PrarthonaPaul force-pushed the WFLY-19078 branch from 1983491 to 9ecf0d1 Compare March 25, 2024 19:36

wildfly-bot bot requested a review from fjuma March 25, 2024 19:37

fjuma reviewed Mar 26, 2024

View reviewed changes

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java Show resolved Hide resolved

fjuma reviewed Mar 26, 2024

View reviewed changes

...oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemSchema.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 26, 2024

View reviewed changes

elytron-oidc-client/src/main/java/org/wildfly/extension/elytron/oidc/RealmDefinition.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 26, 2024

View reviewed changes

...lient/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcSubsystemTransformers.java Outdated Show resolved Hide resolved

pferraro requested changes Mar 27, 2024

View reviewed changes

wildfly-bot bot requested a review from fjuma March 27, 2024 20:01

PrarthonaPaul force-pushed the WFLY-19078 branch from 0a82e2f to ef08014 Compare March 27, 2024 20:05

PrarthonaPaul commented Mar 27, 2024

View reviewed changes

fjuma requested a review from pferraro March 28, 2024 17:33

PrarthonaPaul force-pushed the WFLY-19078 branch 2 times, most recently from 7f1a894 to fe349ef Compare April 2, 2024 00:30

darranl added Critical Doesn't block a release but deserves higher priority than most. Use sparingly. 32.x WildFly 32 missing-reqs Features missing one or more of the pre-merge requirements labels Apr 2, 2024

WFLY-19078 [Community] - Update the elytron-oidc-client subsystem par…

767c8de

…ser to enumerate schema versions

PrarthonaPaul force-pushed the WFLY-19078 branch from fe349ef to 767c8de Compare April 2, 2024 14:37

bstansberry reviewed Apr 2, 2024

View reviewed changes

fjuma approved these changes Apr 2, 2024

View reviewed changes

fjuma mentioned this pull request Apr 3, 2024

[WFLY-16532][Preview] add ability to configure additional scope for authentication request #17785

Closed

PrarthonaPaul mentioned this pull request Apr 3, 2024

WFLY-16532 [Preview] elytron-oidc-client: add ability to configure additional scope for authentication request #17790

Merged

PrarthonaPaul closed this Apr 4, 2024

PrarthonaPaul mentioned this pull request Apr 10, 2024

[WFLY-19177] [preview] Update elytron-oidc-client subsystem transformer to add a transformer from model version 3 to model version 2 #17803

Merged

	private static void from3(ChainedTransformationDescriptionBuilder chainedBuilder) {
	ResourceTransformationDescriptionBuilder builder = chainedBuilder.createBuilder(VERSION_3_0_0_COMMUNITY.getVersion(), VERSION_2_0_0.getVersion());
	builder.addChildResource(PathElement.pathElement(SECURE_SERVER))
	.getAttributeBuilder();
	}


		public class ElytronOidcClientSubsystemRegistrar {

		static final String NAME = "elytron-oidc-client";

WFLY-19078 [Preview] - Update the elytron-oidc-client subsystem parser to enumerate schema versions #17705

WFLY-19078 [Preview] - Update the elytron-oidc-client subsystem parser to enumerate schema versions #17705

Conversation

PrarthonaPaul commented Mar 11, 2024 • edited by wildfly-bot bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pferraro Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pferraro Mar 12, 2024 • edited

Choose a reason for hiding this comment

PrarthonaPaul Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstansberry Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrarthonaPaul Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrarthonaPaul Mar 27, 2024 • edited

Choose a reason for hiding this comment

darranl commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma left a comment

Choose a reason for hiding this comment

fjuma commented Apr 2, 2024

fjuma commented Apr 2, 2024

PrarthonaPaul commented Apr 4, 2024

PrarthonaPaul commented Mar 11, 2024 •

edited by wildfly-bot bot

pferraro Mar 20, 2024 •

edited

pferraro Mar 12, 2024 •

edited

PrarthonaPaul Mar 12, 2024 •

edited

bstansberry Mar 24, 2024 •

edited

PrarthonaPaul Mar 27, 2024 •

edited

PrarthonaPaul Mar 27, 2024 •

edited