vtctldclient: --strict rejects unknown vindex params in ApplyVSchema (#…

…14862)

Signed-off-by: Max Englander <[email protected]>

changelog/19.0/19.0.0/summary.md

@@ -19,6 +19,9 @@
     - [`SHOW VSCHEMA KEYSPACES` Query](#show-vschema-keyspaces)
   - **[Planned Reparent Shard](#planned-reparent-shard)**
     - [`--tolerable-replication-lag` Sub-flag](#tolerable-repl-lag)
+- **[Minor Changes](#minor-changes)**
+  - **[Apply VSchema](#apply-vschema)**
+    - [`--strict` sub-flag and `strict` gRPC field](#strict-flag-and-field)
 
 ## <a id="major-changes"/>Major Changes
 
@@ -99,3 +102,10 @@ This feature is opt-in and not specifying this sub-flag makes Vitess ignore the
 
 A new flag in VTOrc with the same name has been added to control the behaviour of the PlannedReparentShard calls that VTOrc issues.
 
+## <a id="minor-changes"/>Minor Changes
+
+### <a id="apply-vschema"/>Apply VSchema
+
+#### <a id="strict-flag-and-field"/>`--strict` sub-flag and `strict` gRPC field
+
+A new sub-flag `--strict` has been added to the command `ApplyVSchema` `vtctl` command that produces an error if unknown params are found in any Vindexes. An equivalent `strict` field has been added to the `ApplyVSchema` gRPC `vtctld` command.

go/cmd/vtctldclient/command/vschemas.go

@@ -40,7 +40,7 @@ var (
 	}
 	// ApplyVSchema makes an ApplyVSchema gRPC call to a vtctld.
 	ApplyVSchema = &cobra.Command{
-		Use:                   "ApplyVSchema {--vschema=<vschema> || --vschema-file=<vschema file> || --sql=<sql> || --sql-file=<sql file>} [--cells=c1,c2,...] [--skip-rebuild] [--dry-run] <keyspace>",
+		Use:                   "ApplyVSchema {--vschema=<vschema> || --vschema-file=<vschema file> || --sql=<sql> || --sql-file=<sql file>} [--cells=c1,c2,...] [--skip-rebuild] [--dry-run] [--strict] <keyspace>",
 		Short:                 "Applies the VTGate routing schema to the provided keyspace. Shows the result after application.",
 		DisableFlagsInUseLine: true,
 		Args:                  cobra.ExactArgs(1),
@@ -56,6 +56,7 @@ var applyVSchemaOptions = struct {
 	DryRun      bool
 	SkipRebuild bool
 	Cells       []string
+	Strict      bool
 }{}
 
 func commandApplyVSchema(cmd *cobra.Command, args []string) error {
@@ -75,6 +76,7 @@ func commandApplyVSchema(cmd *cobra.Command, args []string) error {
 		SkipRebuild: applyVSchemaOptions.SkipRebuild,
 		Cells:       applyVSchemaOptions.Cells,
 		DryRun:      applyVSchemaOptions.DryRun,
+		Strict:      applyVSchemaOptions.Strict,
 	}
 
 	var err error
@@ -156,6 +158,7 @@ func init() {
 	ApplyVSchema.Flags().BoolVar(&applyVSchemaOptions.DryRun, "dry-run", false, "If set, do not save the altered vschema, simply echo to console.")
 	ApplyVSchema.Flags().BoolVar(&applyVSchemaOptions.SkipRebuild, "skip-rebuild", false, "Skip rebuilding the SrvSchema objects.")
 	ApplyVSchema.Flags().StringSliceVar(&applyVSchemaOptions.Cells, "cells", nil, "Limits the rebuild to the specified cells, after application. Ignored if --skip-rebuild is set.")
+	ApplyVSchema.Flags().BoolVar(&applyVSchemaOptions.Strict, "strict", false, "If set, treat unknown vindex params as errors.")
 	Root.AddCommand(ApplyVSchema)
 
 	Root.AddCommand(GetVSchema)

go/vt/vtctl/grpcvtctldserver/server.go

@@ -369,10 +369,14 @@ func (s *VtctldServer) ApplyVSchema(ctx context.Context, req *vtctldatapb.ApplyV
 		err = vterrors.Wrapf(err, "BuildKeyspace(%s)", req.Keyspace)
 		return nil, err
 	}
+	response := &vtctldatapb.ApplyVSchemaResponse{
+		VSchema:             vs,
+		UnknownVindexParams: make(map[string]*vtctldatapb.ApplyVSchemaResponse_ParamList),
+	}
 
 	// Attach unknown Vindex params to the response.
-	unknownVindexParams := make(map[string]*vtctldatapb.ApplyVSchemaResponse_ParamList)
 	var vdxNames []string
+	var unknownVindexParams []string
 	for name := range ksVs.Vindexes {
 		vdxNames = append(vdxNames, name)
 	}
@@ -384,15 +388,18 @@ func (s *VtctldServer) ApplyVSchema(ctx context.Context, req *vtctldatapb.ApplyV
 			if len(ups) == 0 {
 				continue
 			}
-			unknownVindexParams[name] = &vtctldatapb.ApplyVSchemaResponse_ParamList{Params: ups}
+			response.UnknownVindexParams[name] = &vtctldatapb.ApplyVSchemaResponse_ParamList{Params: ups}
+			unknownVindexParams = append(unknownVindexParams, fmt.Sprintf("%s (%s)", name, strings.Join(ups, ", ")))
 		}
 	}
 
-	if req.DryRun { // we return what was passed in and parsed, plus unknown vindex params
-		return &vtctldatapb.ApplyVSchemaResponse{
-			VSchema:             vs,
-			UnknownVindexParams: unknownVindexParams,
-		}, nil
+	if req.Strict && len(unknownVindexParams) > 0 { // return early if unknown params found in strict mode
+		err = vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.WrongArguments, "unknown vindex params: %s", strings.Join(unknownVindexParams, "; "))
+		return response, err
+	}
+
+	if req.DryRun { // return early if dry run
+		return response, err
 	}
 
 	if err = s.ts.SaveVSchema(ctx, req.Keyspace, vs); err != nil {
@@ -411,10 +418,8 @@ func (s *VtctldServer) ApplyVSchema(ctx context.Context, req *vtctldatapb.ApplyV
 		err = vterrors.Wrapf(err, "GetVSchema(%s)", req.Keyspace)
 		return nil, err
 	}
-	return &vtctldatapb.ApplyVSchemaResponse{
-		VSchema:             updatedVS,
-		UnknownVindexParams: unknownVindexParams,
-	}, nil
+	response.VSchema = updatedVS
+	return response, nil
 }
 
 // Backup is part of the vtctlservicepb.VtctldServer interface.

go/vt/vtctl/grpcvtctldserver/server_test.go

@@ -352,6 +352,7 @@ func TestApplyVSchema(t *testing.T) {
 		req       *vtctldatapb.ApplyVSchemaRequest
 		exp       *vtctldatapb.ApplyVSchemaResponse
 		shouldErr bool
+		err       string
 	}{
 		{
 			name: "normal",
@@ -436,6 +437,46 @@ func TestApplyVSchema(t *testing.T) {
 				},
 			},
 			shouldErr: false,
+		}, {
+			name: "strict unknown params",
+			req: &vtctldatapb.ApplyVSchemaRequest{
+				Keyspace: "testkeyspacesharded",
+				VSchema: &vschemapb.Keyspace{
+					Sharded: true,
+					Vindexes: map[string]*vschemapb.Vindex{
+						"lookup1": {
+							Type: "lookup",
+							Params: map[string]string{
+								"hello":   "world",
+								"goodbye": "world",
+							},
+						},
+					},
+				},
+				SkipRebuild: true,
+				Strict:      true,
+			},
+			exp: &vtctldatapb.ApplyVSchemaResponse{
+				VSchema: &vschemapb.Keyspace{
+					Sharded: true,
+					Vindexes: map[string]*vschemapb.Vindex{
+						"lookup1": {
+							Type: "lookup",
+							Params: map[string]string{
+								"hello":   "world",
+								"goodbye": "world",
+							},
+						},
+					},
+				},
+				UnknownVindexParams: map[string]*vtctldatapb.ApplyVSchemaResponse_ParamList{
+					"lookup1": {
+						Params: []string{"goodbye", "hello"},
+					},
+				},
+			},
+			shouldErr: true,
+			err:       "unknown vindex params: lookup1 (goodbye, hello)",
 		}, {
 			name: "dry run",
 			req: &vtctldatapb.ApplyVSchemaRequest{
@@ -451,8 +492,7 @@ func TestApplyVSchema(t *testing.T) {
 				},
 			},
 			shouldErr: false,
-		},
-		{
+		}, {
 			name: "dry run with invalid params",
 			req: &vtctldatapb.ApplyVSchemaRequest{
 				Keyspace: "testkeyspacesharded",
@@ -468,8 +508,7 @@ func TestApplyVSchema(t *testing.T) {
 			},
 			exp:       &vtctldatapb.ApplyVSchemaResponse{},
 			shouldErr: true,
-		},
-		{
+		}, {
 			name: "dry run with unknown params",
 			req: &vtctldatapb.ApplyVSchemaRequest{
 				Keyspace: "testkeyspacesharded",
@@ -507,6 +546,46 @@ func TestApplyVSchema(t *testing.T) {
 				},
 			},
 			shouldErr: false,
+		}, {
+			name: "strict dry run with unknown params",
+			req: &vtctldatapb.ApplyVSchemaRequest{
+				Keyspace: "testkeyspacesharded",
+				VSchema: &vschemapb.Keyspace{
+					Sharded: true,
+					Vindexes: map[string]*vschemapb.Vindex{
+						"lookup1": {
+							Type: "lookup",
+							Params: map[string]string{
+								"hello":   "world",
+								"goodbye": "world",
+							},
+						},
+					},
+				},
+				DryRun: true,
+				Strict: true,
+			},
+			exp: &vtctldatapb.ApplyVSchemaResponse{
+				VSchema: &vschemapb.Keyspace{
+					Sharded: true,
+					Vindexes: map[string]*vschemapb.Vindex{
+						"lookup1": {
+							Type: "lookup",
+							Params: map[string]string{
+								"hello":   "world",
+								"goodbye": "world",
+							},
+						},
+					},
+				},
+				UnknownVindexParams: map[string]*vtctldatapb.ApplyVSchemaResponse_ParamList{
+					"lookup1": {
+						Params: []string{"goodbye", "hello"},
+					},
+				},
+			},
+			shouldErr: true,
+			err:       "unknown vindex params: lookup1 (goodbye, hello)",
 		},
 	}
 
@@ -558,6 +637,9 @@ func TestApplyVSchema(t *testing.T) {
 			res, err := vtctld.ApplyVSchema(ctx, tt.req)
 			if tt.shouldErr {
 				assert.Error(t, err)
+				if tt.err != "" {
+					assert.ErrorContains(t, err, tt.err)
+				}
 				return
 			}
 

proto/vtctldata.proto

@@ -371,6 +371,8 @@ message ApplyVSchemaRequest {
   repeated string cells = 4;
   vschema.Keyspace v_schema = 5;
   string sql = 6;
+  // Strict returns an error if there are unknown vindex params.
+  bool strict = 7;
 }
 
 message ApplyVSchemaResponse {