Fixed #21204 - introduced global_setting helper function #4891

lzap · 2017-10-05T07:58:07Z

Locked templates made very difficult to change to discovery in global default templates, user needs to unlock 2+2+2 (PXELinux, Grub, Grub2 template + discovery snippet) and make necessary changes just to change the default menu entry. This can be done via global setting.

This patch adds such a helper function, I am doing changes in community templates to leverage this new function.

Security standpoint - I can't think of any setting that should not be rendered. We do have a root password, but that is already available using the existing helper. Please review with that in mind.

theforeman-bot · 2017-10-05T07:58:08Z

There were the following issues with the commit message:

c949fc1 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

lzap · 2017-10-06T13:18:41Z

Also added "default_pxe_item_local" so both global and local menu items can be now configured.

ares · 2017-10-09T06:33:03Z

lib/foreman/renderer.rb

@@ -233,6 +252,10 @@ def load_template_vars
      @template_url = params['url']
    end

+    def global_setting(name, default = nil)
+      SETTINGS[name.to_sym] || default


I believe this was rejected in past because setting contain sensitive information, such as root password for newly provisioned hosts. I suppose we'd need to add a whitelist here. Also the || won't work for settings that has value false, it would always return default value. If you're going to fix this, I'd also suggest renaming global_setting to setting, there's no other setting...

Also note that settings page is only available for admins, there's no permission for this. Making it available through template give ability to everyone who can render (preview) template, to see values.

So how about renaming it to "setting", creating whitelist, fixing default value and allowing override via host params, so people can still override this per host/hostgroup.

Or maybe is cleaner just to use host/hostgroup parameter for this. That would drop this complexity. But to be honest I really like using settings in templates.

The reason for that is this enables users (or admins specifically) to make discovery the default options. This can't be done with host/hostgroup variables.

If that is for admins only, then I think we can drop the host/hg params. Whitelist, default value and renaming is still required I think. I think the whitelist should contain nearly all settings and (sorry to say that), there should be a plugin DSL for extending it. I suppose discovery plugin would require it.

lzap · 2017-11-07T14:23:53Z

Added whitelist, covered everything with tests, added default list of allowed global options which I think are useful and safe. I did not add those which I thought are not much useful. No host/hostgroup override available, I think this is cleaner.

lzap · 2017-12-12T09:18:16Z

@ares this is ready for review and merge

iNecas

Note for me: don't approve PRs without testing, even when there are unit tests that seem to prove it works.

See comments inline.

iNecas · 2017-12-12T14:44:14Z

lib/foreman/renderer.rb

@@ -233,6 +277,11 @@ def load_template_vars
      @template_url = params['url']
    end

+    def global_setting(name, default = nil)
+      raise(FilteredGlobalSettingAccessed, _('Global setting "%s" is not accessible in safe-mode') % name) if Setting[:safemode_render] && !ALLOWED_GLOBAL_SETTINGS.include?(name.to_sym)
+      SETTINGS[name.to_sym] || default


You should use Setting, not SETTINGS, the SETTINGS are just for things that come from settings file

Also, you should check on blank?, instead of false value. Otherwise, boolean values, such as safemode_render, would not work.

lzap · 2017-12-19T08:28:56Z

Done, rebased.

dLobatog

Needs to be rebased, please @lzap

lzap · 2018-01-15T09:24:25Z

Rebased.

ares · 2018-01-15T15:59:28Z

app/models/setting/provisioning.rb

@@ -59,6 +59,8 @@ def self.default_settings
      self.set('dns_conflict_timeout', N_("Timeout for DNS conflict validation (in seconds)"), 3, N_('DNS conflict timeout')),
      self.set('clean_up_failed_deployment', N_("Foreman will delete virtual machine if provisioning script ends with non zero exit code"), true, N_('Clean up failed deployment')),
      self.set('name_generator_type', N_("Random gives unique names, MAC-based are longer but stable (and only works with bare-metal)"), 'Random-based', N_("Type of name generator"), nil, {:collection => Proc.new {NameGenerator::GENERATOR_TYPES} }),
