Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for overridding whether resources are enabled #5506

Merged
merged 23 commits into from
Mar 8, 2022

Conversation

amanda11
Copy link
Contributor

@amanda11 amanda11 commented Dec 16, 2021

Add support to override the enabled setting for actions, aliases, rules, sensors.

Order of loading of settings is:

  1. Get value from pack configuration
  2. Override with default value from global.yaml
  3. Override with default value from <pack>.yaml
  4. Override with exceptions for specific resource from <pack>.yaml

Looks for override config in /opt/stackstorm/configs/overrides directory (subsequent PR to get this directory created with base examples files in st2-package will be required).

Example files:

/opt/stackstorm/configs/overrides/global.yaml to disable all sensors and aliases

---
sensors:
  defaults:
    enabled: false
aliases:
  defaults:
    enabled: false

/opt/stackstorm/configs/overrides/<packname>.yaml to disable all actions exception for .delete_row

---
actions:
  defaults:
    enabled: false
  exceptions:
    delete_row:
      enabled: true

Overrides for packs are only specified in the pack.yaml as did not want to add to global.yaml as then there are two places to potentially configure the same type of override.
Could be extended in future to allow overriding of other metadata, but limiting to enabled parameter to start.

Resolves #3973

Supports pack.yaml files in /opt/stackstorm/configs/overrides of following format:
---
actions:
  defaults:
    enabled: false
  exceptions:
    delete_row:
      enabled: true
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Dec 16, 2021
@amanda11 amanda11 added this to the 3.7.0 milestone Dec 16, 2021
@amanda11 amanda11 marked this pull request as draft December 16, 2021 14:31
@amanda11 amanda11 changed the title [WIP] Add support for overridding whether resources are enabled Add support for overridding whether resources are enabled Dec 19, 2021
@amanda11 amanda11 marked this pull request as ready for review December 19, 2021 12:23
@amanda11
Copy link
Contributor Author

amanda11 commented Dec 19, 2021

Ready for review now, some points:

  • I was for initial PR aiming to only consider data at the top level - so adding on support for overriding action parameters I think should be added as an additional PR - as I think that is an extension. The structure used in the yaml could easily be extended to include paramters, but I think that level of overridding is an enhancement on top of this PR rather than part of this PR.
  • up for debate whether to a) just allow enabled to be overridden b) allow everything to be overridden that is at the top-level resource, but for most of these I wasn't sure much value - e.g. description, runner_type etc aren't really things I can see any value in overriding - but if want we could either just allow anything to be overridden, or extend set, or allow anything with validation so can detect if they have put in mis-spelt names etc. If we allow more to be overridden will that cause more problems than help, e.g. changing runner_type, entry_points etc really changes the whole action dynamic, and so probably shouldn't be allowed - so I don't think its as simple as just allow everything to be overridden - as if you are changing runner_type for example you should be installing a new version of the pack.

@amanda11 amanda11 requested a review from a team December 21, 2021 16:46
"enabled",
]

def override(self, pack_name, type, content):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to avoid hijacking the type keyword and use resource_type instead?

@arm4b
Copy link
Member

arm4b commented Dec 22, 2021

Good stuff!

One concern though as I'm looking at the https://docs.stackstorm.com/reference/pack_configs.html, when the entire pack config is expected to be located in the /opt/stackstorm/configs/<pack>.yaml.

Logically, /opt/stackstorm/configs/<pack>.yaml is already the one config to override the pack settings. Then the new override dir within the config dir feels a bit off compared to the st2 configuration we already had (looking at the RBAC structure as well). The answer on how to configure stuff and where to look at should be a really obvious and a single place to the users. I think multiple configs for a single pack might confuse the people.

With a single config, the advantage seems to me is the design consistency, as we won't need to introduce a new configuration structure /opt/stackstorm/configs/overrides/<pack>.yaml. Additionally, all the deployments like Ansible, Puppet, K8s, or anything custom users created for themselves to manage the pack configs would work. Just plug-and-play by adding a new configuration section.

Talking about the tradeoffs, do you see any advantages with the /overrides/ structure?
What would be the difficulties to rely on the content overrides within the original pack config file?

@nzlosh
Copy link
Contributor

nzlosh commented Dec 23, 2021

While it might appear to make sense to have overrides mixed in with the pack config, overrides are both pack config and individual unit config. They are not well defined which makes them a poor fit in the config/<pack>.yaml because pack config has a well defined schema. Attempting to maintain override variables inside config/<pack>.yaml puts a burden on the core developer to maintain a list of valid overrides as well as the pack developer to include them in the schema. This effort yields very little advantage. The only override that makes sense in pack config is the global override policy.

The difference between pack configuration and action parameters is that configuration usually contains values which are common to all the resources in the pack, and rarely change.

As stated above, overrides are specific to a unit (action/rule/sensor) and should not be part of pack config. Overriding unit config can change often as the pack evolves or override needs change with the environment St2 is running in.

This article https://www.redhat.com/sysadmin/etc-configuration-directories sums up the approach of having separate config files which the same principles apply to packs. Even St2 packs use separate metadata file per unit rather than a single metadata for all units.

Organizing configuration files into directories can make administration easier to manage and to delegate. Files may be organized by package or function and configuration management solutions such as Ansible can even benefit from smaller template files. You may need to standardize on file names and locations, and you may need to update file verification tools, but using a dot d folder to house multiple configuration files can save you a lot of effort.

To summarise: Overrides should not be mixed with pack config data. They should at least be in a separate directory from config data to avoid name collision: overrides sounds like a very good and clear choice. I still prefer the mirror the pack structure idea, but if the compromise is to put all unit overrides into a single file, then having them in overrides/<pack>.yaml will be a good solution for easier administration and use by automation tools.

@amanda11
Copy link
Contributor Author

amanda11 commented Jan 4, 2022

I think its clearer to have overrides separate to config (the directory can be created by the st2-package install - so shouldn't affect any of the deployers - so I don't think there is a problem with needing to change installers). But I think Carlos explanation is really good.

Maybe cleaner to move up to /opt/stackstorm/overrides rather than under configs/overrides - might be even more clearer to users then?

@arm4b
Copy link
Member

arm4b commented Jan 4, 2022

Good points @nzlosh, I'm good as long as we have a good reasoning behind it and consider the corner cases there 👍
And yeah, I think /opt/stackstorm/overrides/ looks a bit cleaner alternative indeed.

Would appreciate more 👀 from the @StackStorm/maintainers and @StackStorm/contributors around the feature, syntax, and new functionality in general.

@cognifloyd
Copy link
Member

cognifloyd commented Jan 4, 2022

👍 /opt/stackstorm/overrides

Prior art / maybe something to provide inspiration here

The overrides/<pack>.yaml looks very similar to something else I did.

About enabling/disabling rules/actions/other resources. I needed to enable/disable rules based on which instance of stackstorm our packs gets installed in. eg no rules in dev, and only datacenter-relevant rules in prod instances.
To accomplish this, I created a pack_resources.yaml file in the root of our pack repos. This file is consumed by a deploy_pack workflow which installs the pack and then runs a manage_resources action that enables the various resources according to pack_resources.yaml.

I made a copy of the relevant actions so I could post them here: https://github.com/cognifloyd/st2-pack-lifecycle

The format is similar to the proposed overrides/<pack>.yaml:

<webui_base_url_hostname>:
  <resource_type>:
    - <resource_ref>
  • I use the webui's hostname to distinguish between different st2 instances: (basically urlparse(st2common.config.cfg.CONF.webui.webui_base_url).hostname).
  • resource_type is one of rules, policies, sensors, triggers, actions, aliases
  • resource_ref is either the fully qualified ref (chatops.post_message) or it implicitly uses the pack name of the current pack (post_message). pack_resources.yaml can only enable resources for its own pack, not any other pack.

Here's an example:

st2.example.com:
  rules:
    - autoremediation.disk.foobar

One downside to this approach is that it would not work for packs on the exchange. So, admins still need a way to do configure those packs per installation without modifying the packs.

Also, it only handles enabling resources assuming that they will be disabled by default.

Anywho these are some ideas - maybe pieces could be useful to reuse here.

@arm4b arm4b added the feature label Jan 4, 2022
@amanda11
Copy link
Contributor Author

amanda11 commented Feb 1, 2022

Some discussions from TSC:

  • With the proposed implementation, the overrides is NOT a policy. If a user uses the CLI to disable/enable an action then that will have the same affect as now - and will take place until the pack is reloaded or re-installed. Enabling/disabling via the CLI/API still only affects until reload. It wasn't intended to make this a policy - but clear documentation would be required to ensure no-one was mislead
  • Do we want to have a hierarchical structure to configs, for example instead of /opt/stackstorm/overrides/.yaml we could have /opt/stackstorm/overrides/packname/local.yaml
  • We discussed in Disable/Enable Sensors/Rules/Aliases via blacklisting/whitelisting in pack config #3973 (comment) and Disable/Enable Sensors/Rules/Aliases via blacklisting/whitelisting in pack config #3973 (comment) the idea of having a full mirrored overrides of the pack, similar to the "local" concept discussed in TSC meetings. So there is some prior discussion already, but would welcome more comments.
  • Need appropriate logging to show that overrides are taking place (I had moved some logging down from info->debug), but I think it was agreed that we'd need to have something in the reload to indicate overrides were taking place - so probably needs some more clever logging to the INFO level to make it clear. Would need to be visible on pack install, and reload.

