Implement a builder-style provisioning API #91

jeremycline · 2024-06-25T22:18:15Z

This is based on top of PR #86 so I'd recommend just looking at the last commit in the series. It is a first draft of a builder-style interface to allow library users to select the backend for tools when provisioning.

It's not ready to be merged since there's no tests, documentation, or
implementation of default behavior when no backend is explicitly
selected. Additionally, I've not gated backends behind feature flags,
either. I'd love feedback on the general approach and what to do by
default.

My personal opinion is that the default option should iterate through
all available backends until one succeeds.

dongsupark · 2024-06-26T13:29:33Z

Code looks good in general. 👍

My personal opinion is that the default option should iterate through
all available backends until one succeeds.

Looks good to me.

anhvoms · 2024-06-26T15:56:54Z

+1 on having the default iterate through all available backend and letting user pass in their own backend

jeremycline · 2024-06-28T21:10:07Z

Okay, I've made a few changes and this should have all the things implemented (but not documented yet). At this point I probably could break some of these changes out into smaller PRs, like the ssh authorized_key tests/changes.

After a bunch of fiddling I decided I didn't love the "iterate through all the backends" approach and decided that the user needs to specify which ones they want to use. It makes the interface more verbose, and to be honest I don't really like what I've built, but my experience writing library APIs in Rust is not large.

However, what I've got is, I think, service-able and can do the things we want it to. I've added some feature flags to show how a user could restrict which backends get used.

dongsupark

I like the approach in general.
While I am in the middle of reading code, I left comments below:

dongsupark · 2024-07-02T10:06:38Z

libazureinit/src/provision/mod.rs

+    pub struct HostnameBackend;
+    #[derive(Default)]
+    pub struct PasswordBackend;
+    #[derive(Default)]


Maybe UseraddBackend is missing?

libazureinit/src/provision/mod.rs

libazureinit/src/provision/user.rs

libazureinit/src/provision/mod.rs

SeanDougherty · 2024-07-05T20:37:57Z

libazureinit/src/provision/user.rs

+                      "Provisioning agent created this user based on username provided in IMDS",
+                    )
+                    .arg("--groups")
+                    .arg("adm,audio,cdrom,dialout,dip,floppy,lxd,netdev,plugdev,sudo,video")


For Azure Linux, we will want to specify an alternative list of default groups for the user (example).

There's a few different ways to specify this, but perhaps an environment variable is the most logical? As that is how we are handling distro-specific knowledge in the other situations (i.e., binary paths).

To illustrate current behavior, here is a test trying to call path_useradd with this --groups list, and there are a few groups that aren't recognized on AzL. (See photo)

*Note: this test was built off of #100 so there may be some additional differences in the code, but the result of calling useradd should be the same as, at the time of this writing, both PRs use the same --groups list

Hm, yes. I added a commit to this PR that sets it via environment variable, but I'll mull on it more over the weekend.

I do worry about the discoverability of this approach. I think it's a little weird to have a library accept configuration this way instead of exposing an API and letting the application using the library handle it (either via build-time environment variables, or CLI options, or what have you).

Okay, I added a User type in commit 3b4b94b and threaded it through the API. It bundles all the user-related configuration together and allows you to optionally provide a list of supplementary groups. How does that look to you?

Reran the functional tests on AzL 3.0 using this branch

This has done the trick, thank you Jeremy

dongsupark · 2024-07-08T15:51:20Z

libazureinit/src/provision/mod.rs

