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

Update host user creation RFD to allow for permanent users #27074

Merged
merged 3 commits into from
Jun 7, 2023

Conversation

lxea
Copy link
Contributor

@lxea lxea commented May 29, 2023

Related #26892

@github-actions github-actions bot added rfd Request for Discussion size/md labels May 29, 2023
@codingllama
Copy link
Contributor

@lxea I suggest you assign your original RFD reviewers here, they are bound to have more context and do a better job than the bot reviewers.

Comment on lines 160 to 162
If multiple roles matching a node specify the `create_host_user_mode`
option with both `drop` and `remain`, the more restrictive `drop`
option will be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this, it means that unless someone is very very careful with how they define their roles, they might accidentally end up with drop behavior instead of the correct one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a role option, or should this be a host option like ssh_service.disable_create_host_user?

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 suggested this when speaking to @r0mant however I think we want this as a role option

What would be preferred behavior in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the downside to letting remain take precedence over drop?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be a role option, yeah. I think it's ok to default to keep, then it will also be consistent with automatic user provisioning for database and Windows access which always keep the user. As long as there's no surprise change in behavior for existing users which are using the current setting.

That said, I think that the option on the SSH service should be updated accordingly as well, and converted to an enum: create_host_user_mode: off|keep|drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

@espadolini so essentially you mean if create_host_user=off && create_host_user_mode=remain, when connecting to an old node, the node should read it as create_host_user=off, right?

Is there any real benefit to this flexibility? I think having old nodes default to drop behavior when create_host_user_mode=remain is reasonable.

If we want to disable host user creation on a specific node, including old nodes, we can allow the user to set create_host_user=off or create_host_user_mode=off and prioritize that setting over role options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should deprecate existing "create_host_user" option so we don't have to maintain both and follow our normal deprecation logic:

  • If "create_host_user_mode" is set, it always takes precedence.
  • Keep "create_host_user" for backwards compatibility and respect it if it's set (unless there's "creaet_host_user_mode") but remove it from all user-facing docs.

@lxea Can you update the RFD with this deprecation plan?

Copy link
Contributor

@klizhentas klizhentas Jun 7, 2023

Choose a reason for hiding this comment

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

What is the deprecation plan? I assume we should trigger role migration and update all old roles to the new option. In this case new role version will reject the deprecated option.

The alternative is to keep the option, but instead treat true as drop and keep as the other additional value, it's a hack, but should work :)

Copy link
Contributor Author

@lxea lxea Jun 7, 2023

Choose a reason for hiding this comment

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

I've updated it for this and included a bit about the plan for migration after the deprecated option is removed. The implementation PR implements the precedence as you mention here.

What is the consensus on the creaet_host_user_mode option in ssh_service, do we want this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the service-level "mode" option is not needed as it may make things confusing around conflict resolution as we discussed above. Just keeping the "on/off" toggle we have should be fine for now IMO.

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm after you add the SSH service option to the scope as well.

@klizhentas @xinding33 Can you folks review the UX here when you get a chance?

rfd/0057-automatic-user-provisioning.md Outdated Show resolved Hide resolved
Comment on lines 160 to 162
If multiple roles matching a node specify the `create_host_user_mode`
option with both `drop` and `remain`, the more restrictive `drop`
option will be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be a role option, yeah. I think it's ok to default to keep, then it will also be consistent with automatic user provisioning for database and Windows access which always keep the user. As long as there's no surprise change in behavior for existing users which are using the current setting.

That said, I think that the option on the SSH service should be updated accordingly as well, and converted to an enum: create_host_user_mode: off|keep|drop.

@r0mant r0mant requested a review from klizhentas June 6, 2023 23:18
@lxea lxea force-pushed the lxea/host-users-rfd-update-mode branch from b95ba9e to 08fa58a Compare June 7, 2023 10:19
@lxea lxea added this pull request to the merge queue Jun 7, 2023
Merged via the queue into master with commit 12c95c5 Jun 7, 2023
21 checks passed
@lxea lxea deleted the lxea/host-users-rfd-update-mode branch June 7, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfd Request for Discussion size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants