From 5c8a77e411489843009226c2edabe398f1a6deba Mon Sep 17 00:00:00 2001
From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Date: Tue, 31 Oct 2023 14:22:03 +0200
Subject: [PATCH] Online DDL: lint DDL strategy flags (#14373)

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
---
 go/vt/schema/ddl_strategy.go      |  9 +++++
 go/vt/schema/ddl_strategy_test.go | 66 ++++++++++++++++++++++++-------
 2 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/go/vt/schema/ddl_strategy.go b/go/vt/schema/ddl_strategy.go
index 88400d423fd..bc33c8cb3cf 100644
--- a/go/vt/schema/ddl_strategy.go
+++ b/go/vt/schema/ddl_strategy.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"regexp"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/google/shlex"
@@ -115,6 +116,14 @@ func ParseDDLStrategy(strategyVariable string) (*DDLStrategySetting, error) {
 	if _, err := setting.RetainArtifactsDuration(); err != nil {
 		return nil, err
 	}
+
+	switch setting.Strategy {
+	case DDLStrategyVitess, DDLStrategyOnline, DDLStrategyMySQL, DDLStrategyDirect:
+		if opts := setting.RuntimeOptions(); len(opts) > 0 {
+			return nil, fmt.Errorf("invalid flags for %v strategy: %s", setting.Strategy, strings.Join(opts, " "))
+		}
+	}
+
 	return setting, nil
 }
 
diff --git a/go/vt/schema/ddl_strategy_test.go b/go/vt/schema/ddl_strategy_test.go
index 8ad6ff592dc..ba7d029b8b7 100644
--- a/go/vt/schema/ddl_strategy_test.go
+++ b/go/vt/schema/ddl_strategy_test.go
@@ -41,19 +41,23 @@ func TestIsDirect(t *testing.T) {
 
 func TestIsCutOverThresholdFlag(t *testing.T) {
 	tt := []struct {
-		s      string
-		expect bool
-		val    string
-		d      time.Duration
+		s           string
+		expect      bool
+		expectError string
+		val         string
+		d           time.Duration
 	}{
 		{
-			s: "something",
+			s:           "something",
+			expectError: "invalid flags",
 		},
 		{
-			s: "-cut-over-threshold",
+			s:           "-cut-over-threshold",
+			expectError: "invalid flags",
 		},
 		{
-			s: "--cut-over-threshold",
+			s:           "--cut-over-threshold",
+			expectError: "invalid flags",
 		},
 		{
 			s:      "--cut-over-threshold=",
@@ -87,6 +91,11 @@ func TestIsCutOverThresholdFlag(t *testing.T) {
 	for _, ts := range tt {
 		t.Run(ts.s, func(t *testing.T) {
 			setting, err := ParseDDLStrategy("online " + ts.s)
+			if ts.expectError != "" {
+				assert.ErrorContains(t, err, ts.expectError)
+				return
+			}
+
 			assert.NoError(t, err)
 
 			val, isCutOver := isCutOverThresholdFlag(ts.s)
@@ -104,19 +113,23 @@ func TestIsCutOverThresholdFlag(t *testing.T) {
 
 func TestIsExpireArtifactsFlag(t *testing.T) {
 	tt := []struct {
-		s      string
-		expect bool
-		val    string
-		d      time.Duration
+		s           string
+		expect      bool
+		expectError string
+		val         string
+		d           time.Duration
 	}{
 		{
-			s: "something",
+			s:           "something",
+			expectError: "invalid flags",
 		},
 		{
-			s: "-retain-artifacts",
+			s:           "-retain-artifacts",
+			expectError: "invalid flags",
 		},
 		{
-			s: "--retain-artifacts",
+			s:           "--retain-artifacts",
+			expectError: "invalid flags",
 		},
 		{
 			s:      "--retain-artifacts=",
@@ -150,6 +163,10 @@ func TestIsExpireArtifactsFlag(t *testing.T) {
 	for _, ts := range tt {
 		t.Run(ts.s, func(t *testing.T) {
 			setting, err := ParseDDLStrategy("online " + ts.s)
+			if ts.expectError != "" {
+				assert.ErrorContains(t, err, ts.expectError)
+				return
+			}
 			assert.NoError(t, err)
 
 			val, isRetainArtifacts := isRetainArtifactsFlag(ts.s)
@@ -183,7 +200,7 @@ func TestParseDDLStrategy(t *testing.T) {
 		cutOverThreshold     time.Duration
 		expireArtifacts      time.Duration
 		runtimeOptions       string
-		err                  error
+		expectError          string
 	}{
 		{
 			strategyVariable: "direct",
@@ -317,10 +334,29 @@ func TestParseDDLStrategy(t *testing.T) {
 			runtimeOptions:   "",
 			analyzeTable:     true,
 		},
+
+		{
+			strategyVariable: "vitess --alow-concrrnt", // intentional typo
+			strategy:         DDLStrategyVitess,
+			options:          "",
+			runtimeOptions:   "",
+			expectError:      "invalid flags",
+		},
+		{
+			strategyVariable: "vitess --declarative --max-load=Threads_running=100",
+			strategy:         DDLStrategyVitess,
+			options:          "--declarative --max-load=Threads_running=100",
+			runtimeOptions:   "--max-load=Threads_running=100",
+			expectError:      "invalid flags",
+		},
 	}
 	for _, ts := range tt {
 		t.Run(ts.strategyVariable, func(t *testing.T) {
 			setting, err := ParseDDLStrategy(ts.strategyVariable)
+			if ts.expectError != "" {
+				assert.ErrorContains(t, err, ts.expectError)
+				return
+			}
 			assert.NoError(t, err)
 			assert.Equal(t, ts.strategy, setting.Strategy)
 			assert.Equal(t, ts.options, setting.Options)