daemon,o/devicestate,asserts: add confdb-control api #15104

st3v3nmw · 2025-02-20T07:01:59Z

This PR adds the confdb-control API as specified in SD186.

I've removed the mandatory requirement for groups since it's possible to have a confdb-control assertion without any delegations. Previously, it would raise an error:

$ sudo curl --unix-socket /run/snapd.socket -X POST \
  -d '{"action": "delegate", "operator-id": "alice", "views": ["canonical/confdb/view", "canonical/another/view"], "authentications": ["operator-key"]}' \
  "http://localhost/v2/confdb"
{"type":"sync","status-code":200,"status":"OK","result":null}

$ snap known confdb-control
type: confdb-control
brand-id: generic
model: generic-classic
serial: <serial>
groups:
  -
    authentications:
      - operator-key
    operators:
      - alice
    views:
      - canonical/another/view
      - canonical/confdb/view
sign-key-sha3-384: t9yuKGLyiezBq_PXMJZsGdkTukmL7MgrgqXAlxxiZF4TYryOjZcy48nnjDmEHQDp

AcLBUgQAAQoABgUCZ7bRbAAATAUQAHPCWwk5LfdGdU+A1iuaoHEA6Hxnd+WgaNWFQuCyUldDzDAR
NF3F9Js8Zj/nDjFkWAtf6I09+6O9f4mHFEltfkCfyDbUzs/c68M4Ti3kigpDuqMkoggNzF07j0tE
aYwNo3KtWQtD9kJz/rrAh48ZlKUuLfXdnDPuLuYP6gGy2AtlyKRhvE6ZZ4wrNiDlIf5TXE4CaiKh
/IxOyLZMDg5pcJTMwIUdEHWIX8+DBG9t9Ix3WZwJptwuPWIQDq5isKuhlR6Ul/P1sarMeQbxivKG
VUMlL/Sjt+PqRBKeufWtlkUf3Lg3OoUc3XJzCnj1Y1coRtWpXfSgtcCH2MF7/HoMlL1TLf8HSYX+
8bwGtlQYddYzsf/mY4Y35JAYaqqh62KLgTtem/BfZEdasIduOb/ZiljZ7ETDndr3PEgelfeUgY5m
4Z3jxOTjvdE7tWDRdDdw2L9J+VXvqbx3fprPy2Go/ewU7fpMcxxfkVRp3uyRqyLUefYk3dbLULmx
3lrqtAIY1cA7pPocS6ntpv1khjNBI30GAuJSxz1h8fQozZGNiFrw1pR8xLTvt/1z6iTPnGsXi9Bn
IodMN3Od2clhVV3L3BrFPwze4CKPgd0PXa/kNULf8KRnHzCDVuyJDrYfplWQzCwKWZWJF8vxEGPV
lvqt7pQh8PkLMQRkzhSM62VufmdA

$ sudo curl --unix-socket /run/snapd.socket -X POST \
  -d '{"action": "undelegate", "operator-id": "alice"}' \
  "http://localhost/v2/confdb"
{"type":"sync","status-code":200,"status":"OK","result":null}%

$ snap known confdb-control
error: searching assertions failed: broken assertion storage, searching for confdb-control:
       broken assertion storage, cannot decode assertion: assertion confdb-control: "groups" stanza
       is mandatory

$ cat /var/lib/snapd/assertions/asserts-v0/confdb-control/generic/generic-classic/<serial>/active
type: confdb-control
revision: 1
brand-id: generic
model: generic-classic
serial: <serial>
sign-key-sha3-384: t9yuKGLyiezBq_PXMJZsGdkTukmL7MgrgqXAlxxiZF4TYryOjZcy48nnjDmEHQDp

