SmartState: Make docker registry & repo configurable for 'image-inspector'. #8439

cben · 2016-05-04T09:33:10Z

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1378007

Based on #7036 by @dtrieu80 (preserving Author), believe I addressed comments there.

Currently, job.rb has a hardcoded reference to the image-inspector. It assumes that it can be retrievable from docker.io.
In some instances, the docker.io registry is inaccessible, so we need to be able to configure a different registry (local or other reachable registry).

@dtrieu80 Note that this will probably not suffice to do image scanning in a disconnected installation — the image inspector needs internet access to download OpenSCAP definitions.
That part is tracked in https://bugzilla.redhat.com/show_bug.cgi?id=1378007 / openshift/image-inspector#18.

Rebased on master, use Settings instead of deprecated Config.
Tag left as constant in code as @simon3z said our code strictly depends on a specific version.
@moolitayer suggested assuming the new setting is always there ("can crash and burn").
But the actual failure mode if user will delete these settings is bad: nil silently becomes empty string => we do create a pod with malformed image name e.g. docker.io/:v1.0.z => the pod never runs (RunContainerError or ImagePullBackOff state) => wait_pod goes into infinite loop.
I didn't want to add exception catching and reporting for a scenario which "shouldn't happen", so made the setting optional with a default value in the code, as already done with miq_namespace. Easy but non-DRY...
What's worse, if the configuration exists but is bad — including the original motivation here of docker.io being inaccessible on some networks — the pod never runs and ~~we go into infinite loop~~. The user gets no diagnostic :-(

[@moolitayer has some PRs for better error handling, anyway out of scope here.]

P.S. I haven't tested the full scenario of running my own registry.
But I see a pod with customized image name being created:

[----] I, [2016-05-...]  INFO -- : ... MIQ(ManageIQ::Providers::Kubernetes::ContainerManager::Scanning::Job#start) creating pod ...: {"apiVersion":"v1","kind":"Pod","metadata":...,"spec":{"restartPolicy":"Never","containers":[{"name":"image-inspector","image":"myregistry.example.com/openshift-fork/image-inspector-spoon:v1.0.z", ...}],"volumes":...}}

$ oc describe pod -n management-infra manageiq-img-scan-2b05b
Name:       manageiq-img-scan-2b05b
Namespace:  management-infra
Image(s):   myregistry.example.com/openshift-fork/image-inspector-spoon:2.0
...

miq-bot · 2016-05-04T09:41:57Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

cben · 2016-05-04T11:32:25Z

Now really rebased. Ready for review.
cc @enoodle

Fixed a place where INSPECTOR_NAMESPACE was used without trying config first.
Hopefully the new names INSPECTOR_NAMESPACE_FALLBACK etc. will make this mistake harder to make. But it's a classic case where DRY would be safer...

cben · 2016-05-04T11:39:21Z

@miq-bot add_label enhancement, providers/containers, smart_state

enoodle · 2016-05-04T11:46:06Z

LGTM 👍
Maybe we should open an issue for the points you raised in the commit message?

moolitayer · 2016-05-04T12:11:39Z

app/models/manageiq/providers/kubernetes/container_manager/scanning/job.rb

@@ -353,6 +353,8 @@ def add_secret_to_pod_def(pod_def, inspector_admin_secret_name)
  end

  def inspector_image
-    'docker.io/openshift/image-inspector:v2.0.z'
+    registry = Settings.ems.ems_kubernetes.image_inspector_registry || INSPECTOR_REGISTRY_FALLBACK
+    repo = Settings.ems.ems_kubernetes.image_inspector_repository || INSPECTOR_REPOSITORY_FALLBACK


Current code handles the possibility of leaves missing(e.g :image_inspector_registry) and it would make sense to handle upper level elements as well (e.g ems_kubernetes) I recently came across following code in miq_schedule_worker, maybe you can use something similar?

https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_schedule_worker/runner.rb#L66

@cben @moolitayer also remember that empty strings are different from nil:

> nil || "fallback value" => "fallback value" > "" || "fallback value" => ""

You may want to use blank?.

AFAICT Settings has no API like fetch_path. Added .try(:foo) on all levels.

I dropped the .blank? intentionally. I did test that

:foo:

gets parsed as nil (emtpy value in YAML is shorthand for null); one has to explicitly write:

:foo: ""

to express an empty string. IMHO that's an explicitly broken config, same as ".#@illegal>" or "no-such-registry.com"...
I generally dislike assigning magical behavior to empty string, unless there is no other way to express it. Here, if you want to "leave it unset", you can delete the line, or use empty/null value.

And remember that we don't really need to support missing options (unless I'm missing something). It was just simpler than signaling errors.

