From f03a2fd74c1c10d18577a23023a3e96db84e1d46 Mon Sep 17 00:00:00 2001 From: Erez Freiberger Date: Sun, 5 Mar 2017 14:53:42 +0200 Subject: [PATCH] Identifying container images by registry, name and digest The image-ref is not a reliable source to identify images when it comes from the docker daemon (in the docker://... form). This will identify the images from the information that we can parse. Doing so will also enable us to commit the images to the @data hash when we identify the images instead from a collection function (get_images), which will simplify image collection from Openshift. --- app/models/container_image.rb | 1 + .../container_manager/refresh_parser.rb | 25 ++++++---- .../container_manager/refresh_parser.rb | 8 +-- .../container_manager/refresh_parser_spec.rb | 49 +++++++++++++++---- .../container_manager/refresh_parser_spec.rb | 35 +++++++++++-- .../container_manager/refresher_spec.rb | 2 +- 6 files changed, 91 insertions(+), 29 deletions(-) diff --git a/app/models/container_image.rb b/app/models/container_image.rb index f1d1cc2ed7a..bd52d3f431a 100644 --- a/app/models/container_image.rb +++ b/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 diff --git a/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb b/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb index 46be38ca2ae..8f96b426e7b 100644 --- a/app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb +++ b/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], diff --git a/app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb b/app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb index bb226a14c72..00a4b31b523 100644 --- a/app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb +++ b/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) diff --git a/spec/models/manageiq/providers/kubernetes/container_manager/refresh_parser_spec.rb b/spec/models/manageiq/providers/kubernetes/container_manager/refresh_parser_spec.rb index 7c2364fd5db..44b8f74bed6 100644 --- a/spec/models/manageiq/providers/kubernetes/container_manager/refresh_parser_spec.rb +++ b/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,7 +638,7 @@ 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) @@ -624,6 +646,15 @@ 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 diff --git a/spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb b/spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb index 8e19f1d7afd..bdac862c945 100644 --- a/spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb +++ b/spec/models/manageiq/providers/openshift/container_manager/refresh_parser_spec.rb @@ -154,8 +154,8 @@ },] 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,]} @@ -163,10 +163,39 @@ 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]} diff --git a/spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb b/spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb index ac0fe0350bd..70742a56df5 100644 --- a/spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb +++ b/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