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: enhance munge key management API #34

Merged
merged 13 commits into from
Jul 3, 2024

Conversation

NucciTheBoss
Copy link
Member

This pull request is beefy (:cow:) as it adds the mungectl command to the Slurm snap to simplify munge key management for Slurm deploys. mungectl enables us to easily manage keys between nodes by allowing us to exchange the keys back and forth as base64 encoded strings. Here's what a flow would look like:

# [controller]: Generate a new key, save as a secret, and restart munged.
$ slurm.mungectl generate
$ slurm.mungectl get | tee super-secret.key
$ snap restart slurm.munged

# [compute]: Copy super-secret.key, set using mungectl, and restart munged.
$ cat super-secret.key | slurm.mungectl set 
$ snap restart slurm.munged

The idea here is that we don't have to worry about exposing the key on the host in a tempfile. Rather than have a temporary staging location, we can instead use pipes to get and send the new key to the snap. mungectl worries about decoding/encoding the munge key into base64 text and removing extraneous white space picked up in the process.

Integration tests and unit tests have been updated/added to verify the functionality of mungectl. I licensed it under GPL-3.0 since it is a custom submodule that we are adding. Maybe one day we'll break it out into its own repository, but for now we'll treat it like a submodule.

Let me know if you have any questions! 😄

Misc.

I centralized everything into one Makefile and updated the CI to use make instead of tox. Eventually, when we replace slurmhelpers with something that doesn't break in classic confinement, I'll drop tox and everything can have nice Makefile syntax instead.

I mothballed slurmhelpers into src since it's also a submodule, and I wanted to decrease the amount of top-level directories that we have in this repository 😅

Related issues

Closes #14

Signed-off-by: Jason C. Nucciarone <[email protected]>
delete top-level slurmhelpers and move into src

Signed-off-by: Jason C. Nucciarone <[email protected]>
Also, needed to update ruff since it now uses the check
subcommand before performing formatting/lints.

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss added the enhancement New feature or request label Jul 2, 2024
@NucciTheBoss NucciTheBoss requested a review from jedel1043 July 2, 2024 22:34
@NucciTheBoss NucciTheBoss self-assigned this Jul 2, 2024
Signed-off-by: Jason C. Nucciarone <[email protected]>
Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

I'm going back and forth with the name, and I was thinking that since the command is only used to manage the key, maybe that should be signaled by the command name. What about something like mungekeyctl?

@NucciTheBoss
Copy link
Member Author

I agree that we could have better grammar for the command interface, however, I think the name mungekeyctl clashes with mungekey. Both commands perform some level of key management, so I'm worried that some coming from the outside in will get confused with what the differences between the commands are. E.g. which one do I call?? when in actuality we added mungekeyctl to make our lives as DevOps engineers easier 😅

I see that we have two options here:

Option 1

Keep mungectl but use verb-noun command grammar:

$ mungectl get-key  #=> mungectl get
$ mungectl set-key  #=> mungectl set
$ mungectl generate-key  #=> mungectl generate

The pros of this approach is that the name matches other common system utilities - systemctl, snapctl, kubectl, etc. - and keeps the door open for expanding mungectl's functionality if there are other commands/munge macros that we want to add later down the line. Cobra is also capable of generating the necessary shell completions.

Option 2

Rename to mungekeyx and keep the verb command grammar:

$ mungekeyx get  #=> mungectl get
$ mungekeyx set  #=> mungectl set
$ mungekeyx generate  #=> mungectl generate

mungekeyx - similar to javax - would that it is a extension to the mungekey command rather than conflicting/superseding it. One con with this approach though is that if we want to add any further functionality to munge within the snap, we'd likely need to create a new root command. Not the end of the world, but it's always nice to just have one root command with aliases for commonly used commands.

My preferred option is option 1. Let me know what your preferred option is @jedel1043!

@jedel1043
Copy link
Contributor

Option 1 is better IMO.

@NucciTheBoss
Copy link
Member Author

Option 1 is better IMO.

Coolio, I'll do that then!

Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
Signed-off-by: Jason C. Nucciarone <[email protected]>
Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Liking the new API!

@NucciTheBoss
Copy link
Member Author

NucciTheBoss commented Jul 3, 2024

Great! I'll land this so we can get to working on integrating COS!

We'll worry about potentially spinning out mungectl after we get a working demo with the COS dashboard 🤩

I'll be back next week to go goblin mode 👺

@NucciTheBoss NucciTheBoss merged commit 8164815 into main Jul 3, 2024
4 checks passed
@NucciTheBoss NucciTheBoss deleted the nuccitheboss/enhance-munge-api branch July 8, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Flesh out a good API to get/set/generate munge keys
2 participants