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

Correctly ignore files from .dockerignore #508

Merged
merged 14 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from 12 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
1 change: 0 additions & 1 deletion examples/examples_dotnet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ func TestNginxCs(t *testing.T) {
}

func TestDotNet(t *testing.T) {

test := getCsharpBaseOptions(t).
With(integration.ProgramTestOptions{
Dir: path.Join(getCwd(t), "dotnet"),
Expand Down
1 change: 0 additions & 1 deletion examples/examples_nodejs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ func TestNginx(t *testing.T) {
}

func TestDockerfileWithMultipleTargets(t *testing.T) {

test := getJsOptions(t).
With(integration.ProgramTestOptions{
Dir: path.Join(getCwd(t), "dockerfile-with-targets"),
Expand Down
35 changes: 31 additions & 4 deletions provider/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"net"

"github.com/docker/distribution/reference"
"github.com/moby/buildkit/session"
"github.com/moby/moby/registry"
"net"
"path/filepath"

"github.com/docker/cli/cli/config"
"github.com/docker/cli/cli/config/configfile"
Expand All @@ -23,6 +23,8 @@ import (
controlapi "github.com/moby/buildkit/api/services/control"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin"

buildCmd "github.com/docker/cli/cli/command/image/build"
)

const defaultDockerfile = "Dockerfile"
Expand Down Expand Up @@ -90,8 +92,33 @@ func (p *dockerNativeProvider) dockerBuild(ctx context.Context,
return "", nil, err
}

// make the build context
tar, err := archive.TarWithOptions(img.Build.Context, &archive.TarOptions{})
// make the build context and ensure to exclude dockerignore file patterns
dockerIgnorePath := filepath.Join(build.Context, ".dockerignore")
ignorePatterns, err := getIgnore(dockerIgnorePath)
ignorePatterns = buildCmd.TrimBuildFilesFromExcludes(ignorePatterns, img.Build.Dockerfile, false)

// warn user about accidentally copying build files
if build.BuilderVersion == defaultBuilder {
for _, pattern := range ignorePatterns {
if pattern == "!Dockerfile" || pattern == "!.Dockerignore" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if these patterns will fire - .dockerignore is typically lowercase.

Rather than try to check if the pattern matches, should we store the ignorePatterns on line 97 in a different variable from the return value of buildCmd.TrimBuildFilesFromExcludes and check len(before) != len(after)?

Copy link
Contributor

Choose a reason for hiding this comment

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

	ignorePatterns, err := getIgnore(dockerIgnorePath)
	// copy ignore patterns to compare
	initialIgnorePatterns := append([]string, ignorePatterns...)
	
	ignorePatterns = buildCmd.TrimBuildFilesFromExcludes(ignorePatterns, img.Build.Dockerfile, false)

	// warn user about accidentally copying build files, link to GitHub Issue: 
	if build.BuilderVersion == defaultBuilder && len(initialIgnorePatterns) != len(ignorePatterns) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oooh, I like it! good idea!

msg := "It looks like you are trying to dockerignore a build file such as Dockerfile or .dockerignore. " +
"Due to limitations when running this provider in Buildkit mode, your build files may get copied " +
"into your image. Please ensure any copied file systems do not include build files."
Copy link
Contributor

Choose a reason for hiding this comment

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

This only applies to .dockerignore and the Dockerfile. Perhaps we should tone this message down, that seems very low impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how is this?

err = p.host.Log(ctx, "warning", urn, msg)
if err != nil {
return "", nil, err
}
}
}
}

if err != nil {
return "", nil, err
}

tar, err := archive.TarWithOptions(img.Build.Context, &archive.TarOptions{
ExcludePatterns: ignorePatterns,
})
if err != nil {
return "", nil, err
}
Expand Down
35 changes: 20 additions & 15 deletions provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,22 +483,10 @@ func (accumulator *contextHashAccumulator) hexSumContext() string {

func hashContext(dockerContextPath string, dockerfile string) (string, error) {
// exclude all files listed in dockerignore
dockerIgnorePath := dockerfile + ".dockerignore"
dockerIgnore, err := os.ReadFile(dockerIgnorePath)
if err != nil {
if os.IsNotExist(err) {
dockerIgnorePath = filepath.Join(dockerContextPath, ".dockerignore")
dockerIgnore, err = os.ReadFile(dockerIgnorePath)
if err != nil && !os.IsNotExist(err) {
return "", fmt.Errorf("unable to read %s file: %w", dockerIgnorePath, err)
}
} else {
return "", fmt.Errorf("unable to read %s file: %w", dockerIgnorePath, err)
}
}
ignorePatterns, err := dockerignore.ReadAll(bytes.NewReader(dockerIgnore))
dockerIgnorePath := filepath.Join(dockerContextPath, ".dockerignore")
ignorePatterns, err := getIgnore(dockerIgnorePath)
if err != nil {
return "", fmt.Errorf("unable to parse %s file: %w", dockerIgnorePath, err)
return "", err
}
ignoreMatcher, err := fileutils.NewPatternMatcher(ignorePatterns)
if err != nil {
Expand Down Expand Up @@ -554,3 +542,20 @@ func hashContext(dockerContextPath string, dockerfile string) (string, error) {
// create a hash of the entire input of the hash accumulator
return accumulator.hexSumContext(), nil
}

func getIgnore(dockerIgnorePath string) ([]string, error) {
var ignorePatterns []string
dockerIgnore, err := os.ReadFile(dockerIgnorePath)
if err != nil {
if os.IsNotExist(err) {
// .dockerignore files are optional
Copy link
Contributor

Choose a reason for hiding this comment

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

great comment! i found it helpful

return ignorePatterns, nil
}
return ignorePatterns, fmt.Errorf("unable to read %s file: %w", dockerIgnorePath, err)
}
ignorePatterns, err = dockerignore.ReadAll(bytes.NewReader(dockerIgnore))
if err != nil {
return ignorePatterns, fmt.Errorf("unable to parse %s file: %w", ".dockerignore", err)
}
return ignorePatterns, nil
}