[keycloak-dev] Change in enabling / disabling features

Tue Oct 3 11:12:52 EDT 2017

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