fix(ssh_import_id): honor ssh_import_id on the default user - #7050
Open
Mohith1612 wants to merge 1 commit into
Open
fix(ssh_import_id): honor ssh_import_id on the default user#7050Mohith1612 wants to merge 1 commit into
Mohith1612 wants to merge 1 commit into
Conversation
handle() read ssh_import_id from the top-level config when the user was the default one, and from the user definition for everyone else. An ssh_import_id set inside a default user definition was therefore parsed, matched by the module's own activation check, and then silently dropped. Merge both sources for the default user. A top-level ssh_import_id keeps working on its own, and non-default users are unaffected. Fixes canonicalGH-4306
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Commit Message
Additional Context
Fixes GH-4306.
handle()picks the source ofssh_import_idbased on whether the user isthe default one:
So
ssh_import_idinside a default user definition is read bynormalize_users_groups, counted by the module's ownis_key_in_nested_dictactivation check, and then dropped without a logline. That is the surprise reported in the issue.
This uses
util.uniq_mergerather than a plain concatenation because the keyalso accepts a comma separated string, and merging keeps the existing
de-duplication.
Driving the real
handle()withimport_ssh_idsmocked, before and after:user: {name: dave, ssh_import_id: [lp:user]}[]['lp:user']['lp:top']['lp:top', 'lp:user']['lp:top']['lp:top']['lp:bob']['lp:bob']Only the first two rows change. A top-level
ssh_import_idon its own isuntouched, and non-default users are untouched.
This does change behaviour on upgrade: an instance whose config sets
ssh_import_idinside the default user block will start importing keys thatwere previously ignored. I think that is the point of the fix, since the
config was asking for exactly that, but it is worth calling out explicitly
given the module installs SSH keys.
Doc-only alternative. @TheRealFalcon's analysis on the issue said "At the
very least we need a doc change for this, but I think supporting the user
config makes more sense", so I went with the behaviour fix. If you would
rather keep the current behaviour, the alternative is to document that the
default user takes
ssh_import_idonly from the top level, and I am happy toswap this PR for that instead. Nothing in the module docs or the schema
mentions the distinction today.
Test Steps
handle()'s user loop had no unit coverage, so this adds four parametrizedcases. Two of them fail without the code change and pass with it, and two are
controls that pass either way to show nothing else moved:
Full runs:
Merge type