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.

Sign up for GitHub

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

Jump to bottom

port Docker updater improvements from Azure DevOps #8192

Merged
merged 2 commits into from
Nov 30, 2023
Merged

port Docker updater improvements from Azure DevOps #8192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d595bc
Add support for digests when updating helm charts
ryanbrandenburg Oct 25, 2023
650e132
Allow docker version prefixes to contain underscores
ryanbrandenburg Oct 25, 2023
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
26 changes: 15 additions & 11 deletions docker/lib/dependabot/docker/file_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ class FileParser < Dependabot::FileParsers::Base

FROM = /FROM/i
PLATFORM = /--platform\=(?<platform>\S+)/
TAG = /:(?<tag>[\w][\w.-]{0,127})/
TAG_NO_PREFIX = /(?<tag>[\w][\w.-]{0,127})/
TAG = /:#{TAG_NO_PREFIX}/
DIGEST = /(?<digest>[0-9a-f]{64})/
NAME = /\s+AS\s+(?<name>[\w-]+)/
FROM_LINE =
%r{^#{FROM}\s+(#{PLATFORM}\s+)?(#{REGISTRY}/)?
#{IMAGE}#{TAG}?(?:@sha256:#{DIGEST})?#{NAME}?}x
TAG_WITH_DIGEST = /^#{TAG_NO_PREFIX}(?:@sha256:#{DIGEST})?/x

AWS_ECR_URL = /dkr\.ecr\.(?<region>[^.]+)\.amazonaws\.com/

Expand Down Expand Up @@ -168,18 +170,20 @@ def manifest_files

def parse_helm(img_hash)
repo = img_hash.fetch("repository", nil)
tag = img_hash.key?("tag") ? img_hash.fetch("tag", nil) : img_hash.fetch("version", nil)
tag_value = img_hash.key?("tag") ? img_hash.fetch("tag", nil) : img_hash.fetch("version", nil)
registry = img_hash.fetch("registry", nil)

if !repo.nil? && !registry.nil? && !tag.nil?
["#{registry}/#{repo}:#{tag}"]
elsif !repo.nil? && !tag.nil?
["#{repo}:#{tag}"]
elsif !repo.nil?
[repo]
else
[]
end
tag_details = tag_value.to_s.match(TAG_WITH_DIGEST).named_captures
tag = tag_details["tag"]
digest = tag_details["digest"]

return [] unless repo
return [repo] unless tag

image = "#{repo}:#{tag}"
image.prepend("#{registry}/") if registry
image.append("@sha256:#{digest}/") if digest
[image]
end
end
end
Expand Down
18 changes: 11 additions & 7 deletions docker/lib/dependabot/docker/file_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def update_helm(file, content)
old_tags.each do |old_tag|
old_tag_regex = /^\s+(?:-\s)?(?:tag|version):\s+["']?#{old_tag}["']?(?=\s|$)/
modified_content = modified_content.gsub(old_tag_regex) do |old_img_tag|
old_img_tag.gsub(old_tag.to_s, new_yaml_tag(file).to_s)
old_img_tag.gsub(old_tag.to_s, new_helm_tag(file).to_s)
brettfo marked this conversation as resolved.
Show resolved Hide resolved
end
end
modified_content
Expand Down Expand Up @@ -187,11 +187,6 @@ def new_yaml_image(file)
"#{prefix}#{dependency.name}#{tag}#{digest}"
end

def new_yaml_tag(file)
element = dependency.requirements.find { |r| r[:file] == file.name }
element.fetch(:source)[:tag] || ""
end

def old_yaml_images(file)
previous_requirements(file).map do |r|
prefix = r.fetch(:source)[:registry] ? "#{r.fetch(:source)[:registry]}/" : ""
Expand All @@ -203,10 +198,19 @@ def old_yaml_images(file)

def old_helm_tags(file)
previous_requirements(file).map do |r|
r.fetch(:source)[:tag] || ""
tag = r.fetch(:source)[:tag] || ""
digest = r.fetch(:source)[:digest] ? "@sha256:#{r.fetch(:source)[:digest]}" : ""
"#{tag}#{digest}"
end
end

def new_helm_tag(file)
element = dependency.requirements.find { |r| r[:file] == file.name }
tag = element.fetch(:source)[:tag] || ""
digest = element.fetch(:source)[:digest] ? "@sha256:#{element.fetch(:source)[:digest]}" : ""
"#{tag}#{digest}"
end

def requirements(file)
dependency.requirements
.select { |r| r[:file] == file.name }
Expand Down
4 changes: 2 additions & 2 deletions docker/lib/dependabot/docker/tag.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
WORDS_WITH_BUILD = /(?:(?:-[a-z]+)+-[0-9]+)+/
VERSION_REGEX = /v?(?<version>[0-9]+(?:\.[0-9]+)*(?:_[0-9]+|\.[a-z0-9]+|#{WORDS_WITH_BUILD}|-(?:kb)?[0-9]+)*)/i
VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.\-]*)?$/i
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-]*-)?#{VERSION_REGEX}$/i
VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-_]*-)?#{VERSION_REGEX}$/i

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '0-a-' and containing many repetitions of '0-a-'.
Copy link
Contributor

