[keycloak-dev] Change in enabling / disabling features

Tue Oct 3 06:26:36 EDT 2017

On Oct 3, 2017 06:11, "Stian Thorgersen" <sthorger at redhat.com> wrote:

Having '-preview' in the name clearly communicates the fact that it's a
tech preview feature. When it is made fully supported it should be enabled
out of the box by default. If we don't do this then users that haven't
enabled the feature when it was tech preview won't have it enabled when
it's fully supported. Alternatively, if you automatically change it from
disabled to enabled when it's moved from tech preview you don't know if
users actually had chosen to disable it or if that was just the default. In
either case I think it's cleaner and safer to rename, because the feature
does indeed change from preview to supported.

The case you point out with feature not getting disabled ...

Consider the case where user disables a preview feature with
-Dfeature.authorization-preview=false. It's a no-op for product, but not
for community distro. When we upgrade feature to stable user will get a
warning for an unknown feature, and the old config will not turn off the
feature any more - on product as well. So did we solve anything?

Do we want -Dfeature.authorization-preview=false to keep having effect even
after it's not called feature.authorization-preview any more? That's what I
call backwards compatibility. Will it remain in place forever or we give
ourself extra work not only adding it in the first place, but also removing
it at some point?

My point is that '-preview' adds complexity to maintain, may introduce
extra bugs, JIRAs, will require extra documentation ... so what it brings
to the table should outweigh the burden. Maybe there is another way to
achieve obviousness of the fact that some feature is 'preview' without
actually naming the feature, and all config options as such.

On 2 October 2017 at 15:37, Marko Strukelj <mstrukel at redhat.com> wrote:

> The more I look at the idea of having '-preview' in feature names the more
> I think it will cause more trouble than it's worth.
>
> The reason is it complicates migration when we promote feature to stable -
> dropping '-preview' from its name, and it causes configuration names to be
> fluid over time.
>
> For example, consider how moving authorization from preview to stable
> would look for a user.
>
> User got used to using -Dfeature.authorization-preview=true
>
> Now we change this setting to be called -Dfeature.authorization=true. User
> still using -Dfeature.authorization-preview=true, will get a warning upon
> startup: 'Unknown feature system property: feature.authorization-preview',
> and the system property will have no effect. If user uses Keycloak
> distribution then authorization-preview will be enabled by default. So no
> need to explicitly enable it - a no-op operation in the first place, and a
> no-op operation at upgrade. If user sets the property to false, then after
> upgrade the feature will suddenly be enabled when before it was not.
>
> It doesn't seem right to drop configuration like that. If we want old
> config to keep working then every migration from preview to stable also has
> to add backwards compatibility logic, so that surprises like this don't
> happen. Also all documentation needs to be changed wherever the feature
> name is referenced.
>
> Whenever we promote any feature from '-preview' to stable this will be
> needed, and it may become standard ritual that may to most users seem
> completely redundant. Why not just call it -Dfeature.authorization in the
> first place, and maybe just display a warning if any 'preview' feature is
> enabled, something like:
>
> [WARN] The following preview features are enabled: ...
>
> WDYT?
>
>
> On Wed, Aug 30, 2017 at 8:28 AM, Stian Thorgersen <sthorger at redhat.com>
> wrote:
>
>> In standalone.xml I'd use "feature.scripts" sys properties instead of
>> the longer "keycloak.feature.scripts". It's just shorter and simpler and
>> won't cause any conflicts.
>>
>> We need to support the old "keycloak.profile.feature.<featurename>=enabled|disabled"
>> as is, including if set in profile.properties. This is important in case
>> customers have explicitly disabled impersonation for example. +1 To
>> printing a warning in this case. Then let's remove the old deprecated
>> properties in Keycloak 3.5x as well as the profile.properties.
>>
>> On 28 August 2017 at 15:57, Marko Strukelj <mstrukel at redhat.com> wrote:
>>
>>> This is a heads up about an upcoming change in how to enable or disable
>>> features in Keycloak.
>>>
>>> Keycloak has a notion of features, which makes it possible to disable
>>> certain functionality that is enabled by default, or enable certain
>>> functionality that is disabled by default.
>>>
>>> There are four sets of functionality you can enable or disable as
>>> features:
>>> impersonation, script,  authorization, and docker. See [1] for more info.
>>>
>>> Currently a file called profile.properties can be used to enable /
>>> disable
>>> features, or system properties can be used, which override any
>>> configuration inside profile.properties as explained in [1].
>>>
>>> This is an aberration from how other configuration is specified on the
>>> server via standalone.xml.
>>>
>>> Also, the reason config file is called profile.properties, and not
>>> features.properties is because the set of enabled/disabled features is
>>> different for upstream project (where all but 'docker' are enabled), for
>>> product based on Keycloak - RHSSO (where only 'impersonation' is
>>> enabled),
>>> and for technology preview versions of RHSSO.
>>>
>>> This additionally complicates things. The idea is to simplify that and
>>> remove the notion of profiles, stop using profile.properties, and move
>>> all
>>> configuration to standalone.xml where all the available features will be
>>> listed with default values:
>>>
>>> <subsystem xmlns="urn:jboss:domain:keycloak-server:1.1">
>>>
>>> ...
>>>
>>>   <features>
>>>     <feature name="authorization" enabled="true" />
>>>     <feature name="impersonation" enabled="true" />
>>>     <feature name="scripts" enabled="true" />
>>>     <feature name="docker" enabled="false" />
>>>   </features>
>>>
>>> ...
>>>
>>> </subsystem>
>>>
>>> This is how configuration would look like in Keycloak distribution. In
>>> product distributions different features would be enabled / disabled - no
>>> more named profiles.
>>>
>>> However, in order to allow system properties override the idea is to do
>>> it
>>> slightly differently:
>>>
>>>   <features>
>>>     <feature name="authorization"
>>> enabled="${keycloak.feature.authorization:true}" />
>>>     <feature name="impersonation"
>>> enabled="${keycloak.feature.impersonation:true}" />
>>>     <feature name="scripts" enabled="${keycloak.feature.scripts:true}"
>>> />
>>>     <feature name="docker" enabled="${keycloak.feature.docker:false}" />
>>>   </features>
>>>
>>>
>>> We should probably also facilitate transition for current deployments,
>>> and
>>> take old style system properties into account if they are used -
>>> converting
>>> them to the new ones before configuration is applied.
>>>
>>> Deprecated: -Dkeycloak.profile.feature.impersonation=enabled
>>> New style:    -Dkeycloak.feature.impersonation=true
>>>
>>> We don't want to keep support for this indefinitely (maybe for one minor
>>> version?) so if use of deprecated system properties is detected a warning
>>> will be logged.
>>>
>>> Also, Stian proposed that features not considered stable yet, should
>>> carry
>>> a suffix '-preview' in the name. So the actual feature name, and system
>>> property name may still change for some features.
>>>
>>> You can follow progress on JIRA issue [2].
>>>
>>> -
>>>
>>> [1]
>>> https://keycloak.gitbooks.io/documentation/server_installati
>>> on/topics/profiles.html
>>> [2] https://issues.jboss.org/browse/KEYCLOAK-4994
>>> _______________________________________________
>>> keycloak-dev mailing list
>>> keycloak-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>
>>
>>
>