Add stricter func name validation #544
Conversation
The valid function names contain a combination of lowercase letters, numbers and separators (., -, _). Name can only start and end with a letter or a number and there can only be one separator in the row (e.g. my---funcname is not valid, however my-func_name is valid). Fixes fnproject#503
| return errors.New("Function name must be lowercase") | ||
| // Match multiple groups of lowercase charaters and numbers, followed by 0 or 1 instances | ||
| // of the separators; the string also needs to end with a character or a number (not separator) | ||
| isValid := regexp.MustCompile(`^([a-z0-9]+[._-]?)*[a-z0-9]$`).MatchString |
There was a problem hiding this comment.
we should compile this at init time and have a global var to use just to call MatchString, regex compile is slow and this func may have repeated invocations (eg in deploy --all)
There was a problem hiding this comment.
you've hit a very sore subject, congrats :)
a) name validation and image validation are inconsistent across all methods that use them not just in the cli but also server side. for example, here we're doing name validation this way, but in deploy we're not doing this validation at all (or build). it's possible for users to change the name of their file in the function after init, at which point, they'll get an entirely different error from the server if it's invalid because we're using https://golang.org/pkg/net/url/#PathEscape to validate the name server side https://github.com/fnproject/fn/blob/master/api/models/fn.go#L156 - further, we aren't validating the image field at all server side when uploading (just name), which will result in runtime errors for users, we should fix that too. there's a few issues here anyway, it would be nice to unify them all not just in the cli but across cli and fn (really we should push to server, but it's nice to error in cli for the build step before bumping and pushing updates)
b) we're constructing image names from not just fn name but other fields as well, we should handle doing the validation of the image at this level most likely https://github.com/fnproject/cli/blob/master/common/funcfile.go#L346 to avoid errors when trying to docker build. similarly, in the server, we should do that same validation for users that use the update fn API (deploy also uses it).
hopefully, this is not overwhelming, I'd say our validation of image and name is a bit of a mess and hopefully the above is useful in order to remedy things. I guess I'd say we should align all these things and it may not necessarily be the method suggested here, so I'm pointing out everything :) thanks for digging in
|
Thanks for the explanation! I figured there would be multiple places for this :) I am thinking perhaps fixing the b) as part of this PR as well and have a separate issue for a) and unifying it. How does that sound @rdallman ? |
|
sounds good, thanks. I should've split a) into two parts, sorry - I think here we would be well served to validate the image name (ie after constructing from name) across init, build, bump, push and deploy |
The valid function names contain a combination of lowercase letters, numbers
and separators (., -, _). Name can only start and end with a letter or
a number and there can only be one separator in the row (e.g. my---funcname
is not valid, however my-func_name is valid).
Fixes #503