From a2ccc205d24844104b09ab84b7d8e194c0db657a Mon Sep 17 00:00:00 2001 From: Konstantin Gizdarski <53313394+kosigz-lyft@users.noreply.github.com> Date: Thu, 15 Oct 2020 00:40:51 -0700 Subject: [PATCH] Validate that Flyte labels are K8s compliant (#131) * v0.0.1 * fix build * wip * wip * no build brb * no build brb * rm k8s validation * wip * feedback * lowercase error strings is convention in golang * also add validation on project creation * test * add integration test? * last line * rename some stuff * lint * done Co-authored-by: Konstantin Gizdarski --- .../pkg/manager/impl/project_manager.go | 6 +++ .../pkg/manager/impl/project_manager_test.go | 34 +++++++++++++++++ .../impl/validation/project_validator.go | 27 ++++++++++++++ flyteadmin/tests/project.go | 37 +++++++++++++++++++ 4 files changed, 104 insertions(+) diff --git a/flyteadmin/pkg/manager/impl/project_manager.go b/flyteadmin/pkg/manager/impl/project_manager.go index 10c5268942..f1da4608fd 100644 --- a/flyteadmin/pkg/manager/impl/project_manager.go +++ b/flyteadmin/pkg/manager/impl/project_manager.go @@ -70,6 +70,12 @@ func (m *ProjectManager) UpdateProject(ctx context.Context, projectUpdate admin. return nil, err } + // Run validation on the request, specifically checking for labels, and return err if validation does not succeed. + err = validation.ValidateProjectLabels(projectUpdate) + if err != nil { + return nil, err + } + // Transform the provided project into a model and apply to the DB. projectUpdateModel := transformers.CreateProjectModel(&projectUpdate) err = projectRepo.UpdateProject(ctx, projectUpdateModel) diff --git a/flyteadmin/pkg/manager/impl/project_manager_test.go b/flyteadmin/pkg/manager/impl/project_manager_test.go index 0d9a3eaa45..c09d342ca0 100644 --- a/flyteadmin/pkg/manager/impl/project_manager_test.go +++ b/flyteadmin/pkg/manager/impl/project_manager_test.go @@ -124,3 +124,37 @@ func TestProjectManager_CreateProjectError(t *testing.T) { }) assert.EqualError(t, err, "Domains are currently only set system wide. Please retry without domains included in your request.") } + +func TestProjectManager_CreateProjectErrorDueToBadLabels(t *testing.T) { + mockRepository := repositoryMocks.NewMockRepository() + mockRepository.ProjectRepo().(*repositoryMocks.MockProjectRepo).CreateFunction = func( + ctx context.Context, namespace models.Project) error { + return errors.New("uh oh") + } + projectManager := NewProjectManager(mockRepository, + runtimeMocks.NewMockConfigurationProvider( + getMockApplicationConfigForProjectManagerTest(), nil, nil, nil, nil, nil)) + _, err := projectManager.CreateProject(context.Background(), admin.ProjectRegisterRequest{ + Project: &admin.Project{ + Id: "flyte-project-id", + Name: "flyte-project-name", + Description: "flyte-project-description", + }, + }) + assert.EqualError(t, err, "uh oh") + + _, err = projectManager.CreateProject(context.Background(), admin.ProjectRegisterRequest{ + Project: &admin.Project{ + Id: "flyte-project-id", + Name: "flyte-project-name", + Description: "flyte-project-description", + Labels: &admin.Labels{ + Values: map[string]string{ + "foo": "#badlabel", + "bar": "baz", + }, + }, + }, + }) + assert.EqualError(t, err, "invalid label value [#badlabel]: [a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')]") +} diff --git a/flyteadmin/pkg/manager/impl/validation/project_validator.go b/flyteadmin/pkg/manager/impl/validation/project_validator.go index 2a955c82de..746160749b 100644 --- a/flyteadmin/pkg/manager/impl/validation/project_validator.go +++ b/flyteadmin/pkg/manager/impl/validation/project_validator.go @@ -24,6 +24,9 @@ func ValidateProjectRegisterRequest(request admin.ProjectRegisterRequest) error if err := ValidateEmptyStringField(request.Project.Id, projectID); err != nil { return err } + if err := ValidateProjectLabels(*request.Project); err != nil { + return err + } if errs := validation.IsDNS1123Label(request.Project.Id); len(errs) > 0 { return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid project id [%s]: %v", request.Project.Id, errs) } @@ -40,6 +43,13 @@ func ValidateProjectRegisterRequest(request admin.ProjectRegisterRequest) error return nil } +func ValidateProjectLabels(request admin.Project) error { + if err := ValidateProjectLabelsAlphanumeric(request); err != nil { + return err + } + return nil +} + // Validates that a specified project and domain combination has been registered and exists in the db. func ValidateProjectAndDomain( ctx context.Context, db repositories.RepositoryInterface, config runtimeInterfaces.ApplicationConfiguration, projectID, domainID string) error { @@ -62,3 +72,20 @@ func ValidateProjectAndDomain( } return nil } + +// Given an admin.Project, checks if the project has labels and if it does, checks if the labels are K8s compliant, +// i.e. alphanumeric + - and _ +func ValidateProjectLabelsAlphanumeric(request admin.Project) error { + if request.Labels == nil || len(request.Labels.Values) == 0 { + return nil + } + for key, value := range request.Labels.Values { + if errs := validation.IsDNS1123Label(key); len(errs) > 0 { + return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid label key [%s]: %v", key, errs) + } + if errs := validation.IsDNS1123Label(value); len(errs) > 0 { + return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid label value [%s]: %v", value, errs) + } + } + return nil +} diff --git a/flyteadmin/tests/project.go b/flyteadmin/tests/project.go index 686ef0eea8..3a5ac771b0 100644 --- a/flyteadmin/tests/project.go +++ b/flyteadmin/tests/project.go @@ -160,3 +160,40 @@ func TestUpdateProjectLabels(t *testing.T) { assert.Equal(t, barExists, true) assert.Equal(t, barVal, "baz") } + +func TestUpdateProjectLabels_BadLabels(t *testing.T) { + ctx := context.Background() + client, conn := GetTestAdminServiceClient() + defer conn.Close() + + // Create a new project. + req := admin.ProjectRegisterRequest{ + Project: &admin.Project{ + Id: "potato", + Name: "spud", + }, + } + _, err := client.RegisterProject(ctx, &req) + assert.Nil(t, err) + + // Verify the project has been registered. + projects, err := client.ListProjects(ctx, &admin.ProjectListRequest{}) + assert.Nil(t, err) + assert.NotEmpty(t, projects.Projects) + + // Attempt to modify the name of the Project. Labels and name should be + // modified. + _, err = client.UpdateProject(ctx, &admin.Project{ + Id: "potato", + Name: "foobar", + Labels: &admin.Labels{ + Values: map[string]string{ + "foo": "#bar", + "bar": "baz", + }, + }, + }) + + // Assert that update went through without an error. + assert.EqualError(t, err, "invalid label value [#bar]: [a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')]") +}