release: use locking around lazy intialized state #6306

bboozzoo · 2018-12-13T12:28:18Z

Some state data in the release package, namely AppArmor level/summary &
kernel/parser features as well as Seccomp feature set, is only initialized when
needed. Since there is a chance, this information could be accessed in
concurrently, we need to add some locking around the code that mutates the data.

Some state data in the release package, namely AppArmor level/summary & kernel/parser features as well as Seccomp feature set, is only initialized when needed. Since there is a chance, this information could be accessed in concurrently, we need to add some locking around the code that mutates the data. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

release/apparmor.go

zyga

LGTM but let's please just use one mutex for both AppArmor variables.

pedronis · 2018-12-13T12:32:42Z

using sync.Once would be more idiomatic

Refactor the AppArmor and Seccomp handling code to use sync.Once, in hope for making the code more idiomatic. Drop or update the necessary tests. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2018-12-14T09:16:36Z

@pedronis pushed the change to use sync.Once.

pedronis

some questions/comments

pedronis · 2018-12-14T18:51:16Z

release/apparmor.go

 )

+func assessAppArmorOnce() {
+	appArmorAssessOnce.Do(assessAppArmor)


were the actual functions like assesAppArmor etc tested?

Yes, they are tested in a couple of places:

https://github.com/snapcore/snapd/pull/6306/files#diff-ecc06490ecae634064182423ba57f533R197

https://github.com/snapcore/snapd/pull/6306/files#diff-ecc06490ecae634064182423ba57f533R163

https://github.com/snapcore/snapd/pull/6306/files#diff-ecc06490ecae634064182423ba57f533R138

release/apparmor_test.go

I need to re-read the new code

codecov-io · 2018-12-15T00:06:31Z

Codecov Report

Merging #6306 into master will increase coverage by 0.02%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master    #6306      +/-   ##
==========================================
+ Coverage   78.95%   78.98%   +0.02%     
==========================================
  Files         561      561              
  Lines       43650    43664      +14     
==========================================
+ Hits        34463    34486      +23     
+ Misses       6387     6378       -9     
  Partials     2800     2800

Impacted Files	Coverage Δ
release/seccomp.go	`92.59% <90%> (+32.59%)`	⬆️
release/apparmor.go	`95.71% <96.77%> (+0.35%)`	⬆️
overlord/hookstate/hookmgr.go	`73.55% <0%> (-0.97%)`	⬇️
image/image.go	`72.98% <0%> (-0.08%)`	⬇️
wrappers/core18.go	`49.47% <0%> (ø)`	⬆️
overlord/ifacestate/handlers.go	`62.7% <0%> (+0.51%)`	⬆️
cmd/snap/cmd_run.go	`62.52% <0%> (+1.71%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3c9121...d91930d. Read the comment docs.

…n mocking Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…se-accesses

stolowski · 2018-12-19T10:22:59Z

release/apparmor.go

+//	appArmorProbeParserOnce sync.Once
+//	appArmorProbeKernelOnce sync.Once
+//	appArmorAssessOnce      sync.Once
+type appArmorOnceBattery struct {


Not super fond of the name.

stolowski · 2018-12-19T10:44:17Z

release/apparmor.go

-	if appArmorLevel == UnknownAppArmor {
-		assessAppArmor()
-	}
+	appArmorAssess()


Do we have a test that checks if the assessment is cached and actually done once? Since this extra if goes away, it would be good to have a test, in case the implementation based on sync.Once gets replaced in the future with new Go.

I looked into this, in the end I reorganized things quite a bit, mostly because the Mock* functions below are complicated/long but didn't help fully with testing this. I'm pushing the new code now, with it commenting the Once.Do bits always make some test fail.

…g them swapping full structures, add various tests increasing coverage and proving that the once-only logic is in place

stolowski · 2018-12-20T09:49:48Z

release/apparmor.go

-		appArmorKernelFeatures, appArmorKernelError = probeAppArmorKernelFeatures()
-	}
-	return appArmorKernelFeatures, appArmorKernelError
+	return appArmorAssessment.KernelFeatures()


Mostly curiuous - is there a reason we use slightly different pattern here and above? I mean explicit call to assess(), rather than having Level() and Summary() methods that to sync.Once internally?

These ones are used both internally and externally and also participate in the interface. Summary and Level are just exposed externally so having another layer of methods didn't seem interesting just for the symmetry.

zyga · 2018-12-20T11:04:10Z

release/apparmor.go

-		appArmorLevel = NoAppArmor
-		appArmorSummary = "apparmor not enabled"
-		return
+		return NoAppArmor, "apparmor not enabled"


This is nice, after writing this I was thinking about changing the code around to avoid the risk of forgetting to return. This makes the problem moot.

stolowski · 2018-12-20T12:47:19Z

release/apparmor.go

-		appArmorLevel = NoAppArmor
-		appArmorSummary = "apparmor not enabled"
-		return
+		return NoAppArmor, "apparmor not enabled"
 	}


What about other errors? I think with the change to the func AppArmorKernelFeatures() ([]string, error) method we are no longer returning actual appArmorKernelError from AppArmorKernelFeatures, no?

appArmorKernelError doesn't exist anymore as a global, it's stored in the kernelError field of the impl of appArmorProber, and returned in their KernelFeatures methods and then out of AppArmorKernelFeatures

stolowski

LGTM. But I don't think "Simple" label is true anymore ;)

chipaca

Nice. Thank you!

bboozzoo

@pedronis thanks for the cleanup!

bboozzoo added the Simple 😃 A small PR which can be reviewed quickly label Dec 13, 2018

bboozzoo requested a review from zyga December 13, 2018 12:28

zyga reviewed Dec 13, 2018

View reviewed changes

release/apparmor.go Outdated Show resolved Hide resolved

zyga previously approved these changes Dec 13, 2018

View reviewed changes

release: refactor to use sync.Once for lazy initialization

1e57482

Refactor the AppArmor and Seccomp handling code to use sync.Once, in hope for making the code more idiomatic. Drop or update the necessary tests. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis reviewed Dec 14, 2018

View reviewed changes

bboozzoo and others added 5 commits December 17, 2018 07:47

release: tweak AppArmor tests: drop unused helpers, always assess whe…

5e5f1ab

…n mocking Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/sync-relea…

f4f650d

…se-accesses

Merge remote-tracking branch 'origin/master' into bboozzoo/sync-relea…

9546590

…se-accesses

avoid a test order requirement, also increase back coverage slightly

f6f927d

make go1.6 vet happy

f487fc4

stolowski reviewed Dec 19, 2018

View reviewed changes

pedronis added 4 commits December 19, 2018 20:42

add probing once tests

2819bdd

reorg code mostly to simplify/clarify the Mock* method logic by havin…

f0b8f75

…g them swapping full structures, add various tests increasing coverage and proving that the once-only logic is in place

some consistency renaming

c689713

another renaming for consistency

d91930d

stolowski reviewed Dec 20, 2018

View reviewed changes

zyga reviewed Dec 20, 2018

View reviewed changes

stolowski reviewed Dec 20, 2018

View reviewed changes

stolowski approved these changes Dec 20, 2018

View reviewed changes

pedronis removed the Simple 😃 A small PR which can be reviewed quickly label Dec 20, 2018

chipaca approved these changes Dec 20, 2018

View reviewed changes

bboozzoo commented Dec 20, 2018

View reviewed changes

pedronis merged commit 318896a into canonical:master Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release: use locking around lazy intialized state #6306

release: use locking around lazy intialized state #6306

bboozzoo commented Dec 13, 2018

zyga left a comment

pedronis commented Dec 13, 2018

bboozzoo commented Dec 14, 2018

pedronis left a comment

pedronis Dec 14, 2018

bboozzoo Dec 17, 2018

codecov-io commented Dec 15, 2018 •

edited

Loading

stolowski Dec 19, 2018

stolowski Dec 19, 2018

pedronis Dec 19, 2018

stolowski Dec 20, 2018

pedronis Dec 20, 2018

zyga Dec 20, 2018

stolowski Dec 20, 2018

pedronis Dec 20, 2018

stolowski left a comment

chipaca left a comment

bboozzoo left a comment

release: use locking around lazy intialized state #6306

release: use locking around lazy intialized state #6306

Conversation

bboozzoo commented Dec 13, 2018

zyga left a comment

Choose a reason for hiding this comment

pedronis commented Dec 13, 2018

bboozzoo commented Dec 14, 2018

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

codecov-io commented Dec 15, 2018 • edited Loading

Codecov Report

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

stolowski left a comment

Choose a reason for hiding this comment

chipaca left a comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 15, 2018 •

edited

Loading