Preventing duplicate registration of capabilities

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Preventing duplicate registration of capabilities

Brian Stansberry
tl;dr;

Currently on a server (let’s ignore an HC as it’s a tangent) we allow the same capability to be registered by more than one resource. In the large majority but not all cases a user doing this has made a mistake we should catch. I plan to make whether this is allowed configurable by the capability author. I *propose* making the default behavior ‘false’ which which is a behavior change and will require some cleanup.

Only user impact is if some capability needs to allow multiple registrations and doesn’t pick this up and configure it, and that isn’t caught before release.

Long version

In most cases a server configuration should only have a single resource that registers a given capability. If more than one registers it, which really provides it? More than one will likely fail due to conflicts over things like MSC service names.

But, the capability registry actually allows more than one registration point (i.e. resource address) for the same capability.

In the large bulk of cases this doesn’t matter. Only one type of resource provides a capability, and if the user tries to configure two of that type of resource with the same name it will fail due to resource duplication (you can’t add socket-binding=http if there already is a resource at that address.)

Elytron changes all this a bit as that subsystem has different resources with different types that all provide the same capability, but each with different implementation deals (*analogous* to an API providing Set but you can configure use of HashSet or LinkedHashSet or ConcurrentSkipListSet). Here we want to enforce that the user doesn’t configure two resources that provide the same capability. If they do, right now this will fail in runtime (due to MSC service name conflicts) but we want to detect the mistake earlier.

OTOH there are some specialized cases where it is valid for two resources to register the same capability. Clustering has one (see WFLY-8040); there are others on a Host Controller. Basically the capability implementations know about each other, check for duplication at runtime and if present deal with it in a correct manner.

I plan to make whether a capability allows multiple registration points to be configurable. See https://github.com/wildfly/wildfly-core/pull/2154

Question is whether the default behavior should remain ‘true’ (allow multiple registrations).

Pros:

1) Any existing capabilties that really need multiple registration can remain unchanged. This is more problematic for other projects based on WildFly Core that use capabilities as they may not know about this until they switch to core 3.0. But are there such projects? (I specifically copied a few folks to directly check).
2) Some subsystem unit test setups that don’t need this but inadvertently rely on it still work. (I’ve already fixed these in WildFly and WildFly Core so this only relates to other projects based on WildFly Core that use capabilities.)

Cons:

1) The Elytron use case is more standard, IMO, while the use cases like the clustering one that allow multiple registrations already require special logic to make that work. So I’d rather isolate any configuration to allow it to work to the cases that already are taking special steps.
2) The elytron team is really busy and doesn’t need more things to do.


My thought is the Cons outweigh the Pros, so I plan to change the default behavior. But your thoughts are welcome.

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Preventing duplicate registration of capabilities

Darran Lofthouse
It is not a problem for us to go through our subsystem and change a
value across all resources if you need us to if the decision goes that way.

BUT ;-)

We do expect other subsystems to have resources that advertise our
capabilities, 'security' is already one.  An external one would be KeyCloak.

If Elytron was fully configured for unique names and KeyCloak allowed
duplicates would this be enough to get a Stage.MODEL error?

On 07/02/17 18:28, Brian Stansberry wrote:

