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

feat: pipeline and model name validation #5872

Merged

Conversation

driev
Copy link

@driev driev commented Sep 4, 2024

Pipelines and models with dots in their names cause problems, as the scheduler will trim the name from the first occurrence of the dot, when attempting to retrieve the resource.

Adding some validation to model and pipeline names to highlight this scenerio and return descriptive error messages.

Only alphanumeric names with underscores and hyphens are valid.

It's probably worth starting with the utils/validate_test.go file.

TODO :

  • add model name validation check

@driev driev marked this pull request as draft September 4, 2024 14:42
@driev driev changed the title adding pipeline and model name validation feat: pipeline and model name validation Sep 4, 2024
@driev driev marked this pull request as ready for review September 4, 2024 17:07
Copy link
Member

@lc525 lc525 left a comment

Choose a reason for hiding this comment

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

I think some minor changes are needed around the regex given what k8s supports (and corresponding test cases), but this lgtm as a structure otherwise.

import "regexp"

func CheckName(name string) bool {
ok, err := regexp.Match("^[a-zA-Z0-9][a-zA-Z0-9-_]*[a-zA-Z0-9]$", []byte(name))
Copy link
Member

Choose a reason for hiding this comment

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

The spec on what names are allowed in k8s is this one: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

k8s will already reject all names that don't obey the regex: [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* (this is the regex that kubectl reports when trying to create a resource with an invalid name). In particular, upper-case letters and underscores are also excluded.

We'll still want to do this validation explicitly so that we deal in an uniform way with k8s and non-k8s deployments.

Beyond k8s's existing validation, we want to reserve the . for Seldon -- this is because internally we do header-based routing and a pipeline's name (for example) is referred to as '[name].pipeline`.

I'm not sure why we don't just trim the element after the last dot when post-processing such names (and allow dots in user-provided names). I don't have the context on whether there are any other reasons why we might reserve the ., but perhaps we take this as a separate issue to investigate.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, some more investigation is necessary. It would be great to have this validation aligned with k8s.

scheduler/pkg/store/utils/validate_test.go Show resolved Hide resolved
@lc525 lc525 added the v2 label Sep 5, 2024
Copy link
Member

@lc525 lc525 left a comment

Choose a reason for hiding this comment

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

lgtm

@driev driev merged commit 6605caf into SeldonIO:v2 Sep 9, 2024
3 checks passed
@driev driev deleted the INFRA-1049/model-and-pipeline-name-validation branch September 9, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants