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

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Nov 2, 2021

fixes #6103 by adding 2 dockerfile lint rules:

  • DockerfileCopyOver1000Files
  • DockerfileCopyContainsGitDir

The rules have the following output when triggered (note skaffold lint is run against modified version of examples/microservices here w/ misconfigurations added to the Dockerfiles, not the actual examples/microservices):

$ skaffold lint
leeroy-web/Dockerfile:3:1: ID000005: DockerfileCommandLintRule: Found 'COPY . .', where the source dir: "." contains a '.git'
 directory at /usr/local/google/home/aprindle/lint-samples/leeroy-web/.git.  This has the potential to dramatically slow 'skaffold
 dev' down as skaffold whill watch all of the files in the .git directory as skaffold watches all sources files referenced in 
dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical files are 
modified during any git related operation. Consider adding a .dockerignore file 
(https://docs.docker.com/engine/reference/builder/#dockerignore-file) removing the '.git' directory. skaffold respects files 
ignored via the .dockerignore
COPY . .
^
leeroy-app/Dockerfile:3:1: ID000004: DockerfileCommandLintRule: Found 'COPY . .', where the source dir: "." has over 1000
 files.  This has the potential to dramatically slow 'skaffold dev' down as skaffold watches all sources files referenced in 
dockerfile COPY directives for changes. If you notice skaffold rebuilding images unnecessarily when non-image-critical files 
are modified, consider changing this to 'COPY $REQUIRED_SOURCE_FILE(s) .' for each required source file instead of or
 adding a .dockerignore file (https://docs.docker.com/engine/reference/builder/#dockerignore-file) ignoring non-image-critical
 files.  skaffold respects files ignored via the .dockerignore
COPY . .
^

@aaron-prindle aaron-prindle requested a review from a team as a code owner November 2, 2021 23:23
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch 2 times, most recently from 2820aa1 to ea4895c Compare November 2, 2021 23:25
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #6793 (4ae997e) into main (290280e) will decrease coverage by 1.14%.
The diff coverage is 62.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6793      +/-   ##
==========================================
- Coverage   70.48%   69.33%   -1.15%     
==========================================
  Files         515      543      +28     
  Lines       23150    24768    +1618     
==========================================
+ Hits        16317    17173     +856     
- Misses       5776     6454     +678     
- Partials     1057     1141      +84     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/flags.go 89.00% <0.00%> (-1.82%) ⬇️
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/cmd/cmd.go 70.49% <75.00%> (-0.57%) ⬇️
cmd/skaffold/app/cmd/delete.go 69.23% <80.00%> (+13.67%) ⬆️
cmd/skaffold/app/cmd/debug.go 100.00% <100.00%> (ø)
... and 138 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1081e6...4ae997e. Read the comment docs.

@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch from ea4895c to 1f44d8d Compare November 2, 2021 23:51
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

did a very quick pass on this and just have some nits, will take a deeper look and comb through the logic more

pkg/skaffold/lint/linters.go Outdated Show resolved Hide resolved
pkg/skaffold/lint/types.go Outdated Show resolved Hide resolved
pkg/skaffold/lint/linters.go Outdated Show resolved Hide resolved
pkg/skaffold/lint/linters.go Outdated Show resolved Hide resolved
@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch from 1f44d8d to 0f117d9 Compare November 3, 2021 00:05
pkg/skaffold/lint/dockerfiles.go Outdated Show resolved Hide resolved
pkg/skaffold/lint/dockerfiles_test.go Outdated Show resolved Hide resolved
@MarlonGamez MarlonGamez self-assigned this Nov 3, 2021
@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch from 0f117d9 to a0c6e22 Compare November 3, 2021 18:23
@MarlonGamez
Copy link
Contributor

MarlonGamez commented Nov 4, 2021

Found a bug when testing locally, not completely sure what's happening though. Seems like the lint condition is firing when it shouldn't be.

Running this in the Cloud Code Go Getting Started sample app.

Output:

❯ skaf-dev lint
/skaffold.yaml:3:13: ID000001: YamlFieldLintRule: Found 'apiVersion' field with value that is not the latest skaffold apiVersion. Modify the apiVersion to the latest version: `apiVersion: skaffold/v2beta25` or run the 'skaffold fix' command to have skaffold upgrade this for you.
apiVersion: skaffold/v2beta4
            ^
/skaffold.yaml:19:1: ID000002: YamlFieldLintRule: It is a skaffold best practice to specify a static port (vs skaffold dynamically choosing one) for port forwarding container based resources skaffold deploys.  This is helpful because wit this the local ports are predictable across dev sessions which  makes testing/debugging easier. It is recommended to add the following stanza at the end of your skaffold.yaml for each shown deployed resource:
portForward:
- resourceType: deployment
  resourceName: go-hello-world
  port: 8080
  localPort: 32581

^
Dockerfile:9:1: ID000005: DockerfileCommandLintRule: Found 'COPY go.mod .', where the source dir: "go.mod" contains a '.git' directory at /Users/marlongamez/Projects/go-hello-world/.git.  This has the potential to dramatically slow 'skaffold dev' down as skaffold will watch all of the files in the .git directory as skaffold watches all sources files referenced in dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical files are modified during any git related operation. Consider adding a .dockerignore file (https://docs.docker.com/engine/reference/builder/#dockerignore-file) removing the '.git' directory. skaffold respects files ignored via the .dockerignore
COPY go.mod go.sum ./
^
Dockerfile:9:1: ID000005: DockerfileCommandLintRule: Found 'COPY go.sum .', where the source dir: "go.sum" contains a '.git' directory at /Users/marlongamez/Projects/go-hello-world/.git.  This has the potential to dramatically slow 'skaffold dev' down as skaffold will watch all of the files in the .git directory as skaffold watches all sources files referenced in dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical files are modified during any git related operation. Consider adding a .dockerignore file (https://docs.docker.com/engine/reference/builder/#dockerignore-file) removing the '.git' directory. skaffold respects files ignored via the .dockerignore
COPY go.mod go.sum ./
^
Dockerfile:13:1: ID000005: DockerfileCommandLintRule: Found 'COPY . .', where the source dir: "." contains a '.git' directory at /Users/marlongamez/Projects/go-hello-world/.git.  This has the potential to dramatically slow 'skaffold dev' down as skaffold will watch all of the files in the .git directory as skaffold watches all sources files referenced in dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical files are modified during any git related operation. Consider adding a .dockerignore file (https://docs.docker.com/engine/reference/builder/#dockerignore-file) removing the '.git' directory. skaffold respects files ignored via the .dockerignore
COPY . ./
^
Dockerfile:31:1: ID000005: DockerfileCommandLintRule: Found 'COPY template /template', where the source dir: "template" contains a '.git' directory at /Users/marlongamez/Projects/go-hello-world/.git.  This has the potential to dramatically slow 'skaffold dev' down as skaffold will watch all of the files in the .git directory as skaffold watches all sources files referenced in dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical files are modified during any git related operation. Consider adding a .dockerignore file (https://docs.docker.com/engine/reference/builder/#dockerignore-file) removing the '.git' directory. skaffold respects files ignored via the .dockerignore
COPY template ./template
^

My project directory looks like this:

.
├── .git
├── Dockerfile
├── README.md
├── curl-out
├── go.mod
├── go.sum
├── img
│   ├── create-k8s-cluster.png
│   ├── deploy-config.png
│   ├── deploy-success.png
│   ├── diagram.png
│   ├── edit-configurations.png
│   ├── image-repo.png
│   ├── kubernetes-url.png
│   ├── run-debug-dialog.png
│   └── status-bar.png
├── kubernetes-manifests
│   ├── hello.deployment.yaml
│   └── hello.service.yaml
├── main.go
├── skaffold.yaml
└── template
    ├── index.html
    └── static
        ├── KE-hello-world-dark.svg
        ├── KE-hello-world.svg
        ├── cloud_bg.svg
        ├── dark_bg.svg
        ├── kubernetes-engine-icon.png
        └── lightbulb_icon.svg

It seems to be firing the .git copy error on files such as go.mod, go.sum, and the directory template/

@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch 3 times, most recently from 3563fa8 to 5e675dd Compare November 8, 2021 06:19
@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Nov 8, 2021

Found a bug when testing locally, not completely sure what's happening though. Seems like the lint condition is firing when it shouldn't be.

@MarlonGamez Thanks for finding this issue 🎊. This is related to the lint logic treating a Dockerfile's deps accidentally as all of the deps for each COPY command versus mapping COPY command-> specific deps. What happened above is that each COPY command was flagged as a lint error when a single COPY was invalid when actually it was supposed to only flag the single COPY . <DEST> command. This is fixed now and the unit tests now cover this case explicitly.

@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch 3 times, most recently from 295954d to a28e576 Compare November 8, 2021 18:51
Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

LGTM ✅ Thanks for addressing the commenets/issues. Looks like merge conflicts just need to be fixed then this should be good to merge

@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch 2 times, most recently from fb71f11 to dd78c08 Compare November 9, 2021 01:13
@aaron-prindle aaron-prindle force-pushed the fix-6103_dockerfile-support-v2 branch from dd78c08 to 4ae997e Compare November 9, 2021 17:38
@tejal29 tejal29 merged commit fe0b543 into GoogleContainerTools:main Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Implement fixes for misconfigurations in user Dockerfile
3 participants