[keycloak-dev] Change in enabling / disabling features

Wed Oct 4 02:49:29 EDT 2017

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