Choose a reason for hiding this comment

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

@abdulapopoola I'm not the greatest regex wizard but I think this would be a pre-existing condition (IE, this was not caused by our change to this REGEX but was already "inefficient" and just somehow grandfather-ed in). Do you have any advise for resolving this given that that's the case.

Copy link
Member

Choose a reason for hiding this comment

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

As a team, I think we accept RE as necessary and that codeQL flagging might be too sensitive; tagging @deivid-rodriguez and @jakecoffman to get their thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine for CodeQL to flag these but I don't like it that it flags issues on any changed lines, even if they were present also before the changes. It places unnecessary burden on contributions like here.

Anyways, I believe it's fine to ignore.

Still, I had a look and I think this change may fix it, and it does not seem to break any specs:

diff --git a/docker/lib/dependabot/docker/tag.rb b/docker/lib/dependabot/docker/tag.rb
index 65c21e267..8eb805614 100644
--- a/docker/lib/dependabot/docker/tag.rb
+++ b/docker/lib/dependabot/docker/tag.rb
@@ -8,7 +8,7 @@ module Dependabot
     class Tag
       WORDS_WITH_BUILD = /(?:(?:-[a-z]+)+-[0-9]+)+/
       VERSION_REGEX = /v?(?<version>[0-9]+(?:\.[0-9]+)*(?:_[0-9]+|\.[a-z0-9]+|#{WORDS_WITH_BUILD}|-(?:kb)?[0-9]+)*)/i
-      VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.\-]*)?$/i
+      VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.]*)?$/i
       VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-]*-)?#{VERSION_REGEX}$/i
       VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i
       NAME_WITH_VERSION =

Just in case we want to change that separately.

VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-_]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i

Check failure

Code scanning / CodeQL

Inefficient regular expression High

This part of the regular expression may cause exponential backtracking on strings starting with '0-a-' and containing many repetitions of '0-a-'.
NAME_WITH_VERSION =
/
#{VERSION_WITH_PFX}|
Expand Down
28 changes: 28 additions & 0 deletions docker/spec/dependabot/docker/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,34 @@
end
end

context "with a _ in the tag" do
let(:dockerfile_fixture_name) { "underscore" }

its(:length) { is_expected.to eq(1) }

describe "the first dependency" do
subject(:dependency) { dependencies.first }
let(:expected_requirements) do
[{
requirement: nil,
groups: [],
file: "Dockerfile",
source: {
registry: "registry-host.io:5000",
tag: "someRepo_19700101.4"
}
}]
end

it "has the right details" do
expect(dependency).to be_a(Dependabot::Dependency)
expect(dependency.name).to eq("myreg/ubuntu")
expect(dependency.version).to eq("someRepo_19700101.4")
expect(dependency.requirements).to eq(expected_requirements)
end
end
end

context "with a private registry and a tag" do
let(:dockerfile_fixture_name) { "private_tag" }

