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.

Sign up for GitHub

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

Jump to bottom

o/ifacestate,tests: apparmor prompting not running if manager creation failed #14421

Merged
merged 2 commits into from
Aug 24, 2024
Merged

o/ifacestate,tests: apparmor prompting not running if manager creation failed #14421

merged 2 commits into from
Aug 24, 2024

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Aug 23, 2024 •
edited
Loading

If creation of the interfaces requests manager fails for some reason, we need to mark the manager's internal useAppArmorPrompting value to false so that calls to AppArmorPromptingRunning() will return false, and we don't try to access a nil-pointer manager backend.

Extend the apparmor-prompting-snapd-startup test to cover this scenario.

Further, we address one reason for this failure: lack of /run/snapd/ during the interface manager startup. We should ensure that this directory exists as part of the requestprompts backend attempting to open the max prompt ID mmap.

This is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-31381

@olivercalder olivercalder added this to the 2.65 milestone Aug 23, 2024
@olivercalder olivercalder requested a review from pedronis August 23, 2024 15:18
@olivercalder olivercalder force-pushed the prompting-fix-nil-backends-on-error branch from 566d4c9 to d087a72 Compare August 23, 2024 15:36
@olivercalder olivercalder marked this pull request as ready for review August 23, 2024 15:36
pedronis
pedronis reviewed Aug 23, 2024
View reviewed changes
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

@olivercalder
Copy link
Member Author

I would suggest rebase merging rather than squashing, as these are two separate bugs which are being fixed, the latter triggering the former.

@olivercalder
Copy link
Member Author

Looks like apparmor-prompting-snapd-startup passed on noble with the changes 🎉 so if there's an error during manager creation, prompting is successfully marked as disabled and API calls return that information without an error

olivercalder
olivercalder commented Aug 23, 2024
View reviewed changes
Copy link
Member Author

@olivercalder olivercalder left a comment

Choose a reason for hiding this comment

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

I think this can be addressed in #14409 rather than included in this PR? But if it makes validation fail we'll need to push a fix, more likely include #14409 in 2.65. But I don't think this should be necessary.

tests/main/apparmor-prompting-snapd-startup/task.yaml Outdated
echo "Check that we received one notices for the non-expired rule"
snap debug api --fail "/v2/notices?after=$CURRTIME&types=interfaces-requests-rule-update&user-id=1000" | jq
snap debug api "/v2/notices?after=$CURRTIME&types=interfaces-requests-rule-update&user-id=1000" | jq '.result | length' | MATCH 1
snap debug api "/v2/notices?after=$CURRTIME&types=interfaces-requests-rule-update&user-id=1000" | jq '.result.[0].key' | MATCH "0000000000000002"
Copy link
Member Author

Choose a reason for hiding this comment

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

Per #14409, this needs to be

Suggested change
snap debug api "/v2/notices?after=$CURRTIME&types=interfaces-requests-rule-update&user-id=1000" | jq '.result.[0].key' | MATCH "0000000000000002"
snap debug api "/v2/notices?after=$CURRTIME&types=interfaces-requests-rule-update&user-id=1000" | jq '.result[0].key' | MATCH "0000000000000002"

tests/main/apparmor-prompting-snapd-startup/task.yaml Outdated

echo "Check that the non-expired rule is still valid (must be done with UID 1000)"
sudo -iu '#1000' snap debug api /v2/interfaces/requests/rules | jq '.result | length' | MATCH 1
sudo -iu '#1000' snap debug api /v2/interfaces/requests/rules | jq '.result.[0].id' | MATCH "0000000000000002"
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here:

Suggested change
sudo -iu '#1000' snap debug api /v2/interfaces/requests/rules | jq '.result.[0].id' | MATCH "0000000000000002"
sudo -iu '#1000' snap debug api /v2/interfaces/requests/rules | jq '.result[0].id' | MATCH "0000000000000002"

