Skip to content

Commit

Permalink
feat: add label spec validation (#181)
Browse files Browse the repository at this point in the history
* feat: add label under internal

* feat: add labels validation and its unit test

* feat: add validation for metadata

* refactor: resource metadata validation for more readability

* feat: add label validation for job spec

* refactor: rename label to labels

* chore: update error message for better clarity

* refactor: separate key and value validation

* fix: change labels to not return error
  • Loading branch information
irainia authored and deryrahman committed Nov 17, 2023
1 parent eee95a0 commit ee6adc6
Show file tree
Hide file tree
Showing 7 changed files with 411 additions and 29 deletions.
6 changes: 2 additions & 4 deletions core/job/handler/v1beta1/job_adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/goto/optimus/core/job"
"github.com/goto/optimus/internal/errors"
"github.com/goto/optimus/internal/lib/labels"
"github.com/goto/optimus/internal/lib/window"
"github.com/goto/optimus/internal/models"
"github.com/goto/optimus/internal/utils"
Expand Down Expand Up @@ -128,10 +129,7 @@ func fromJobProto(js *pb.JobSpecification) (*job.Spec, error) {
jobSpecBuilder := job.NewSpecBuilder(version, name, owner, schedule, window, task).WithDescription(js.Description)

if js.Labels != nil {
labels, err := job.NewLabels(js.Labels)
if err != nil {
return nil, err
}
labels := labels.FromMap(js.Labels)
jobSpecBuilder = jobSpecBuilder.WithLabels(labels)
}

Expand Down
23 changes: 13 additions & 10 deletions core/job/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/goto/optimus/core/tenant"
"github.com/goto/optimus/internal/errors"
"github.com/goto/optimus/internal/lib/labels"
"github.com/goto/optimus/internal/lib/window"
)

Expand All @@ -24,7 +25,7 @@ type Spec struct {
task Task

description string
labels map[string]string
labels labels.Labels
metadata *Metadata
hooks []*Hook
asset Asset
Expand Down Expand Up @@ -60,7 +61,7 @@ func (s *Spec) Description() string {
return s.description
}

func (s *Spec) Labels() map[string]string {
func (s *Spec) Labels() labels.Labels {
return s.labels
}

Expand Down Expand Up @@ -105,9 +106,18 @@ func (s *SpecBuilder) Build() (*Spec, error) {
if s.spec.version <= 0 {
return nil, errors.InvalidArgument(EntityJob, "version is less than or equal to zero")
}

if s.spec.owner == "" {
return nil, errors.InvalidArgument(EntityJob, "owner is empty")
}

if s.spec.labels != nil {
if err := s.spec.labels.Validate(); err != nil {
msg := fmt.Sprintf("labels is invalid: %v", err)
return nil, errors.InvalidArgument(EntityJob, msg)
}
}

return s.spec, nil
}

Expand Down Expand Up @@ -136,7 +146,7 @@ func (s *SpecBuilder) WithMetadata(metadata *Metadata) *SpecBuilder {
return s
}

func (s *SpecBuilder) WithLabels(labels map[string]string) *SpecBuilder {
func (s *SpecBuilder) WithLabels(labels labels.Labels) *SpecBuilder {
s.spec.labels = labels
return s
}
Expand Down Expand Up @@ -675,13 +685,6 @@ func (s *SpecUpstreamBuilder) WithSpecHTTPUpstream(httpUpstreams []*SpecHTTPUpst
return s
}

func NewLabels(labels map[string]string) (map[string]string, error) {
if err := validateMap(labels); err != nil {
return nil, err
}
return labels, nil
}

// TODO: check whether this is supposed to be here or in utils
func validateMap(input map[string]string) error {
for key := range input {
Expand Down
32 changes: 21 additions & 11 deletions core/job/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/goto/optimus/core/job"
"github.com/goto/optimus/core/tenant"
"github.com/goto/optimus/internal/lib/labels"
"github.com/goto/optimus/internal/lib/window"
"github.com/goto/optimus/internal/models"
)
Expand All @@ -27,7 +28,7 @@ func TestEntitySpec(t *testing.T) {
jobTaskConfig, _ := job.ConfigFrom(map[string]string{"sample_task_key": "sample_value"})
jobTask := job.NewTask("bq2bq", jobTaskConfig)
description := "sample description"
labels := map[string]string{"key": "value"}
lbl := labels.FromMap(map[string]string{"key": "value"})
hook, _ := job.NewHook("sample-hook", jobTaskConfig)
jobAlertConfig, _ := job.ConfigFrom(map[string]string{"sample_alert_key": "sample_value"})

Expand All @@ -50,7 +51,7 @@ func TestEntitySpec(t *testing.T) {
t.Run("should return values as inserted", func(t *testing.T) {
specA, err := job.NewSpecBuilder(jobVersion, "job-A", "sample-owner", jobSchedule, jobWindow, jobTask).
WithDescription(description).
WithLabels(labels).WithHooks([]*job.Hook{hook}).WithAlerts([]*job.AlertSpec{alert}).
WithLabels(lbl).WithHooks([]*job.Hook{hook}).WithAlerts([]*job.AlertSpec{alert}).
WithSpecUpstream(specUpstream).
WithAsset(asset).
WithMetadata(jobMetadata).
Expand Down Expand Up @@ -81,7 +82,7 @@ func TestEntitySpec(t *testing.T) {
assert.Equal(t, jobTask.Config(), specA.Task().Config())

assert.Equal(t, description, specA.Description())
assert.Equal(t, labels, specA.Labels())
assert.Equal(t, lbl, specA.Labels())

assert.Equal(t, []*job.Hook{hook}, specA.Hooks())
assert.Equal(t, hook.Name(), specA.Hooks()[0].Name())
Expand Down Expand Up @@ -115,6 +116,23 @@ func TestEntitySpec(t *testing.T) {
assert.Equal(t, jobMetadata.Scheduler(), specA.Metadata().Scheduler())
assert.Equal(t, jobMetadata.Scheduler(), specA.Metadata().Scheduler())
})

t.Run("should return nil and error if labels is invalid", func(t *testing.T) {
lbl := labels.FromMap(map[string]string{
"test_key": "",
})

actualSpec, actualError := job.NewSpecBuilder(jobVersion, "job-A", "sample-owner", jobSchedule, jobWindow, jobTask).
WithDescription(description).
WithLabels(lbl).WithHooks([]*job.Hook{hook}).WithAlerts([]*job.AlertSpec{alert}).
WithSpecUpstream(specUpstream).
WithAsset(asset).
WithMetadata(jobMetadata).
Build()

assert.Nil(t, actualSpec)
assert.ErrorContains(t, actualError, "labels is invalid")
})
})

t.Run("Specs", func(t *testing.T) {
Expand Down Expand Up @@ -304,12 +322,4 @@ func TestEntitySpec(t *testing.T) {
assert.Empty(t, jobConfig)
})
})

t.Run("NewLabels", func(t *testing.T) {
t.Run("should return error if the labels map is invalid", func(t *testing.T) {
jobLabels, err := job.NewLabels(map[string]string{"": ""})
assert.Error(t, err)
assert.Empty(t, jobLabels)
})
})
}
24 changes: 21 additions & 3 deletions core/resource/resource.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package resource

import (
"fmt"
"reflect"
"strings"

"github.com/goto/optimus/core/tenant"
"github.com/goto/optimus/internal/errors"
"github.com/goto/optimus/internal/lib/labels"
)

const (
Expand All @@ -16,7 +18,22 @@ const (
type Metadata struct {
Version int32
Description string
Labels map[string]string
Labels labels.Labels
}

func (m *Metadata) Validate() error {
if m == nil {
return errors.InvalidArgument(EntityResource, "metadata is nil")
}

if m.Labels != nil {
if err := m.Labels.Validate(); err != nil {
msg := fmt.Sprintf("labels is invalid: %v", err)
return errors.InvalidArgument(EntityResource, msg)
}
}

return nil
}

type Name string
Expand Down Expand Up @@ -58,8 +75,9 @@ func NewResource(fullName, kind string, store Store, tnnt tenant.Tenant, meta *M
return nil, errors.InvalidArgument(EntityResource, "empty resource spec for "+fullName)
}

if meta == nil {
return nil, errors.InvalidArgument(EntityResource, "empty resource metadata for "+fullName)
if err := meta.Validate(); err != nil {
msg := fmt.Sprintf("metadata for %s is invalid: %v", fullName, err)
return nil, errors.InvalidArgument(EntityResource, msg)
}

return &Resource{
Expand Down
54 changes: 53 additions & 1 deletion core/resource/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,60 @@ import (

"github.com/goto/optimus/core/resource"
"github.com/goto/optimus/core/tenant"
"github.com/goto/optimus/internal/lib/labels"
)

func TestMetadata(t *testing.T) {
t.Run("Validate", func(t *testing.T) {
t.Run("should return error if metadata is nil", func(t *testing.T) {
var meta *resource.Metadata

actualError := meta.Validate()

assert.ErrorContains(t, actualError, "metadata is nil")
})

t.Run("should return error if labels is invalid", func(t *testing.T) {
meta := &resource.Metadata{
Version: 1,
Description: "Description for testing",
Labels: labels.Labels{
"test_key": "",
},
}

actualError := meta.Validate()

assert.ErrorContains(t, actualError, "labels is invalid")
})

t.Run("should return nil if labels is not specified", func(t *testing.T) {
meta := &resource.Metadata{
Version: 1,
Description: "Description for testing",
}

actualError := meta.Validate()

assert.NoError(t, actualError)
})

t.Run("should return nil if metadata is valid", func(t *testing.T) {
meta := &resource.Metadata{
Version: 1,
Description: "Description for testing",
Labels: labels.Labels{
"test_key": "test_value",
},
}

actualError := meta.Validate()

assert.NoError(t, actualError)
})
})
}

func TestName(t *testing.T) {
t.Run("NameFrom", func(t *testing.T) {
t.Run("returns empty and error if name is empty", func(t *testing.T) {
Expand Down Expand Up @@ -45,7 +97,7 @@ func TestNewResource(t *testing.T) {
spec := map[string]any{"a": "b"}
_, err := resource.NewResource("proj.set.res_name", "table", resource.Bigquery, tnnt, nil, spec)
assert.NotNil(t, err)
assert.EqualError(t, err, "invalid argument for entity resource: empty resource metadata for proj.set.res_name")
assert.ErrorContains(t, err, "metadata for proj.set.res_name is invalid")
})
t.Run("returns error when invalid resource metadata", func(t *testing.T) {
meta := resource.Metadata{
Expand Down
99 changes: 99 additions & 0 deletions internal/lib/labels/labels.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package labels

import (
"errors"
"fmt"
"regexp"
)

const (
maxLabelsLength = 32
maxLabelsKVLength = 63
)

var errNilLabels = errors.New("labels is nil")

var (
kvInvalidChar = regexp.MustCompile(`[^a-z0-9\-_]+`)
kvValidStartEndChar = regexp.MustCompile(`^[a-z0-9].*[a-z0-9]$`)
)

type Labels map[string]string

func (l Labels) Validate() error {
if l == nil {
return errNilLabels
}

if len(l) > maxLabelsLength {
return fmt.Errorf("labels length is more than [%d]", maxLabelsLength)
}

for key, value := range l {
if err := l.validateKey(key); err != nil {
return err
}

if err := l.validateValue(value); err != nil {
return fmt.Errorf("error validating value for key [%s]: %w", key, err)
}
}

return nil
}

func (l Labels) validateValue(value string) error {
if value == "" {
return errors.New("value is empty")
}

if len(value) > maxLabelsKVLength {
return fmt.Errorf("value length is more than [%d]", maxLabelsKVLength)
}

if l.kvContainsInvalidChar(value) {
return errors.New("value should only be combination of lower case letters, numerics, underscores, and/or dashes")
}

if !l.kvContainsValidStartAndEndChar(value) {
return errors.New("value should start and end with alphanumerics only")
}

return nil
}

func (l Labels) validateKey(key string) error {
if key == "" {
return errors.New("key is empty")
}

if len(key) > maxLabelsKVLength {
return fmt.Errorf("key length is more than [%d]", maxLabelsKVLength)
}

if l.kvContainsInvalidChar(key) {
return errors.New("key should only be combination of lower case letters, numerics, underscores, and/or dashes")
}

if !l.kvContainsValidStartAndEndChar(key) {
return errors.New("key should start and end with alphanumerics only")
}

return nil
}

func (Labels) kvContainsValidStartAndEndChar(s string) bool {
return kvValidStartEndChar.MatchString(s)
}

func (Labels) kvContainsInvalidChar(s string) bool {
return kvInvalidChar.MatchString(s)
}

func FromMap(incoming map[string]string) Labels {
if incoming == nil {
return make(map[string]string)
}

return Labels(incoming)
}
Loading

0 comments on commit ee6adc6

Please sign in to comment.