Expand Down
82 changes: 82 additions & 0 deletions docker/spec/dependabot/docker/file_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,48 @@
its(:content) { is_expected.to include "image:\n repository: 'nginx'\n tag: 1.14.3\n" }
end

context "when a digest is used" do
let(:helmfile) do
Dependabot::DependencyFile.new(
content: helmfile_body,
name: "values.yaml"
)
end
let(:helmfile_body) { fixture("helm", "yaml", "digest.yaml") }
let(:helm_dependency) do
Dependabot::Dependency.new(
name: "ubuntu",
version: "sha256:c9cf959fd83770dfdefd8fb42cfef0761432af36a764c077aed54bbc5bb25368",
previous_version: "sha256:295c7be079025306c4f1d65997fcf7adb411c88f139ad1d34b537164aa060369",
requirements: [{
requirement: nil,
groups: [],
file: "values.yaml",
source: { tag: "sha256:c9cf959fd83770dfdefd8fb42cfef0761432af36a764c077aed54bbc5bb25368" }
}],
previous_requirements: [{
requirement: nil,
groups: [],
file: "values.yaml",
source: { tag: "sha256:295c7be079025306c4f1d65997fcf7adb411c88f139ad1d34b537164aa060369" }
}],
package_manager: "docker"
)
end

its(:length) { is_expected.to eq(1) }

describe "the updated helmfile" do
subject(:updated_helmfile) do
updated_files.find { |f| f.name == "values.yaml" }
end

its(:content) do
is_expected.to include "version: 'sha256:c9cf959fd83770dfdefd8fb42cfef0761432af36a7"
end
end
end

context "when there are multiple images" do
let(:helmfile) do
Dependabot::DependencyFile.new(
Expand Down Expand Up @@ -1316,6 +1358,46 @@
end
end

context "with version number" do
let(:helmfile) do
Dependabot::DependencyFile.new(
content: helmfile_body,
name: "values.yaml"
)
end
let(:helmfile_body) { fixture("helm", "yaml", "values.yaml") }
let(:helm_dependency) do
Dependabot::Dependency.new(
name: "nginx",
version: "1.14.3",
previous_version: "1.14.2",
requirements: [{
requirement: nil,
groups: [],
file: "values.yaml",
source: { tag: "1.14.3" }
}],
previous_requirements: [{
requirement: nil,
groups: [],
file: "values.yaml",
source: { tag: "1.14.2" }
}],
package_manager: "docker"
)
end

its(:length) { is_expected.to eq(1) }

describe "the updated helmfile" do
subject(:updated_helmfile) do
updated_files.find { |f| f.name == "values.yaml" }
end

its(:content) { is_expected.to include "image:\n repository: 'nginx'\n tag: 1.14.3\n" }
end
end

brettfo marked this conversation as resolved.
Show resolved Hide resolved
context "when version has single quotes" do
let(:helmfile) do
Dependabot::DependencyFile.new(
Expand Down
18 changes: 18 additions & 0 deletions docker/spec/fixtures/docker/dockerfiles/underscore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
FROM registry-host.io:5000/myreg/ubuntu:someRepo_19700101.4

### SYSTEM DEPENDENCIES

RUN apt-get update \
&& apt-get upgrade -y \
&& apt-get install -y --no-install-recommends \
build-essential \
dirmngr \
git \

### RUBY

# Install Ruby 2.4
RUN apt-key adv --keyserver keyserver.ubuntu.com --recv-keys C3173AA6 \
&& echo "deb http://ppa.launchpad.net/brightbox/ruby-ng/ubuntu zesty main" > /etc/apt/sources.list.d/brightbox.list \
&& apt-get update
RUN apt-get install -y ruby2.4 ruby2.4-dev
7 changes: 7 additions & 0 deletions docker/spec/fixtures/helm/yaml/digest.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
foo:
image: nginx:1.14.2

bar:
image:
repository: 'canonical/ubuntu'
version: 'sha256:295c7be079025306c4f1d65997fcf7adb411c88f139ad1d34b537164aa060369'