MCO-1333: Validate OCL Containerfiles

pkg/controller/build/buildrequest/buildrequest.go

@@ -5,11 +5,14 @@ import (
 	_ "embed"
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 	"text/template"
 
 	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
 	mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned"
+	command "github.com/openshift/imagebuilder/dockerfile/command"
+	parser "github.com/openshift/imagebuilder/dockerfile/parser"
 	"github.com/openshift/machine-config-operator/pkg/controller/build/constants"
 	"github.com/openshift/machine-config-operator/pkg/controller/build/utils"
 	chelpers "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -39,6 +42,29 @@ const (
 	machineConfigJSONFilename string = "machineconfig.json.gz"
 )
 
+var basicSyntaxRegex = regexp.MustCompile(`(?m)(?i)^\s*FROM`)
+
+// instructionRequirements maps Containerfile instructions that MUST have arguments to their requirement descriptions
+// Instructions not in this map either don't require arguments or have optional arguments
+var instructionRequirements = map[string]string{
+	command.Add:        "source and destination paths",
+	command.Arg:        "a variable name",
+	command.Cmd:        "a command or arguments",
+	command.Copy:       "source and destination paths",
+	command.Entrypoint: "a command or arguments",
+	command.Env:        "key=value pairs",
+	command.Expose:     "a port",
+	command.From:       "a base image",
+	command.Label:      "key=value pairs",
+	command.Onbuild:    "an instruction to execute",
+	command.Run:        "a command",
+	command.Shell:      "a shell command array",
+	command.StopSignal: "a signal",
+	command.User:       "a username or UID",
+	command.Volume:     "a mount point",
+	command.Workdir:    "a directory path",
+}
+
 // Represents the request to build a layered OS image.
 type buildRequestImpl struct {
 	opts              BuildRequestOpts
@@ -69,6 +95,13 @@ func newBuildRequest(opts BuildRequestOpts) BuildRequest {
 		}
 	}
 
+	// Validate user's Containerfile if provided
+	if br.userContainerfile != "" {
+		if err := br.validateContainerfileSyntax(br.userContainerfile); err != nil {
+			klog.Warningf("User Containerfile validation failed")
+		}
+	}
+
 	return br
 }
 
@@ -346,7 +379,124 @@ func (br buildRequestImpl) renderContainerfile() (string, error) {
 		return "", fmt.Errorf("could not execute containerfile template: %w", err)
 	}
 
