Skip to content

feat: introduce optional stateful user management#36

Closed
JulienMalka wants to merge 1 commit intonikstur:mainfrom
JulienMalka:stateful-users
Closed

feat: introduce optional stateful user management#36
JulienMalka wants to merge 1 commit intonikstur:mainfrom
JulienMalka:stateful-users

Conversation

@JulienMalka
Copy link
Copy Markdown
Contributor

In non-nixos settings, it is frequent that some users/groups are created by default by the distribution, or impurely by the user. Because userborn is stateless, those users/groups are disabled when they are not included in the userborn configuration. This change introduce a stateful mode, activated by the USERBORN_STATEFUL environement variable, where userborn maintain a list of users/groups it owns and only act on entities of that list.

In non-nixos settings, it is frequent that some users/groups are created
by default by the distribution, or impurely by the user. Because
userborn is stateless, those users/groups are disabled when they are not
included in the userborn configuration. This change introduce a stateful
mode, activated by the USERBORN_STATEFUL environement variable, where
userborn maintain a list of users/groups it owns and only act on
entities of that list.
Copy link
Copy Markdown
Owner

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

Thank you for raising this issue and working on this implementation!

However, I feel that this is very complicated. In fact too complicated for my liking. It touches much of the core code of Userborn which makes it impossible for me to say in good conscience that this change is correct.

I believe this can be implemented in a much simpler and less intrusive way. See #38

Comment on lines +54 to +65
let json = serde_json::to_string_pretty(&managed)
.with_context(|| "Failed to serialize managed entities")?;

let mut file = OpenOptions::new()
.write(true)
.create(true)
.truncate(true)
.open(&self.state_file_path)
.with_context(|| format!("Failed to create/open state file: {:?}", self.state_file_path))?;

file.write_all(json.as_bytes())
.with_context(|| "Failed to write state file")?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This whole thing could be `

Suggested change
let json = serde_json::to_string_pretty(&managed)
.with_context(|| "Failed to serialize managed entities")?;
let mut file = OpenOptions::new()
.write(true)
.create(true)
.truncate(true)
.open(&self.state_file_path)
.with_context(|| format!("Failed to create/open state file: {:?}", self.state_file_path))?;
file.write_all(json.as_bytes())
.with_context(|| "Failed to write state file")?;
fs::write(&self.state_file_path, serde_json::to_vec_pretty(&managed).context("")?).context("")?

file.write_all(json.as_bytes())
.with_context(|| "Failed to write state file")?;

log::debug!("Saved managed entities to state file: {:?}", self.state_file_path);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
log::debug!("Saved managed entities to state file: {:?}", self.state_file_path);
log::info!("Saved managed entities to state file: {:?}", self.state_file_path);

Comment on lines +35 to +40
let mut file = File::open(&self.state_file_path)
.with_context(|| format!("Failed to open state file: {:?}", self.state_file_path))?;

let mut contents = String::new();
file.read_to_string(&mut contents)
.with_context(|| "Failed to read state file")?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
let mut file = File::open(&self.state_file_path)
.with_context(|| format!("Failed to open state file: {:?}", self.state_file_path))?;
let mut contents = String::new();
file.read_to_string(&mut contents)
.with_context(|| "Failed to read state file")?;
let contents = fs::read(&self.state_file_path).context("")?;

file.read_to_string(&mut contents)
.with_context(|| "Failed to read state file")?;

let managed: ManagedEntities = serde_json::from_str(&contents)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
let managed: ManagedEntities = serde_json::from_str(&contents)
let managed: ManagedEntities = serde_json::from_slice(&contents)

use serde::Deserialize;

#[derive(Deserialize, Debug)]
#[derive(Deserialize, Debug, Clone, PartialEq, serde::Serialize)]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

serde is not used if I see that correctly.

Suggested change
#[derive(Deserialize, Debug, Clone, PartialEq, serde::Serialize)]
#[derive(Deserialize, Debug, Clone, PartialEq)]

log::info!("Running in stateful mode");

// Initialize state manager
let state_manager = StateManager::new(&directory);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This would currently write this file right to /etc. This should be the StateDirectory of the Userborn service.

if !previous.users.contains(user) {
log::info!(" + Taking ownership of user: {}", user);
} else {
log::info!(" ~ Managing user: {}", user);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This would log this info on every single invocation. That's too much noise for an info log.

@JulienMalka
Copy link
Copy Markdown
Contributor Author

Thanks for the review! I do agree that your version is more clean and minimalist, so closing this!

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.

2 participants