client,daemon,features: expose feature flags supported/enabled info #13681

olivercalder · 2024-03-09T03:26:41Z

Adds features.All() to return a map from feature flag names to a struct indicating whether that feature is supported and/or enabled. A callback functions may be defined for a feature flag which returns true if that feature flag is supported by the system, otherwise false.

Features which are set to a value other than true or false are not included in the map, as these are not feature flags.

If no callback function is defined for a given feature flag, it is assumed that that feature flag is supported. Currently, callbacks are defined for QuotaGroups (check that systemd version >= 230) and UserDaemons (check that user units are supported).

Features may be enabled but not supported. In this case, the feature flag has been set to true, but the underlying feature is not supported by the system.

This information is now included in the response to the /v2/system-info endpoint under a new "features" field. This lets clients get information about which features are supported and/or enabled without querying /v2/snaps/system/conf, which requires authentication.

codecov-commenter · 2024-03-09T03:39:07Z

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 78.89%. Comparing base (11952d7) to head (1c85159).

Files	Patch %	Lines
features/features.go	91.66%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13681   +/-   ##
=======================================
  Coverage   78.88%   78.89%           
=======================================
  Files        1040     1040           
  Lines      134088   134126   +38     
=======================================
+ Hits       105782   105816   +34     
- Misses      21698    21701    +3     
- Partials     6608     6609    +1

