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

Migrate to schnauzer v2 for configuration file templates #3346

Merged
merged 5 commits into from
Aug 28, 2023

Conversation

cbgbt
Copy link
Contributor

@cbgbt cbgbt commented Aug 15, 2023

NOTE This is against the ootb-settings-extension feature branch.

Issue number:
#3133

Description of changes:

  • Adds support for "optional" settings extensions in templates. This lets us e.g. use AWS settings if the extension is installed, but not otherwise. Right now, the implementation does nothing because there's no concept of a setting being present or not.
  • Uses the new schnauzer v2 rendering library in thar-be-settings, and changes all current templates to the new format
  • Simplify schnauzer implementation by moving some Option types to have a serde(default).

Existing templates were found by checking for all files installed into %{_cross_templatedir}.

Testing done:

  • All built-in test suites pass
  • Booted and checked logs/behavior of a k8s and ecs variant
  • Fuzz testing (I'm generating a corpus of settings objects and asserting that the old template system produces the same results as the new one)

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@cbgbt cbgbt marked this pull request as draft August 15, 2023 23:50
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 15, 2023

This is what I get for doing some rebasing and not re-running the build 😅

@@ -1,3 +1,4 @@
+++
Copy link
Contributor

Choose a reason for hiding this comment

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

I keep seeing this and wondering if we should make the +++ delimiter optional, but then I think what a weird template this is and start wondering why it's a template at all.

We've gotten to a good place as far as ownership for the settings themselves and for related helper functions, but we still don't have a clear picture for who should own the template, which seems like it's inherently a cross-cutting concern that has to reflect the builder's intent.

For Kubernetes, we try to have a one-size-fits-all template, but it ends up requiring a lot of extensions. For containerd, we package all the templates with the software, which leads to this no-op template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we must always require the +++ delimiter at the start of a template, otherwise it is impossible for the parser to appropriately disambiguate a template body with +++ in it, especially if that body is a TOML document.

As far as who owns the templates, I've been thinking they may want to live in their own sub-packages that can be overridden, and use e.g. Provides: template(kubelet-kubeconfig))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-reading this while not being in a rush, I agree that this probably shouldn't be a template at all.

@@ -1,3 +1,7 @@
[required-extensions]
kubernetes = { version = "v1", helpers = ["any_enabled"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to make any_enabled a std helper or does it assume too much about the data shape to work for anything other than Kubernetes?

@cbgbt cbgbt force-pushed the ootb-settings-extensions branch from 7f6e634 to 6efc144 Compare August 25, 2023 00:26
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 25, 2023

^ Rebases onto develop

@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 25, 2023

The above:

  • Fixes two templates that changed in develop during the rebase
  • Moves the any_enabled helper to std as requested by @bcressey

@cbgbt cbgbt force-pushed the ootb-settings-extensions branch from 6efc144 to d88fae3 Compare August 26, 2023 00:26
cbgbt added 4 commits August 27, 2023 23:56
Bottlerocket invokes thar-be-settings whenever it needs to re-render
configuration file templates defined in the Bottlerocket API. This
change modifies thar-be-settings to use the schnauzer v2 renderer, and
also modifies all existing configuration templates in Bottlerocket to
use the new template format.
In order for helpers to be composable, they must implement the
`call_inner` method for the `HelperDef` trait. By failing to implement
this for `SettingExtensionTemplateHelper`, helpers used e.g. in an `if`
block would not return correct results.
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 28, 2023

This should be ready to merge. I finished fuzz testing, which uncovered two bugs:

  • I missed a comma in one of the template headers
  • helpers in handlebars implement either call or call_inner depending on how they are defined. call_inner is composable, but call is not. I modified our wrapper helper to pass calls to both through to the wrapped helper.

@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 28, 2023

Some details about fuzz testing:

  • I implemented random model generation for Bottlerocket in this branch. It's a mess and can't really be done in a maintainable way to merge upstream, but it's helpful here.
  • I generated 5k settings JSON blobs for each variant
  • I wrote a tool which clones two Bottlerocket branches and renders a list of templates from both, one with schnauzer::v1 and the other with schnauzer::v2 and compares the ouput.
  • The compared templates are these
  • I did have to make a few changes to helpers to avoid network calls and to make iteration through HashMaps deterministic, but otherwise left things unchanged.

All 135,000 models now render identically for both tools.

@cbgbt cbgbt marked this pull request as ready for review August 28, 2023 00:35
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Great testing coverage!

@cbgbt cbgbt merged this pull request into bottlerocket-os:ootb-settings-extensions Aug 28, 2023
@cbgbt cbgbt deleted the cfgrender branch August 28, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants