Skip to content

Commit

Permalink
Fix manifest validation
Browse files Browse the repository at this point in the history 
Test for e.g. app memory > 0 were failing for the wrong reason. 0 fails
to be parsed as an amount with a unit by the bytefmt library as it
doesn't contain a byte unit.

Adding tests for missing units showed the errors message could be
improved by splitting validation into that for having a unit and that
for being a positive amount.

go-playground/validator doesn't allow us to give the true path to the
erroneous value unfortunately. E.g. the cf on vms errors would be along
the lines of `For application 'test-app': Process "web": Disk quota must
use a supported unit: B, K, KB, M, MB, G, GB, T, or TB`. So best effort
on field identification for now, until we can rethink the validation
code.
  • Loading branch information
Kieron Browne committed Jan 30, 2023
1 parent 114fe00 commit 8135b70
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 35 deletions.
45 changes: 40 additions & 5 deletions api/handlers/shared.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,12 @@ func wireValidator() (*validator.Validate, ut.Translator, error) {
return nil, nil, err
}
// Register custom validators
err = v.RegisterValidation("megabytestring", megabyteFormattedString, true)
err = v.RegisterValidation("amountWithUnit", amountWithUnit, true)
if err != nil {
return nil, nil, err
}

err = v.RegisterValidation("positiveAmountWithUnit", positiveAmountWithUnit, true)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -140,6 +145,26 @@ func wireValidator() (*validator.Validate, ut.Translator, error) {

v.RegisterStructValidation(checkRoleTypeAndOrgSpace, payloads.RoleCreate{})

err = v.RegisterTranslation("amountWithUnit", trans, func(ut ut.Translator) error {
return ut.Add("amountWithUnit", "{0} must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB", false)
}, func(ut ut.Translator, fe validator.FieldError) string {
t, _ := ut.T("amountWithUnit", fe.Field())
return t
})
if err != nil {
return nil, nil, err
}

err = v.RegisterTranslation("positiveAmountWithUnit", trans, func(ut ut.Translator) error {
return ut.Add("positiveAmountWithUnit", "{0} must be greater than 0MB", false)
}, func(ut ut.Translator, fe validator.FieldError) string {
t, _ := ut.T("positiveAmountWithUnit", fe.Field())
return t
})
if err != nil {
return nil, nil, err
}

err = v.RegisterTranslation("cannot_have_both_org_and_space_set", trans, func(ut ut.Translator) error {
return ut.Add("cannot_have_both_org_and_space_set", "Cannot pass both 'organization' and 'space' in a create role request", false)
}, func(ut ut.Translator, fe validator.FieldError) string {
Expand Down Expand Up @@ -262,15 +287,25 @@ func checkRoleTypeAndOrgSpace(sl validator.StructLevel) {
}
}

// Custom field validators
func megabyteFormattedString(fl validator.FieldLevel) bool {
var unitAmount = regexp.MustCompile(`^\d+(?:B|K|KB|M|MB|G|GB|T|TB)$`)

func amountWithUnit(fl validator.FieldLevel) bool {
val, ok := fl.Field().Interface().(string)
if !ok {
return true // the value is optional, and is set to nil
}

return unitAmount.MatchString(val)
}

func positiveAmountWithUnit(fl validator.FieldLevel) bool {
val, ok := fl.Field().Interface().(string)
if !ok {
return true // the value is optional, and is set to nil
}

_, err := bytefmt.ToMegabytes(val)
return err == nil
mbs, err := bytefmt.ToMegabytes(val)
return err == nil && mbs > 0
}

func routeString(fl validator.FieldLevel) bool {
Expand Down
68 changes: 45 additions & 23 deletions api/handlers/space_manifest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,40 +128,36 @@ var _ = Describe("SpaceManifest", func() {
})
})

When("the manifest contains multiple apps", func() {
When("the application name is missing", func() {
BeforeEach(func() {
requestBody = strings.NewReader(`---
version: 1
applications:
- name: app1
- name: app2
- {}
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Applications must contain at maximum 1 item")
expectUnprocessableEntityError("Name is a required field")
})

It("doesn't call applyManifestAction", func() {
Expect(manifestApplier.ApplyCallCount()).To(Equal(0))
})
})

When("the application name is missing", func() {
When("the application memory does not have a unit", func() {
BeforeEach(func() {
requestBody = strings.NewReader(`---
version: 1
applications:
- {}
- name: test-app
memory: 1024
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Name is a required field")
})

It("doesn't call applyManifestAction", func() {
Expect(manifestApplier.ApplyCallCount()).To(Equal(0))
expectUnprocessableEntityError("Memory must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB")
})
})

Expand All @@ -171,16 +167,12 @@ var _ = Describe("SpaceManifest", func() {
version: 1
applications:
- name: test-app
memory: 0
memory: 0M
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Memory' Error:Field validation for 'Memory' failed on the 'megabytestring' tag")
})

It("doesn't call applyManifestAction", func() {
Expect(manifestApplier.ApplyCallCount()).To(Equal(0))
expectUnprocessableEntityError("Memory must be greater than 0MB")
})
})

Expand Down Expand Up @@ -225,6 +217,23 @@ var _ = Describe("SpaceManifest", func() {
})
})

When("the application process disk doesn't supply a unit", func() {
BeforeEach(func() {
requestBody = strings.NewReader(`---
version: 1
applications:
- name: test-app
processes:
- type: web
disk_quota: 1024
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("DiskQuota must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB")
})
})

When("the application process disk is not a positive integer", func() {
BeforeEach(func() {
requestBody = strings.NewReader(`---
Expand All @@ -233,16 +242,29 @@ var _ = Describe("SpaceManifest", func() {
- name: test-app
processes:
- type: web
disk_quota: 0
disk_quota: 0M
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Processes[0].DiskQuota' Error:Field validation for 'DiskQuota' failed on the 'megabytestring' tag")
expectUnprocessableEntityError("DiskQuota must be greater than 0MB")
})
})

It("doesn't call applyManifestAction", func() {
Expect(manifestApplier.ApplyCallCount()).To(Equal(0))
When("the application process memory doesn't supply a unit", func() {
BeforeEach(func() {
requestBody = strings.NewReader(`---
version: 1
applications:
- name: test-app
processes:
- type: web
memory: 1024
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Memory must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB")
})
})

Expand All @@ -254,12 +276,12 @@ var _ = Describe("SpaceManifest", func() {
- name: test-app
processes:
- type: web
memory: 0
memory: 0GB
`)
})

It("response with an unprocessable entity error", func() {
expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Processes[0].Memory' Error:Field validation for 'Memory' failed on the 'megabytestring' tag")
expectUnprocessableEntityError("Memory must be greater than 0MB")
})

It("doesn't call applyManifestAction", func() {
Expand Down
14 changes: 7 additions & 7 deletions api/payloads/manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

type Manifest struct {
Version int `yaml:"version"`
Applications []ManifestApplication `yaml:"applications"`
Applications []ManifestApplication `yaml:"applications" validate:"dive"`
}

type ManifestApplication struct {
Expand All @@ -21,13 +21,13 @@ type ManifestApplication struct {
NoRoute bool `yaml:"no-route"`
Command *string `yaml:"command"`
Instances *int `yaml:"instances" validate:"omitempty,gte=0"`
Memory *string `yaml:"memory" validate:"megabytestring"`
DiskQuota *string `yaml:"disk_quota" validate:"megabytestring"`
Memory *string `yaml:"memory" validate:"amountWithUnit,positiveAmountWithUnit"`
DiskQuota *string `yaml:"disk_quota" validate:"amountWithUnit,positiveAmountWithUnit"`
// AltDiskQuota supports `disk-quota` with a hyphen for backwards compatibility.
// Do not set both DiskQuota and AltDiskQuota.
//
// Deprecated: Use DiskQuota instead
AltDiskQuota *string `yaml:"disk-quota" validate:"megabytestring"`
AltDiskQuota *string `yaml:"disk-quota" validate:"amountWithUnit,positiveAmountWithUnit"`
HealthCheckHTTPEndpoint *string `yaml:"health-check-http-endpoint"`
HealthCheckInvocationTimeout *int64 `yaml:"health-check-invocation-timeout" validate:"omitempty,gte=1"`
HealthCheckType *string `yaml:"health-check-type" validate:"omitempty,oneof=none process port http"`
Expand All @@ -42,17 +42,17 @@ type ManifestApplication struct {
type ManifestApplicationProcess struct {
Type string `yaml:"type" validate:"required"`
Command *string `yaml:"command"`
DiskQuota *string `yaml:"disk_quota" validate:"megabytestring"`
DiskQuota *string `yaml:"disk_quota" validate:"amountWithUnit,positiveAmountWithUnit"`
// AltDiskQuota supports `disk-quota` with a hyphen for backwards compatibility.
// Do not set both DiskQuota and AltDiskQuota.
//
// Deprecated: Use DiskQuota instead
AltDiskQuota *string `yaml:"disk-quota" validate:"megabytestring"`
AltDiskQuota *string `yaml:"disk-quota" validate:"amountWithUnit,positiveAmountWithUnit"`
HealthCheckHTTPEndpoint *string `yaml:"health-check-http-endpoint"`
HealthCheckInvocationTimeout *int64 `yaml:"health-check-invocation-timeout" validate:"omitempty,gte=1"`
HealthCheckType *string `yaml:"health-check-type" validate:"omitempty,oneof=none process port http"`
Instances *int `yaml:"instances" validate:"omitempty,gte=0"`
Memory *string `yaml:"memory" validate:"megabytestring"`
Memory *string `yaml:"memory" validate:"amountWithUnit,positiveAmountWithUnit"`
Timeout *int64 `yaml:"timeout" validate:"omitempty,gte=1"`
}

Expand Down

0 comments on commit 8135b70

Please sign in to comment.