Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(manifest): add manifest validation boilerplate #2815

Merged
merged 14 commits into from
Sep 10, 2021
Merged

chore(manifest): add manifest validation boilerplate #2815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f651188
chore(manifest): add manifest validation boilerplate
iamhopaul123 Sep 8, 2021
af8f84e
Generalize error type
iamhopaul123 Sep 9, 2021
b51f67f
Make all receiver pointer to avoid unnecessary copy
iamhopaul123 Sep 9, 2021
3cc0a90
Merge branch 'mainline' into add-manifest-validate-boilerplate
iamhopaul123 Sep 9, 2021
a20f0e6
Resolve conflict
iamhopaul123 Sep 9, 2021
88dff5f
Add unit tests for each Validate function that throws or wraps errors
iamhopaul123 Sep 9, 2021
2944cc2
Add gid/uid validation and refactor errFieldMutualExclusive
iamhopaul123 Sep 10, 2021
d27eeb6
Merge branch 'mainline' into add-manifest-validate-boilerplate
iamhopaul123 Sep 10, 2021
042e95e
Edit errFieldMutualExclusive msg
iamhopaul123 Sep 10, 2021
54e8a52
Merge branch 'mainline' into add-manifest-validate-boilerplate
iamhopaul123 Sep 10, 2021
9cc2ec9
Resolve conflict
iamhopaul123 Sep 10, 2021
dc44317
Check if the struct is set before doing validations
iamhopaul123 Sep 10, 2021
173bddc
Address comments
iamhopaul123 Sep 10, 2021
33529ac
min/max should be required and auth.IAM default to true
iamhopaul123 Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions internal/pkg/manifest/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package manifest

import (
"fmt"

"github.com/dustin/go-humanize/english"
)

// ErrInvalidWorkloadType occurs when a user requested a manifest template type that doesn't exist.
Expand Down Expand Up @@ -50,3 +52,39 @@ func (e *ErrUnknownProvider) Is(target error) bool {
_, ok := target.(*ErrUnknownProvider)
return ok
}

type errFieldMustBeSpecified struct {
missingField string
conditionalFields []string
}

func (e *errFieldMustBeSpecified) Error() string {
errMsg := fmt.Sprintf(`"%s" must be specified`, e.missingField)
if len(e.conditionalFields) == 0 {
return errMsg
}
return fmt.Sprintf(`%s if "%s" %s specified`, errMsg, english.WordSeries(e.conditionalFields, "or"),
english.PluralWord(len(e.conditionalFields), "is", "are"))
}

type errFieldMutualExclusive struct {
firstField string
secondField string
mustExist bool
}

func (e *errFieldMutualExclusive) Error() string {
if e.mustExist {
return fmt.Sprintf(`must specify one of "%s" and "%s"`, e.firstField, e.secondField)
}
return fmt.Sprintf(`must specify one, not both, of "%s" and "%s"`, e.firstField, e.secondField)
}

type errMinGreaterThanMax struct {
min int
max int
}

func (e *errMinGreaterThanMax) Error() string {
return fmt.Sprintf("min value %d cannot be greater than max value %d", e.min, e.max)
}
iamhopaul123 marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 5 additions & 3 deletions internal/pkg/manifest/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import (
)

var (
errUnmarshalEFSOpts = errors.New(`cannot unmarshal efs field into bool or map`)
errInvalidEFSConfiguration = errors.New(`must specify one, not both, of "uid/gid" and "id/root_dir/auth"`)
errUnmarshalEFSOpts = errors.New(`cannot unmarshal "efs" field into bool or map`)
)