+      self.set('default_pxe_item_global', N_("Default PXE (PXELinux, PXEGrub, PXEGrub2) menu item in global template - 'local', 'discovery' or custom"), 'local', N_("Default PXE global template entry")),
+      self.set('default_pxe_item_local', N_("Default PXE (PXELinux, PXEGrub2) menu item in local template - 'local_chain_hd0', 'local' or custom"), 'local_chain_hd0', N_("Default PXE local template entry")),


how custom is supposed to work? looking at here I'm unable to specify any other value that local/discovery or is that the limitation for grub1?

if custom values are desired, I guess my comment above about limiting the option does not make sense...

Grub1 only supports numbers for that option, so there is a small helper array that is then converted to number. I assume that users will sync the array in cloned templates, or simply delete whole thing which does not make much sense in read/write (cloned) template.

I wanted consistent naming in settings across all loaders.

ares · 2018-01-15T16:02:59Z

lib/foreman/renderer.rb

@@ -232,6 +275,11 @@ def load_template_vars
      @template_url = params['url']
    end

+    def global_setting(name, default = nil)


what's the point of having default if setting already provide default? when 2 defaults are useful?

The default of default (the option :-) ) is used in templates to have sane default if users does not opt-in and leaves it blank (that's what we ship Foreman with). What do you recommend instead?

I still don't get it, if I use global_settings(:some, 'value'), I'll get value back if there's no Setting[:some]. But in that case, why would I call the global_settings in first place? I'd just use value. Or I'd do global_settigs(:root_pass) || 'change_me' instead of global_settigns(:root_pass, 'change_me')

lzap · 2018-01-17T12:31:05Z

Rebased, one line changed.

iNecas

No other comments from me

lzap · 2018-02-13T10:25:39Z

Thanks @ares @dLobatog are we good to go?

ares · 2018-02-13T15:38:51Z

@lzap I'd still like to understand the value of default parameter of global_setting but then it's good from my side

lzap · 2018-02-15T11:29:02Z

While I don't insist on the default option for the helper, I think it might be useful - it's a generic helper after all which can be used for more things. The helper-default will be used when setting is blank meaning that template writers can still provide some default option when user decided to delete the value from settings for some reason. This enables you to add "third state", you can't set nil Ruby value for example via settings, this allows you to create an option "and set to blank for XYZ" and then you can use this mechanism appropriately to set it to nil or any other value - for example an empty array or hash. This can make template code easier to read for example.

So it makes sense, not in this context, but for the future.

ares · 2018-02-19T13:28:48Z

lib/foreman/renderer.rb

@@ -232,6 +275,11 @@ def load_template_vars
      @template_url = params['url']
    end

+    def global_setting(name, default = nil)
+      raise(FilteredGlobalSettingAccessed, _('Global setting "%s" is not accessible in safe-mode') % name) if Setting[:safemode_render] && !ALLOWED_GLOBAL_SETTINGS.include?(name.to_sym)
+      Setting[name.to_sym].blank? ? default : Setting[name.to_sym]


this does not work well with false (valid value) which is considered blank, I guess the behavior should differ if settings_type is boolean

ares · 2018-02-19T13:31:42Z

Thanks @lzap, I'm fine with default value now. During testing I found it does not work well with boolean though. After fixing that, thumbs up.

lzap · 2018-02-22T16:35:11Z

Amended the boolean thing, rebased.

ares

Thanks @lzap, works fine now.

Daniel asked for rebase which was done already

lzap · 2018-02-23T08:38:57Z

Thanks, @ares or @dLobatog mind merging theforeman/theforeman.org#1013 discovery docs, so I can carry on with PR that documents this new feature? Thanks.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Oct 5, 2017

lzap mentioned this pull request Oct 5, 2017

Refs #21204 - default PXE entry from settings theforeman/community-templates#430

Merged

lzap force-pushed the global-setting-21204 branch from c949fc1 to 3b76330 Compare October 5, 2017 08:10

theforeman-bot added Needs testing and removed Waiting on contributor labels Oct 5, 2017

lzap force-pushed the global-setting-21204 branch from 3b76330 to 9931d29 Compare October 6, 2017 13:18

ares reviewed Oct 9, 2017

View reviewed changes

lzap force-pushed the global-setting-21204 branch from 9931d29 to dea8a81 Compare November 7, 2017 14:22

iNecas approved these changes Dec 12, 2017

View reviewed changes

iNecas suggested changes Dec 12, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Dec 12, 2017

lzap force-pushed the global-setting-21204 branch from dea8a81 to 2ef79da Compare December 19, 2017 08:28

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Dec 19, 2017

dLobatog previously requested changes Dec 20, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Dec 20, 2017

lzap force-pushed the global-setting-21204 branch from 2ef79da to cab3bcb Compare January 15, 2018 09:24

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jan 15, 2018

theforeman-bot added the Waiting on contributor label Jan 15, 2018

ares reviewed Jan 15, 2018

View reviewed changes

lzap force-pushed the global-setting-21204 branch from cab3bcb to ce4a528 Compare January 17, 2018 12:31

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 17, 2018

iNecas approved these changes Feb 12, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 12, 2018

ares reviewed Feb 19, 2018

View reviewed changes

ares added the Waiting on contributor label Feb 19, 2018

Fixes #21204 - introduced global_setting helper function

d77945b

lzap force-pushed the global-setting-21204 branch from ce4a528 to d77945b Compare February 22, 2018 16:35

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Feb 22, 2018

ares added Ready to merge and removed Needs re-review Needs testing labels Feb 23, 2018

ares approved these changes Feb 23, 2018

View reviewed changes

ares merged commit bdcdb09 into theforeman:develop Feb 23, 2018

lzap deleted the global-setting-21204 branch February 23, 2018 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #21204 - introduced global_setting helper function #4891

Fixed #21204 - introduced global_setting helper function #4891

lzap commented Oct 5, 2017

theforeman-bot commented Oct 5, 2017

lzap commented Oct 6, 2017 •

edited

Loading

ares Oct 9, 2017

ares Oct 9, 2017

lzap Oct 9, 2017

lzap Oct 9, 2017

lzap Oct 9, 2017

ares Oct 9, 2017

lzap commented Nov 7, 2017

lzap commented Dec 12, 2017

iNecas left a comment

iNecas Dec 12, 2017

iNecas Dec 12, 2017

lzap commented Dec 19, 2017

dLobatog left a comment

lzap commented Jan 15, 2018

ares Jan 15, 2018

lzap Jan 17, 2018

ares Jan 15, 2018

lzap Jan 17, 2018

ares Feb 13, 2018

lzap commented Jan 17, 2018

iNecas left a comment

lzap commented Feb 13, 2018

ares commented Feb 13, 2018

lzap commented Feb 15, 2018 •

edited

Loading

ares Feb 19, 2018

ares commented Feb 19, 2018

lzap commented Feb 22, 2018

ares left a comment

lzap commented Feb 23, 2018

Fixed #21204 - introduced global_setting helper function #4891

Fixed #21204 - introduced global_setting helper function #4891

Conversation

lzap commented Oct 5, 2017

theforeman-bot commented Oct 5, 2017

lzap commented Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Nov 7, 2017

lzap commented Dec 12, 2017

iNecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Dec 19, 2017

dLobatog left a comment

Choose a reason for hiding this comment

lzap commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Jan 17, 2018

iNecas left a comment

Choose a reason for hiding this comment

lzap commented Feb 13, 2018

ares commented Feb 13, 2018

lzap commented Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

ares commented Feb 19, 2018

lzap commented Feb 22, 2018

ares left a comment

Choose a reason for hiding this comment

lzap commented Feb 23, 2018

lzap commented Oct 6, 2017 •

edited

Loading

lzap commented Feb 15, 2018 •

edited

Loading