> tl;dr;
>
> Currently on a server (let’s ignore an HC as it’s a tangent) we allow the same capability to be registered by more than one resource. In the large majority but not all cases a user doing this has made a mistake we should catch. I plan to make whether this is allowed configurable by the capability author. I *propose* making the default behavior ‘false’ which which is a behavior change and will require some cleanup.
>
> Only user impact is if some capability needs to allow multiple registrations and doesn’t pick this up and configure it, and that isn’t caught before release.
>
> Long version
>
> In most cases a server configuration should only have a single resource that registers a given capability. If more than one registers it, which really provides it? More than one will likely fail due to conflicts over things like MSC service names.
>
> But, the capability registry actually allows more than one registration point (i.e. resource address) for the same capability.
>
> In the large bulk of cases this doesn’t matter. Only one type of resource provides a capability, and if the user tries to configure two of that type of resource with the same name it will fail due to resource duplication (you can’t add socket-binding=http if there already is a resource at that address.)
>
> Elytron changes all this a bit as that subsystem has different resources with different types that all provide the same capability, but each with different implementation deals (*analogous* to an API providing Set but you can configure use of HashSet or LinkedHashSet or ConcurrentSkipListSet). Here we want to enforce that the user doesn’t configure two resources that provide the same capability. If they do, right now this will fail in runtime (due to MSC service name conflicts) but we want to detect the mistake earlier.
>
> OTOH there are some specialized cases where it is valid for two resources to register the same capability. Clustering has one (see WFLY-8040); there are others on a Host Controller. Basically the capability implementations know about each other, check for duplication at runtime and if present deal with it in a correct manner.
>
> I plan to make whether a capability allows multiple registration points to be configurable. See https://github.com/wildfly/wildfly-core/pull/2154
>
> Question is whether the default behavior should remain ‘true’ (allow multiple registrations).
>
> Pros:
>
> 1) Any existing capabilties that really need multiple registration can remain unchanged. This is more problematic for other projects based on WildFly Core that use capabilities as they may not know about this until they switch to core 3.0. But are there such projects? (I specifically copied a few folks to directly check).
> 2) Some subsystem unit test setups that don’t need this but inadvertently rely on it still work. (I’ve already fixed these in WildFly and WildFly Core so this only relates to other projects based on WildFly Core that use capabilities.)
>
> Cons:
>
> 1) The Elytron use case is more standard, IMO, while the use cases like the clustering one that allow multiple registrations already require special logic to make that work. So I’d rather isolate any configuration to allow it to work to the cases that already are taking special steps.
> 2) The elytron team is really busy and doesn’t need more things to do.
>
>
> My thought is the Cons outweigh the Pros, so I plan to change the default behavior. But your thoughts are welcome.
>
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Preventing duplicate registration of capabilities

Brian Stansberry

> On Feb 7, 2017, at 12:54 PM, Darran Lofthouse <[hidden email]> wrote:
>
> It is not a problem for us to go through our subsystem and change a
> value across all resources if you need us to if the decision goes that way.
>
> BUT ;-)
>
> We do expect other subsystems to have resources that advertise our
> capabilities, 'security' is already one.  An external one would be KeyCloak.
>
> If Elytron was fully configured for unique names and KeyCloak allowed
> duplicates would this be enough to get a Stage.MODEL error?
>

This is not possible because we require all registrations to use the exact same capability (x == y.) We could change that to x.equals(y) — which I suspect you’ll reply asking for ;) — but even there we’d include this field in the equality check.

What you describe is an expansion on the point of my first “Con”. In the elytron-style case, the people implementing the capability are not thinking about coordinating with other impls. They expect the kernel to make it just work.
.

