fix: Validate parsed steps without re-encoding them as YAML

A job script that starts with a blank line or a tab, or whose first line is indented more than the next, does not run correctly on a stock runner with FF_SCRIPT_TO_STEP_MIGRATION on. The step request the runner writes does not decode, and a script indented on every line runs with its indentation dropped. gitlab-org/gitlab-runner!7660 makes the runner quote the strings it writes. Once it does, these scripts reach step-runner's validation, which fails on them. This MR fixes that half, so these scripts run as written once both land.

Closes #480

flowchart LR
    read["ReadSteps: decode YAML into typed Spec and Step"] --> validate["validate against spec.json and step.json"]
    validate --> before["before: yaml.Marshal to text, then yaml.Unmarshal to any"]
    validate --> after["after: untype walks the typed values in memory"]
    before -->|"script with a leading blank line, a leading tab or a more-indented first line"| fail["parse error: job fails at validation"]
    before -->|"other scalars"| check["JSON schema check"]
    after --> check
    check --> run["Compile and run: script reaches the shell as decoded"]
    fail ~~~ run

Step Runner checks each parsed step against its JSON schema. To do that it wrote the step back out as YAML text and parsed the text again. go-yaml v3 writes some multi-line strings in a form it cannot read back (go-yaml issue 76), so the document had parsed and then failed validation. On main the errors are such as validating function schema: yaml: line 1: did not find expected key in the tests and, through the runner, validating function schema: yaml: line 6: did not find expected '-' indicator. The line number varies. A leading tab reports found a tab character where an indentation space is expected.

An upgrade would not help yet:

  • The fix is go-yaml PR 414, merged on go-yaml's main branch on 13 October. No tagged release has it, and the newest v4 tag, v4.0.0-rc.6, predates it.
  • go-yaml's README keeps v3 to security fixes, so the fix will not land there.

The change

  • schema/v1/validate.go: untype walks the typed value with reflection and builds the any tree directly, with the yaml tag name and omitempty rules the encoder applied. Strings are copied as they are, so validateSpec and validateStep no longer have an error to return. Encoding to a yaml.Node goes through the same text, which is why the walk builds the tree itself.
  • schema/v1/signature.go: the outputs reification decodes the original node. It marshaled a map and parsed it again, which broke for a string in an output default.
  • schema/v1/step.go: a func or step reference decodes from the original node too, so a rev that starts with a tab reads as written.
  • TestUntypeMirrorsEveryReachableShape walks every type reachable from a spec or step and fails if one gains a shape untype does not copy: an IsZero or marshal method, an inline or flow tag, an embedded or unexported field, a struct with omitempty, a time.Duration or a yaml.Node.

How it was tested

  • TestReadPreservesScalars reads seven script shapes. On main four fail (leading blank line, first line indented more than the next, blank lines then unindented text, leading tab) and all seven pass here.
  • TestCanRunAScript gains an end-to-end case through testutil.StepRunner that fails on main with the error above.
  • TestReadPreservesOutputDefaults covers the outputs reification, TestReadPreservesReferenceScalars covers a func and a step reference whose rev starts with a tab, and TestUntype compares the walk with the old YAML round trip for a spec and a step that use every field type.

The table covers the default helper, not FF_CONCRETE. Job scripts of each shape ran with gitlab-runner's step mode on a self-managed GitLab 19.5.0 instance, and the output was compared with the legacy script path, the same job with the flag off:

Job script Stock runner Companion runner MR only Both changes
Ordinary, or a tab inside a line output equals legacy output equals legacy output equals legacy
Every line indented runs, indentation dropped output equals legacy output equals legacy
Leading blank line request does not decode fails in validation output equals legacy
First line indented more than the rest request does not decode fails in validation output equals legacy
Blank lines, then unindented text request does not decode fails in validation output equals legacy
Leading tab request does not decode fails in validation output equals legacy

gitlab-runner takes step-runner through go.mod, so it needs a release that includes this change. The CHANGELOG entry is a separate commit on this branch.

An AI coding assistant wrote this change and this description. The assistant also ran the job scripts in the table above on a test VM running a self-managed GitLab 19.5.0 instance, and ran go test ./schema/... and golangci-lint run ./schema/... on the changed tree, plus the cmd/ci, cmd/run and pkg/api/service tests after make build-individual-commands. The reference decode is covered by TestReadPreservesReferenceScalars only.

Edited by Andrew Dunn

Merge request reports

Loading
Loading