From ee6adc68d13f83c0ecc58e625d55427fbdca6da7 Mon Sep 17 00:00:00 2001 From: Anwar Hidayat <15167551+irainia@users.noreply.github.com> Date: Fri, 17 Nov 2023 14:08:23 +0700 Subject: [PATCH] feat: add label spec validation (#181) * 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 --- core/job/handler/v1beta1/job_adapter.go | 6 +- core/job/spec.go | 23 +-- core/job/spec_test.go | 32 ++-- core/resource/resource.go | 24 ++- core/resource/resource_test.go | 54 ++++++- internal/lib/labels/labels.go | 99 ++++++++++++ internal/lib/labels/labels_test.go | 202 ++++++++++++++++++++++++ 7 files changed, 411 insertions(+), 29 deletions(-) create mode 100644 internal/lib/labels/labels.go create mode 100644 internal/lib/labels/labels_test.go diff --git a/core/job/handler/v1beta1/job_adapter.go b/core/job/handler/v1beta1/job_adapter.go index ce92a759ba..07b9737714 100644 --- a/core/job/handler/v1beta1/job_adapter.go +++ b/core/job/handler/v1beta1/job_adapter.go @@ -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" @@ -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) } diff --git a/core/job/spec.go b/core/job/spec.go index a620ce7944..e0dea0ae8c 100644 --- a/core/job/spec.go +++ b/core/job/spec.go @@ -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" ) @@ -24,7 +25,7 @@ type Spec struct { task Task description string - labels map[string]string + labels labels.Labels metadata *Metadata hooks []*Hook asset Asset @@ -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 } @@ -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 } @@ -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 } @@ -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 { diff --git a/core/job/spec_test.go b/core/job/spec_test.go index 75d48a8a0a..de4b9b6e1c 100644 --- a/core/job/spec_test.go +++ b/core/job/spec_test.go @@ -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" ) @@ -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"}) @@ -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). @@ -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()) @@ -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) { @@ -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) - }) - }) } diff --git a/core/resource/resource.go b/core/resource/resource.go index 9ba6cb49d2..1aa03fd220 100644 --- a/core/resource/resource.go +++ b/core/resource/resource.go @@ -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 ( @@ -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 @@ -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{ diff --git a/core/resource/resource_test.go b/core/resource/resource_test.go index b79f539b38..92f345b460 100644 --- a/core/resource/resource_test.go +++ b/core/resource/resource_test.go @@ -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) { @@ -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{ diff --git a/internal/lib/labels/labels.go b/internal/lib/labels/labels.go new file mode 100644 index 0000000000..b9b23b28c2 --- /dev/null +++ b/internal/lib/labels/labels.go @@ -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) +} diff --git a/internal/lib/labels/labels_test.go b/internal/lib/labels/labels_test.go new file mode 100644 index 0000000000..c9faec0f72 --- /dev/null +++ b/internal/lib/labels/labels_test.go @@ -0,0 +1,202 @@ +package labels_test + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/goto/optimus/internal/lib/labels" +) + +func TestFromMap(t *testing.T) { + t.Run("should return empty if incoming labels are nil", func(t *testing.T) { + var incoming map[string]string + + actualLabels := labels.FromMap(incoming) + + assert.NotNil(t, actualLabels) + assert.Empty(t, actualLabels) + }) + + t.Run("should return labels and nil if no error is encountered", func(t *testing.T) { + incoming := map[string]string{ + "key1": "value1", + } + + actualLabels := labels.FromMap(incoming) + + assert.NotEmpty(t, actualLabels) + }) +} + +func TestLabels(t *testing.T) { + t.Run("should return error if labels is nil", func(t *testing.T) { + var incoming labels.Labels + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "labels is nil") + }) + + t.Run("should return error if labels is more than 32 in length", func(t *testing.T) { + l := make(map[string]string) + for i := 0; i < 33; i++ { + key := fmt.Sprintf("key-%d", i) + value := fmt.Sprintf("value-%d", i) + l[key] = value + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "labels length is more than [32]") + }) + + t.Run("should return error if labels key is empty", func(t *testing.T) { + l := map[string]string{ + "": "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key is empty") + }) + + t.Run("should return error if labels key length is more than 63", func(t *testing.T) { + l := map[string]string{ + strings.Repeat("a", 64): "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key length is more than [63]") + }) + + t.Run("should return error if labels key contains characters outside alphanumerics, underscores, and dashes", func(t *testing.T) { + l := map[string]string{ + "invalid_key_with_!": "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key should only be combination of lower case letters, numerics, underscores, and/or dashes") + }) + + t.Run("should return error if labels key contains upper case letters", func(t *testing.T) { + l := map[string]string{ + "invalid_key_with_A": "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key should only be combination of lower case letters, numerics, underscores, and/or dashes") + }) + + t.Run("should return error if labels key does not start with alphanumeric", func(t *testing.T) { + l := map[string]string{ + "-invalid_key": "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key should start and end with alphanumerics only") + }) + + t.Run("should return error if labels key does not end with alphanumeric", func(t *testing.T) { + l := map[string]string{ + "invalid_key-": "test_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "key should start and end with alphanumerics only") + }) + + t.Run("should return error if labels value is empty", func(t *testing.T) { + l := map[string]string{ + "test_key": "", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value is empty") + }) + + t.Run("should return error if labels value length is more than 63", func(t *testing.T) { + l := map[string]string{ + "test_key": strings.Repeat("a", 64), + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value length is more than [63]") + }) + + t.Run("should return error if labels value contains characters outside alphanumerics, underscores, and dashes", func(t *testing.T) { + l := map[string]string{ + "test_key": "invalid_value_with_!", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value should only be combination of lower case letters, numerics, underscores, and/or dashes") + }) + + t.Run("should return error if labels value contains upper case letters", func(t *testing.T) { + l := map[string]string{ + "test_key": "invalid_value_with_A", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value should only be combination of lower case letters, numerics, underscores, and/or dashes") + }) + + t.Run("should return error if labels value does not start with alphanumeric", func(t *testing.T) { + l := map[string]string{ + "test_key": "-invalid_value", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value should start and end with alphanumerics only") + }) + + t.Run("should return error if labels value does not end with alphanumeric", func(t *testing.T) { + l := map[string]string{ + "test_key": "invalid_value-", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.ErrorContains(t, actualError, "value should start and end with alphanumerics only") + }) + + t.Run("should return nil if 'labels' follow valid spec", func(t *testing.T) { + l := map[string]string{ + "test_key": "test_value", + "id": "abcd-efgh-ijkl-mnop", + "resource-name": "1_resource_test", + "job-name": "job_test_1", + } + incoming := labels.FromMap(l) + + actualError := incoming.Validate() + + assert.NoError(t, actualError) + }) +}