simon3z · 2016-05-10T07:57:57Z

@moolitayer @enoodle @cben please test this thoroughly to make sure it's not breaking SmartState Analysis.

enoodle · 2016-05-10T13:08:39Z

@simon3z I ran it successfully.
@cben part of it seems to be already in from #8531 so you can rebase on upstream/master.
LGTM 👍

simon3z · 2016-05-11T13:04:17Z

@cben what's missing to get this out of WIP?

simon3z · 2016-05-12T21:49:12Z

Since this is in WIP I am re-assigning it to @cben

@miq-bot assign cben

jrafanie · 2016-05-13T13:45:09Z

Closing/reopening since #8684 was merged to fix master

miq-bot · 2016-05-17T21:59:57Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

nimrodshn · 2016-10-10T14:11:05Z

rebased on current master and this works just fine - walked through the entire process.
@cben @simon3z

miq-bot · 2016-10-10T14:11:18Z

@cben Cannot apply the following label because they are not recognized: smart_state

cben · 2016-10-11T06:52:17Z

Thanks @nimrodshn. (This was never really WIP just neglected)
BTW we saw that if we configure non-existant registry/repo, so the pod can't run, the job does eventually time out. (error handling improved since I wrote PR description)

cben · 2016-10-11T07:11:05Z

Added BZ to commit and PR. @Fryguy this is ready.

@miq-bot add-label euwe/yes
since the BZ is targetted to 5.7.0

simon3z · 2016-10-11T09:13:28Z

Added BZ to commit and PR. @Fryguy this is ready.

@cben if you set :image_inspector_registry: and image_inspector_repository: in the settings.yml how do you plan to do the productization?

The settings were intended to be "overrides": how can you productize INSPECTOR_REGISTRY_FALLBACK and INSPECTOR_REPOSITORY_FALLBACK? (which by the way aren't FALLBACKs but rather DEFAULTs).

cben · 2016-10-11T11:48:14Z

You can't override the _FALLBACKs, but the idea was you should always have these settings (I'm including them in settings.yml here) and the fallbacks should never come into play.
They are just an implementation detail of what happens if user goes into Advanced Settings (or the file) and deletes those settings altogether — having a hard-coded fallback turned out much simpler than giving a clear error to user that these settings are required.

Productiization: The only question that might concern upstream here is whether settings.yml is at all possible to override, and the short answer is yes (config/environments/production.yml takes priority).

simon3z · 2016-10-12T07:42:02Z

What I don't like of this PR:

