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.

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

feat(debian): Add example broker binary #224

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Feb 23, 2024

This adds a new binary package with the example broker, but with these limitations:

  • The service is not enabled nor started ever during installing phase
  • The broker is masked on systemd by default so even installing the package, one need to use systemclt unmask authd-example-broker to get it working
  • The broker systemd service is not installed by default, so to be enabled you need to manually run /usr/share/doc/authd-example-broker/examples/authd-example-broker.installer.sh install

UDENG-2382

@3v1n0 3v1n0 requested a review from a team as a code owner February 23, 2024 17:07
@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.19%. Comparing base (ba8bb9f) to head (10b16e8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
- Coverage   86.23%   86.19%   -0.04%     
==========================================
  Files          65       65              
  Lines        5106     5106              
  Branches        9        9              
==========================================
- Hits         4403     4401       -2     
- Misses        489      490       +1     
- Partials      214      215       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Before we go on on that one:

  • Code sanity is failing
  • From our previous discussion in the previous PR, we were ok to land this new binary BUT not to have a systemd unit, even masks.

The goal was to have the example runner, when being started *manually, to drop its config, and clean it up on exit.

That way, we really signifies this is a manual/automated test tool, not to be used by anything else. Then, the test setup can drop a temporary systemd unit if we need to test reboot.

@jibel
Copy link
Collaborator

jibel commented Feb 26, 2024

AFAIK, this is not planned work. Where is the task in Jira for this work?
Besides what is the rationale behind this change?

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Feb 26, 2024

Before we go on on that one:

* Code sanity is failing

It was just a temporary issue, it's green now.

* From our previous discussion in the previous PR, we were ok to land this new binary BUT not to have a systemd unit, even masks.

Well, I didn't get a no on my proposal on relying on systemd to handle that.

The goal was to have the example runner, when being started *manually, to drop its config, and clean it up on exit.

However things can be dropped, but IMHO not doing that implies that we can't test the ability of authd to dbus-activate the example daemon when the user selects it, that I feel is something would be nice to be able to trigger.

And having to do systemctl unmaks before of any of that (even after having installed the package) looks to me a quite strong protection.

But as you said, that can indeed be done at test phase too. So I guess I'll just install the systemd service in debian examples for an easy copy&paste, is that fine?

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Feb 26, 2024

AFAIK, this is not planned work. Where is the task in Jira for this work? Besides what is the rationale behind this change?

No, it wasn't but it was the natural outcome of having a PPA, since I had to do 90% of this work to get a testing PPA working with a testable broker, thus I decided to split that work into another PR that is something still need to have proper integration tests in a autopkgtests.

@didrocks
Copy link
Member

didrocks commented Feb 26, 2024

But as you said, that can indeed be done at test phase too. So I guess I'll just install the systemd service in debian examples for an easy copy&paste, is that fine?

yes, and same for the configuration. I still think the binary itself can install/remove the configuration if it’s not there are startup time for easy testing.

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Feb 26, 2024

I'm marking this as draft and rebase on #223 so that review should be easier once that one lands...

@3v1n0 3v1n0 marked this pull request as draft February 26, 2024 16:50
@3v1n0 3v1n0 force-pushed the example-broker-binary branch 2 times, most recently from 03bfcad to 713c5fa Compare February 26, 2024 21:15
@3v1n0 3v1n0 marked this pull request as ready for review March 5, 2024 11:17
@3v1n0 3v1n0 requested a review from didrocks March 5, 2024 11:20
Comment on lines +1 to +6
#!/usr/bin/dh-exec

usr/bin/examplebroker-bin => ${env:AUTHD_DAEMONS_PATH}/authd-examplebroker

examplebroker/com.ubuntu.authd.ExampleBroker.conf /usr/share/dbus-1/system.d
examplebroker/com.ubuntu.authd.ExampleBroker.service /usr/share/dbus-1/system-services
Copy link
Member

Choose a reason for hiding this comment

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

I think this is still not what we want. IMHO, the examplebroker should be an executable that, when executed, exports the broker on dbus and cleans it up when the program finishes running. This way, we don't need to worry about having to install/uninstall it. Its purpose is to test authd, not the broker behaviour, so we don't need anything directly configured on dbus (as authd does not directly care about it).

Copy link
Collaborator Author

@3v1n0 3v1n0 Mar 8, 2024

Choose a reason for hiding this comment

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

I see the point, but that wouldn't allow proper testing, especially if we want to do proper autopkgtests were you just install everything and then you run a client that would work as the system would be configured properly.

so we don't need anything directly configured on dbus (as authd does not directly care about it).

In fact authd does care about it, because if that would happen when we run the daemon, then we wouldn't be able of testing the case that authd does dbus-activation of the service when that you select the broker on the brokers list.

Also, not having a script like this would imply that we rely on the autopkgtest script to do that, but IMHO it's not nice to maintain the same in two different places.

What we may want to do, in case, is to make the examplebrorker to do the conf setup in case that's not set, and I can do that, but I feel we should still provide a way to create what would be the real setup in a properly configured machine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, also having those files installed doesn't allow anything but just having the service to own the name, and while these may be handled by the service itself, as I said, I'd prefer to be able to do dbus-activation (but only through systemd that it's not installed by default).

3v1n0 added 12 commits March 27, 2024 08:13
In order to do integration tests, autopkgtests or manual tests we may
need to have a simple broker installed.

So expose the example broker as a separate binary pacakge so that we can
easily install it when required.
Using default names would make those files to use the first package, but
let's be cleaner.
These are now installed as examples and can be manually added to the
proper locations for testing purposes
This wasn't supported by older dh-exec but it is now, so let's use it to
be more consistent with other files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants