daemon: add notices API #13364

olivercalder · 2023-11-15T00:35:23Z

This PR adds the notices API endpoints, which are originally from pebble. The spec for pebble notices is JUu48

Various modifications are required to adapt the notices API to snapd. The most notable are the removal of the custom notice type and the removal of the POST notices handler, both of which result from the fact that snapd does not have a notices client or user-defined notices.

Additionally, snapd uses the strutil package internal to snapd, while pebble uses strutil from github.com/canonical/x-go. The prior addition of MultiCommaSeparatedList to the x-go/strutil package must be ported to snapd/strutil as well.

The corresponding PR from pebble can be found at: canonical/pebble#296

olivercalder · 2023-11-15T00:39:22Z

NEWS.md will need to be modified to note that the notices API has been added. This can likely occur along with other API documentation changes.

miguelpires

LGTM

miguelpires · 2023-11-16T10:31:01Z

daemon/api_notices.go

+		if !noticeType.Valid() {
+			// Ignore invalid notice types (so requests from newer clients
+			// with unknown types succeed).
+			continue


nitpick: there could be a short test for this

Good point, thanks! Technically, I'm not sure that this check is even necessary, since the notice filter within overlord/state/notices.go will happily accept arbitrary strings, but I agree it's safer to sanity check them here before they make it to the state notices code. I'll add a test for this.

Actually, if the user only has invalid notice types, the current behavior would return all notices (assuming no other filters), since this is equivalent to an empty types filter. I'll add a check and tests to make sure no notices are returned in this case.

miguelpires · 2023-11-16T10:44:54Z

daemon/api_notices_test.go

+	elapsed := time.Since(start)
+	c.Check(elapsed > 10*time.Millisecond, Equals, true)
+	c.Check(elapsed < time.Second, Equals, true)


This feels a bit brittle. It's probably fine but I'm thinking about the slower systems on which spread runs, we've had issues with timing dependent things before I think. Just something to be on the lookout in the first runs

I think checking elapsed > 10*time.Millisecond is the brittle part, as it could be that the goroutine waiting to cancel the context runs for a while before start time is recorded. I think the solution is to record the start time before spawning the cancel goroutine. Checking that elapsed time is less than 1s should be safe, I would think, on all systems.

miguelpires · 2023-11-16T10:48:17Z

daemon/api_notices_test.go

+
+	st := s.d.Overlord().State()
+	go func() {
+		time.Sleep(10 * time.Millisecond)


If we do see any timing-related issues and end up having to mock WaitNotices, we could also change this one

Meulengracht

Thank you, the code looks nice in general, and nice tests! A couple of comments

daemon/api_notices.go

strutil/strutil.go

Meulengracht · 2023-11-29T07:46:49Z

strutil/strutil.go

@@ -231,6 +231,16 @@ func CommaSeparatedList(str string) []string {
 	return filtered
 }

+// MultiCommaSeparatedList parses each string in strs with CommaSeparatedList
+// and returns the concatenation of all parsed values.
+func MultiCommaSeparatedList(strs []string) []string {


Nitpick, but I think it provides clearer intent:

Suggested change

func MultiCommaSeparatedList(strs []string) []string {

func FlattenCommaSeparatedLists(strs []string) []string {

I agree and prefer your approach to this naming and description, but this is an existing function in the "github.com/canonical/x-go/strutil" package, so for compatibility's sake I don't think we should change them here, if the code body is otherwise unchanged.

Ah, a bit annoying we have a copy of another package, but I see

codecov-commenter · 2023-11-30T03:29:30Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d01f435) 78.80% compared to head (d1cf801) 78.85%.
Report is 22 commits behind head on master.

Files	Patch %	Lines
daemon/api_notices.go	96.00%	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   #13364      +/-   ##
==========================================
+ Coverage   78.80%   78.85%   +0.04%     
==========================================
  Files        1022     1024       +2     
  Lines      127295   127648     +353     
==========================================
+ Hits       100317   100654     +337     
- Misses      20697    20707      +10     
- Partials     6281     6287       +6

Flag	Coverage Δ
unittests	`78.85% <96.70%> (+0.04%)`	⬆️

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.

Meulengracht

Thanks for the changes, +1

pedronis

thanks, one remark about ReadAccess

pedronis · 2023-11-30T18:41:04Z

daemon/api_notices.go

+		ReadAccess: openAccess{},
+	}
+
+	noticeCmd = &Command{
+		Path:       "/v2/notices/{id}",
+		GET:        getNotice,
+		ReadAccess: openAccess{},


until we have the distinction of public|private coming together with the per-user work we probably want authenticatedAccess here (that also matches UserOK on the pebble side)

Good point, thanks!

pedronis · 2023-11-30T18:44:44Z

daemon/request.go

I see that this was added just like this on the pebble side, I wonder though if both there and here more helper could move here, otoh it's really an orthogonal issue to the PR

olivercalder · 2023-12-01T08:54:47Z

@ernestl or @Meulengracht , could you please advise me on what I need to do for recording the addition of the notices API in NEWS.md?

pedronis

thank you

olivercalder · 2023-12-06T17:05:54Z

Added "Skip" spread so I can rebase without running spread tests. Previous spread run had only 1 failure, on Ubuntu 20.04-arm (log here: https://github.com/snapcore/snapd/actions/runs/7105573085/job/19355042068?pr=13364)

This is a port from Pebble into snapd of the notices API. The original PR can be found at: canonical/pebble#296 Signed-off-by: Oliver Calder <[email protected]>

This new function is useful for parsing `url.Values` where there may be multiple values for the same key, and those values may themselves be comma-separated values. This is a port of a commit from the `x-go` package, which was originally intended to be used in pebble. The commit can be found here: canonical/x-go@32684ae Signed-off-by: Oliver Calder <[email protected]>

As notices are originally from pebble, various adjustments must be made in order for the ported API to work with snapd. Signed-off-by: Oliver Calder <[email protected]>

Unlike pebble, snapd does not have a client for notices, and notices should not be created from outside of snapd. As a result, there is no need for a POST handler on the notices API. Signed-off-by: Oliver Calder <[email protected]>

In snapd, there are no user-defined notices, so there is no need for a custom notice type. Signed-off-by: Oliver Calder <[email protected]>

Invalid notice types are ignored when parsing the API request to construct the notice filter. However, if only invalid notice types are included in the request, discarding all types results in a filter indistinguishable from one which never had any types in the first place, and thus should return notices of any type. This commit fixes this behaviour, so no notices are returned if only invalid notice types are requested. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

Most importantly, wait to lock the state until after the optional duration has been parsed, in case an error occurs. Also, moves the types query parsing/sanitization into its own function, since this cleans up the GET notices handler and better matches future handling of user ID filtering. Signed-off-by: Oliver Calder <[email protected]>

TODO: probably revert this commit once per-user notices are implemented, as that will provide the proper restrictions so that non-privileged users cannot read private notices intended for other users. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Nov 15, 2023

olivercalder requested a review from miguelpires November 15, 2023 00:36

olivercalder added Needs Samuele review Needs a review from Samuele before it can land Needs Documentation 📖 and removed Needs Documentation -auto- Label automatically added which indicates the change needs documentation labels Nov 15, 2023

olivercalder force-pushed the notices-api branch from 95ab176 to 8a529f9 Compare November 15, 2023 16:08

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Nov 15, 2023

olivercalder marked this pull request as ready for review November 15, 2023 16:09

olivercalder requested a review from Meulengracht November 15, 2023 16:10

olivercalder force-pushed the notices-api branch from 8a529f9 to 6382ece Compare November 15, 2023 16:25

miguelpires approved these changes Nov 16, 2023

View reviewed changes

Meulengracht reviewed Nov 29, 2023

View reviewed changes

olivercalder mentioned this pull request Nov 29, 2023

fix(daemon): small refactor of notices api canonical/pebble#337

Merged

olivercalder requested a review from Meulengracht November 30, 2023 03:19

Meulengracht approved these changes Nov 30, 2023

View reviewed changes

olivercalder requested a review from pedronis November 30, 2023 15:40

pedronis reviewed Nov 30, 2023

View reviewed changes

pedronis self-requested a review December 4, 2023 16:38

pedronis approved these changes Dec 4, 2023

View reviewed changes

olivercalder added the Skip spread Indicate that spread job should not run label Dec 6, 2023

benhoyt and others added 6 commits December 6, 2023 11:13

daemon: add notices API

b6bb10f

This is a port from Pebble into snapd of the notices API. The original PR can be found at: canonical/pebble#296 Signed-off-by: Oliver Calder <[email protected]>

daemon: adjusted notices API to be compatible with snapd

e6931e0

As notices are originally from pebble, various adjustments must be made in order for the ported API to work with snapd. Signed-off-by: Oliver Calder <[email protected]>

daemon: remove postNotices from the notices API

8830b51

Unlike pebble, snapd does not have a client for notices, and notices should not be created from outside of snapd. As a result, there is no need for a POST handler on the notices API. Signed-off-by: Oliver Calder <[email protected]>

daemon: remove custom notice type from notices API

0d16ce6

In snapd, there are no user-defined notices, so there is no need for a custom notice type. Signed-off-by: Oliver Calder <[email protected]>

olivercalder added 4 commits December 6, 2023 11:13

daemon: make notices API tests less brittle on slow machines

0d6084c

Signed-off-by: Oliver Calder <[email protected]>

NEWS.md: added notices state and api to NEWS.md

00e825b

Signed-off-by: Oliver Calder <[email protected]>

olivercalder force-pushed the notices-api branch from 3e4ba1f to 00e825b Compare December 6, 2023 17:15

olivercalder closed this Dec 6, 2023

olivercalder reopened this Dec 6, 2023

olivercalder merged commit 593f383 into canonical:master Dec 6, 2023
64 of 72 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: add notices API #13364

daemon: add notices API #13364

olivercalder commented Nov 15, 2023

olivercalder commented Nov 15, 2023

miguelpires left a comment

miguelpires Nov 16, 2023

olivercalder Nov 16, 2023

olivercalder Nov 16, 2023

miguelpires Nov 16, 2023

olivercalder Nov 16, 2023

miguelpires Nov 16, 2023

Meulengracht left a comment

Meulengracht Nov 29, 2023

olivercalder Nov 29, 2023

Meulengracht Nov 30, 2023

codecov-commenter commented Nov 30, 2023

Meulengracht left a comment

pedronis left a comment

pedronis Nov 30, 2023

olivercalder Dec 1, 2023

pedronis Nov 30, 2023

olivercalder commented Dec 1, 2023

pedronis left a comment

olivercalder commented Dec 6, 2023 •

edited

Loading

	func MultiCommaSeparatedList(strs []string) []string {
	func FlattenCommaSeparatedLists(strs []string) []string {

daemon: add notices API #13364

daemon: add notices API #13364

Conversation

olivercalder commented Nov 15, 2023

olivercalder commented Nov 15, 2023

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

Meulengracht 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

codecov-commenter commented Nov 30, 2023

Codecov Report

Meulengracht 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

Choose a reason for hiding this comment

olivercalder commented Dec 1, 2023

pedronis left a comment

Choose a reason for hiding this comment

olivercalder commented Dec 6, 2023 • edited Loading

olivercalder commented Dec 6, 2023 •

edited

Loading