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(a)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(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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>