Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dockerfile: add suggestions to how to fix certain errors #2218

Merged
merged 5 commits into from
Jul 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/dockerfile/builder/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ func Build(ctx context.Context, c client.Client) (*client.Result, error) {
})

if err != nil {
return errors.Wrapf(err, "failed to create LLB definition")
return err
}

def, err := st.Marshal(ctx)
Expand Down
24 changes: 22 additions & 2 deletions frontend/dockerfile/dockerfile2llb/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/moby/buildkit/frontend/dockerfile/shell"
"github.com/moby/buildkit/solver/pb"
"github.com/moby/buildkit/util/apicaps"
"github.com/moby/buildkit/util/suggest"
"github.com/moby/buildkit/util/system"
specs "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
Expand Down Expand Up @@ -216,6 +217,13 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
allDispatchStates.states[0].stageName = ""
}

allStageNames := make([]string, 0, len(allDispatchStates.states))
for _, s := range allDispatchStates.states {
if s.stageName != "" {
allStageNames = append(allStageNames, s.stageName)
}
}

eg, ctx := errgroup.WithContext(ctx)
for i, d := range allDispatchStates.states {
reachable := isReachable(target, d)
Expand All @@ -233,6 +241,7 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
err = parser.WithLocation(err, d.stage.Location)
}
}()
origName := d.stage.BaseName
ref, err := reference.ParseNormalizedNamed(d.stage.BaseName)
if err != nil {
return errors.Wrapf(err, "failed to parse stage name %q", d.stage.BaseName)
Expand All @@ -243,7 +252,7 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
}
d.stage.BaseName = reference.TagNameOnly(ref).String()
var isScratch bool
if metaResolver != nil && reachable && !d.unregistered {
if metaResolver != nil && reachable {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy from image will now load image config here although it is not needed to get an error in the correct place. The config would have been still downloaded together with the image layers before.

prefix := "["
if opt.PrefixPlatform && platform != nil {
prefix += platforms.Format(*platform) + " "
Expand All @@ -255,7 +264,7 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State,
LogName: fmt.Sprintf("%s load metadata for %s", prefix, d.stage.BaseName),
})
if err != nil {
return errors.Wrap(err, d.stage.BaseName)
return suggest.WrapError(errors.Wrap(err, origName), origName, append(allStageNames, commonImageNames()...), true)
}
var img Image
if err := json.Unmarshal(dt, &img); err != nil {
Expand Down Expand Up @@ -1535,3 +1544,14 @@ func summarizeHeredoc(doc string) string {
}
return summary
}

func commonImageNames() []string {
repos := []string{
"alpine", "busybox", "centos", "debian", "golang", "ubuntu", "fedora",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CentOS is about to die soon :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I noticed this is used only for typo-checking, so it's fine to retain centos here)

}
out := make([]string, 0, len(repos)*4)
for _, name := range repos {
out = append(out, name, "docker.io/library"+name, name+":latest", "docker.io/library"+name+":latest")
}
return out
}
72 changes: 72 additions & 0 deletions frontend/dockerfile/dockerfile_mount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var mountTests = []integration.Test{
testMountEnvAcrossStages,
testMountMetaArg,
testMountFromError,
testMountInvalid,
}

func init() {
Expand Down Expand Up @@ -88,6 +89,77 @@ RUN [ ! -f /mytmp/foo ]
require.NoError(t, err)
}

func testMountInvalid(t *testing.T, sb integration.Sandbox) {
f := getFrontend(t, sb)

dockerfile := []byte(`
FROM scratch
RUN --mont=target=/mytmp,type=tmpfs /bin/true
`)

dir, err := tmpdir(
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
require.NoError(t, err)
defer os.RemoveAll(dir)

c, err := client.New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalDirs: map[string]string{
builder.DefaultLocalNameDockerfile: dir,
builder.DefaultLocalNameContext: dir,
},
}, nil)
require.Error(t, err)
require.Contains(t, err.Error(), "unknown flag: mont")
require.Contains(t, err.Error(), "did you mean mount?")

dockerfile = []byte(`
FROM scratch
RUN --mount=typ=tmpfs /bin/true
`)

dir, err = tmpdir(
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
require.NoError(t, err)
defer os.RemoveAll(dir)

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalDirs: map[string]string{
builder.DefaultLocalNameDockerfile: dir,
builder.DefaultLocalNameContext: dir,
},
}, nil)
require.Error(t, err)
require.Contains(t, err.Error(), "unexpected key 'typ'")
require.Contains(t, err.Error(), "did you mean type?")