the "defaults" (e.g. openshift/image-inspector) are expressed twice (why? who will remember that?)
by storing the "defaults" right away in the database we lose one degree of freedom to change these default values in the future (once they're in the db you need a complex migration)
if we store them in the database all the try(...) seems redundant (of course deleting keys in the settings is going to cause issues and the user is very well advised against changing that by the UI)

So an easier approach that would satisfy (especially) 2 would be:

INSPECTOR_REPOSITORY_DEFAULT = 'openshift/image-inspector'
...
repository = Settings.ems.ems_kubernetes.image_inspector_repository.presence || INSPECTOR_REPOSITORY_DEFAULT

  :ems_kubernetes:
    ...
    :image_inspector_repository:

(note that :image_inspector_repository: in the settings is empty by default)

Productization would still happen as for other values using overrides (:prepend).

In case the new setting mechanism allows us to easily change these values in the future (satisfies 2) then we can continue with what you have here but I suggest to drop the duplication of values (e.g. drop the _FALLBACKs) and the redundant try.

cben · 2016-10-13T13:37:10Z

(2) is not a problem since the Configuration Revamp (#7432) — settings_changes table only stores settings user has modified. When we'll ship new value in .yml, it takes effect IFF user hasn't touched, which sounds like exactly what we want.

Having defaults in .yml is better than in code, because user can see what's the default behavior and understand what kind of values the setting takes.

(1) DRY is undeniable point.
I wanted the FALLBACKs because I didn't like user not getting any clear feedback in (unlikely) situation when these setting had been deleted; but we have same problem in more likely situation where these settings are bad (bad/unreachable registry, no such image etc).
=> We'll drop the FALLBACKs and trys and figure out with @nimrodshn if user gets useful feedback when the pod fails to run...

miq-bot · 2016-10-17T16:24:44Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

Tag (:2.1) deliberately left non-configurable as we depend on a specific version's interface. (Minor 2.1.z versions will be activated simply by re-pointing the 2.1 tag.) https://bugzilla.redhat.com/show_bug.cgi?id=1378007

miq-bot · 2017-02-20T16:42:49Z

Checked commit cben@63b46ca with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

cben · 2017-02-21T17:04:04Z

Dropped the FALLBACKs with @nimrodshn, code now assumes these settings exist.
Rebased & re-tested now.
@simon3z PTAL.

miq-bot · 2017-02-21T17:05:07Z

@cben Cannot apply the following label because they are not recognized: smart_state

simon3z · 2017-02-21T22:49:46Z

@cben it seems that 4.5 is going to be "the one" :-)

LGTM 👍
cc @chessbyte @enoodle

@miq-bot assign roliveri

@lavenel if we want to take advantage of this we would need a QE cycle. Let me know.

moolitayer

LGTM 👍

@cben are you planning a subsequent pr to productization?
(do we want to use the settings there? currently we are replacing the entire inspector_image method afair)

simon3z · 2017-03-07T23:24:37Z

@cben I don't feel comfortable to have this in euwe. Removing the label for the time being.

@miq-bot remove_label euwe/yes

cben force-pushed the inspector-img-registry branch from 429af3f to 5ea6893 Compare May 4, 2016 09:55

cben force-pushed the inspector-img-registry branch from 5ea6893 to 7483143 Compare May 4, 2016 11:35

miq-bot added enhancement providers/containers core/smart state labels May 4, 2016

moolitayer reviewed May 4, 2016
View reviewed changes

miq-bot added the wip label May 5, 2016

chessbyte changed the title ~~SmartState: Make docker registry & repo configurable for 'image-inspector'.~~ [WIP] SmartState: Make docker registry & repo configurable for 'image-inspector'. May 5, 2016

cben force-pushed the inspector-img-registry branch from f593e4d to 0b05edd Compare May 8, 2016 11:03

chessbyte closed this May 9, 2016

chessbyte reopened this May 9, 2016

chessbyte assigned roliveri May 10, 2016

cben force-pushed the inspector-img-registry branch from 0b05edd to ec8a0e2 Compare May 10, 2016 13:49

miq-bot assigned cben and unassigned roliveri May 12, 2016

jrafanie closed this May 13, 2016

jrafanie reopened this May 13, 2016

cben force-pushed the inspector-img-registry branch 2 times, most recently from 1b6c245 to 8551ef2 Compare May 29, 2016 08:34

cben changed the title ~~[WIP] SmartState: Make docker registry & repo configurable for 'image-inspector'.~~ SmartState: Make docker registry & repo configurable for 'image-inspector'. May 29, 2016

simon3z mentioned this pull request Jul 15, 2016

Make registry configurable with image 'image-inspector' #7036

Closed

nimrodshn force-pushed the inspector-img-registry branch from 8551ef2 to 46b3298 Compare October 9, 2016 11:38

cben force-pushed the inspector-img-registry branch from 46b3298 to 5219697 Compare October 11, 2016 07:09

miq-bot added the euwe/yes label Oct 11, 2016

nimrodshn force-pushed the inspector-img-registry branch from 5219697 to 27c27d0 Compare November 23, 2016 11:56

cben force-pushed the inspector-img-registry branch from 27c27d0 to d549c55 Compare February 20, 2017 16:13

Make registry & repo configurable for 'image-inspector'

63b46ca

Tag (:2.1) deliberately left non-configurable as we depend on a specific version's interface. (Minor 2.1.z versions will be activated simply by re-pointing the 2.1 tag.) https://bugzilla.redhat.com/show_bug.cgi?id=1378007

cben force-pushed the inspector-img-registry branch from d549c55 to 63b46ca Compare February 20, 2017 16:40

miq-bot assigned roliveri and unassigned cben Feb 21, 2017

moolitayer approved these changes Feb 22, 2017

View reviewed changes

enoodle approved these changes Feb 22, 2017

View reviewed changes

simon3z approved these changes Feb 22, 2017

View reviewed changes

Fryguy merged commit 1cacd32 into ManageIQ:master Feb 22, 2017

Fryguy added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 22, 2017

miq-bot removed the euwe/yes label Mar 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SmartState: Make docker registry & repo configurable for 'image-inspector'. #8439

SmartState: Make docker registry & repo configurable for 'image-inspector'. #8439

cben commented May 4, 2016 •

edited

Loading

miq-bot commented May 4, 2016

cben commented May 4, 2016

cben commented May 4, 2016

enoodle commented May 4, 2016 •

edited

Loading

moolitayer May 4, 2016

simon3z May 4, 2016

cben May 4, 2016

simon3z commented May 10, 2016

enoodle commented May 10, 2016

simon3z commented May 11, 2016

simon3z commented May 12, 2016

jrafanie commented May 13, 2016

miq-bot commented May 17, 2016

nimrodshn commented Oct 10, 2016

miq-bot commented Oct 10, 2016

cben commented Oct 11, 2016

cben commented Oct 11, 2016

simon3z commented Oct 11, 2016

cben commented Oct 11, 2016 •

edited

Loading

simon3z commented Oct 12, 2016

cben commented Oct 13, 2016

miq-bot commented Oct 17, 2016

miq-bot commented Feb 20, 2017

cben commented Feb 21, 2017

miq-bot commented Feb 21, 2017

simon3z commented Feb 21, 2017

moolitayer left a comment

simon3z commented Mar 7, 2017

SmartState: Make docker registry & repo configurable for 'image-inspector'. #8439

SmartState: Make docker registry & repo configurable for 'image-inspector'. #8439

Conversation

cben commented May 4, 2016 • edited Loading

miq-bot commented May 4, 2016

cben commented May 4, 2016

cben commented May 4, 2016

enoodle commented May 4, 2016 • edited Loading

moolitayer May 4, 2016

Choose a reason for hiding this comment

simon3z May 4, 2016

Choose a reason for hiding this comment

cben May 4, 2016

Choose a reason for hiding this comment

simon3z commented May 10, 2016

enoodle commented May 10, 2016

simon3z commented May 11, 2016

simon3z commented May 12, 2016

jrafanie commented May 13, 2016

miq-bot commented May 17, 2016

nimrodshn commented Oct 10, 2016

miq-bot commented Oct 10, 2016

cben commented Oct 11, 2016

cben commented Oct 11, 2016

simon3z commented Oct 11, 2016

cben commented Oct 11, 2016 • edited Loading

simon3z commented Oct 12, 2016

cben commented Oct 13, 2016

miq-bot commented Oct 17, 2016

miq-bot commented Feb 20, 2017

cben commented Feb 21, 2017

miq-bot commented Feb 21, 2017

simon3z commented Feb 21, 2017

moolitayer left a comment

Choose a reason for hiding this comment

simon3z commented Mar 7, 2017

cben commented May 4, 2016 •

edited

Loading

enoodle commented May 4, 2016 •

edited

Loading

cben commented Oct 11, 2016 •

edited

Loading