Validate that Flyte labels are K8s compliant (flyteorg#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 <[email protected]>

Author: kosigz-lyft

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)

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])?')]")
+}

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
+}

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])?')]")
+}