-	return out.String(), nil
+	renderedContainerfile := out.String()
+
+	// Validate the rendered Containerfile
+	if err := br.validateContainerfileSyntax(renderedContainerfile); err != nil {
+		return "", fmt.Errorf("rendered containerfile validation failed: %w", err)
+	}
+
+	return renderedContainerfile, nil
+}
+
+// validateContainerfileSyntax validates the syntax of a Containerfile using basic checks and the imagebuilder parser.
+func (br buildRequestImpl) validateContainerfileSyntax(containerfile string) error {
+	if containerfile == "" {
+		return fmt.Errorf("containerfile is empty")
+	}
+
+	if err := br.validateBasicSyntax(containerfile); err != nil {
+		return err
+	}
+
+	result, err := parser.Parse(strings.NewReader(containerfile))
+	if err != nil {
+		// For rendered templates, advanced syntax is expected
+		// Only fail on obvious syntax errors
+		if !isKnownParserLimitation(err) {
+			return fmt.Errorf("failed to parse Containerfile: %w", err)
+		}
+		// Log but don't fail for advanced syntax
+		klog.V(4).Infof("Containerfile uses syntax the parser doesn't support, skipping detailed parser validation: %v", err)
+		return nil
+	}
+
+	if result.AST == nil || len(result.AST.Children) == 0 {
+		return fmt.Errorf("parsed Containerfile has no instructions")
+	}
+
+	// Validate each instruction
+	for _, child := range result.AST.Children {
+		if err := br.validateInstruction(child); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// validateBasicSyntax performs basic sanity checks that don't require parsing.
+func (br buildRequestImpl) validateBasicSyntax(containerfile string) error {
+	// Check for at least one FROM instruction using regex
+	if !basicSyntaxRegex.MatchString(containerfile) {
+		return fmt.Errorf("Containerfile must contain at least one FROM instruction")
+	}
+
+	return nil
+}
+
+// isKnownParserLimitation checks if a parser error is from known limitations of the
+// imagebuilder parser library that buildah/podman handle correctly.
+func isKnownParserLimitation(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	errMsg := err.Error()
+	knownPatterns := []string{
+		"<<",
+		"<<-",
+		"heredoc",
+		"can't find = in",
+		"must be of the form: name=value",
+	}
+
+	for _, pattern := range knownPatterns {
+		if strings.Contains(errMsg, pattern) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// validateInstruction validates that an instruction is valid and has required arguments.
+func (br buildRequestImpl) validateInstruction(node *parser.Node) error {
+	if node == nil || node.Value == "" {
+		return nil
+	}
+
+	instruction := node.Value
+
+	// First check if this is a valid Dockerfile instruction
+	if _, exists := command.Commands[instruction]; !exists {
+		return fmt.Errorf("unknown Dockerfile instruction: %s", node.Value)
+	}
+
+	// NONE is valid without additional arguments
+	if instruction == command.Healthcheck && node.Next != nil && strings.EqualFold(node.Next.Value, "NONE") {
+		return nil
+	}
+
+	// Then check if it has required arguments
+	requirement, requiresArgs := instructionRequirements[instruction]
+	if requiresArgs {
+		// Check if the node has arguments
+		hasArgs := false
+		if node.Next != nil && node.Next.Value != "" {
+			hasArgs = true
+		}
+		// Also check if there's a raw attribute that contains args
+		if !hasArgs && node.Attributes != nil && len(node.Attributes) > 0 {
+			hasArgs = true
+		}
+
+		if !hasArgs {
+			return fmt.Errorf("%s instruction requires %s", strings.ToUpper(instruction), requirement)
+		}
+	}
+
+	return nil
 }
 
 // podToJob creates a Job with the spec of the given Pod

pkg/controller/build/buildrequest/buildrequest_test.go

@@ -334,3 +334,174 @@ RUN rpm-ostree install && \
 		},
 	}
 }
+
+// TestValidateContainerfileSyntax tests the Containerfile validation logic
+func TestValidateContainerfileSyntax(t *testing.T) {
+	t.Parallel()
+
+	br := &buildRequestImpl{}
+
+	testCases := []struct {
+		name          string
+		containerfile string
+		expectError   bool
+		errorContains string
+	}{
+		{
+			name: "Valid simple Containerfile",
+			containerfile: `FROM registry.example.com/base:latest
+RUN echo "hello"
+COPY . /app`,
+			expectError: false,
+		},
+		{
+			name:          "Empty Containerfile",
+			containerfile: "",
+			expectError:   true,
+			errorContains: "containerfile is empty",
+		},
+		{
+			name:          "No FROM instruction",
+			containerfile: `RUN echo "hello"
+COPY . /app`,
+			expectError:   true,
+			errorContains: "must contain at least one FROM instruction",
+		},
+		{
+			name:          "FROM without arguments",
+			containerfile: `FROM`,
+			expectError:   true,
+			errorContains: "FROM instruction requires a base image",
+		},
+		{
+			name: "RUN without arguments",
+			containerfile: `FROM registry.example.com/base:latest
+RUN`,
+			expectError:   true,
+			errorContains: "RUN instruction requires a command",
+		},
+		{
+			name: "COPY without arguments",
+			containerfile: `FROM registry.example.com/base:latest
+COPY`,
+			expectError:   true,
+			errorContains: "COPY instruction requires source and destination paths",
+		},
+		{
+			name: "WORKDIR without arguments",
+			containerfile: `FROM registry.example.com/base:latest
+WORKDIR`,
+			expectError:   true,
+			errorContains: "WORKDIR instruction requires a directory path",
+		},
+		{
+			name: "Valid Containerfile with comments",
+			containerfile: `# Base image
+FROM registry.example.com/base:latest
+# Install dependencies
+RUN yum install -y python3`,
+			expectError: false,
+		},
+		{
+			name: "Advanced syntax with heredoc - should pass",
+			containerfile: `FROM registry.example.com/base:latest
+RUN bash <<'EOF'
+set -xeuo pipefail
+echo "hello"
+EOF
+`,
+			expectError: false, // Hybrid validator allows advanced syntax
+		},
+	}
+
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := br.validateContainerfileSyntax(testCase.containerfile)
+
+			if testCase.expectError {
+				assert.Error(t, err)
+				if testCase.errorContains != "" {
+					assert.Contains(t, err.Error(), testCase.errorContains)
+				}
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+// TestValidateContainerfileInBuildRequest tests that validation happens during the build request flow
+func TestValidateContainerfileInBuildRequest(t *testing.T) {
+	t.Parallel()
+
+	t.Run("Valid user Containerfile renders successfully", func(t *testing.T) {
+		opts := getBuildRequestOpts()
+		opts.MachineOSConfig.Spec.Containerfile[0].Content = `FROM configs AS final
+RUN rpm-ostree install vim && ostree container commit`
+
+		br := newBuildRequest(opts)
+		configMaps, err := br.ConfigMaps()
+
+		assert.NoError(t, err)
+		assert.NotNil(t, configMaps)
+		// Verify the Containerfile ConfigMap was created
+		assert.Greater(t, len(configMaps), 0)
+	})
+}
+
+func TestValidateUnknownInstruction(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name          string
+		containerfile string
+		expectError   bool
+		errorContains string
+	}{
+		{
+			name: "Unknown instruction BADINSTRUCTION",
+			containerfile: `FROM registry.example.com/base:latest
+BADINSTRUCTION this should fail
+RUN echo "test"`,
+			expectError:   true,
+			errorContains: "unknown Dockerfile instruction",
+		},
+		{
+			name: "Unknown instruction INVALIDCMD",
+			containerfile: `FROM registry.example.com/base:latest
+RUN echo "valid"
+INVALIDCMD foo bar`,
+			expectError:   true,
+			errorContains: "unknown Dockerfile instruction",
+		},
+		{
+			name: "Valid instructions only",
+			containerfile: `FROM registry.example.com/base:latest
+RUN echo "test"
+COPY . /app
+ENV FOO=bar`,
+			expectError: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			br := &buildRequestImpl{}
+			err := br.validateContainerfileSyntax(tc.containerfile)
+
+			if tc.expectError {
+				assert.Error(t, err)
+				if tc.errorContains != "" {
+					assert.Contains(t, err.Error(), tc.errorContains)
+				}
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}