@olivercalder olivercalder mentioned this pull request Aug 23, 2024
Closed
@olivercalder
Copy link
Member Author

olivercalder commented Aug 23, 2024 •
edited
Loading

Spread tests were restarted for all tests, rather than just the failures, so I restarted manually from the results of run 1 of https://github.com/canonical/snapd/actions/runs/10528423541

This seems to have cancelled the checks below but marked them all as succeeded... This is incorrect, but the actual tests are still running in Run 2 of the original GH Action run:

The current run (run 2) of the originally-failed tests can be found here: https://github.com/canonical/snapd/actions/runs/10528423541

@olivercalder
Copy link
Member Author

olivercalder commented Aug 23, 2024 •
edited
Loading

Prompting client integration tests built on top of this change passed in locally-run spread for me, and should appear here under ubuntu-noble tests: #14387

@olivercalder
Copy link
Member Author

olivercalder commented Aug 23, 2024 •
edited
Loading

Test results are here: https://github.com/canonical/snapd/actions/runs/10528423541/job/29179028207

Failures:

  • google-distro-1:debian-sid-64:tests/main/nfs-support (non-required, unrelated)
  • openstack:fedora-40-64: cannot allocate new Openstack server aug231745-874590: server status is ERROR
  • google:ubuntu-24.10-64:tests/main/snap-quota-cpu (flakey, unrelated)
  • google-core:ubuntu-core-24-64:tests/main/degraded is still running but will likely fail, as on master, but it's unrelated

So I think this is good to merge whenever you're ready @ernestl

alfonsosanchezbeato
alfonsosanchezbeato approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nit about the test

tests/main/apparmor-prompting-snapd-startup/task.yaml Outdated
systemctl stop snapd.service snapd.socket
# Try for a while to make sure it's not in failure mode
echo "Check that systemctl is-failed is never true after a while"
retry --wait 1 -n 30 systemctl is-failed snapd.service snapd.socket && exit 1
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: this relies in very hidden behavior of set -e, I think that not retry ... would work too

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll try it with this instead

tests/main/apparmor-prompting-snapd-startup/task.yaml Outdated
systemctl stop snapd.service snapd.socket
# Try for a while to make sure it's not in failure mode
echo "Check that systemctl is-failed is never true after a while"
retry --wait 1 -n 30 systemctl is-failed snapd.service snapd.socket && exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@olivercalder
o/ifacestate,tests: apparmor prompting not running if manager creatio…
e38202a 
…n failed

If creation of the interfaces requests manager fails for some reason, we
need to mark the manager's internal `useAppArmorPrompting` value to
false so that calls to `AppArmorPromptingRunning()` will return false,
and we don't try to access a nil-pointer manager backend.

Extend the apparmor-prompting-snapd-startup test to cover this scenario.

Additionally, fix a few syntax inconsistencies in the spread test while
we're at it.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder
i/p/requestprompts: ensure /run/snapd exists before opening max promp…
46fb37f 
…t id file

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder olivercalder force-pushed the prompting-fix-nil-backends-on-error branch from d087a72 to 46fb37f Compare August 23, 2024 19:51
@olivercalder
Copy link
Member Author

Tests are being very weird (all passing immediately), I'm going to close and reopen.

@olivercalder olivercalder reopened this Aug 23, 2024
@ernestl ernestl merged commit 83ca902 into canonical:master Aug 24, 2024
150 of 159 checks passed
@ernestl ernestl modified the milestones: 2.65, 2.63.1, 2.65.1 Aug 24, 2024
@ernestl ernestl mentioned this pull request Aug 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alfonsosanchezbeato alfonsosanchezbeato alfonsosanchezbeato approved these changes

@pedronis pedronis pedronis approved these changes

@bboozzoo bboozzoo Awaiting requested review from bboozzoo

@ernestl ernestl Awaiting requested review from ernestl

Assignees
No one assigned
Labels
cherry-picked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olivercalder @pedronis @alfonsosanchezbeato @ernestl