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

feat: add dockerfile support to skaffold lint and top 2 dockerfile rules #6793

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions cmd/skaffold/app/cmd/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/spf13/cobra"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/lint"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
)

var outFormat string
Expand All @@ -35,15 +36,21 @@ func NewCmdLint() *cobra.Command {
{Value: &outFormat, Name: "format", DefValue: lint.PlainTextOutput,
Usage: "Output format. One of: plain-text(default) or json"}}).
Hidden().
NoArgs(lintCmd)
NoArgs(doLint)
}

func lintCmd(ctx context.Context, out io.Writer) error {
func doLint(ctx context.Context, out io.Writer) error {
// createRunner initializes state for objects (Docker client, etc.) lint uses
_, _, runCtx, err := createRunner(ctx, out, opts)
log.Entry(ctx).Debugf("starting skaffold lint with runCtx: %v", runCtx)
if err != nil {
return err
}
return lint.Lint(ctx, out, lint.Options{
Filename: opts.ConfigurationFile,
RepoCacheDir: opts.RepoCacheDir,
OutFormat: outFormat,
Modules: opts.ConfigurationFilter,
Profiles: opts.Profiles,
})
}, runCtx)
}
73 changes: 71 additions & 2 deletions pkg/skaffold/docker/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ func GetDependencies(ctx context.Context, buildCfg BuildConfig, cfg Config) ([]s
return resultPair(result)
}

// GetDependencies finds the sources dependency for the given docker artifact.
// it caches the results for the computed dependency which can be used by `GetDependenciesCached`
// All paths are relative to the workspace.
func GetDependenciesByDockerCopyFromTo(ctx context.Context, buildCfg BuildConfig, cfg Config) (map[string][]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(buildCfg.workspace, buildCfg.dockerfilePath)
if err != nil {
return nil, fmt.Errorf("normalizing dockerfilePath path: %w", err)
}
ftToDependencies := getDependenciesByDockerCopyFromTo(ctx, buildCfg.workspace, buildCfg.dockerfilePath, absDockerfilePath, buildCfg.args, cfg)
return resultPairForDockerCopyFromTo(ftToDependencies)
}

// GetDependenciesCached reads from cache finds the sources dependency for the given docker artifact.
// All paths are relative to the workspace.
func GetDependenciesCached(ctx context.Context, buildCfg BuildConfig, cfg Config) ([]string, error) {
Expand All @@ -103,6 +115,17 @@ func resultPair(deps interface{}) ([]string, error) {
}
}

func resultPairForDockerCopyFromTo(deps interface{}) (map[string][]string, error) {
switch t := deps.(type) {
case error:
return nil, t
case map[string][]string:
return t, nil
default:
return nil, fmt.Errorf("internal error when retrieving cache result of type %T", t)
}
}

