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

Create Azimuth user outside of image build #251

Merged
merged 26 commits into from
Nov 12, 2024
Merged

Conversation

assumptionsandg
Copy link
Contributor

@assumptionsandg assumptionsandg commented Sep 30, 2024

Copy link
Collaborator

@mkjpryor mkjpryor left a 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 change should modify the linux-data-volumes role. IMHO, the stuff for setting up HOME for the azimuth user should be in a separate role, with a separate ansible-init playbook (that executes after the data volumes one).

@assumptionsandg
Copy link
Contributor Author

I don't think this change should modify the linux-data-volumes role. IMHO, the stuff for setting up HOME for the azimuth user should be in a separate role, with a separate ansible-init playbook (that executes after the data volumes one).

The reason we did it like this was to ensure the user was created before the /data mountpoint permissions are setup and after the volume is mounted. Though maybe we can change the mount-point permissions in the user role instead.

@mkjpryor
Copy link
Collaborator

I would be equally happy with splitting this logic into two roles - one that runs before data volumes and is responsible for creating the user and one that runs after and is responsible for configuring the home. Chowning in a role that runs after data volumes also sounds reasonable though.

The responsibility of the data volumes role is to identify volumes, format and mount them at the correct place. It shouldn't have any knowledge of what those volumes are used for I think. I have a preference for many small roles that have one responsibility, so a new role that is responsible for setting up the azimuth user makes sense to me.

@mkjpryor
Copy link
Collaborator

Possibly the data-volumes role shouldn't even chown the directories itself - it should just identity, format and mount them for root and subsequent roles should decide what permissions they have. You could imagine that happening for other purposes than just the home directory.

@assumptionsandg
Copy link
Contributor Author

Possibly the data-volumes role shouldn't even chown the directories itself - it should just identity, format and mount them for root and subsequent roles should decide what permissions they have. You could imagine that happening for other purposes than just the home directory.

I've pushed some initial changes for this up at 0f006d2, though maybe this should be in another change.

Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, but I think this looks a lot better

ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
ansible/roles/linux-user/tasks/main.yml Outdated Show resolved Hide resolved
ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
Comment on lines 33 to 44
- name: Ensure the Azimuth user is created
ansible.builtin.user:
name: "azimuth"
uid: "{{ azimuth_uid }}"
shell: "/bin/bash"
become: true

- name: Ensure the Azimuth group has the correct GID
ansible.builtin.group:
name: "azimuth"
gid: "{{ azimuth_gid }}"
become: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we create/update the group first, then specify the GID in the ansible.builtin.user call, rather than relying on the group auto-creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a GID option in the user module. Though i suppose we could create the group first, and then specify the group name in the user call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets create the group first, then reference it in the user creation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@assumptionsandg

My mistake r.e. GID, but as you say you can do it using the group name. Better to be explicit I think.

ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
create: true
marker: ""
block: |
Note that this user storage (/home/azimuth) is EPHEMERAL and is removed on platform deletion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line doesn't make much sense without the context from the rest of the description in the UI metadata.

Maybe just say "The azimuth user's home directory is ephemeral ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean the data volume, I don't know if we're implying the home directory is there in the UI metadata description.

ansible/roles/linux-user/files/user-create-playbook.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

Just one small change remaining I think

Comment on lines 33 to 44
- name: Ensure the Azimuth user is created
ansible.builtin.user:
name: "azimuth"
uid: "{{ azimuth_uid }}"
shell: "/bin/bash"
become: true

- name: Ensure the Azimuth group has the correct GID
ansible.builtin.group:
name: "azimuth"
gid: "{{ azimuth_gid }}"
become: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

@assumptionsandg

My mistake r.e. GID, but as you say you can do it using the group name. Better to be explicit I think.

@JohnGarbutt JohnGarbutt requested a review from mkjpryor November 12, 2024 11:30
Copy link
Collaborator

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

LGTM

@mkjpryor mkjpryor merged commit 8987479 into main Nov 12, 2024
13 checks passed
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.

3 participants