Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images #13263

zeari · 2016-12-20T09:43:24Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1405937
dockerImageManifest/dockerImageMetadata could be nil when parsing openshift image data.

@enoodle @simon3z Please review
@miq-bot add_label providers/containers, bug

zeari · 2016-12-20T09:45:38Z

@cben @zakiva

zeari · 2016-12-20T09:49:52Z

@enoodle I couldnt find the description of these fields under openshift, can you point me towards the types.go file that describes these?

enoodle · 2016-12-20T09:57:21Z

@zeari https://github.com/openshift/origin/blob/master/pkg/image/api/v1/types.go

zeari · 2016-12-20T10:06:01Z

@zeari https://github.com/openshift/origin/blob/master/pkg/image/api/v1/types.go

So both are omitempty and could be nil. 👍

enoodle · 2016-12-20T10:09:15Z

@zeari Also dockerImageReference could be empty. actually all the fields can be empty except dockerImageLayers that we don't use currently anyway.

enoodle · 2016-12-20T10:10:10Z

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

@@ -176,25 +176,29 @@ def parse_openshift_image(openshift_image)
        ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id


dockerImageReference Could also be empty.

@enoodle Done

@zeari I am sorry but on second thought maybe this is not enough also. without id and ref there is not much meaning to parser_container_image. Maybe we should also try to get the name from the "regular" metadata, which also might be empty.

id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name] || ""

also anyway the field name should start with a lowercase d IIUC

So if id ends up being "", ref ends up being "docker-pullable://",
does parser_container_image which calls parse_image_name all work?

Can you add some unit test where at least some of these things are missing?

why not the following:

id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name] ref = "ContainerImage::DOCKER_PULLABLE_PREFIX#{id}"

or

id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name] ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id.to_s

In other words, make creation of the ref defensive.

I am not sure what is the meaning of an image without a name... also parse_container_image might return weird results.
Maybe we should find a way to ignore this "image" if that happens?

enoodle · 2016-12-20T11:21:54Z

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

@@ -176,25 +176,29 @@ def parse_openshift_image(openshift_image)
        ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id


@zeari I am sorry but on second thought maybe this is not enough also. without id and ref there is not much meaning to parser_container_image. Maybe we should also try to get the name from the "regular" metadata, which also might be empty.

id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name] || ""

also anyway the field name should start with a lowercase d IIUC

cben · 2016-12-20T12:21:39Z