func getDependencies(ctx context.Context, workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) interface{} {
// If the Dockerfile doesn't exist, we can't compute the dependency.
// But since we know the Dockerfile is a dependency, let's return a list
Expand All @@ -112,7 +135,7 @@ func getDependencies(ctx context.Context, workspace string, dockerfilePath strin
return []string{dockerfilePath}
}

fts, err := readCopyCmdsFromDockerfile(ctx, false, absDockerfilePath, workspace, buildArgs, cfg)
fts, err := ReadCopyCmdsFromDockerfile(ctx, false, absDockerfilePath, workspace, buildArgs, cfg)
if err != nil {
return err
}
Expand All @@ -124,7 +147,7 @@ func getDependencies(ctx context.Context, workspace string, dockerfilePath strin

deps := make([]string, 0, len(fts))
for _, ft := range fts {
deps = append(deps, ft.from)
deps = append(deps, ft.From)
}

files, err := WalkWorkspace(workspace, excludes, deps)
Expand All @@ -151,6 +174,52 @@ func getDependencies(ctx context.Context, workspace string, dockerfilePath strin
return dependencies
}

func getDependenciesByDockerCopyFromTo(ctx context.Context, workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) interface{} {
// If the Dockerfile doesn't exist, we can't compute the dependency.
// But since we know the Dockerfile is a dependency, let's return a list
// with only that file. It makes errors down the line more actionable
// than returning an error now.
if _, err := os.Stat(absDockerfilePath); os.IsNotExist(err) {
return []string{dockerfilePath}
}

fts, err := ReadCopyCmdsFromDockerfile(ctx, false, absDockerfilePath, workspace, buildArgs, cfg)
if err != nil {
return err
}

excludes, err := readDockerignore(workspace, absDockerfilePath)
if err != nil {
return fmt.Errorf("reading .dockerignore: %w", err)
}

ftToDependencies := map[string][]string{}
for _, ft := range fts {
files, err := WalkWorkspace(workspace, excludes, []string{ft.From})
if err != nil {
return fmt.Errorf("walking workspace: %w", err)
}

// Always add dockerfile even if it's .dockerignored. The daemon will need it anyways.
if !filepath.IsAbs(dockerfilePath) {
files[dockerfilePath] = true
} else {
files[absDockerfilePath] = true
}

// Ignore .dockerignore
delete(files, ".dockerignore")

var dependencies []string
for file := range files {
dependencies = append(dependencies, file)
}
sort.Strings(dependencies)
ftToDependencies[ft.String()] = dependencies
}
return ftToDependencies
}

// readDockerignore reads patterns to ignore
func readDockerignore(workspace string, absDockerfilePath string) ([]string, error) {
var excludes []string
Expand Down
77 changes: 62 additions & 15 deletions pkg/skaffold/docker/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,23 @@ import (
"github.com/GoogleContainerTools/skaffold/proto/v1"
)

type FromTo struct {
// From is the relative path (wrt. the skaffold root directory) of the dependency on the host system.
From string
// To is the destination location in the container. Must use slashes as path separator.
To string
// ToIsDir indicates if the `to` path must be treated as directory
ToIsDir bool
// StartLine indicates the starting line in the dockerfile of the copy command
StartLine int
// EndLine indiciates the ending line in the dockerfile of the copy command
EndLine int
}

func (f *FromTo) String() string {
return fmt.Sprintf("From:%s, To:%s, ToIsDir:%t, StartLine: %d, EndLine: %d", f.From, f.To, f.ToIsDir, f.StartLine, f.EndLine)
}

type from struct {
image string
as string
Expand All @@ -55,23 +72,20 @@ type copyCommand struct {
dest string
// destIsDir indicates if dest must be treated as directory.
destIsDir bool
}

type fromTo struct {
// from is the relative path (wrt. the skaffold root directory) of the dependency on the host system.
from string
// to is the destination location in the container. Must use slashes as path separator.
to string
// toIsDir indicates if the `to` path must be treated as directory
toIsDir bool
// startLine indicates the starting line in the dockerfile of the copy command
startLine int
// endLine indiciates the ending line in the dockerfile of the copy command
endLine int
}

var (
// RetrieveImage is overridden for unit testing
RetrieveImage = retrieveImage
)

func readCopyCmdsFromDockerfile(ctx context.Context, onlyLastImage bool, absDockerfilePath, workspace string, buildArgs map[string]*string, cfg Config) ([]fromTo, error) {
// ReadCopyCmdsFromDockerfile parses a given dockerfile for COPY commands accounting for build args, env vars, globs, etc
// and returns an array of FromTos specifying the files that will be copied 'from' local dirs 'to' container dirs in the COPY statements
func ReadCopyCmdsFromDockerfile(ctx context.Context, onlyLastImage bool, absDockerfilePath, workspace string, buildArgs map[string]*string, cfg Config) ([]FromTo, error) {
r, err := ioutil.ReadFile(absDockerfilePath)
if err != nil {
return nil, err
Expand Down Expand Up @@ -105,6 +119,38 @@ func readCopyCmdsFromDockerfile(ctx context.Context, onlyLastImage bool, absDock
return expandSrcGlobPatterns(workspace, cpCmds)
}

func ExtractOnlyCopyCommands(absDockerfilePath string) ([]FromTo, error) {
r, err := ioutil.ReadFile(absDockerfilePath)
if err != nil {
return nil, err
}

res, err := parser.Parse(bytes.NewReader(r))
if err != nil {
return nil, fmt.Errorf("parsing dockerfile %q: %w", absDockerfilePath, err)
}

var copied []FromTo
workdir := "/"
envs := make([]string, 0)
for _, node := range res.AST.Children {
switch node.Value {
case command.Add, command.Copy:
cpCmd, err := readCopyCommand(node, envs, workdir)
if err != nil {
return nil, err
}

if cpCmd != nil && len(cpCmd.srcs) > 0 {
for _, src := range cpCmd.srcs {
copied = append(copied, FromTo{From: src, To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine})
}
}
}
}
return copied, nil
}

// filterUnusedBuildArgs removes entries from the build arguments map that are not found in the dockerfile
func filterUnusedBuildArgs(dockerFile io.Reader, buildArgs map[string]*string) (map[string]*string, error) {
res, err := parser.Parse(dockerFile)
Expand Down Expand Up @@ -162,15 +208,15 @@ func expandBuildArgs(nodes []*parser.Node, buildArgs map[string]*string) error {
return nil
}

func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]fromTo, error) {
var fts []fromTo
func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]FromTo, error) {
var fts []FromTo
for _, cpCmd := range cpCmds {
matchesOne := false

for _, p := range cpCmd.srcs {
path := filepath.Join(workspace, p)
if _, err := os.Stat(path); err == nil {
fts = append(fts, fromTo{from: filepath.Clean(p), to: cpCmd.dest, toIsDir: cpCmd.destIsDir})
fts = append(fts, FromTo{From: filepath.Clean(p), To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine})
matchesOne = true
continue
}
Expand All @@ -189,7 +235,7 @@ func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]fromTo, e
return nil, fmt.Errorf("getting relative path of %s", f)
}

fts = append(fts, fromTo{from: rel, to: cpCmd.dest, toIsDir: cpCmd.destIsDir})
fts = append(fts, FromTo{From: rel, To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine})
}
matchesOne = true
}
Expand Down Expand Up @@ -266,7 +312,6 @@ func extractCopyCommands(ctx context.Context, nodes []*parser.Node, onlyLastImag
}
}
}

return copied, nil
}

Expand Down Expand Up @@ -311,6 +356,8 @@ func readCopyCommand(value *parser.Node, envs []string, workdir string) (*copyCo
srcs: srcs,
dest: dest,
destIsDir: destIsDir,
startLine: value.StartLine,
endLine: value.EndLine,
}, nil
}

