refactor tests and add FROM/AS lint rule

Signed-off-by: Talon Bowler <talon.bowler@docker.com>

frontend/dockerfile/dockerfile2llb/convert.go

@@ -139,10 +139,6 @@ func ListTargets(ctx context.Context, dt []byte) (*targets.List, error) {
 	return l, nil
 }
 
-func consistentCasing(s string) bool {
-	return s == strings.ToLower(s) || s == strings.ToUpper(s)
-}
-
 func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchState, error) {
 	if len(dt) == 0 {
 		return nil, errors.Errorf("the Dockerfile cannot be empty")
@@ -202,7 +198,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 	}
 
 	for _, node := range dockerfile.AST.Children {
-		if !consistentCasing(node.Value) {
+		if !isConsistentCasing(node.Value) {
 			msg := linter.RuleCommandCasing.Format(node.Value)
 			linter.RuleCommandCasing.Run(opt.Warn, node.Location(), msg)
 		}
@@ -1952,3 +1948,7 @@ func isEnabledForStage(stage string, value string) bool {
 	}
 	return false
 }
+
+func isConsistentCasing(s string) bool {
+	return s == strings.ToLower(s) || s == strings.ToUpper(s)
+}

frontend/dockerfile/dockerfile_test.go

@@ -6766,99 +6766,79 @@ ARG BUILDKIT_SBOM_SCAN_STAGE=true
 }
 
 func testLintWarnings(t *testing.T, sb integration.Sandbox) {
-	integration.SkipOnPlatform(t, "windows")
-	f := getFrontend(t, sb)
+	testLintStageName(t, sb)
+	testLintNoEmptyContinuations(t, sb)
+	testCommandCasing(t, sb)
+}
 
-	// empty line in here is intentional to cause line continuation warning
+func testLintStageName(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`
-# set of commands which should cause warnings
-FROM scratch as BadStageName
-Copy Dockerfile .
-COPY Dockerfile \
-
-.
-
-# set of commands which should not cause warnings
-FROM scratch as base
-copy Dockerfile .
-COPY Dockerfile .
-`)
-
-	dir := integration.Tmpdir(
-		t,
-		fstest.CreateFile("Dockerfile", dockerfile, 0600),
-	)
-
-	c, err := client.New(sb.Context(), sb.Address())
-	require.NoError(t, err)
-	defer c.Close()
+# warning: stage name should be lowercase
+FROM scratch AS BadStageName
 
-	status := make(chan *client.SolveStatus)
-	statusDone := make(chan struct{})
-	done := make(chan struct{})
-
-	var warnings []*client.VertexWarning
+# warning: 'as' should match 'FROM' cmd casing.
+FROM scratch as base2
 
-	go func() {
-		defer close(statusDone)
-		for {
-			select {
-			case st, ok := <-status:
-				if !ok {
-					return
-				}
-				warnings = append(warnings, st.Warnings...)
-			case <-done:
-				return
-			}
-		}
-	}()
+# warning: 'AS' should match 'from' cmd casing.
+from scratch AS base3
 
-	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
-		FrontendAttrs: map[string]string{
-			"platform": "linux/amd64,linux/arm64",
+FROM scratch AS base4
+from scratch as base5
+`)
+	compareLintWarnings(t, sb, dockerfile, []expectedLintWarning{
+		{
+			Short:  "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)",
+			Detail: "Stage names should be lowercase",
+			Level:  1,
 		},
-		LocalMounts: map[string]fsutil.FS{
-			dockerui.DefaultLocalNameDockerfile: dir,
-			dockerui.DefaultLocalNameContext:    dir,
+		{
+			Short:  "Lint Rule 'FromAsCasing': 'as' and 'FROM' keywords' casing do not match (line 6)",
+			Detail: "The 'as' keyword should match the case of the 'from' keyword",
+			Level:  1,
 		},
-	}, status)
-	require.NoError(t, err)
+		{
+			Short:  "Lint Rule 'FromAsCasing': 'AS' and 'from' keywords' casing do not match (line 9)",
+			Detail: "The 'as' keyword should match the case of the 'from' keyword",
+			Level:  1,
+		},
+	})
+}
 
-	select {
-	case <-statusDone:
-	case <-time.After(10 * time.Second):
-		close(done)
-	}
+func testLintNoEmptyContinuations(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+FROM scratch
+# warning: empty continuation line
+COPY Dockerfile \
 
-	<-statusDone
+.
+COPY Dockerfile \
+.
+`)
 
-	// two platforms only show one warning
-	require.Equal(t, 3, len(warnings))
-	sort.Slice(warnings, func(i, j int) bool {
-		w1 := warnings[i]
-		w2 := warnings[j]
-		if len(w1.Range) == 0 {
-			return true
-		} else if len(w2.Range) == 0 {
-			return false
-		}
-		return w1.Range[0].Start.Line < w2.Range[0].Start.Line
+	compareLintWarnings(t, sb, dockerfile, []expectedLintWarning{
+		{
+			Short:  "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 6)",
+			Detail: "Empty continuation lines will become errors in a future release",
+			URL:    "https://github.com/moby/moby/pull/33719",
+			Level:  1,
+		},
 	})
+}
 
-	require.Equal(t, "Lint Rule 'StageNameCasing': Stage name 'BadStageName' should be lowercase (line 3)", string(warnings[0].Short))
-	require.Equal(t, "Stage names should be lowercase", string(warnings[0].Detail[0]))
-	require.Equal(t, "", warnings[0].URL)
-	require.Equal(t, 1, warnings[0].Level)
-
-	require.Equal(t, "Lint Rule 'CommandCasing': Command 'Copy' should be consistently cased (line 4)", string(warnings[1].Short))
-	require.Equal(t, "Commands should be in consistent casing (all lower or all upper)", string(warnings[1].Detail[0]))
-	require.Equal(t, "", warnings[1].URL)
-	require.Equal(t, 1, warnings[1].Level)
-
-	require.Equal(t, "Lint Rule 'NoEmptyContinuations': Empty continuation line (line 7)", string(warnings[2].Short))
-	require.Equal(t, "Empty continuation lines will become errors in a future release", string(warnings[2].Detail[0]))
-	require.Equal(t, "https://github.com/moby/moby/pull/33719", warnings[2].URL)
+func testCommandCasing(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+# warning: 'FROM' should be either lowercased or uppercased
+From scratch as base
+FROM scratch AS base2
+from scratch as base3
+`)
+	compareLintWarnings(t, sb, dockerfile, []expectedLintWarning{
+		{
+			Short:  "Lint Rule 'CommandCasing': Command 'From' should be consistently cased (line 3)",
+			Detail: "Commands should be in consistent casing (all lower or all upper)",
+			Level:  1,
+		},
+	})
 }
 
 // #3495
@@ -7059,6 +7039,82 @@ RUN rm -f /foo-2030.1
 	}
 }
 
+func compareLintWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte, expected []expectedLintWarning) {
+	// As a note, this test depends on the `expected` lint
+	// warnings to be in order of appearance in the Dockerfile.
+
+	integration.SkipOnPlatform(t, "windows")
+	f := getFrontend(t, sb)
+
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	status := make(chan *client.SolveStatus)
+	statusDone := make(chan struct{})
+	done := make(chan struct{})
+
+	var warnings []*client.VertexWarning
+
+	go func() {
+		defer close(statusDone)
+		for {
+			select {
+			case st, ok := <-status:
+				if !ok {
+					return
+				}
+				warnings = append(warnings, st.Warnings...)
+			case <-done:
+				return
+			}
+		}
+	}()
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		FrontendAttrs: map[string]string{
+			"platform": "linux/amd64,linux/arm64",
+		},
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, status)
+	require.NoError(t, err)
+
+	select {
+	case <-statusDone:
+	case <-time.After(10 * time.Second):
+		close(done)
+	}
+
+	<-statusDone
+
+	// two platforms only show one warning
+	require.Equal(t, len(expected), len(warnings))
+	sort.Slice(warnings, func(i, j int) bool {
+		w1 := warnings[i]
+		w2 := warnings[j]
+		if len(w1.Range) == 0 {
+			return true
+		} else if len(w2.Range) == 0 {
+			return false
+		}
+		return w1.Range[0].Start.Line < w2.Range[0].Start.Line
+	})
+	for i, w := range warnings {
+		require.Equal(t, expected[i].Short, string(w.Short))
+		require.Equal(t, expected[i].Detail, string(w.Detail[0]))
+		require.Equal(t, expected[i].URL, w.URL)
+		require.Equal(t, expected[i].Level, w.Level)
+	}
+}
+
 func timeMustParse(t *testing.T, layout, value string) time.Time {
 	tm, err := time.Parse(layout, value)
 	require.NoError(t, err)
@@ -7429,6 +7485,13 @@ type nopWriteCloser struct {
 
 func (nopWriteCloser) Close() error { return nil }
 
+type expectedLintWarning struct {
+	Short  string
+	Detail string
+	URL    string
+	Level  int
+}
+
 type secModeSandbox struct{}
 
 func (*secModeSandbox) UpdateConfigFile(in string) string {

frontend/dockerfile/instructions/parse.go

@@ -67,14 +67,6 @@ func newParseRequestFromNode(node *parser.Node) parseRequest {
 	}
 }
 
-func validStageNameCasing(cmdArgs []string) bool {
-	if len(cmdArgs) != 3 {
-		return true
-	}
-	stageName := cmdArgs[2]
-	return stageName == strings.ToLower(stageName)
-}
-
 func ParseInstruction(node *parser.Node) (v interface{}, err error) {
 	return ParseInstructionWithLinter(node, nil)
 }
@@ -97,10 +89,14 @@ func ParseInstructionWithLinter(node *parser.Node, lintWarn linter.LintWarnFunc)
 	case command.Copy:
 		return parseCopy(req)
 	case command.From:
-		if lintWarn != nil && !validStageNameCasing(req.args) {
+		if lintWarn != nil && !isValidStageNameCasing(req.args) {
 			msg := linter.RuleStageNameCasing.Format(req.args[2])
 			linter.RuleStageNameCasing.Run(lintWarn, node.Location(), msg)
 		}
+		if lintWarn != nil && !doesFromCaseMatchAsCase(req) {
+			msg := linter.RuleFromAsCasing.Format(req.command, req.args[1])
+			linter.RuleFromAsCasing.Run(lintWarn, node.Location(), msg)
+		}
 		return parseFrom(req)
 	case command.Onbuild:
 		return parseOnBuild(req)
@@ -832,3 +828,41 @@ func allInstructionNames() []string {
 	}
 	return out
 }
+
+func isLowerCased(s string) bool {
+	return s == strings.ToLower(s)
+}
+
+func isUpperCased(s string) bool {
+	return s == strings.ToUpper(s)
+}
+
+func isConsistentCasing(s string) bool {
+	return isLowerCased(s) || isUpperCased(s)
+}
+
+func isValidStageNameCasing(cmdArgs []string) bool {
+	if len(cmdArgs) != 3 {
+		return true
+	}
+	stageName := cmdArgs[2]
+	return isLowerCased(stageName)
+}
+
+func doesFromCaseMatchAsCase(req parseRequest) bool {
+	if len(req.args) < 3 {
+		return true
+	}
+	// consistent casing for the command is handled elsewhere.
+	// If the command is not consistent, there's no need to
+	// add an additional lint warning for the `as` argument.
+	if !isConsistentCasing(req.command) {
+		return true
+	}
+	var lowerCaseCmd = isLowerCased(req.command)
+	if lowerCaseCmd {
+		return isLowerCased(req.args[1])
+	} else {
+		return isUpperCased(req.args[1])
+	}
+}

frontend/dockerfile/linter/ruleset.go

@@ -12,6 +12,13 @@ var (
 			return fmt.Sprintf("Stage name '%s' should be lowercase", stageName)
 		},
 	}
+	RuleFromAsCasing = LinterRule[func(string, string) string]{
+		Name:        "FromAsCasing",
+		Description: "The 'as' keyword should match the case of the 'from' keyword",
+		Format: func(from, as string) string {
+			return fmt.Sprintf("'%s' and '%s' keywords' casing do not match", as, from)
+		},
+	}
 	RuleNoEmptyContinuations = LinterRule[func() string]{
 		Name:        "NoEmptyContinuations",
 		Description: "Empty continuation lines will become errors in a future release",