AcLBUgQAAQoABgUCZ7bRfgAAnOYQACRgCaFL3hGaDAfNsM9YIx7m0KqXSEYrF4TOLj4JtdjlH8l3
8yqkmgI9wD98OekqwRzjziBLFgY5HId3jVz6u91KfArA8BCj1Gru56c2dY8RusoPX94eVtNKMjpJ
EuUx276x7hEHNQe72QOGH5khFB5zSwmSNHjgUTCtzGz7hLZL6tY32Gt4TF+sT1Nf5kiA7JTdV2tw
WCDSEORf2bs4IsJR3jTvNSydYeqn95RvGkDL4zA5lmYygVQtCWemWjjhpZWJVQ5aPlm7IxEf14RC
QU7Ja6UnV9KOGQbIQcdUYQZMC7u2QXaMes0OcXFo1JO+EmTW9nJA0YYill8nAB+e22OS4eUekFLd
5F2mttxJC7az7UEO3B2dLvaQyvQ7Vsx5qbOyu7fq8HazwNQiMH1C34RpfBAXd2mCCQRmVWWpiNDv
sp25Xr6PiM8kg/uRYHAAfRFj1PgNAJPDA5qLW1kfCnq4R3KvpLA+iqCLVDv261kQZY8ArQN4dBaf
92ixC8Fo+uwh4H7hMI+6XeVQy5XLolOCuBYK5VaArY8Wttya44+CYWQLeDGzTJ5b7lyemKHZ9C0W
s5FFh7Q7zeZcL2/xlCgYiGjmbNmK9SqbCbSYxpma9RDaUGxRrWKt+gEOhdvj8Qj4RUaJsLS10K3Y
vmYbsQbEWBI50lU4tRpEitMLFiFZ

github-actions · 2025-02-20T09:15:12Z

Mon Feb 24 08:47:33 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13493784267

No spread failures reported

miguelpires

thanks, some comments

miguelpires · 2025-02-21T11:49:08Z

daemon/api_confdb.go