Expand Down
18 changes: 9 additions & 9 deletions pkg/skaffold/docker/syncmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func SyncMap(ctx context.Context, workspace string, dockerfilePath string, build
}

// only the COPY/ADD commands from the last image are syncable
fts, err := readCopyCmdsFromDockerfile(ctx, true, absDockerfilePath, workspace, buildArgs, cfg)
fts, err := ReadCopyCmdsFromDockerfile(ctx, true, absDockerfilePath, workspace, buildArgs, cfg)
if err != nil {
return nil, err
}
Expand All @@ -57,7 +57,7 @@ func SyncMap(ctx context.Context, workspace string, dockerfilePath string, build
// walkWorkspaceWithDestinations walks the given host directories and determines their
// location in the container. It returns a map of host path by container destination.
// Note: if you change this function, you might also want to modify `WalkWorkspace`.
func walkWorkspaceWithDestinations(workspace string, excludes []string, fts []fromTo) (map[string]string, error) {
func walkWorkspaceWithDestinations(workspace string, excludes []string, fts []FromTo) (map[string]string, error) {
dockerIgnored, err := NewDockerIgnorePredicate(workspace, excludes)
if err != nil {
return nil, err
Expand All @@ -66,7 +66,7 @@ func walkWorkspaceWithDestinations(workspace string, excludes []string, fts []fr
// Walk the workspace
srcByDest := make(map[string]string)
for _, ft := range fts {
absFrom := filepath.Join(workspace, ft.from)
absFrom := filepath.Join(workspace, ft.From)

fi, err := os.Stat(absFrom)
if err != nil {
Expand Down Expand Up @@ -100,23 +100,23 @@ func walkWorkspaceWithDestinations(workspace string, excludes []string, fts []fr
return err
}

srcByDest[path.Join(ft.to, filepath.ToSlash(relBase))] = relPath
srcByDest[path.Join(ft.To, filepath.ToSlash(relBase))] = relPath
return nil
}); err != nil {
return nil, fmt.Errorf("walking %q: %w", absFrom, err)
}
case mode.IsRegular():
ignored, err := dockerIgnored(filepath.Join(workspace, ft.from), fi)
ignored, err := dockerIgnored(filepath.Join(workspace, ft.From), fi)
if err != nil {
return nil, err
}

if !ignored {
if ft.toIsDir {
base := filepath.Base(ft.from)
srcByDest[path.Join(ft.to, base)] = ft.from
if ft.ToIsDir {
base := filepath.Base(ft.From)
srcByDest[path.Join(ft.To, base)] = ft.From
} else {
srcByDest[ft.to] = ft.from
srcByDest[ft.To] = ft.From
}
}
}
Expand Down
Loading