+        hostname: impl Into<String>,
+        username: impl Into<String>,
+        ssh_keys: impl Into<Vec<PublicKeys>>,
+    ) -> Self {


The code looks okay as long as we have only 3 parameters for new().

Just a thought. I am wondering how much scalable it could be, for example when we would have 10 - 20 backends.
Then callers need to pass 10 - 20 parameters to new(), which could be tricky to follow and maintain.
However, as far as I understand, Rust does not have a native way of dealing variable args, right?
So we might need to split those into separate structs in the future.

Yeah, new() is just for required arguments and any change there would be an API breakage. I felt like these three would always be required, but we could also accept no arguments at all to new() and if you don't want to create a user for whatever reason, you don't have to. At that point, though, I don't know why you'd use the API.

When adding the group I added a dedicated type for user configuration so it reduces the number of parameters to two.

jeremycline · 2024-07-09T20:22:08Z

libazureinit/src/provision/user.rs

+
+    /// A list of supplemental group names to add the user to.
+    ///
+    /// If any of the groups do not exist on the host, provisioning will fail.


We could, instead, probe for groups that exist and only add the user to those groups. I don't love that because it feels spooky and unexpected ("I asked it to add the user to these groups and it didn't, but also didn't fail"), but on the other hand you wouldn't have to hand-tune the list for each distribution.

This provides a unified interface to provision the host with the option to select the tool used for setting the hostname, creating the user, and so on. By default, the library will try all the provisioning methods it knows of until one succeeds. Users of the library can optionally specify a subset to attempt when provisioning. This allows users to decide which tool or tools to use when provisioning. Some feature flags have been added to `azure-init` which enable provisioning with a tool, letting you build binaries for a particular platform relatively easily.

jeremycline · 2024-07-11T15:43:45Z

Okay, I cleaned up the Rust docs and ensured all the publicly exported types have documentation, I think this is now ready.

dongsupark

Thanks

peytonr18 · 2024-07-11T16:16:39Z

Thank you Jeremy!!!

Implement a single `Provision` interface This provides a unified interface to provision the host with the option to select the tool used for setting the hostname, creating the user, and so on. By default, the library will try all the provisioning methods it knows of until one succeeds. Users of the library can optionally specify a subset to attempt when provisioning. This allows users to decide which tool or tools to use when provisioning. Some feature flags have been added to `azure-init` which enable provisioning with a tool, letting you build binaries for a particular platform relatively easily.

jeremycline force-pushed the provision-builder-api branch from 0af28d4 to 0fb670c Compare June 26, 2024 14:13

jeremycline requested a review from anhvoms June 26, 2024 15:05

jeremycline force-pushed the provision-builder-api branch 2 times, most recently from 841044a to 8849b20 Compare June 28, 2024 18:41

dongsupark reviewed Jul 2, 2024

View reviewed changes

libazureinit/src/provision/user.rs Outdated Show resolved Hide resolved

libazureinit/src/provision/mod.rs Outdated Show resolved Hide resolved

jeremycline mentioned this pull request Jul 2, 2024

user: simplify ssh key provisioning and drop unsafe usage #95

Merged

jeremycline force-pushed the provision-builder-api branch 2 times, most recently from 35bb8a0 to 7f9f689 Compare July 3, 2024 13:58

SeanDougherty reviewed Jul 5, 2024

View reviewed changes

jeremycline force-pushed the provision-builder-api branch 2 times, most recently from bd29ce6 to 56ddd3f Compare July 5, 2024 21:09

dongsupark reviewed Jul 8, 2024

View reviewed changes

jeremycline force-pushed the provision-builder-api branch from 624ce3e to 3b4b94b Compare July 9, 2024 20:13

jeremycline commented Jul 9, 2024

View reviewed changes

SeanDougherty mentioned this pull request Jul 10, 2024

Get username either by IMDS or from a local OVF file #100

Merged

jeremycline force-pushed the provision-builder-api branch from 3b4b94b to 4199f8b Compare July 11, 2024 15:37

jeremycline changed the title ~~RFC: Implement a builder-style provisioning API~~ Implement a builder-style provisioning API Jul 11, 2024

jeremycline marked this pull request as ready for review July 11, 2024 15:39

jeremycline force-pushed the provision-builder-api branch from 4199f8b to 4aef549 Compare July 11, 2024 15:40

dongsupark approved these changes Jul 11, 2024

View reviewed changes

dongsupark merged commit bc83b93 into Azure:main Jul 11, 2024
5 checks passed

jeremycline mentioned this pull request Jul 11, 2024

Enable passwordless sudo for the user #101

Merged

jeremycline deleted the provision-builder-api branch July 11, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a builder-style provisioning API #91

Implement a builder-style provisioning API #91

jeremycline commented Jun 25, 2024

dongsupark commented Jun 26, 2024

anhvoms commented Jun 26, 2024

jeremycline commented Jun 28, 2024

dongsupark left a comment

dongsupark Jul 2, 2024

SeanDougherty Jul 5, 2024 •

edited

Loading

jeremycline Jul 5, 2024

jeremycline Jul 9, 2024

SeanDougherty Jul 10, 2024

dongsupark Jul 8, 2024

jeremycline Jul 9, 2024 •

edited

Loading

jeremycline Jul 9, 2024

jeremycline Jul 9, 2024

jeremycline commented Jul 11, 2024

dongsupark left a comment

peytonr18 commented Jul 11, 2024

Implement a builder-style provisioning API #91

Implement a builder-style provisioning API #91

Conversation

jeremycline commented Jun 25, 2024

dongsupark commented Jun 26, 2024

anhvoms commented Jun 26, 2024

jeremycline commented Jun 28, 2024

dongsupark left a comment

Choose a reason for hiding this comment

dongsupark Jul 2, 2024

Choose a reason for hiding this comment

SeanDougherty Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

jeremycline Jul 5, 2024

Choose a reason for hiding this comment

jeremycline Jul 9, 2024

Choose a reason for hiding this comment

SeanDougherty Jul 10, 2024

Choose a reason for hiding this comment

dongsupark Jul 8, 2024

Choose a reason for hiding this comment

jeremycline Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

jeremycline Jul 9, 2024

Choose a reason for hiding this comment

jeremycline Jul 9, 2024

Choose a reason for hiding this comment

jeremycline commented Jul 11, 2024

dongsupark left a comment

Choose a reason for hiding this comment

peytonr18 commented Jul 11, 2024

SeanDougherty Jul 5, 2024 •

edited

Loading

jeremycline Jul 9, 2024 •

edited

Loading