Merge pull request #14185 from enoodle/better_identify_container_images

Identifying container images by digest only

app/models/container_image.rb

@@ -36,6 +36,7 @@ class ContainerImage < ApplicationRecord
  after_create :raise_creation_event
 
  def full_name
+ docker_id if image_ref && image_ref.start_with?(DOCKER_PULLABLE_PREFIX)
  result = ""
  result << "#{container_image_registry.full_name}/" unless container_image_registry.nil?
  result << name

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -26,16 +26,10 @@ def ems_inv_to_hashes(inventory)
  get_endpoints(inventory)
  get_services(inventory)
  get_component_statuses(inventory)
- get_images
  EmsRefresh.log_inv_debug_trace(@data, "data:")
  @data
  end
 
- def get_images
- images = @data_index.fetch_path(:container_image, :by_ref_and_registry_host_port).try(:values) || []
- process_collection(images, :container_images) { |n| n }
- end
-
  def get_nodes(inventory)
  process_collection(inventory["node"], :container_nodes) { |n| parse_node(n) }
  @data[:container_nodes].each do |cn|
@@ -691,13 +685,17 @@ def parse_container_image(image, imageID)
  end
  end
 
+ # old docker references won't yield a digest and will always be distinct
+ container_image_identity = container_image[:digest] || container_image[:image_ref]
  stored_container_image = @data_index.fetch_path(
- :container_image, :by_ref_and_registry_host_port, "#{host_port}:#{container_image[:image_ref]}")
+ :container_image, :by_digest, container_image_identity)
 
  if stored_container_image.nil?
  @data_index.store_path(
- :container_image, :by_ref_and_registry_host_port,
- "#{host_port}:#{container_image[:image_ref]}", container_image)
+ :container_image, :by_digest,
+ container_image_identity, container_image
+ )
+ process_collection_item(container_image, :container_images) { |img| img }
  stored_container_image = container_image
  end
 
@@ -768,6 +766,15 @@ def parse_image_name(image, image_ref)
  image_ref_parts = image_definition_re.match(image_ref)
 
  hostname = image_parts[:host] || image_parts[:host2] || image_parts[:localhost]
