Skip to content

Commit

Permalink
Merge #39824
Browse files Browse the repository at this point in the history 
39824: sql: Fix bug with negative parsing validation for zone constraints r=solongordon a=rohany

Fixes #39801.

This PR fixes the parsing issues brought up in #39801, and refactors the test cases for zone validation to specifically test this behavior. Previously this behavior was tested, but falsely passed due to a logic error.

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
  • Loading branch information
@rohany
craig[bot] and rohany committed Aug 29, 2019
2 parents 9baf0f1 + a3257c9 commit eac2276
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 37 deletions.
4 changes: 4 additions & 0 deletions pkg/sql/set_zone_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,10 @@ func validateZoneAttrsAndLocalities(

// Check that each constraint matches some store somewhere in the cluster.
for _, constraint := range toValidate {
// We skip validation for negative constraints. See the function-level comment.
if constraint.Type == config.Constraint_PROHIBITED {
continue
}
var found bool
node:
for _, node := range nodes.Nodes {
Expand Down
93 changes: 56 additions & 37 deletions pkg/sql/set_zone_config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,69 +87,88 @@ func TestValidateZoneAttrsAndLocalities(t *testing.T) {
},
}

nodes := &serverpb.NodesResponse{}
for _, store := range stores {
nodes.Nodes = append(nodes.Nodes, statuspb.NodeStatus{
genStatusFromStore := func(nodeAttrs []string, storeAttrs []string, storeLocality []roachpb.Tier) statuspb.NodeStatus {
return statuspb.NodeStatus{
StoreStatuses: []statuspb.StoreStatus{
{
Desc: roachpb.StoreDescriptor{
Attrs: roachpb.Attributes{
Attrs: store.storeAttrs,
Attrs: storeAttrs,
},
Node: roachpb.NodeDescriptor{
Attrs: roachpb.Attributes{
Attrs: store.nodeAttrs,
Attrs: nodeAttrs,
},
Locality: roachpb.Locality{
Tiers: store.storeLocality,
Tiers: storeLocality,
},
},
},
},
},
})
}
}

nodes := &serverpb.NodesResponse{}
for _, store := range stores {
nodes.Nodes = append(nodes.Nodes, genStatusFromStore(store.nodeAttrs, store.storeAttrs, store.storeLocality))
}

// Different cluster settings to test validation behavior.
getNodes := func(_ context.Context, _ *serverpb.NodesRequest) (*serverpb.NodesResponse, error) {
return nodes, nil
}

// Regressions for negative constraint validation
singleAttrNode := func(_ context.Context, _ *serverpb.NodesRequest) (*serverpb.NodesResponse, error) {
nodes := &serverpb.NodesResponse{}
nodes.Nodes = append(nodes.Nodes, genStatusFromStore([]string{}, []string{"ssd"}, []roachpb.Tier{}))
return nodes, nil
}
singleLocalityNode := func(_ context.Context, _ *serverpb.NodesRequest) (*serverpb.NodesResponse, error) {
nodes := &serverpb.NodesResponse{}
nodes.Nodes = append(nodes.Nodes, genStatusFromStore([]string{}, []string{}, []roachpb.Tier{{Key: "region", Value: "us-east1"}}))
return nodes, nil
}

const expectSuccess = 0
const expectParseErr = 1
const expectValidateErr = 2
for i, tc := range []struct {
cfg string
expectErr int
nodes nodeGetter
}{
{`nonsense`, expectParseErr},
{`range_max_bytes: 100`, expectSuccess},
{`range_max_byte: 100`, expectParseErr},
{`constraints: ["+region=us-east1"]`, expectSuccess},
{`constraints: {"+region=us-east1": 2, "+region=eu-west1": 1}`, expectSuccess},
{`constraints: ["+region=us-eas1"]`, expectValidateErr},
{`constraints: {"+region=us-eas1": 2, "+region=eu-west1": 1}`, expectValidateErr},
{`constraints: {"+region=us-east1": 2, "+region=eu-wes1": 1}`, expectValidateErr},
{`constraints: ["+regio=us-east1"]`, expectValidateErr},
{`constraints: ["+rack=17"]`, expectSuccess},
{`constraints: ["+rack=18"]`, expectValidateErr},
{`constraints: ["+rach=17"]`, expectValidateErr},
{`constraints: ["+highcpu"]`, expectSuccess},
{`constraints: ["+lowmem"]`, expectSuccess},
{`constraints: ["+ssd"]`, expectSuccess},
{`constraints: ["+highcp"]`, expectValidateErr},
{`constraints: ["+owmem"]`, expectValidateErr},
{`constraints: ["+sssd"]`, expectValidateErr},
{`lease_preferences: [["+region=us-east1", "+ssd"], ["+geo=us", "+highcpu"]]`, expectSuccess},
{`lease_preferences: [["+region=us-eat1", "+ssd"], ["+geo=us", "+highcpu"]]`, expectValidateErr},
{`lease_preferences: [["+region=us-east1", "+foo"], ["+geo=us", "+highcpu"]]`, expectValidateErr},
{`lease_preferences: [["+region=us-east1", "+ssd"], ["+geo=us", "+bar"]]`, expectValidateErr},
{`constraints: ["-region=us-east1"]`, expectSuccess},
{`constraints: ["-regio=us-eas1"]`, expectSuccess},
{`constraints: {"-region=us-eas1": 2, "-region=eu-wes1": 1}`, expectSuccess},
{`constraints: ["-foo=bar"]`, expectSuccess},
{`constraints: ["-highcpu"]`, expectSuccess},
{`constraints: ["-ssd"]`, expectSuccess},
{`constraints: ["-fake"]`, expectSuccess},
{`nonsense`, expectParseErr, getNodes},
{`range_max_bytes: 100`, expectSuccess, getNodes},
{`range_max_byte: 100`, expectParseErr, getNodes},
{`constraints: ["+region=us-east1"]`, expectSuccess, getNodes},
{`constraints: {"+region=us-east1": 2, "+region=eu-west1": 1}`, expectSuccess, getNodes},
{`constraints: ["+region=us-eas1"]`, expectValidateErr, getNodes},
{`constraints: {"+region=us-eas1": 2, "+region=eu-west1": 1}`, expectValidateErr, getNodes},
{`constraints: {"+region=us-east1": 2, "+region=eu-wes1": 1}`, expectValidateErr, getNodes},
{`constraints: ["+regio=us-east1"]`, expectValidateErr, getNodes},
{`constraints: ["+rack=17"]`, expectSuccess, getNodes},
{`constraints: ["+rack=18"]`, expectValidateErr, getNodes},
{`constraints: ["+rach=17"]`, expectValidateErr, getNodes},
{`constraints: ["+highcpu"]`, expectSuccess, getNodes},
{`constraints: ["+lowmem"]`, expectSuccess, getNodes},
{`constraints: ["+ssd"]`, expectSuccess, getNodes},
{`constraints: ["+highcp"]`, expectValidateErr, getNodes},
{`constraints: ["+owmem"]`, expectValidateErr, getNodes},
{`constraints: ["+sssd"]`, expectValidateErr, getNodes},
{`lease_preferences: [["+region=us-east1", "+ssd"], ["+geo=us", "+highcpu"]]`, expectSuccess, getNodes},
{`lease_preferences: [["+region=us-eat1", "+ssd"], ["+geo=us", "+highcpu"]]`, expectValidateErr, getNodes},
{`lease_preferences: [["+region=us-east1", "+foo"], ["+geo=us", "+highcpu"]]`, expectValidateErr, getNodes},
{`lease_preferences: [["+region=us-east1", "+ssd"], ["+geo=us", "+bar"]]`, expectValidateErr, getNodes},
{`constraints: ["-region=us-east1"]`, expectSuccess, singleLocalityNode},
{`constraints: ["-ssd"]`, expectSuccess, singleAttrNode},
{`constraints: ["-regio=us-eas1"]`, expectSuccess, getNodes},
{`constraints: {"-region=us-eas1": 2, "-region=eu-wes1": 1}`, expectSuccess, getNodes},
{`constraints: ["-foo=bar"]`, expectSuccess, getNodes},
{`constraints: ["-highcpu"]`, expectSuccess, getNodes},
{`constraints: ["-ssd"]`, expectSuccess, getNodes},
{`constraints: ["-fake"]`, expectSuccess, getNodes},
} {
var zone config.ZoneConfig
err := yaml.UnmarshalStrict([]byte(tc.cfg), &zone)
Expand All @@ -159,7 +178,7 @@ func TestValidateZoneAttrsAndLocalities(t *testing.T) {
t.Fatalf("#%d: expected parse err for %q; got success", i, tc.cfg)
}

err = validateZoneAttrsAndLocalities(context.Background(), getNodes, &zone)
err = validateZoneAttrsAndLocalities(context.Background(), tc.nodes, &zone)
if err != nil && tc.expectErr == expectSuccess {
t.Errorf("#%d: expected success for %q; got %v", i, tc.cfg, err)
} else if err == nil && tc.expectErr == expectValidateErr {
Expand Down

0 comments on commit eac2276

Please sign in to comment.