dockerfile = []byte(`
FROM scratch
RUN --mount=type=tmp /bin/true
`)

dir, err = tmpdir(
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
require.NoError(t, err)
defer os.RemoveAll(dir)

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalDirs: map[string]string{
builder.DefaultLocalNameDockerfile: dir,
builder.DefaultLocalNameContext: dir,
},
}, nil)
require.Error(t, err)
require.Contains(t, err.Error(), "unsupported mount type \"tmp\"")
require.Contains(t, err.Error(), "did you mean tmpfs?")
}

func testMountRWCache(t *testing.T, sb integration.Sandbox) {
f := getFrontend(t, sb)

Expand Down
32 changes: 32 additions & 0 deletions frontend/dockerfile/dockerfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ var allTests = []integration.Test{
testDockerfileLowercase,
testExportCacheLoop,
testWildcardRenameCache,
testDockerfileInvalidInstruction,
}

var fileOpTests = []integration.Test{
Expand Down Expand Up @@ -2152,6 +2153,37 @@ func testDockerfileInvalidCommand(t *testing.T, sb integration.Sandbox) {
require.Contains(t, stdout.String(), "did not complete successfully")
}

func testDockerfileInvalidInstruction(t *testing.T, sb integration.Sandbox) {
skipDockerd(t, sb)
f := getFrontend(t, sb)
f.RequiresBuildctl(t)
dockerfile := []byte(`
FROM scratch
FNTRYPOINT ["/bin/sh", "-c", "echo invalidinstruction"]
`)

dir, err := tmpdir(
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
require.NoError(t, err)
defer os.RemoveAll(dir)

c, err := client.New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
LocalDirs: map[string]string{
builder.DefaultLocalNameDockerfile: dir,
builder.DefaultLocalNameContext: dir,
},
}, nil)

require.Error(t, err)
require.Contains(t, err.Error(), "unknown instruction: FNTRYPOINT")
require.Contains(t, err.Error(), "did you mean ENTRYPOINT?")
}

func testDockerfileADDFromURL(t *testing.T, sb integration.Sandbox) {
skipDockerd(t, sb)
f := getFrontend(t, sb)
Expand Down
26 changes: 18 additions & 8 deletions frontend/dockerfile/instructions/bflag.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package instructions
import (
"fmt"
"strings"

"github.com/moby/buildkit/util/suggest"
)

// FlagType is the type of the build flag
Expand Down Expand Up @@ -139,12 +141,12 @@ func (bf *BFlags) Parse() error {
// go ahead and bubble it back up here since we didn't do it
// earlier in the processing
if bf.Err != nil {
return fmt.Errorf("Error setting up flags: %s", bf.Err)
return fmt.Errorf("error setting up flags: %s", bf.Err)
}

for _, arg := range bf.Args {
if !strings.HasPrefix(arg, "--") {
return fmt.Errorf("Arg should start with -- : %s", arg)
return fmt.Errorf("arg should start with -- : %s", arg)
}

if arg == "--" {
Expand All @@ -162,11 +164,11 @@ func (bf *BFlags) Parse() error {

flag, ok := bf.flags[arg]
if !ok {
return fmt.Errorf("Unknown flag: %s", arg)
return suggest.WrapError(fmt.Errorf("unknown flag: %s", arg), arg, allFlags(bf.flags), true)
}

if _, ok = bf.used[arg]; ok && flag.flagType != stringsType {
return fmt.Errorf("Duplicate flag specified: %s", arg)
return fmt.Errorf("duplicate flag specified: %s", arg)
}

bf.used[arg] = flag
Expand All @@ -175,7 +177,7 @@ func (bf *BFlags) Parse() error {
case boolType:
// value == "" is only ok if no "=" was specified
if index >= 0 && value == "" {
return fmt.Errorf("Missing a value on flag: %s", arg)
return fmt.Errorf("missing a value on flag: %s", arg)
}

lower := strings.ToLower(value)
Expand All @@ -184,18 +186,18 @@ func (bf *BFlags) Parse() error {
} else if lower == "true" || lower == "false" {
flag.Value = lower
} else {
return fmt.Errorf("Expecting boolean value for flag %s, not: %s", arg, value)
return fmt.Errorf("expecting boolean value for flag %s, not: %s", arg, value)
}

case stringType:
if index < 0 {
return fmt.Errorf("Missing a value on flag: %s", arg)
return fmt.Errorf("missing a value on flag: %s", arg)
}
flag.Value = value

case stringsType:
if index < 0 {
return fmt.Errorf("Missing a value on flag: %s", arg)
return fmt.Errorf("missing a value on flag: %s", arg)
}
flag.StringValues = append(flag.StringValues, value)

Expand All @@ -207,3 +209,11 @@ func (bf *BFlags) Parse() error {

return nil
}

func allFlags(flags map[string]*Flag) []string {
var names []string
for name := range flags {
names = append(names, name)
}
return names
}
23 changes: 21 additions & 2 deletions frontend/dockerfile/instructions/commands_runmount.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strconv"
"strings"

"github.com/moby/buildkit/util/suggest"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -57,6 +58,20 @@ func isValidMountType(s string) bool {
return ok
}

func allMountTypes() []string {
types := make([]string, 0, len(allowedMountTypes)+2)
for k := range allowedMountTypes {
types = append(types, k)
}
if isSecretMountsSupported() {
types = append(types, "secret")
}
if isSSHMountsSupported() {
types = append(types, "ssh")
}
return types
}

func runMountPreHook(cmd *RunCommand, req parseRequest) error {
st := &mountState{}
st.flag = req.flags.AddStrings("mount")
Expand Down Expand Up @@ -173,10 +188,11 @@ func parseMount(value string, expander SingleWordExpander) (*Mount, error) {
// if we don't have an expander, defer evaluation to later
continue
}

switch key {
case "type":
if !isValidMountType(strings.ToLower(value)) {
return nil, errors.Errorf("unsupported mount type %q", value)
return nil, suggest.WrapError(errors.Errorf("unsupported mount type %q", value), value, allMountTypes(), true)
}
m.Type = strings.ToLower(value)
case "from":
Expand Down Expand Up @@ -234,7 +250,10 @@ func parseMount(value string, expander SingleWordExpander) (*Mount, error) {
}
m.GID = &gid
default:
return nil, errors.Errorf("unexpected key '%s' in '%s'", key, field)
allKeys := []string{
"type", "from", "source", "target", "readonly", "id", "sharing", "required", "mode", "uid", "gid", "src", "dst", "ro", "rw", "readwrite",
}
return nil, suggest.WrapError(errors.Errorf("unexpected key '%s' in '%s'", key, field), key, allKeys, true)
}
}

Expand Down
20 changes: 15 additions & 5 deletions frontend/dockerfile/instructions/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/docker/docker/api/types/strslice"
"github.com/moby/buildkit/frontend/dockerfile/command"
"github.com/moby/buildkit/frontend/dockerfile/parser"
"github.com/moby/buildkit/util/suggest"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -63,7 +64,7 @@ func ParseInstruction(node *parser.Node) (v interface{}, err error) {
err = parser.WithLocation(err, node.Location())
}()
req := newParseRequestFromNode(node)
switch node.Value {
switch strings.ToLower(node.Value) {
case command.Env:
return parseEnv(req)
case command.Maintainer:
Expand Down Expand Up @@ -101,8 +102,7 @@ func ParseInstruction(node *parser.Node) (v interface{}, err error) {
case command.Shell:
return parseShell(req)
}

return nil, &UnknownInstruction{Instruction: node.Value, Line: node.StartLine}
return nil, suggest.WrapError(&UnknownInstruction{Instruction: node.Value, Line: node.StartLine}, node.Value, allInstructionNames(), false)
}

// ParseCommand converts an AST to a typed Command
Expand All @@ -124,7 +124,7 @@ type UnknownInstruction struct {
}

func (e *UnknownInstruction) Error() string {
return fmt.Sprintf("unknown instruction: %s", strings.ToUpper(e.Instruction))
return fmt.Sprintf("unknown instruction: %s", e.Instruction)
}

type parseError struct {
Expand All @@ -133,7 +133,7 @@ type parseError struct {
}

func (e *parseError) Error() string {
return fmt.Sprintf("dockerfile parse error line %d: %v", e.node.StartLine, e.inner.Error())
return fmt.Sprintf("dockerfile parse error on line %d: %v", e.node.StartLine, e.inner.Error())
}

func (e *parseError) Unwrap() error {
Expand Down Expand Up @@ -755,3 +755,13 @@ func getComment(comments []string, name string) string {
}
return ""
}

func allInstructionNames() []string {
out := make([]string, len(command.Commands))
i := 0
for name := range command.Commands {
out[i] = strings.ToUpper(name)
i++
}
return out
}