[keycloak-dev] Change in enabling / disabling features

[Marko Strukelj]

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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>