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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Fix RFC-1123 hostname string validation for ensembler names #257

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Oct 3, 2022

TL;DR

Problem: Users create ensembler names with underscores -> Invalid Kaniko job names -> Invalid K8s jobs -> Router deployments fail

Solution: Add additional API checks to block usage of underscores -> Discovery of bug in existing go-validator validation tag to validate ensembler names -> Update go-validator -> Done

Context

Pyfunc ensemblers are built as a web service and containerised into a single image upon being deployed as part of a router deployment. This image building process takes place in Kaniko builders run as jobs on a Kubernetes cluster. The Turing API names these jobs automatically, in a way which incorporates the user-registered name of the ensemblers to be built:

kanikoJobName := ib.nameGenerator.generateBuilderName(
	request.ProjectName,
	request.ResourceName,  // this refers to the name of the ensembler
	request.ResourceID,
	request.VersionID,
)

where the method generateBuilderName is given by:

func (n *ensemblerJobNameGenerator) generateBuilderName(
	projectName string,
	modelName string,
	modelID models.ID,
	versionID string,
) string {
	partialVersionID := getPartialVersionID(versionID, 5)
	return fmt.Sprintf("batch-%s-%s-%d-%s", projectName, modelName, modelID, partialVersionID)
}

Errors thus occur when users define their ensembler names with underscores (_) , because this causes the job names generated to similarly contain them, rendering the job names of the Kaniko builders invalid when run in a Kubernetes cluster, since Kubernetes requires the job names to fulfil standards defined in RFC 1123, i.e.

  • contain only lowercase alphanumeric characters or '-'
  • start with an alphanumeric character
  • end with an alphanumeric character

Router deployments using ensemblers that have underscores in their names thus fail immediately since the image building jobs get rejected instantly.

Fix

The original intention of the fix was to introduce additional API checks to validate the ensembler names submitted by users and then return appropriate errors to prevent the above mentioned errors from occurring.

However, it was found that the existing GenericEnsembler struct that gets created when handling ensembler creation/update API calls already contains the necessary go-playground validator hostname_rfc1123 validation for those RFC 1123 standards on the Name field:

type GenericEnsembler struct {
	Model

	ProjectID ID `json:"project_id" gorm:"column:project_id"`

	Type EnsemblerType `json:"type" gorm:"column:type" validate:"required,oneof=pyfunc"`

	Name string `json:"name" gorm:"column:name" validate:"required,hostname_rfc1123,lte=20,gte=3"`
}

Clearly, there was an apparent bug with the hostname_rfc1123 validation tag that led to the observed bug that this PR is trying to address. Upon further investigation, it was found that the existing version (v10.9.0) of the go-playground validator used by the Turing API does not contain the necessary bug fix for the hostname_rfc1123 validation tag.

Fortunately, all that's needed is to update its version to v10.11.0, which includes the required bug fix (PR #912). And since v10.11.1 is the newest release with an additional bug fix, why not just update the dependency to the latest version? 馃檭

Miscellaneous

This PR also makes a tiny modification to an incorrect docstring mentioning Kaniko jobs as 'pods'.

@deadlycoconuts deadlycoconuts added the type: bug Something isn't working label Oct 3, 2022
@deadlycoconuts deadlycoconuts self-assigned this Oct 3, 2022
@deadlycoconuts deadlycoconuts requested a review from a team October 3, 2022 15:44
Copy link
Contributor

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for tracking this down! 馃帀

@deadlycoconuts deadlycoconuts force-pushed the add_validation_checks_for_ensembler_names branch from d6e73a6 to 790fd7b Compare October 4, 2022 03:41
@deadlycoconuts deadlycoconuts merged commit dd801e2 into caraml-dev:main Oct 4, 2022
@deadlycoconuts deadlycoconuts deleted the add_validation_checks_for_ensembler_names branch October 7, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants