interfaces/builtin: implement broadcom-asic-control interface

[morphis]

This implements a new interface necessary to access certain device nodes exposed by the kernel drivers from Broadcom for their ASIC on switch devices.

This requires specific kernel modules to be available and loaded which will be soon shipped with the official Ubuntu kernel.

[morphis]

Support for kernel modules was now added.

[jhodapp]

Just a couple of things I'm curious about.

[jhodapp]

What's the difference between this and line 145 returning true for the AutoConnect method?

[morphis]

See the comment in https://github.com/snapcore/snapd/pull/3623/files#diff-7657fbc6427572b16bb91d9568774554R144

AutoConnect is more or less a relict from older times. Base declaration has priority.

[jhodapp]

Ah yes, I forgot to comment on that comment. :) It didn't make sense to me. Can you reword that comment on line 144?

[morphis]

What doesn't make sense? It's the same comment used on all other interfaces for the AutoConnect method.

[jhodapp]

So the comment states this:

// Allow what is allowed in the declarations

Maybe if you point to where the reader of the code could know what exactly is allowed, then it'd make more sense to me. It's just too general of a comment to add much value in my opinion.

[jhodapp]

This hasn't been changed to being called "core" instead of "os"?

[morphis]

The type of a core snap is still "os". See https://github.com/snapcore/snapd/blob/master/snap/types.go#L31 for details

[jhodapp]

Sounds like a bug in snapd that should be fixed before there's hundreds or thousands of interfaces.

[zyga]

You will be happy to learn I fixed this a moment ago :)

[jhodapp]

Nice!!

[zyga]

I pushed some API changes and cleanups.

[zyga]

I pushed API cleanups and did a small review of the code. I think you need an ack from @jdstrand to land this and a review from @niemeyer on the ImplicitOnClassic flag.

[zyga]

I wonder if this should be implicit on classic where it will show up for nearly everyone. It feels like a thing that we could do via classic gadgets if it is really needed there. WDYT?

[jdstrand]

I think that this interface is very similar to dcdbas-control, which is implicit classic, and therefore we shouldn't make this PR do something else. If we want to clean this up and make conditional on gadgets, etc, that could be a future improvement.

[zyga]

Can you please just declare more snaps like you did above? I'd much rather see snaptest.MockInfo than hand-made structures.

[morphis]

Done

[zyga]

Can you please split this per backend. It will be easier to follow and you can use some shorter variable names (e.g. spec instead of apparmorSpec).

[morphis]

Done.

[jhodapp]

@zyga @jdstrand Can we have someone else review the ImplicitOnClassic flag usage since Gustavo is out for the next week? We need to keep this review rolling and merged so it gets into the next possible snapd release.

Thanks for staying on top of this review @zyga

[jdstrand]

The base declaration, apparmor policy, udev rules and kernel modules all look ok, but please make the requested policy changes.

[jdstrand]

Since these are all only reads, I think the above can all be collapsed into (which is easier to read and futureproof):

/sys/module/linux_bcm_knet/{,**} r,
/sys/module/linux_kernel_bde/{,**} r,
/sys/module/linux_user_bde/{,**} r,

[morphis]

Done

[jdstrand]

For consistency, please use 'rw' here.

[morphis]

Done

[zyga]

I wonder if this should be implicit on classic where it will show up for nearly everyone. It feels like a thing that we could do via classic gadgets if it is really needed there. WDYT?

[jdstrand]

I think that this interface is very similar to dcdbas-control, which is implicit classic, and therefore we shouldn't make this PR do something else. If we want to clean this up and make conditional on gadgets, etc, that could be a future improvement.

[jhodapp]

@jdstrand Your last comment about implicit classic I don't quite follow. Are you simply stating that it doesn't need any special review/permission in order to use implicit classic before it gets merged, or something else?

[jhodapp]

I'm happy now, looks very good!

[jdstrand]

@jhodapp - I was saying that I don't think the implicit parts of this interface need to change.

[jdstrand]

The security policy changes look good, thanks.

[codecov-io]

Codecov Report

Merging #3623 into master will increase coverage by 0.02%.
The diff coverage is 91.17%.

@@            Coverage Diff             @@
##           master    #3623      +/-   ##
==========================================
+ Coverage   75.19%   75.21%   +0.02%     
==========================================
  Files         386      387       +1     
  Lines       33418    33452      +34     
==========================================
+ Hits        25127    25162      +35     
+ Misses       6480     6478       -2     
- Partials     1811     1812       +1

Impacted Files	Coverage Δ
interfaces/builtin/broadcom_asic_control.go	91.17% <91.17%> (ø)
overlord/ifacestate/helpers.go	63% <0%> (+0.66%)	⬆️
interfaces/sorting.go	100% <0%> (+1.28%)	⬆️
cmd/snap/cmd_aliases.go	95% <0%> (+1.66%)	⬆️

---

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 1bb7d6d...1005db0. Read the comment docs.

[jhodapp]

@zyga can you approve this and merge please?

[zyga]

LGTM