[keycloak-dev] Change in enabling / disabling features

Wed Oct 4 05:54:04 EDT 2017

This all sounds messy and complicated compared to just having it called
authorization-preview and renamed to authorization.

On 4 October 2017 at 09:42, Marko Strukelj <mstrukel at redhat.com> wrote:

> Migration scripts use conditional logic - which currently doesn't remove
> existing configuration for a specific feature if already present.
>
> Once configuration for a feature is in a standalone.xml you have two
> choices during script upgrade:
>   a) you can remove the old configuration and write in the new one - with
> the new default.
>   b) you don't touch the configuration at all - maybe user made changes,
> and now you will delete their changes - you probably don't want that unless
> policy is - don't make any changes to keycloak-subsystem.
>
> I prefer to have policy b) in place, but then you can't automatically
> enable a previously disabled-by-default feature in this case like you point
> out.
> If user changed the configuration - maybe removing system property and
> hardcoding enabled="false" to make sure it doesn't get enabled, then using
> option a) we would automatically enable the feature overwriting user's
> change - and we may get an angry user.
>
> We could probably have an option c) only automatically enable the feature
> if current configuration is exactly as we shipped it - for example value of
> 'enabled' attribute exactly equal to "${feature.authorization:false}".
>
> We can output some text during script execution that some feature is now
> enabled, or that some other feature configuration wasn't migrated because
> it appears to be changed: 'Feature 'authorization' is now supported. Its
> configuration appears to be changed so it won't be migrated automatically.'
>
> We can also notify user during migrate script of every preview feature
> added - 'Feature 'authorization' has preview status and will be disabled by
> default. Use -Dfeature.authorization=true when running the server to enable
> it.'
>
> Or when upgrading to mature with successful automatic migration: 'Feature
> 'authorization' is now supported and is enabled by default. Use
> -Dfeature.authorization=false when running the server to disable it.'
>
> That will make .cli script files bigger but should make for a good user
> experience. I need to make sure that it can be done with .cli scripts in
> the first place.
>
>
> On Wed, Oct 4, 2017 at 8:49 AM, Stian Thorgersen <sthorger at redhat.com>
> wrote:
>
>> That doesn't answer my question really.
>>
>> Let's assume a user doesn't change anything or use system properties. In
>> that case "authorization" would be disabled by default in 7.2. Then in 7.3
>> it should be supported and made enabled by default. How would you do that?
>>
>> If you do it with migration scripts to enable it you don't know if the
>> user has actively decided to disable it or not.
>>
>> So you either left with the option to require users to manually change it
>> (not an option) or you simply have to override what the user has set.
>>
>> No on the other hand if you have -preview to the name you can remove
>> "authorization-preview" and add the new "authorization". In this case
>> you're making it explicit to the user that something has changed, so I
>> think this approach is better rather than just doing it without the users
>> knowledge.
>>
>> On 3 October 2017 at 17:12, Marko Strukelj <mstrukel at redhat.com> wrote:
>>
>>> I guess changing enabled status of features is a requirement, so feature
>>> would become enabled after transition to supported. There would still be
>>> different enabled statuses for community and product. It would just not be
>>> reflected in feature's name.
>>>
>>> If we just use -Dfeature.authorization=true / false (no -preview suffix)
>>> while feature has preview status, then if user is oblivious of the setting
>>> and doesn't use it to override defaults, enabling the feature by default
>>> will have effect.
>>>
>>> But if user wants to make sure feature doesn't get enabled (both while
>>> it's preview, and for the future when it's stable) then
>>> -Dfeature.authorization=false can be set, and now when newer version of
>>> community or product is used this setting will have effect - it will not
>>> become deprecated, it will just work like it worked when the feature had
>>> preview status.
>>>
>>> All we have to figure out is how best to have users aware that some
>>> feature has preview status since it would not be part of feature's name any
>>> more.
>>>
>>> We will want to document it in the docs - point out preview features in
>>> features.adoc, and changes in enabled status in changes.adoc.
>>> And we can log a warning during startup when features with preview
>>> status are enabled. I already list the disabled features, so it's clear
>>> what functionalities of the server are not available.
>>>
>>> I don't know about finegrained permitions Bill has mentioned. The list
>>> of features currently is: authorization-preview, impersonation,
>>> scripts-preview, docker, account2-preview, token-exchange
>>>
>>> (the last two have been added recently - I took the liberty to give
>>> account2 preview status - maybe it should not have it, and maybe
>>> token-exchange should have it?)
>>>
>>> So, I don't know where fine grained permissions come to play here - I
>>> would assume it simply means that if someone enables token-exchange that
>>> also requires authorization in order to take effect. And we can make that
>>> check, and either display warnings that feature can't take effect or
>>> automatically enable the dependency feature.
>>>
>>> But my proposal is essentially just to not name features with '-preview'
>>> at all while still knowing for each feature if it has preview status, so we
>>> can display warnings when it's enabled.
>>>
>>>
>>> On Tue, Oct 3, 2017 at 1:36 PM, Stian Thorgersen <sthorger at redhat.com>
>>> wrote:
>>>
>>>> In what you are proposing what happens when a feature goes from tech
>>>> preview to supported? It would still be disabled by default right?
>>>>
>>>> On 3 October 2017 at 12:26, Marko Strukelj <mstrukel at redhat.com> wrote:
>>>>
>>>>>
>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>