@arm4b
Copy link
Member

arm4b commented Feb 1, 2022

Pack configs has the following structure: /opt/stackstorm/configs/packname.yaml and the new overrides would mimick the similar UX with /opt/stackstorm/overrides/packname.yaml.

@punkrokk If we create the dedicated dir for pack in /opt/stackstorm/overrides/packname/ besides of local.yaml what would we need to place there to justify the nested dir structure? Can you provide more details following the TSC discussion today?

@punkrokk
Copy link
Member

punkrokk commented Feb 2, 2022

Here is the explanation of how splunk does things:
https://docs.splunk.com/Documentation/Splunk/8.2.4/Admin/Wheretofindtheconfigurationfiles

@arm4b
Copy link
Member

arm4b commented Feb 3, 2022

Looks like Splunk uses the following override structure:

$SPLUNK_HOME/etc/system/local/*
$SPLUNK_HOME/etc/system/default/*
$SPLUNK_HOME/etc/apps/A/local/*
$SPLUNK_HOME/etc/apps/A/default/*

That's very different from what we have in st2 considering how pack defaults already shipped by content creators, but an interesting perspective to know. Thanks for sharing.

@amanda11 amanda11 changed the title [WIP]Add support for overridding whether resources are enabled Add support for overridding whether resources are enabled Feb 14, 2022
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few ntipicks, but otherwise, I like it :)

st2common/st2common/content/loader.py Outdated Show resolved Hide resolved

override_dir = os.path.join(cfg.CONF.system.base_path, "overrides")
# Apply global overrides
global_file = os.path.join(override_dir, "global.yaml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do something during pack install to make sure the pack name is not global so people don't shoot themselves in the foot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Nice find of the corner case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cognifloyd I'll take a look at how best to do that, the alternative is to make the global something we can't use as a packname...

st2common/tests/resources/overrides/global.yaml Outdated Show resolved Hide resolved
st2common/st2common/content/loader.py Outdated Show resolved Hide resolved
st2common/st2common/content/loader.py Outdated Show resolved Hide resolved
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍

We'll also need to pre-create the override dir via packaging.
Here's an example for configs: https://github.com/StackStorm/st2-packages/search?q=configs

@pull-request-size pull-request-size bot added size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Mar 4, 2022
@amanda11
Copy link
Contributor Author

amanda11 commented Mar 4, 2022

@armab and @cognifloyd I've added code to prevent pack names that match the reserved pack name list - currently only holds "global" (st2common/st2common/util/pack.py being the main change for that).

Also updated the doc PR in the upgrade notes and pack development to say "global" is reserved, and cannot be used for pack names. See StackStorm/st2docs#1109.

Packnames can contain _ - so if we wanted we could use _global.yml if preferred?

Let us know if that addresses the good spot from @cognifloyd, and any other comments/improvements we might need.

@cognifloyd
Copy link
Member

Ooh. I like _global.yaml

@arm4b
Copy link
Member

arm4b commented Mar 5, 2022

Same here, _global.yaml naming reads like a great idea for this special case 👍

@amanda11
Copy link
Contributor Author

amanda11 commented Mar 8, 2022

@armab @cognifloyd Renamed, any further thoughts or inputs we want to get on override feature?

So to summarise /opt/stackstorm/overrides/_global.yaml is where the global override goes to, and packs go to /opt/stackstorm/overrides/packname.yaml.

_global - is now treated as a reserved name, and not to be used as a pack name.

@cognifloyd
Copy link
Member

I'll merge this and the docs PR once we're satisfied with the docs. 😄 If anyone has additional feedback for this PR, now's the time to add it 😉

@cognifloyd cognifloyd merged commit a80fa2b into master Mar 8, 2022
@cognifloyd cognifloyd deleted the add_override_support branch March 8, 2022 15:44
@cognifloyd
Copy link
Member

If anyone finds anything they'd like to change here, please add a follow-up PR or issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable/Enable Sensors/Rules/Aliases via blacklisting/whitelisting in pack config
5 participants