Skip to content

Commit

Permalink
Add validation for overlapping rules (#242)
Browse files Browse the repository at this point in the history 
* Add overlapping rules validation

* Address PR comments
  • Loading branch information
@terryyylim
terryyylim committed Sep 18, 2022
1 parent 75bf168 commit 93a16cb
Show file tree
Hide file tree
Showing 4 changed files with 271 additions and 1 deletion.
1 change: 1 addition & 0 deletions api/go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ require (
github.com/gojek/fiber v0.0.0-20201008181849-4f0f8284dc84
github.com/gojek/merlin v0.0.0
github.com/gojek/mlp v1.4.7
github.com/golang-collections/collections v0.0.0-20130729185459-604e922904d3
github.com/golang-migrate/migrate/v4 v4.11.0
github.com/google/go-cmp v0.5.7
github.com/google/go-containerregistry v0.8.1-0.20220219142810-1571d7fdc46e
Expand Down
2 changes: 2 additions & 0 deletions api/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,8 @@ github.com/gojek/mlp v0.0.0-20201002030420-4e35e69a9ab8/go.mod h1:IjQCiAzap7TZRZ
github.com/gojek/mlp v1.4.7 h1:GxqqI0rQxUF4pexCk3aHsd5Mu4o4OnnsbGk+MTnW26Y=
github.com/gojek/mlp v1.4.7/go.mod h1:3+/YMTxQenxYSvKzOiEio+UBXPheJ4+/5c1MkbYoLwc=
github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf/go.mod h1:QzhUKaYKJmcbTnCYCAVQrroCOY7vOOI8cSQ4NbuhYf0=
github.com/golang-collections/collections v0.0.0-20130729185459-604e922904d3 h1:zN2lZNZRflqFyxVaTIU61KNKQ9C0055u9CAfpmqUvo4=
github.com/golang-collections/collections v0.0.0-20130729185459-604e922904d3/go.mod h1:nPpo7qLxd6XL3hWJG/O60sR8ZKfMCiIoNap5GvD12KU=
github.com/golang-migrate/migrate/v4 v4.11.0 h1:uqtd0ysK5WyBQ/T1K2uDIooJV0o2Obt6uPwP062DupQ=
github.com/golang-migrate/migrate/v4 v4.11.0/go.mod h1:nqbpDbckcYjsCD5I8q5+NI9Tkk7SVcmaF40Ax1eAWhg=
github.com/golang-sql/civil v0.0.0-20190719163853-cb61b32ac6fe h1:lXe2qZdvpiX5WZkZR4hgp4KJVfY3nMkvmwbVkpv1rVY=
Expand Down
74 changes: 73 additions & 1 deletion api/turing/validation/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,77 @@ func checkDanglingRoutes(
}
}

func validateConditionOrthogonality(
sl validator.StructLevel,
fieldName string,
allRules models.TrafficRules,
) {
trafficConditionsValueMap := map[string]map[string]*set.Set{}
for _, rule := range allRules {
trafficConditionsValueMap[rule.Name] = map[string]*set.Set{}
for _, condition := range rule.Conditions {
conditionValues := []interface{}{}
for _, val := range condition.Values {
conditionValues = append(conditionValues, val)
}
trafficConditionsValueMap[rule.Name][condition.Field] = set.New(conditionValues...)
}
}

allErrors := []string{}
for idx, rule1 := range allRules {
rule1Fields, ok := trafficConditionsValueMap[rule1.Name]
// Check that there are rule conditions
if ok {
// Check that the current rule and the other are orthogonal
rulesOverlap := true

for _, rule2 := range allRules[idx+1:] {
// Rules with same name should be skipped for orthogonality checks since they will fail unique name validation
if rule1.Name == rule2.Name {
continue
}
for field, currSet := range rule1Fields {
isCurrValEmpty, isOtherValEmpty := false, false
if !ok || currSet == nil || currSet.Len() == 0 {
isCurrValEmpty = true
}
otherSet, ok := trafficConditionsValueMap[rule2.Name][field]
if !ok || otherSet == nil || otherSet.Len() == 0 {
isOtherValEmpty = true
}

// If both values non-empty, check overlap.
// If only one of the values is empty, we can skip further checks.
// If both empty, nothing to do.
if !isCurrValEmpty && !isOtherValEmpty {
if currSet.Intersection(otherSet).Len() == 0 {
// At least one field value does not overlap, we can terminate the check for
// this other rule.
rulesOverlap = false
break
}
} else if !isCurrValEmpty || !isOtherValEmpty {
rulesOverlap = false
break
}
}

if rulesOverlap {
allErrors = append(allErrors, fmt.Sprintf("(%s,%s)", rule1.Name, rule2.Name))
}
}
}
}

if len(allErrors) != 0 {
errorMessage := fmt.Sprintf(
"Rules Orthogonality check failed, following pairs of rules are overlapping - %s.", strings.Join(allErrors, ", "),
)
sl.ReportError(allRules, fieldName, "TrafficRules", errorMessage, "")
}
}

func validateRouterConfig(sl validator.StructLevel) {
routerConfig := sl.Current().Interface().(request.RouterConfig)
instance := sl.Validator()
Expand Down Expand Up @@ -252,8 +323,9 @@ func validateRouterConfig(sl validator.StructLevel) {
}
}

// Validate dangling routes
// Validate dangling routes and traffic rules orthogonality checks
if routerConfig.TrafficRules != nil && len(routerConfig.TrafficRules) > 0 {
checkDanglingRoutes(sl, "Routes", routerConfig.Routes, allRuleRoutesSet)
validateConditionOrthogonality(sl, "TrafficRules", routerConfig.TrafficRules)
}
}
195 changes: 195 additions & 0 deletions api/turing/validation/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,85 @@ func TestValidateTrafficRules(t *testing.T) {
},
},
},
"success | valid complex Traffic Rules": {
routes: models.Routes{routeA, routeB, routeC},
defaultRouteID: &routeAID,
defaultTrafficRule: defaultTrafficRule,
trafficRules: models.TrafficRules{
{
Name: "rule-a",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"ID"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeBID},
},
{
Name: "rule-b",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"SG"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-b"},
},
},
Routes: []string{routeAID, routeBID},
},
{
Name: "rule-c",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"VN"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeCID},
},
{
Name: "rule-d",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"SG"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-c"},
},
},
Routes: []string{routeAID, routeBID},
},
},
},
"failure | empty conditions": {
routes: models.Routes{routeA, routeB},
defaultRouteID: &routeAID,
Expand Down Expand Up @@ -645,6 +724,122 @@ func TestValidateTrafficRules(t *testing.T) {
"'Fallback Route (DefaultRouteId): 'route-a' should be associated to all Traffic Rules' tag",
}, " "),
},
"failure | Overlapping Traffic Rules": {
routes: models.Routes{routeA, routeB, routeC},
defaultRouteID: &routeAID,
defaultTrafficRule: defaultTrafficRule,
trafficRules: models.TrafficRules{
{
Name: "rule-a",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeBID},
},
{
Name: "rule-c",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeCID},
},
},
expectedError: "Key: 'RouterConfig.TrafficRules' Error:Field validation for 'TrafficRules' " +
"failed on the 'Rules Orthogonality check failed, following pairs of rules are overlapping - " +
"(rule-a,rule-c).' tag",
},
"failure | Complex overlapping Traffic Rules": {
routes: models.Routes{routeA, routeB, routeC},
defaultRouteID: &routeAID,
defaultTrafficRule: defaultTrafficRule,
trafficRules: models.TrafficRules{
{
Name: "rule-a",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"ID"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeBID},
},
{
Name: "rule-b",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"ID", "SG"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a", "region-b", "region-c"},
},
},
Routes: []string{routeAID, routeBID},
},
{
Name: "rule-c",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"SG"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-a"},
},
},
Routes: []string{routeAID, routeCID},
},
{
Name: "rule-d",
Conditions: []*router.TrafficRuleCondition{
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Country",
Operator: router.InConditionOperator,
Values: []string{"ID"},
},
{
FieldSource: expRequest.HeaderFieldSource,
Field: "X-Region",
Operator: router.InConditionOperator,
Values: []string{"region-b"},
},
},
Routes: []string{routeAID, routeBID},
},
},
expectedError: "Key: 'RouterConfig.TrafficRules' Error:Field validation for 'TrafficRules' " +
"failed on the 'Rules Orthogonality check failed, following pairs of rules are overlapping - " +
"(rule-a,rule-b), (rule-b,rule-c), (rule-b,rule-d).' tag",
},
}

for name, tt := range suite {
Expand Down

0 comments on commit 93a16cb

Please sign in to comment.