-		return BadRequest(fmt.Sprintf(`"confdbs" feature flag is disabled: set '%s' to true`, confName))
+		_, confName := feature.ConfigOption()
+		return BadRequest(
+			fmt.Sprintf(`"%s" feature flag is disabled: set '%s' to true`, feature.String(), confName),


nitpick: can be simplified

Suggested change

fmt.Sprintf(`"%s" feature flag is disabled: set '%s' to true`, feature.String(), confName),

fmt.Sprintf(`feature flag %q is disabled: set '%s' to true`, feature, confName),

miguelpires · 2025-02-21T11:58:44Z

daemon/api_confdb.go

+func getOrCreateConfdbControl(st *state.State, devMgr *devicestate.DeviceManager) (*confdb.Control, int, error) {
+	serial, err := devMgr.Serial()
+	if err != nil {
+		return nil, 0, errors.New("device has no serial assertion")


this should wrap the returned error, this seems like a potential oversimplification

I had checked the possible errors and some wouldn't be meaningful to the API user (like no state entry for key) and some are already captured in the simplified message (like serial assertion not found):

internal error: could not unmarshal state entry %q: %v # device state broken no state entry for key serial assertion not found cannot find %q assertions for format %d higher than supported format %d # assertion format changed

These ones cannot occur unless the overlord/devicestate findSerial implementation changes:

internal error: assertion type cannot be nil internal error: unknown assertion type serial must provide primary key: %v # primary key not in provided headers internal error: unpredefined assertion type for name %q used (unexpected address %p) # ^ unknown assertion type

But I agree that wrapping would be useful for debugging since the errors are not logged.

miguelpires · 2025-02-21T13:41:17Z

daemon/api_confdb.go

+}
+
+// getOrCreateConfdbControl returns the confdb.Control base to build the next revision of the assertion.
+func getOrCreateConfdbControl(st *state.State, devMgr *devicestate.DeviceManager) (*confdb.Control, int, error) {


This is a bit inconsistent, it's returning the delegations of the last assertion but the revision number for the new one. Returning the asserts.ConfdbControl might be clearer:

Suggested change

func getOrCreateConfdbControl(st *state.State, devMgr *devicestate.DeviceManager) (*confdb.Control, int, error) {

func getLatestConfdbControl(st *state.State, devMgr *devicestate.DeviceManager) (*asserts.Control, error) {

miguelpires · 2025-02-21T13:58:44Z

daemon/api_confdb.go

 	if err != nil && !config.IsNoOption(err) {
-		return InternalError(fmt.Sprintf("internal error: cannot check confdbs feature flag: %s", err))
+		return InternalError(
+			fmt.Sprintf("internal error: cannot check %s feature flag: %s", feature.String(), err),


Suggested change

fmt.Sprintf("internal error: cannot check %s feature flag: %s", feature.String(), err),

fmt.Sprintf("internal error: cannot check feature flag %q: %s", feature, err),

miguelpires · 2025-02-21T14:55:08Z

daemon/api_confdb_test.go

+	c.Check(rspe.Message, Equals, `"confdb-control" feature flag is disabled: set 'experimental.confdb-control' to true`)
+}
+
+func (s *confdbSuite) TestValidateFeatureFlagErr(c *C) {


This isn't testing much in the daemon itself. IMO it's not really worth exporting validateFeature to cover a single line of error handling.

miguelpires · 2025-02-21T14:59:11Z

daemon/export_test.go

+func ValidateFeatureFlag(st *state.State, feature features.SnapdFeature) *apiError {
+	return validateFeatureFlag(st, feature)
+}


but if we keep it this can be shortened

Suggested change

func ValidateFeatureFlag(st *state.State, feature features.SnapdFeature) *apiError {

return validateFeatureFlag(st, feature)

}

var ValidateFeatureFlag = validateFeatureFlag

pedronis · 2025-02-21T15:41:06Z

daemon/api_confdb.go

+}
+
+// getOrCreateConfdbControl returns the confdb.Control base to build the next revision of the assertion.
+func getOrCreateConfdbControl(st *state.State, devMgr *devicestate.DeviceManager) (*confdb.Control, int, error) {


it feels like for consistency maybe this also belong to devicestate in some form?

codecov · 2025-02-24T08:47:51Z

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.07%. Comparing base (a272aac) to head (fa22263).
Report is 36 commits behind head on master.

Files with missing lines	Patch %	Lines
daemon/api_confdb.go	94.91%	3 Missing ⚠️
overlord/devicestate/devicemgr.go	83.33%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #15104    +/-   ##
========================================
  Coverage   78.07%   78.07%            
========================================
  Files        1182     1181     -1     
  Lines      157743   158004   +261     
========================================
+ Hits       123154   123369   +215     
- Misses      26943    26974    +31     
- Partials     7646     7661    +15

Flag	Coverage Δ
unittests	`78.07% <92.20%> (+<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.

daemon,asserts: add confdb-control api

14c07ad

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Feb 20, 2025

pedronis requested review from pedronis and miguelpires February 20, 2025 08:47

miguelpires reviewed Feb 21, 2025

View reviewed changes

pedronis reviewed Feb 21, 2025

View reviewed changes

daemon,o/devicestate: refactor error messages & devMgr.ConfdbControl

fa22263

st3v3nmw changed the title ~~daemon,asserts: add confdb-control api~~ daemon,o/devicestate,asserts: add confdb-control api Feb 24, 2025

st3v3nmw requested review from pedronis and miguelpires February 24, 2025 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon,o/devicestate,asserts: add confdb-control api #15104

daemon,o/devicestate,asserts: add confdb-control api #15104

st3v3nmw commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 •

edited

Loading

miguelpires left a comment

miguelpires Feb 21, 2025

miguelpires Feb 21, 2025

st3v3nmw Feb 24, 2025

miguelpires Feb 21, 2025

miguelpires Feb 21, 2025

miguelpires Feb 21, 2025

miguelpires Feb 21, 2025

pedronis Feb 21, 2025

codecov bot commented Feb 24, 2025

	fmt.Sprintf(`"%s" feature flag is disabled: set '%s' to true`, feature.String(), confName),
	fmt.Sprintf(`feature flag %q is disabled: set '%s' to true`, feature, confName),

	func getOrCreateConfdbControl(st state.State, devMgr devicestate.DeviceManager) (*confdb.Control, int, error) {
	func getLatestConfdbControl(st state.State, devMgr devicestate.DeviceManager) (*asserts.Control, error) {

	fmt.Sprintf("internal error: cannot check %s feature flag: %s", feature.String(), err),
	fmt.Sprintf("internal error: cannot check feature flag %q: %s", feature, err),

daemon,o/devicestate,asserts: add confdb-control api #15104

Are you sure you want to change the base?

daemon,o/devicestate,asserts: add confdb-control api #15104

Conversation

st3v3nmw commented Feb 20, 2025

github-actions bot commented Feb 20, 2025 • edited Loading

No spread failures reported

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

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

codecov bot commented Feb 24, 2025

Codecov Report

github-actions bot commented Feb 20, 2025 •

edited

Loading