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(a)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(a)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(a)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(a)redhat.com> wrote:
>>
>>>
>>> On Oct 3, 2017 06:11, "Stian Thorgersen"
<sthorger(a)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(a)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(a)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(a)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(a)lists.jboss.org
>>>>>>
https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>