+ if image_ref.start_with?(ContainerImage::DOCKER_IMAGE_PREFIX) && image_parts[:digest]
+ image_ref = "%{prefix}%{registry}%{name}@%{digest}" % {
+ :prefix => ContainerImage::DOCKER_PULLABLE_PREFIX,
+ :registry => ("#{hostname}:#{image_parts[:port]}/" if hostname && image_parts[:port]),
+ :name => image_parts[:name],
+ :digest => image_parts[:digest],
+ }
+ end
+
  [
  {
  :name => image_parts[:name],

app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb

@@ -14,13 +14,7 @@ def ems_inv_to_hashes(inventory)
  end
 
  def get_openshift_images(inventory)
- process_collection(inventory["image"], :container_images) { |n| parse_openshift_image(n) }
- @data[:container_images].uniq! { |n| n[:image_ref] }
-
- @data[:container_images].each do |ns|
- @data_index.store_path(:container_images, :by_name, ns[:name], ns)
- @data_index.store_path(:container_images, :by_image_ref, ns[:image_ref], ns)
- end
+ inventory["image"].each { |img| parse_openshift_image(img) }
  end
 
  def get_builds(inventory)

spec/models/manageiq/providers/kubernetes/container_manager/refresh_parser_spec.rb

@@ -39,118 +39,139 @@
  describe "parse_image_name" do
  example_ref = "docker://abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"
  example_images = [{:image_name => "example",
+ :image_ref => example_ref,
  :image => {:name => "example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "example:tag",
+ :image_ref => example_ref,
  :image => {:name => "example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "user/example",
+ :image_ref => example_ref,
  :image => {:name => "user/example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "user/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "user/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "example/subname/example",
+ :image_ref => example_ref,
  :image => {:name => "example/subname/example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "example/subname/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "example/subname/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => nil},
 
  {:image_name => "host:1234/subname/example",
+ :image_ref => example_ref,
  :image => {:name => "subname/example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host", :host => "host", :port => "1234"}},
 
  {:image_name => "host:1234/subname/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "subname/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host", :host => "host", :port => "1234"}},
 
  {:image_name => "host.com:1234/subname/example",
+ :image_ref => example_ref,
  :image => {:name => "subname/example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.com", :host => "host.com", :port => "1234"}},
 
  {:image_name => "host.com:1234/subname/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "subname/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.com", :host => "host.com", :port => "1234"}},
 
  {:image_name => "host.com/subname/example",
+ :image_ref => example_ref,
  :image => {:name => "subname/example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.com", :host => "host.com", :port => nil}},
 
  {:image_name => "host.com/example",
+ :image_ref => example_ref,
  :image => {:name => "example", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.com", :host => "host.com", :port => nil}},
 
  {:image_name => "host.com:1234/subname/more/names/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "subname/more/names/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.com", :host => "host.com", :port => "1234"}},
 
  {:image_name => "localhost:1234/name",
+ :image_ref => example_ref,
  :image => {:name => "name", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "localhost", :host => "localhost", :port => "1234"}},
 
  {:image_name => "localhost:1234/name@sha256:1234567abcdefg",
+ :image_ref => example_ref,
  :image => {:name => "name", :tag => nil, :digest => "sha256:1234567abcdefg",
- :image_ref => example_ref},
+ :image_ref => "docker-pullable://localhost:1234/name@sha256:1234567abcdefg"},
  :registry => {:name => "localhost", :host => "localhost", :port => "1234"}},
 
  # host with no port. more than one subdomain (a.b.c.com)
  {:image_name => "reg.access.rh.com/openshift3/image-inspector",
+ :image_ref => example_ref,
  :image => {:name => "openshift3/image-inspector", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "reg.access.rh.com", :host => "reg.access.rh.com", :port => nil}},
 
  # host with port. more than one subdomain (a.b.c.com:1234)
  {:image_name => "host.access.com:1234/subname/more/names/example:tag",
+ :image_ref => example_ref,
  :image => {:name => "subname/more/names/example", :tag => "tag", :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "host.access.com", :host => "host.access.com", :port => "1234"}},
 
  # localhost no port
  {:image_name => "localhost/name",
+ :image_ref => example_ref,
  :image => {:name => "name", :tag => nil, :digest => nil,
  :image_ref => example_ref},
  :registry => {:name => "localhost", :host => "localhost", :port => nil}},
 
  # tag and digest together
  {:image_name => "reg.example.com:1234/name1:tagos@sha256:123abcdef",
+ :image_ref => example_ref,
  :image => {:name => "name1", :tag => "tagos", :digest => "sha256:123abcdef",
- :image_ref => example_ref},
+ :image_ref => "docker-pullable://reg.example.com:1234/name1@sha256:123abcdef"},
  :registry => {:name => "reg.example.com", :host => "reg.example.com", :port => "1234"}},
 
  # digest from new docker-pullable
  {:image_name => "reg.example.com:1234/name1:tagos",
+ :image_ref => "docker-pullable://reg.example.com:1234/name1@sha256:321bcd",
  :image => {:name => "name1", :tag => "tagos", :digest => "sha256:321bcd",
  :image_ref => "docker-pullable://reg.example.com:1234/name1@sha256:321bcd"},
  :registry => {:name => "reg.example.com", :host => "reg.example.com", :port => "1234"}},
 
  {:image_name => "example@sha256:1234567abcdefg",
+ :image_ref => example_ref,
  :image => {:name => "example", :tag => nil, :digest => "sha256:1234567abcdefg",
- :image_ref => example_ref},
+ :image_ref => "docker-pullable://example@sha256:1234567abcdefg"},
  :registry => nil}]
 
  example_images.each do |ex|
  it "tests '#{ex[:image_name]}'" do
- result_image, result_registry = parser.send(:parse_image_name, ex[:image_name], ex[:image][:image_ref])
+ result_image, result_registry = parser.send(:parse_image_name, ex[:image_name], ex[:image_ref])
 
  expect(result_image.except(:registered_on)).to eq(ex[:image])
  expect(result_registry).to eq(ex[:registry])
@@ -595,10 +616,11 @@
  describe "parse_container_image" do
  shared_image_without_host = "shared/image"
  shared_image_with_host = "host:1234/shared/image"
- shared_ref = "shared:ref"
- unique_ref = "unique:ref"
+ shared_ref = "docker-pullable://host:1234/repo/image@sha256:123456"
+ other_registry_ref = "docker-pullable://other-host:4321/repo/image@sha256:123456"
+ unique_ref = "docker-pullable://host:1234/repo/image@sha256:abcdef"
 
- it "returns unique object *identity* for same image but different ref/id" do
+ it "returns unique object *identity* for same image but different digest" do
  [shared_image_with_host, shared_image_without_host].each do |shared_image|
  first_obj = parser.parse_container_image(shared_image, shared_ref)
  second_obj = parser.parse_container_image(shared_image, unique_ref)
@@ -607,7 +629,7 @@
  end
  end
 
- it "returns unique object *content* for same image but different ref/id" do
+ it "returns unique object *content* for same image but different digest" do
  [shared_image_with_host, shared_image_without_host].each do |shared_image|
  first_obj = parser.parse_container_image(shared_image, shared_ref)
  second_obj = parser.parse_container_image(shared_image, unique_ref)
@@ -616,14 +638,23 @@
  end
  end
 
- it "returns same object *identity* for same image and ref/id" do
+ it "returns same object *identity* for same digest" do
  [shared_image_with_host, shared_image_without_host].each do |shared_image|
  first_obj = parser.parse_container_image(shared_image, shared_ref)
  second_obj = parser.parse_container_image(shared_image, shared_ref)
 
  expect(first_obj).to be(second_obj)
  end
  end
+
+ it "returns same object *identity* for same digest and different repo" do
+ [shared_image_with_host, shared_image_without_host].each do |shared_image|
+ first_obj = parser.parse_container_image(shared_image, other_registry_ref)
+ second_obj = parser.parse_container_image(shared_image, shared_ref)
+
+ expect(first_obj).to be(second_obj)
+ end
+ end
  end
 
  describe "cross_link_node" do

spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb

@@ -154,19 +154,48 @@
  },]
  parser.instance_variable_get('@data_index').store_path(
  :container_image,
- :by_ref_and_registry_host_port,
- "#{image_registry}:#{image_registry_port}:#{image_ref}",
+ :by_digest,
+ image_digest,
  parser.instance_variable_get('@data')[:container_images][0])
 
  inventory = {"image" => [image_from_openshift,]}
 
  parser.get_openshift_images(inventory)
  expect(parser.instance_variable_get('@data')[:container_images].size).to eq(1)
  expect(parser.instance_variable_get('@data')[:container_images][0]).to eq(
- parser.instance_variable_get('@data_index')[:container_image][:by_ref_and_registry_host_port].values[0])
+ parser.instance_variable_get('@data_index')[:container_image][:by_digest].values[0])
  expect(parser.instance_variable_get('@data')[:container_images][0][:architecture]).to eq('amd64')
  end
 
+ it "matches images by digest" do
+ FIRST_NAME = "first_name".freeze
+ FIRST_TAG = "first_tag".freeze
+ FIRST_REF = "first_ref".freeze
+ parser.instance_variable_get('@data')[:container_images] = [{
+ :name => FIRST_NAME,
+ :tag => FIRST_TAG,
+ :digest => image_digest,
+ :image_ref => FIRST_REF,
+ :registered_on => Time.now.utc - 2.minutes
+ },]
+ parser.instance_variable_get('@data_index').store_path(
+ :container_image,
+ :by_digest,
+ image_digest,
+ parser.instance_variable_get('@data')[:container_images][0]
+ )
+
+ inventory = {"image" => [image_from_openshift,]}
+
+ parser.get_openshift_images(inventory)
+ expect(parser.instance_variable_get('@data')[:container_images].size).to eq(1)
+ expect(parser.instance_variable_get('@data')[:container_images][0]).to eq(
+ parser.instance_variable_get('@data_index')[:container_image][:by_digest].values[0]
+ )
+ expect(parser.instance_variable_get('@data')[:container_images][0][:architecture]).to eq('amd64')
+ expect(parser.instance_variable_get('@data')[:container_images][0][:name]).to eq(FIRST_NAME)
+ end
+
  context "image registries from openshift images" do
  def parse_single_openshift_image_with_registry
  inventory = {"image" => [image_from_openshift]}

spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb

@@ -95,7 +95,7 @@ def assert_table_counts
  expect(ContainerBuild.count).to eq(3)
  expect(ContainerBuildPod.count).to eq(3)
  expect(ContainerTemplate.count).to eq(26)
- expect(ContainerImage.count).to eq(44)
+ expect(ContainerImage.count).to eq(40)
  end
 
  def assert_ems