-
Notifications
You must be signed in to change notification settings - Fork 28
docs: add reference snippets for deployment #1093
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really cool that you got this to work, and it's great for testing the command snippets on a local machine.
However, I'm not sure if the spread + multipass approach is a suitable solution for CI, which is arguably the more important use case, to continuously test that the docs are up to date. In the CI, we don't need to fetch and launch a VM, because we're already running on a disposable system of our choice. Downloading a VM image each time in the CI is slow and resource-intensive. So I would prefer a solution which allows us to just execute the command blocks on the current system. Then for running the test on a local machine, we can spin up a VM or container and execute the test in there (which could just be done in a shell script, I don't think we need something like spread for that).
I did a quick search for a tool which extracts code blocks from markdown to execute them, and there are a few, but nothing that seems to be widely used or an obvious ideal candidate, so we might need to do some more research and try things out.
Anyway, I don't consider having tests a blocker to merging the cloud-init and landscape documentation, especially since they do indeed closely correspond to the documentation we already have.
Thanks @adombeck !
I agree, they do closely align and I don't think that the maintenance burden would be too high for that reason.
Multipass would not be used for the CI, but only for local tests. So, in this current implementation, the question is whether this is useful for local testing. Here's the spread test Charmcraft uses: https://github.com/canonical/charmcraft/blob/main/spread.yaml and the corresponding workflow: https://github.com/canonical/charmcraft/blob/main/.github/workflows/spread-docs.yaml I believe they use Multipass for local tests and a Google backend for CI, but I'm not sure. I/we would need to ask one of their engineers how they handle the CI. I think that spread testing would have some merit, even if only used locally for people handling docs, but especially if we had more test cases across the documentation in future (e.g., multiple brokers with specific configurations). For now, I'm happy to leave this as an experiment for further research, and push the page with a cloud-init version added. |
It looks like they configure the local system to support SSH login as root and then tell spread to connect to the localhost via SSH: https://github.com/canonical/charmcraft/blob/860b88ecdbc549954f714865058818b5c4ac344f/spread/.extension#L102-L116 That could also work for us.
I agree.
With the approach from Charmcraft for using spread with localhost in the CI, I'm now optimistic that we can do something similar. I still wouldn't block on that, so in my opinion you can go ahead and create a PR for adding the page without tests. |
Thanks @adombeck -- I'll finalise next pulse 👍 |
* Removes spread testing * Adds tabs for brokers * Adds links * Minor rewriting and restructuring
cca3f8a
to
26f3711
Compare
* Follows Landscape deploy ref closely
26f3711
to
bc7d0ee
Compare
@adombeck --- I'm marking this as ready to review. I removed the automated testing and refactored into a standard doc, with tabs for the brokers like elsewhere. I also added an equivalent reference page for cloud init. |
@edibotopic could you please also adapt the PR description (which at least by default is used as the merge commit message)? |
Yes! I forgot that. Also, I'm not sure what's going on with the CLA (edit: passed now). |
The error is "read ETIMEDOUT" which seems like a network issue, so nothing we need to care about. |
|
||
```shell | ||
add-apt-repository -y ppa:ubuntu-enterprise-desktop/authd | ||
apt update && apt upgrade -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove.
|
||
```shell | ||
add-apt-repository -y ppa:ubuntu-enterprise-desktop/authd | ||
apt update && apt upgrade -y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
```shell | ||
add-apt-repository -y ppa:ubuntu-enterprise-desktop/authd | ||
apt update && apt upgrade -y | ||
apt-get install -y authd gnome-shell yaru-theme-gnome-shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we only install gnome-shell and yaru-theme-gnome-shell in the Entra ID tab and not in the Google tab?
If we want to install it or not depends on whether the system is Ubuntu Desktop or Ubuntu Server. Maybe you intended to name the tabs "Ubuntu Desktop" and "Ubuntu Server", same as in https://documentation.ubuntu.com/authd/stable-docs/howto/install-authd/#install-authd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be both. Will fix.
add-apt-repository -y ppa:ubuntu-enterprise-desktop/authd | ||
apt update && apt upgrade -y | ||
apt-get install -y authd | ||
snap install authd-google |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most if not all commands on this page require root privileges, so we should probably prefix all of them with sudo
, similar to how we do it on the install and configure pages, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Landscape, when deploying the script, the admin can opt for the script to be run as the root user on the client machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, that changes things. Then indeed we don't need sudo and can use echo >> <file>
to write to files.
docs/reference/landscape-deploy.md
Outdated
|
||
```shell | ||
sed -i "s|<CLIENT_ID>|$CLIENT_ID|g; s|<ISSUER_ID>|$ISSUER_ID|g" /var/snap/authd-msentraid/current/broker.conf | ||
echo "ssh_allowed_suffixes = @test.onmicrosoft.com" >> /var/snap/authd-msentraid/current/broker.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the user pastes this snippet into a root shell, we will have to use tee
to write to the config file (sudo echo >> <file>
doesn't work because the file redirection is not affected by sudo):
echo "ssh_allowed_suffixes = @test.onmicrosoft.com" >> /var/snap/authd-msentraid/current/broker.conf | |
sudo tee -a /var/snap/authd-msentraid/current/broker.conf > /dev/null <<EOF | |
ssh_allowed_suffixes = @test.on-microsoft.com | |
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also use sed to insert the line below the existing #ssh_allowed_suffixes = [...]
line:
echo "ssh_allowed_suffixes = @test.onmicrosoft.com" >> /var/snap/authd-msentraid/current/broker.conf | |
sudo sed -i '/^#ssh_allowed_suffixes/ a ssh_allowed_suffixes = @test.onmicrosoft.com' /var/snap/authd-msentraid/current/broker.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When deploying the scripts using Landscape, I think it would be done remotely by an admin via a root shell on the client machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have a brief note about running as root at the end, but it might be beneficial to mention it at the start for context.
We just don't want to devote too much space to documenting the Landscape behemoth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note at the end is also fine, whichever you prefer.
Co-authored-by: adombeck <[email protected]>
Co-authored-by: adombeck <[email protected]>
Co-authored-by: adombeck <[email protected]>
Co-authored-by: adombeck <[email protected]>
Co-authored-by: adombeck <[email protected]>
Co-authored-by: adombeck <[email protected]>
{% set ISSUER_ID = '<your_issuer_id>' %} | ||
{% set CLIENT_ID = '<your_client_id>' %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is valid YAML. is it some special cloud-init syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's jinja I believe. That's why I didn't set the highlighter to YAML.
Cloud configs with cloud-init support Jinja templating, although it requires a pre-amble. I could add that or reference the docs.
Alternatively, and perhaps more simply, we could just insert the placeholders in the main YAML content.
- sed -i 's|<CLIENT_ID>|{{ CLIENT_ID }}|g; s|<ISSUER_ID>|{{ ISSUER_ID }}|g' /var/snap/authd-msentraid/current/broker.conf | ||
- echo 'ssh_allowed_suffixes = @test.onmicrosoft.com' >> /var/snap/authd-msentraid/current/broker.conf | ||
- sed -i 's/^\(LOGIN_TIMEOUT\t\t\)[0-9]\+/\1360/' /etc/login.defs | ||
- mkdir -p /etc/authd/brokers.d/ | ||
- cp /snap/authd-msentraid/current/conf/authd/msentraid.conf /etc/authd/brokers.d/ | ||
- snap restart authd-msentraid | ||
- systemctl restart authd | ||
- snap restart authd-msentraid | ||
- systemctl restart ssh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same changes as on the landscape page:
- sed -i 's|<CLIENT_ID>|{{ CLIENT_ID }}|g; s|<ISSUER_ID>|{{ ISSUER_ID }}|g' /var/snap/authd-msentraid/current/broker.conf | |
- echo 'ssh_allowed_suffixes = @test.onmicrosoft.com' >> /var/snap/authd-msentraid/current/broker.conf | |
- sed -i 's/^\(LOGIN_TIMEOUT\t\t\)[0-9]\+/\1360/' /etc/login.defs | |
- mkdir -p /etc/authd/brokers.d/ | |
- cp /snap/authd-msentraid/current/conf/authd/msentraid.conf /etc/authd/brokers.d/ | |
- snap restart authd-msentraid | |
- systemctl restart authd | |
- snap restart authd-msentraid | |
- systemctl restart ssh | |
- sed -i 's|<CLIENT_ID>|{{ CLIENT_ID }}|g; s|<ISSUER_ID>|{{ ISSUER_ID }}|g' /var/snap/authd-msentraid/current/broker.conf | |
- echo 'ssh_allowed_suffixes = @example.onmicrosoft.com' >> /var/snap/authd-msentraid/current/broker.conf | |
- sed -i 's/^\(LOGIN_TIMEOUT\t\t\)[0-9]\+/\1360/' /etc/login.defs | |
- mkdir -p /etc/authd/brokers.d/ | |
- cp /snap/authd-msentraid/current/conf/authd/msentraid.conf /etc/authd/brokers.d/ | |
- snap restart authd-msentraid | |
- systemctl restart authd ssh |
- gnome-shell # only needed for GDM login | ||
- yaru-theme-gnome-shell # only needed for GDM login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just drop those, if cloud-init (or we force) to do a apt dist-upgrade
that's enough to them get them all if they're runnig in a desktop, so we can omit them IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And even from other docs I think we should not mention them, just to dist-upgrade maybe.
UsePAM yes | ||
KbdInteractiveAuthentication yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here we can suggest that they can use
Match *@domain.ext
UsePAM yes
KbdInteractiveAuthentication yes
To ensure that the configuration applies only to the specific user
An important use-case for authd is deploying at scale when, for example, an organisation has multiple user machines that need to authenticate with a cloud provider.
This PR adds reference paged with code snippets for remote script execution using Landscape and provisioning with cloud-init.
UDENG-7785