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&gt; 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 >

Adding per feature toggles is not a good idea. We currently have feature to feature dependencies. For example, token exchange depends on fine-grain admin permissions, and fine grain admin permissions depends on authorization. I've warned Stian about this and I guess he doesn't care? I still struggle to understand why we want to make things more difficult on paying customers. On Mon, Oct 2, 2017 at 9:37 AM, Marko Strukelj <mstrukel(a)redhat.com&gt; 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&gt; > 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&gt; 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 >>> >> >> > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev -- Bill Burke Red Hat

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&gt; 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&gt; 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 >> > >

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&gt; wrote: > > On Oct 3, 2017 06:11, "Stian Thorgersen" <sthorger(a)redhat.com&gt; 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&gt; 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&gt; >> 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&gt; 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 >>>> >>> >>> >> > >

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&gt; 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&gt; > 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&gt; wrote: >> >>> >>> On Oct 3, 2017 06:11, "Stian Thorgersen" <sthorger(a)redhat.com&gt; 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&gt; 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&gt; >>>>> 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 >>>>>> >>>>> >>>>> >>>> >>> >>> >> >

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&gt; 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&gt; > 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&gt; 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&gt; >>> 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&gt; >>>> wrote: >>>> >>>>> >>>>> On Oct 3, 2017 06:11, "Stian Thorgersen" <sthorger(a)redhat.com&gt; 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&gt; >>>>> 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&gt; 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&gt; >>>>>>> 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 >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >> >