// Storage represents the options for external and native storage.
Expand Down Expand Up @@ -154,7 +153,10 @@ func (e *EFSVolumeConfiguration) unsetUIDConfig() {

func (e *EFSVolumeConfiguration) isValid() error {
if !e.EmptyBYOConfig() && !e.EmptyUIDConfig() {
return errInvalidEFSConfiguration
return &errFieldMutualExclusive{
firstField: "uid/gid",
secondField: "id/root_dir/auth",
Comment on lines +157 to +158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do think we should take a list for the inputs instead?
"cannot specify uid, gid with any of id, root_dir, auth"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think I still prefer having
must specify one, not both, of "uid/gid" and "id/root_dir/auth"
which makes the field stand out and more explicit to users? However, this does remind me that we need to make sure they specify both uid and gid.

}
}
return nil
}
Expand Down
21 changes: 11 additions & 10 deletions internal/pkg/manifest/svc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var ServiceTypes = []string{
WorkerServiceType,
}

// Range contains either a Range or a range configuration for Autoscaling ranges
// Range contains either a Range or a range configuration for Autoscaling ranges.
type Range struct {
Value *IntRangeBand // Mutually exclusive with RangeConfig
RangeConfig RangeConfig
Expand All @@ -44,17 +44,12 @@ func (r *Range) IsEmpty() bool {
return r.Value == nil && r.RangeConfig.IsEmpty()
}

// Parse extracts the min and max from RangeOpts
// Parse extracts the min and max from RangeOpts.
func (r *Range) Parse() (min int, max int, err error) {
if r.Value != nil && !r.RangeConfig.IsEmpty() {
return 0, 0, errInvalidRangeOpts
}

if r.Value != nil {
return r.Value.Parse()
}

return *r.RangeConfig.Min, *r.RangeConfig.Max, nil
return aws.IntValue(r.RangeConfig.Min), aws.IntValue(r.RangeConfig.Max), nil
}

// UnmarshalYAML overrides the default YAML unmarshaling logic for the RangeOpts
Expand Down Expand Up @@ -206,12 +201,18 @@ func (a *AdvancedCount) hasAutoscaling() bool {
func (a *AdvancedCount) IsValid() error {
Lou1415926 marked this conversation as resolved.
Show resolved Hide resolved
// Spot translates to desiredCount; cannot specify with autoscaling
if a.Spot != nil && a.hasAutoscaling() {
return errInvalidAdvancedCount
return &errFieldMutualExclusive{
firstField: "spot",
secondField: "range/cpu_percentage/memory_percentage/requests/response_time",
}
}

// Range must be specified if using autoscaling
if a.Range.IsEmpty() && (a.CPU != nil || a.Memory != nil || a.Requests != nil || a.ResponseTime != nil) {
return errInvalidAutoscaling
return &errFieldMustBeSpecified{
missingField: "range",
conditionalFields: []string{"cpu_percentage", "memory_percentage", "requests", "response_time"},
}
}

return nil
Expand Down
52 changes: 31 additions & 21 deletions internal/pkg/manifest/svc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,13 +474,19 @@ func TestCount_UnmarshalYAML(t *testing.T) {
range: 1-10
spot: 3
`),
wantedError: errInvalidAdvancedCount,
wantedError: &errFieldMutualExclusive{
firstField: "spot",
secondField: "range/cpu_percentage/memory_percentage/requests/response_time",
},
},
"Error if autoscaling specified without range": {
inContent: []byte(`count:
cpu_percentage: 30
`),
wantedError: errInvalidAutoscaling,
wantedError: &errFieldMustBeSpecified{
missingField: "range",
conditionalFields: []string{"cpu_percentage", "memory_percentage", "requests", "response_time"},
},
},
"Error if unmarshalable": {
inContent: []byte(`count: badNumber
Expand Down Expand Up @@ -562,20 +568,16 @@ func TestRange_Parse(t *testing.T) {

wantedMin int
wantedMax int
wantedErr error
}{
"error when both range and RangeConfig specified": {
"success with range value": {
input: Range{
Value: &mockRange,
RangeConfig: RangeConfig{
Min: aws.Int(1),
Max: aws.Int(3),
},
},

wantedErr: errInvalidRangeOpts,
wantedMin: 1,
wantedMax: 10,
},
"success": {
"success with range config": {
input: Range{
RangeConfig: RangeConfig{
Min: aws.Int(2),
Expand All @@ -591,13 +593,9 @@ func TestRange_Parse(t *testing.T) {
t.Run(name, func(t *testing.T) {
gotMin, gotMax, err := tc.input.Parse()

if tc.wantedErr != nil {
require.EqualError(t, err, tc.wantedErr.Error())
} else {
require.NoError(t, err)
require.Equal(t, tc.wantedMin, gotMin)
require.Equal(t, tc.wantedMax, gotMax)
}
require.NoError(t, err)
require.Equal(t, tc.wantedMin, gotMin)
require.Equal(t, tc.wantedMax, gotMax)
})
}
}
Expand Down Expand Up @@ -814,7 +812,10 @@ func TestAdvancedCount_IsValid(t *testing.T) {
Requests: aws.Int(1000),
},

expectedErr: errInvalidAdvancedCount,
expectedErr: &errFieldMutualExclusive{
firstField: "spot",
secondField: "range/cpu_percentage/memory_percentage/requests/response_time",
},
},
"invalid with spot count and range": {
input: &AdvancedCount{
Expand All @@ -824,7 +825,10 @@ func TestAdvancedCount_IsValid(t *testing.T) {
},
},

expectedErr: errInvalidAdvancedCount,
expectedErr: &errFieldMutualExclusive{
firstField: "spot",
secondField: "range/cpu_percentage/memory_percentage/requests/response_time",
},
},
"invalid with spot count and range config": {
input: &AdvancedCount{
Expand All @@ -838,7 +842,10 @@ func TestAdvancedCount_IsValid(t *testing.T) {
},
},

expectedErr: errInvalidAdvancedCount,
expectedErr: &errFieldMutualExclusive{
firstField: "spot",
secondField: "range/cpu_percentage/memory_percentage/requests/response_time",
},
},
"invalid with autoscaling fields and no range": {
input: &AdvancedCount{
Expand All @@ -847,7 +854,10 @@ func TestAdvancedCount_IsValid(t *testing.T) {
Requests: aws.Int(1000),
},

expectedErr: errInvalidAutoscaling,
expectedErr: &errFieldMustBeSpecified{
missingField: "range",
conditionalFields: []string{"cpu_percentage", "memory_percentage", "requests", "response_time"},
},
},
}
for name, tc := range testCases {
Expand Down
Loading