From a3257c90d6a73e9d5dade60d7c0911f4cdfaec41 Mon Sep 17 00:00:00 2001 From: Rohan Yadav Date: Thu, 22 Aug 2019 14:32:58 -0400 Subject: [PATCH] sql: Fix bug with negative parsing validation for zone constraints Fixes #39801. Release note: None --- pkg/sql/set_zone_config.go | 4 ++ pkg/sql/set_zone_config_test.go | 93 ++++++++++++++++++++------------- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 843fb91e9e6d..53d040e25cc3 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -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 { diff --git a/pkg/sql/set_zone_config_test.go b/pkg/sql/set_zone_config_test.go index 112725620ab3..9406eedac73c 100644 --- a/pkg/sql/set_zone_config_test.go +++ b/pkg/sql/set_zone_config_test.go @@ -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) @@ -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 {