BTW if you need live data examples, here is another one from Einat's OSE 3.3:
https://gist.github.com/cben/dcbbebacc4b7c6588ef19ffbd6fb38bd
has dockerImageReference but no dockerImageManifest (and was failing at refresh_parser.rb:179:in `parse_openshift_image')

simon3z · 2016-12-21T10:20:57Z

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

@@ -172,29 +172,33 @@ def parse_env_variables(env_variables)
      end

      def parse_openshift_image(openshift_image)
-        id = openshift_image[:dockerImageReference]
+        id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name] || ""


@zeari @enoodle @cben I don't like the last empty string. It doesn't seem to make sense to have an id that can collapse to an empty string for multiple images.

I agree, Maybe we should abort the processing of that image in this case. There is not much meaning to an image without a name.

zeari · 2016-12-22T10:57:17Z

@enoodle @simon3z We shouldnt have empty ids, so this version should be ok.

id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name]
ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id.to_s
In other words, make creation of the ref defensive.

@chessbyte I went with this option

simon3z · 2016-12-22T11:09:40Z

@smarterclayton @deads2k can you confirm that Name in ObjectMeta although it can be empty (omitempty), that's the case only on the client requests (when generateName is used)?

When reading objects from the api-server we'll always get them with a valid (not-empty) Name?

Since all objects are identified by their name in the API I don't expect to be possible to ever retrieve objects with empty/missing names not even for a short time (e.g. generateName generates the name atomically before the object is ever exposed in the list of objects of that Type).

deads2k · 2016-12-22T12:50:59Z

When reading objects from the api-server we'll always get them with a valid (not-empty) Name?

Correct.

Since all objects are identified by their name in the API I don't expect to be possible to ever retrieve objects with empty/missing names not even for a short time (e.g. generateName generates the name atomically before the object is ever exposed in the list of objects of that Type).

Correct. Names are generated before the object is stored, so an API client can never observe an API object without a name.

simon3z · 2016-12-22T15:54:59Z

@zeari overall looks good but I think we need to unit-test parse_openshift_image for these cases we're fixing.

zeari · 2016-12-25T11:41:11Z

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

+                                                        :tag => "abcdefg"
+                                                      })
+    end
+


@enoodle @simon3z PTAL

zeari · 2016-12-25T12:10:31Z

@zeari overall looks good but I think we need to unit-test parse_openshift_image for these cases we're fixing.

Done.

zeari · 2016-12-27T10:31:56Z

ping @simon3z is there something missing here?

simon3z · 2016-12-27T16:09:58Z

LGTM 👍 cc @chessbyte

@miq-bot assign chessbyte

chrispy1 · 2017-01-04T20:41:46Z

@miq-bot add_label euwe/yes
@miq-bot add_label blocker

Fryguy · 2017-01-05T01:20:27Z

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

-        id = openshift_image[:dockerImageReference]
-        ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id
+        id = openshift_image[:dockerImageReference] || openshift_image[:metadata][:name]
+        ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id.to_s


Prefer interpolation over +

Agree. Prefer:

ref = "ContainerImage::DOCKER_PULLABLE_PREFIX#{id}"

oops, should be:

ref = "#{ContainerImage::DOCKER_PULLABLE_PREFIX}#{id}"

Fryguy · 2017-01-05T01:20:50Z

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

-        json = JSON.parse(openshift_image[:dockerImageManifest])
-        new_result[:tag] ||= json["tag"] if json.keys.include?("tag")
+        if openshift_image[:dockerImageManifest].present?
+          json = JSON.parse(openshift_image[:dockerImageManifest])


Is it possible this could be a "bad" JSON payload? If so, does it need to be handled?

I dont think the json can be malformed but it can be nil (which i check for).

@zeari @Fryguy I agree that the JSON payload can hardly be malformed but this part is so critical (not crashing the inventory refresh) that it may deserve some extra care (so that we won't have to deal with this in the future).
In case we can't parse the JSON string we could log an error and continue.

…ages

miq-bot · 2017-01-10T11:07:05Z

Checked commit zeari@077f707 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

zeari · 2017-01-10T11:13:44Z

Is it possible this could be a "bad" JSON payload? If so, does it need to be handled?

ping @Fryguy, I added a rescue on that to log an error and then continue without crashing the refresh.

zeari · 2017-01-10T16:30:37Z

@gtanzillo @blomquisg Can you guys take a look? 😅

simon3z · 2017-01-11T17:30:51Z

@chessbyte we need a backport for euwe.

Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images (cherry picked from commit 5ea1153) https://bugzilla.redhat.com/show_bug.cgi?id=1412312

simaishi · 2017-01-11T18:00:57Z

Euwe backport details:

$ git log -1
commit 9dbe63c21bedd59a75e8eb7cc65fc75da9702bb8
Author: Jason Frey <fryguy9@gmail.com>
Date:   Tue Jan 10 12:24:18 2017 -0500

    Merge pull request #13263 from zeari/fix_nil_dockerManifest
    
    Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images
    (cherry picked from commit 5ea1153a3d20f6332bf7374a1c1ed26b46b04731)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1412312

enoodle suggested changes Dec 20, 2016

View reviewed changes

zeari force-pushed the fix_nil_dockerManifest branch from 5217e6c to 06b597b Compare December 20, 2016 11:07

enoodle suggested changes Dec 20, 2016

View reviewed changes

zeari force-pushed the fix_nil_dockerManifest branch from 06b597b to 297fa43 Compare December 20, 2016 11:24

zeari changed the title ~~Handle nil dockerManifest and nil dockerImageMetadata in openshift images~~ Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images Dec 20, 2016

chessbyte assigned simon3z Dec 20, 2016

chessbyte added bug providers/containers core/smart state and removed core/smart state labels Dec 20, 2016

simon3z reviewed Dec 21, 2016

View reviewed changes

zeari force-pushed the fix_nil_dockerManifest branch from 297fa43 to 356b041 Compare December 22, 2016 10:55

zeari force-pushed the fix_nil_dockerManifest branch from 356b041 to 5520b2f Compare December 25, 2016 11:40

zeari commented Dec 25, 2016

View reviewed changes

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

:tag => "abcdefg"

})

end

Copy link

Author

zeari Dec 25, 2016

Choose a reason for hiding this comment

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

@enoodle @simon3z PTAL

enoodle approved these changes Dec 25, 2016

View reviewed changes

zeari force-pushed the fix_nil_dockerManifest branch from 5520b2f to aaf317b Compare December 25, 2016 12:03

simaishi added blocker euwe/yes labels Jan 4, 2017

Fryguy reviewed Jan 5, 2017

View reviewed changes

miq-bot assigned chessbyte and unassigned simon3z Jan 5, 2017

zeari force-pushed the fix_nil_dockerManifest branch 5 times, most recently from de52589 to 077f707 Compare January 10, 2017 11:04

Handle nil dockerManifest and nil dockerImageMetadata in openshift im…

077f707

…ages

Fryguy merged commit 5ea1153 into ManageIQ:master Jan 10, 2017

Fryguy added this to the Sprint 52 Ending Jan 16, 2017 milestone Jan 10, 2017

simaishi added euwe/backported and removed euwe/yes labels Jan 11, 2017

		@@ -176,25 +176,29 @@ def parse_openshift_image(openshift_image)
		ref = ContainerImage::DOCKER_PULLABLE_PREFIX + id

Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images #13263

Handle nil dockerImageManifest and nil dockerImageMetadata in openshift images #13263

Conversation

zeari commented Dec 20, 2016 • edited by miq-bot Loading

zeari commented Dec 20, 2016

zeari commented Dec 20, 2016

enoodle commented Dec 20, 2016

zeari commented Dec 20, 2016

enoodle commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chessbyte Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Dec 22, 2016

simon3z commented Dec 22, 2016

deads2k commented Dec 22, 2016

simon3z commented Dec 22, 2016

Choose a reason for hiding this comment

zeari commented Dec 25, 2016

zeari commented Dec 27, 2016

simon3z commented Dec 27, 2016

chrispy1 commented Jan 4, 2017

Choose a reason for hiding this comment

chessbyte Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jan 10, 2017

zeari commented Jan 10, 2017

zeari commented Jan 10, 2017

simon3z commented Jan 11, 2017

simaishi commented Jan 11, 2017

zeari commented Dec 20, 2016 •

edited by miq-bot

Loading

chessbyte Dec 20, 2016 •

edited

Loading

chessbyte Jan 5, 2017 •

edited

Loading