Flag	Coverage Δ
unittests	`78.89% <92.10%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder · 2024-03-10T21:57:36Z

Force-pushed to fix conflict with NEWS.md

bboozzoo

thanks, some comments about possibly extending the feature info we return

client/client_test.go

bboozzoo · 2024-03-11T07:54:31Z

daemon/api_general_test.go

+	const featuresKey = "features"
+	resultFeatures := rsp.Result.(map[string]interface{})[featuresKey]
+	c.Check(resultFeatures, check.Not(check.Equals), "")
+	delete(rsp.Result.(map[string]interface{}), featuresKey)


could you add those to the expected value above?

I could, but it should include an entry for every experimental feature, and I don't want adjusting features to require tweaking this test all the time. I'd rather check a few values from this manually (as is done later in the test).

bboozzoo · 2024-03-11T08:02:33Z

features/features.go

+type FeatureInfo struct {
+	Supported bool `json:"supported"`
+	Enabled   bool `json:"enabled"`
+}


could we add a string field eg. UnsupportedReason which would carry information why a given feature is not supported?

I like this idea a lot, thanks!

bboozzoo · 2024-03-11T08:03:53Z

features/features.go

+		// Features implicitly supported if no callback exists
+		supported := true
+		if callback, exists := featuresSupportedCallbacks[feature]; exists {
+			supported = callback()


in which case the callback would need to return more than a bool

It could either return (bool, error) where a false value must always be accompanied by a non-nil error. Or we could simply return error where nil means supported and non-nil means supported. The former is redundant but clearer, while the latter is not redundant (and matches usage of GetMaybe() in Flag()), but is slightly less clear on its own. Any preference between these two approaches?

I've implemented (bool, error) for now. Feel free to suggest otherwise if you'd prefer.

Actually, (bool, string), since it's really a description rather than an error.

pedronis

overall lgtm

features/features.go

bboozzoo

LGTM

pedronis

thanks, comments about the feature unsupported reasons

pedronis · 2024-03-12T08:29:28Z

features/features.go

-	UserDaemons: release.SystemctlSupportsUserUnits,
+	UserDaemons: func() (bool, string) {
+		if !release.SystemctlSupportsUserUnits() {
+			return false, "user session daemons are not supported on this release"


s/release/system/ or s /release/distribution version/

I like that "system" suggests host system, rather than version of snapd, so I think I'll go with "this system's distribution version". Thank you!

features/features.go

will need a re-review

MiguelPires

lgtm, with a suggestion for a test change (maybe for a follow-up?)

features/features.go

MiguelPires · 2024-03-12T10:53:27Z

features/features_test.go

@@ -216,3 +217,66 @@ func (s *featureSuite) TestFlag(c *C) {
 	_, err = features.Flag(tr, features.Layouts)
 	c.Assert(err, ErrorMatches, `layouts can only be set to 'true' or 'false', got "banana"`)
 }
+
+func (s *featureSuite) TestAll(c *C) {


I won't block the PR for it as it is tested but it would be better if this test didn't depend on the inner workings of specific flags like QuotaGroups since it might be broken by changing/adding callbacks. We could export featuresSupportedCallbacks through export_test.go and then set custom callbacks to test whatever we want (enabled but unsupported, support but disabled, etc) and check All's output

Thanks, and good idea, I'll add a task for myself to improve these tests in the future.

pedronis

thank you

ZeyadYasser

LGTM, Thank you!

olivercalder · 2024-03-13T14:00:22Z

Now that #13693 is merged, I can add a callback to this PR for the "apparmor-prompting" feature, then this can be merged.

olivercalder · 2024-03-13T19:15:37Z

Force pushed after rebasing on master to get AppArmorPrompting experimental feature which was merged into snapd master, and to clean up the commit history.

As part of rebase, also added a feature supported callback for AppArmorPrompting which always returns false with the error "requires newer version of snapd", for now, since there are prompting components which are not yet merged into snapd master and thus it should not be supported.

Add `All()` function, which returns a map of feature names to `FeatureInfo` structs. `FeatureInfo` contains boolean values for whether the feature is supported and whether it is enabled. A new map is defined from feature flags to callback functions which return true if the feature is supported and false if it is unsupported. Any feature for which no callback function is defined is assumed to be supported. Currently, callback functions are defined for `QuotaGroups` (check that systemd version >= 230), `UserDaemons` (check that user units are supported), and `AppArmorPrompting` (always return false, for now). Since some components of prompting are not yet merged into snapd master, the experimental "apparmor-prompting" feature is not yet supported. This commit adds a callback for "apparmor-prompting" which returns false and states that it requires a newer version of snapd. Once the rest of the prompting system is ready, the callback for AppArmorPrompting should be modified to instead check that the installed AppArmor has kernel and parser support for prompting, and that the notify socket for communicating with the kernel exists. Any features which are set to values other than true or false are omitted from the map returned by `All()`. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> features: add unsupported reason when feature not supported Signed-off-by: Oliver Calder <oliver.calder@canonical.com> features: improved docstrings for `All()` and `FeatureInfo` Signed-off-by: Oliver Calder <oliver.calder@canonical.com> features: adjusted callback feature unsupported reasons Signed-off-by: Oliver Calder <oliver.calder@canonical.com> features: add supported callback for apparmor prompting Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Add a "features" field containing a map of feature flag names to boolean subfields for whether the feature is "supported" and/or "enabled", along with an "unsupported-reason" if the feature is not supported. Feature flags which are not set to true or false are omitted from this map. Feature flags may be unsupported but nonetheless enabled. This indicates that the feature flag has been set to true, but the backing feature itself is not currently supported. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder · 2024-03-13T20:34:21Z

And force-pushed again to rebase on master with test fixes from #13685

olivercalder added this to the 2.62 milestone Mar 9, 2024

olivercalder requested review from bboozzoo, pedronis, MiguelPires and ZeyadYasser March 9, 2024 03:26

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Mar 9, 2024

olivercalder force-pushed the prompting-sysinfo-experimental-features branch from 0eb480f to daed63b Compare March 10, 2024 21:57

bboozzoo reviewed Mar 11, 2024

View reviewed changes

pedronis reviewed Mar 11, 2024

View reviewed changes

features/features.go Show resolved Hide resolved

olivercalder requested review from bboozzoo and pedronis March 11, 2024 19:13

bboozzoo previously approved these changes Mar 12, 2024

View reviewed changes

pedronis reviewed Mar 12, 2024

View reviewed changes

MiguelPires approved these changes Mar 12, 2024

View reviewed changes

olivercalder requested review from pedronis and bboozzoo March 12, 2024 14:52

olivercalder mentioned this pull request Mar 12, 2024

features: add experimental.apparmor-prompting feature stub #13693

Merged

pedronis approved these changes Mar 12, 2024

View reviewed changes

ZeyadYasser approved these changes Mar 13, 2024

View reviewed changes

olivercalder force-pushed the prompting-sysinfo-experimental-features branch from 2d9777b to 1c85159 Compare March 13, 2024 19:12

olivercalder added 2 commits March 13, 2024 15:32

olivercalder force-pushed the prompting-sysinfo-experimental-features branch from 1c85159 to 370eec0 Compare March 13, 2024 20:33

Meulengracht merged commit b78a22a into snapcore:master Mar 14, 2024
41 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client,daemon,features: expose feature flags supported/enabled info #13681

client,daemon,features: expose feature flags supported/enabled info #13681

olivercalder commented Mar 9, 2024

codecov-commenter commented Mar 9, 2024 •

edited

olivercalder commented Mar 10, 2024

bboozzoo left a comment

bboozzoo Mar 11, 2024

olivercalder Mar 11, 2024

bboozzoo Mar 11, 2024

olivercalder Mar 11, 2024

bboozzoo Mar 11, 2024

olivercalder Mar 11, 2024 •

edited

olivercalder Mar 11, 2024

olivercalder Mar 11, 2024

pedronis left a comment

bboozzoo left a comment

pedronis left a comment

pedronis Mar 12, 2024

olivercalder Mar 12, 2024

MiguelPires left a comment

MiguelPires Mar 12, 2024

olivercalder Mar 12, 2024

pedronis left a comment

ZeyadYasser left a comment

olivercalder commented Mar 13, 2024

olivercalder commented Mar 13, 2024

olivercalder commented Mar 13, 2024 •

edited

client,daemon,features: expose feature flags supported/enabled info #13681

client,daemon,features: expose feature flags supported/enabled info #13681

Conversation

olivercalder commented Mar 9, 2024

codecov-commenter commented Mar 9, 2024 • edited

Codecov Report

olivercalder commented Mar 10, 2024

bboozzoo left a 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

Choose a reason for hiding this comment

olivercalder Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelPires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

olivercalder commented Mar 13, 2024

olivercalder commented Mar 13, 2024

olivercalder commented Mar 13, 2024 • edited

codecov-commenter commented Mar 9, 2024 •

edited

olivercalder Mar 11, 2024 •

edited

olivercalder commented Mar 13, 2024 •

edited