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

Make sure lima user fallback uses same validation as template #2655

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pvdvreede
Copy link

@pvdvreede pvdvreede commented Sep 26, 2024

The regex currently being used is different from the identifier's validation from containerd. The fallback test does allow a starting _ but the validation for the identifier does not.

This results in a bug where the a user that starts with an _ will pass fallback validation (ie not be set to lima for the user), but will then fail the cidata validation here: https://github.com/lima-vm/lima/blob/master/pkg/cidata/template.go#L95.

Error log shows as:
ERRO[0000] [hostagent] identifier "_nixbld1" must match ^[A-Za-z0-9]+(?:[._-](?:[A-Za-z0-9]+))*$: invalid argument fields.level=fatal

This PR sets the same validation check in both spots to fix this and make sure they stay in sync in the future.

The regex currently being used is different from the identifier's validation from containerd. The fallback test does allow an `_` but the validation for the identifier does not.

This results in a bug where the a user that starts with an `_` will pass fallback validation (ie not be set to lima for the user), but will then fail the cidata validation here: https://github.com/lima-vm/lima/blob/master/pkg/cidata/template.go#L95.

Error log shows as:
` ERRO[0000] [hostagent] identifier "_nixbld1" must match ^[A-Za-z0-9]+(?:[._-](?:[A-Za-z0-9]+))*$: invalid argument  fields.level=fatal`

This PR sets the same validation check in both spots to fix this and make sure they stay in sync in the future.

Update warning message to use error msg

fix bad err method call.

Signed-off-by: pvdvreede <[email protected]>
@jandubois
Copy link
Member

The regex currently being used is different from the identifier's validation from containerd.

I think using the containerd identifier validation for Linux hostnames is wrong. If anything we should be using the regex from osutils/user.

However, I think I would prefer to remove the validation in cidata completely and just rely on it being validated by limayaml.FillDefaults (see #2378 (comment) for my latest thoughts).

So I would like to either

  1. close this PR without merging, or
  2. change this PR to use the osutils.user regex in the cidata validation

I'm willing to consider (2) because it is not clear when a reworked version of #2378 will be available.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Oops, sent my review as a regular comment.

So please either close the PR or modify it to use the osutils.user regex also in cidata.

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