> On 07/02/17 18:28, Brian Stansberry wrote:
>> tl;dr;
>>
>> Currently on a server (let’s ignore an HC as it’s a tangent) we allow the same capability to be registered by more than one resource. In the large majority but not all cases a user doing this has made a mistake we should catch. I plan to make whether this is allowed configurable by the capability author. I *propose* making the default behavior ‘false’ which which is a behavior change and will require some cleanup.
>>
>> Only user impact is if some capability needs to allow multiple registrations and doesn’t pick this up and configure it, and that isn’t caught before release.
>>
>> Long version
>>
>> In most cases a server configuration should only have a single resource that registers a given capability. If more than one registers it, which really provides it? More than one will likely fail due to conflicts over things like MSC service names.
>>
>> But, the capability registry actually allows more than one registration point (i.e. resource address) for the same capability.
>>
>> In the large bulk of cases this doesn’t matter. Only one type of resource provides a capability, and if the user tries to configure two of that type of resource with the same name it will fail due to resource duplication (you can’t add socket-binding=http if there already is a resource at that address.)
>>
>> Elytron changes all this a bit as that subsystem has different resources with different types that all provide the same capability, but each with different implementation deals (*analogous* to an API providing Set but you can configure use of HashSet or LinkedHashSet or ConcurrentSkipListSet). Here we want to enforce that the user doesn’t configure two resources that provide the same capability. If they do, right now this will fail in runtime (due to MSC service name conflicts) but we want to detect the mistake earlier.
>>
>> OTOH there are some specialized cases where it is valid for two resources to register the same capability. Clustering has one (see WFLY-8040); there are others on a Host Controller. Basically the capability implementations know about each other, check for duplication at runtime and if present deal with it in a correct manner.
>>
>> I plan to make whether a capability allows multiple registration points to be configurable. See https://github.com/wildfly/wildfly-core/pull/2154
>>
>> Question is whether the default behavior should remain ‘true’ (allow multiple registrations).
>>
>> Pros:
>>
>> 1) Any existing capabilties that really need multiple registration can remain unchanged. This is more problematic for other projects based on WildFly Core that use capabilities as they may not know about this until they switch to core 3.0. But are there such projects? (I specifically copied a few folks to directly check).
>> 2) Some subsystem unit test setups that don’t need this but inadvertently rely on it still work. (I’ve already fixed these in WildFly and WildFly Core so this only relates to other projects based on WildFly Core that use capabilities.)
>>
>> Cons:
>>
>> 1) The Elytron use case is more standard, IMO, while the use cases like the clustering one that allow multiple registrations already require special logic to make that work. So I’d rather isolate any configuration to allow it to work to the cases that already are taking special steps.
>> 2) The elytron team is really busy and doesn’t need more things to do.
>>
>>
>> My thought is the Cons outweigh the Pros, so I plan to change the default behavior. But your thoughts are welcome.
>>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Preventing duplicate registration of capabilities

kkhan
In reply to this post by Brian Stansberry

> On 7 Feb 2017, at 18:28, Brian Stansberry <[hidden email]> wrote:
>
> tl;dr;
>
> Currently on a server (let’s ignore an HC as it’s a tangent) we allow the same capability to be registered by more than one resource. In the large majority but not all cases a user doing this has made a mistake we should catch. I plan to make whether this is allowed configurable by the capability author. I *propose* making the default behavior ‘false’ which which is a behavior change and will require some cleanup.
>
> Only user impact is if some capability needs to allow multiple registrations and doesn’t pick this up and configure it, and that isn’t caught before release.
>
> Long version
>
> In most cases a server configuration should only have a single resource that registers a given capability. If more than one registers it, which really provides it? More than one will likely fail due to conflicts over things like MSC service names.
>
> But, the capability registry actually allows more than one registration point (i.e. resource address) for the same capability.
>
> In the large bulk of cases this doesn’t matter. Only one type of resource provides a capability, and if the user tries to configure two of that type of resource with the same name it will fail due to resource duplication (you can’t add socket-binding=http if there already is a resource at that address.)
>
> Elytron changes all this a bit as that subsystem has different resources with different types that all provide the same capability, but each with different implementation deals (*analogous* to an API providing Set but you can configure use of HashSet or LinkedHashSet or ConcurrentSkipListSet). Here we want to enforce that the user doesn’t configure two resources that provide the same capability. If they do, right now this will fail in runtime (due to MSC service name conflicts) but we want to detect the mistake earlier.
>
> OTOH there are some specialized cases where it is valid for two resources to register the same capability. Clustering has one (see WFLY-8040); there are others on a Host Controller. Basically the capability implementations know about each other, check for duplication at runtime and if present deal with it in a correct manner.
>
> I plan to make whether a capability allows multiple registration points to be configurable. See https://github.com/wildfly/wildfly-core/pull/2154
>
> Question is whether the default behavior should remain ‘true’ (allow multiple registrations).
>
> Pros:
>
> 1) Any existing capabilties that really need multiple registration can remain unchanged. This is more problematic for other projects based on WildFly Core that use capabilities as they may not know about this until they switch to core 3.0. But are there such projects? (I specifically copied a few folks to directly check).
> 2) Some subsystem unit test setups that don’t need this but inadvertently rely on it still work. (I’ve already fixed these in WildFly and WildFly Core so this only relates to other projects based on WildFly Core that use capabilities.)
>
> Cons:
>
> 1) The Elytron use case is more standard, IMO, while the use cases like the clustering one that allow multiple registrations already require special logic to make that work. So I’d rather isolate any configuration to allow it to work to the cases that already are taking special steps.
> 2) The elytron team is really busy and doesn’t need more things to do.
>
>
> My thought is the Cons outweigh the Pros, so I plan to change the default behavior. But your thoughts are welcome.
+1 and it is a major version upgrade, so if we don't do it now it will be too late in the future

>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|

Re: Preventing duplicate registration of capabilities

Brian Stansberry

> On Feb 7, 2017, at 4:36 PM, Kabir Khan <[hidden email]> wrote:
>
>>
>> On 7 Feb 2017, at 18:28, Brian Stansberry <[hidden email]> wrote:
>>
>> tl;dr;
>>
>> Currently on a server (let’s ignore an HC as it’s a tangent) we allow the same capability to be registered by more than one resource. In the large majority but not all cases a user doing this has made a mistake we should catch. I plan to make whether this is allowed configurable by the capability author. I *propose* making the default behavior ‘false’ which which is a behavior change and will require some cleanup.
>>
>> Only user impact is if some capability needs to allow multiple registrations and doesn’t pick this up and configure it, and that isn’t caught before release.
>>
>> Long version
>>
>> In most cases a server configuration should only have a single resource that registers a given capability. If more than one registers it, which really provides it? More than one will likely fail due to conflicts over things like MSC service names.
>>
>> But, the capability registry actually allows more than one registration point (i.e. resource address) for the same capability.
>>
>> In the large bulk of cases this doesn’t matter. Only one type of resource provides a capability, and if the user tries to configure two of that type of resource with the same name it will fail due to resource duplication (you can’t add socket-binding=http if there already is a resource at that address.)
>>
>> Elytron changes all this a bit as that subsystem has different resources with different types that all provide the same capability, but each with different implementation deals (*analogous* to an API providing Set but you can configure use of HashSet or LinkedHashSet or ConcurrentSkipListSet). Here we want to enforce that the user doesn’t configure two resources that provide the same capability. If they do, right now this will fail in runtime (due to MSC service name conflicts) but we want to detect the mistake earlier.
>>
>> OTOH there are some specialized cases where it is valid for two resources to register the same capability. Clustering has one (see WFLY-8040); there are others on a Host Controller. Basically the capability implementations know about each other, check for duplication at runtime and if present deal with it in a correct manner.
>>
>> I plan to make whether a capability allows multiple registration points to be configurable. See https://github.com/wildfly/wildfly-core/pull/2154
>>
>> Question is whether the default behavior should remain ‘true’ (allow multiple registrations).
>>
>> Pros:
>>
>> 1) Any existing capabilties that really need multiple registration can remain unchanged. This is more problematic for other projects based on WildFly Core that use capabilities as they may not know about this until they switch to core 3.0. But are there such projects? (I specifically copied a few folks to directly check).
>> 2) Some subsystem unit test setups that don’t need this but inadvertently rely on it still work. (I’ve already fixed these in WildFly and WildFly Core so this only relates to other projects based on WildFly Core that use capabilities.)
>>
>> Cons:
>>
>> 1) The Elytron use case is more standard, IMO, while the use cases like the clustering one that allow multiple registrations already require special logic to make that work. So I’d rather isolate any configuration to allow it to work to the cases that already are taking special steps.
>> 2) The elytron team is really busy and doesn’t need more things to do.
>>
>>
>> My thought is the Cons outweigh the Pros, so I plan to change the default behavior. But your thoughts are welcome.
> +1 and it is a major version upgrade, so if we don't do it now it will be too late in the future

This is done, so the default behavior beginning in WildFly Core 3.0.0.Beta5 will be that you have to specially configure to allow multiple resources to register the same capabiltity. I only found the two uses of this I mentioned in my first post.

>>
>> --
>> Brian Stansberry
>> Manager, Senior Principal Software Engineer
>> JBoss by Red Hat
>>
>>
>>
>>
>> _______________________________________________
>> wildfly-dev mailing list
>> [hidden email]
>> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev