Using the new options for image-scanning options #45

enoodle · 2017-06-20T15:40:21Z

This will use the options field for per-provider configuration in image scanning.
Also, This will try to use the http_proxy set in the options to connect to the provider instead of the one defined in the settings file. Originally it was planned to have https_proxy and no_proxy options as well for the connection to the provider but as they are not used currently I left them out.

simon3z · 2017-06-20T15:48:05Z

@enoodle nice! I think this would do it.

moolitayer · 2017-08-02T12:22:52Z

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

+ },
+ :cve_url => {
+ :label => N_('CVE location'),
+ :help_text => N_('Enables defining a URL for XCCDF file instead of accessing the Internet'),


I remember something about this being a prefix and not url?

would it be possible to add an example to each value, even only a comment?

I'm thinking this would be great if I need to use one of these I could look here and see what I need.

enoodle · 2017-08-03T13:21:23Z

@moolitayer I can also have the example in the help_text that I plan to present in the UI (when you hover over the field)

moolitayer · 2017-08-03T13:23:03Z

@moolitayer I can also have the example in the help_text that I plan to present in the UI (when you hover over the field)

👍 That would be much better 🌶️

enoodle · 2017-08-03T13:27:05Z

@moolitayer done.

moolitayer · 2017-08-03T14:16:47Z

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

@@ -348,6 +347,7 @@ def inspector_admin_secret
 end

 def pod_definition(inspector_admin_secret_name)
+ @ems_options = ext_management_system.options[:image_inspector_options]


Can we please have ems_options as a method and not an instance variable?
I might be reading this wrong but the usage of @ems_options[:registry] in line 429 looks like a side effect of calling this method first. (you can memoize it if you want to save lookups)

moolitayer · 2017-08-03T18:04:27Z

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

+ },
+ :cve_url => {
+ :label => N_('CVE location'),
+ :help_text => N_('Enables defining a URL path prefix for XCCDF file instead of accessing the default location.


moolitayer · 2017-08-03T18:04:47Z

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

@@ -347,6 +346,10 @@ def inspector_admin_secret
 return nil
 end

+ def ems_options
+ @provider_options ||= ext_management_system.options[:image_inspector_options]


moolitayer · 2017-08-03T18:11:35Z

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

- env << {:name => att.name.upcase,
- :value => att.value} unless att.value.blank?
+ PROXY_ENV_VARIABLES.each_with_object([]) do |var_name, env|
+ if ems_options.keys.include?(var_name.to_sym)


ems_options.key?(var_name.to_sym)

moolitayer · 2017-08-03T18:14:27Z

app/models/manageiq/providers/kubernetes/container_manager_mixin.rb

+ :user => options[:user] || authentication_userid(options[:auth_type]),
+ :pass => options[:pass] || authentication_password(options[:auth_type]),
+ :bearer => options[:bearer] || authentication_token(options[:auth_type] || 'bearer'),
+ :http_proxy => self.options.fetch_path(:proxy_settings, :http_proxy),


ems_options.fetch_path maybe? self.options is nil according to Travis

moolitayer

minor comments and fix travis, other than that
LGTM 👍

moolitayer · 2017-08-03T18:19:29Z

cc @cben @simon3z

simon3z · 2017-08-04T14:57:39Z

@enoodle can you fix travis?

enoodle · 2017-08-06T13:09:00Z

@moolitayer I added tests and now travis is green. PTAL

cben · 2017-08-06T13:16:40Z

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

@@ -0,0 +1,73 @@
+module ManageIQ::Providers::Kubernetes::ContainerManager::Options
+ extend ActiveSupport::Concern


do you need ActiveSupport::Concern? what does it do here? (extend into an extended module is too much for my brain ;-)

I think you don't need it, this simply has methods that become class methods due to extend ...ContainerManager::Options.
or if you want to use ActiveSupport::Concern idiomatically, switch to includeing this module and put the methods under ClassMethods.

cben

Yay, very nice.

cben · 2017-08-06T13:22:04Z

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

@@ -347,6 +346,10 @@ def inspector_admin_secret
 return nil
 end

+ def ems_options


consider renaming to ems_image_inspector_options (or something intermediate).
because ems_options[:registry] sounds like these are top-level options on the ems.

cben · 2017-08-06T13:28:05Z

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

- env << {:name => att.name.upcase,
- :value => att.value} unless att.value.blank?
+ PROXY_ENV_VARIABLES.each_with_object([]) do |var_name, env|
+ next unless ems_options.key?(var_name.to_sym)


not sure it matters, but previous code checking .blank? allowed "unsetting" by setting to empty string, this requires actually unsetting the option — is that even possible with the new per-EMS options?

It is possible to totally unset an option if you set the options without the ones you don't want:
ems.update(:options => {}) or ems.update(:options => {:image_inspector_options => {:http_proxy => "my_proxy"}}). The options are only set from the UI [1] so it will be up to that piece of code to make sure that empty values are not set.
I wonder if it is smart to add tests for the empty option case making sure that they are treated as unset. (then making sure it is so here) - This will prevent us from assigning them the empty value. WDYT?

[1] https://github.com/ManageIQ/manageiq-ui-classic/pull/1652/files#diff-f402e411cd6dc158c724b14d72ead612R445

cben · 2017-08-06T13:31:27Z

app/models/manageiq/providers/kubernetes/container_manager_mixin.rb

+ :user => options[:user] || authentication_userid(options[:auth_type]),
+ :pass => options[:pass] || authentication_password(options[:auth_type]),
+ :bearer => options[:bearer] || authentication_token(options[:auth_type] || 'bearer'),
+ :http_proxy => self.options ? self.options.fetch_path(:proxy_settings, :http_proxy) : nil,


why :proxy_settings, :http_proxy here and just :http_proxy in kubernetes_connect ?

kubernetes_connect gets its options from here, calling raw_connect will eventually kubernetes_connect. I put them in http_proxy so no need for the proxy_settings. Openshift calls kubernetes_connect as well after adding its own options.

miq-bot · 2017-08-06T14:33:56Z

Some comments on commit enoodle@9257f08

spec/models/manageiq/providers/kubernetes/container_manager/scanning/job_spec.rb

⚠️ - 195 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-08-06T14:34:00Z

Checked commit enoodle@9257f08 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. 🍰

cben

👍

moolitayer · 2017-08-06T14:52:32Z

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

@@ -14,6 +14,7 @@ class ManageIQ::Providers::Kubernetes::ContainerManager < ManageIQ::Providers::C
 require_nested :Scanning

 include ManageIQ::Providers::Kubernetes::ContainerManagerMixin
+ include ManageIQ::Providers::Kubernetes::ContainerManager::Options


@enoodle @cben didn't we miss a require nested of the new file here?

enoodle changed the title ~~using the new options for image-scanning options~~ [WIP] Using the new options for image-scanning options Jun 20, 2017

miq-bot added the wip label Jun 20, 2017

enoodle force-pushed the use_new_options_for_provider_settings branch 2 times, most recently from 8b7f74b to 85e515b Compare June 22, 2017 15:56

enoodle mentioned this pull request Jul 5, 2017

Add the ability to edit options settings for providers ManageIQ/manageiq-ui-classic#1652

Merged

enoodle force-pushed the use_new_options_for_provider_settings branch from 85e515b to c4dc70b Compare July 10, 2017 10:40

enoodle mentioned this pull request Jul 10, 2017

Use the kubernetes options for Openshift ManageIQ/manageiq-providers-openshift#32

Merged

enoodle force-pushed the use_new_options_for_provider_settings branch from c4dc70b to c634dab Compare July 11, 2017 11:41

enoodle force-pushed the use_new_options_for_provider_settings branch 2 times, most recently from 405651f to 3456a86 Compare July 18, 2017 14:27

enoodle force-pushed the use_new_options_for_provider_settings branch 2 times, most recently from 59c341c to 408f9a8 Compare August 2, 2017 12:18

enoodle changed the title ~~[WIP] Using the new options for image-scanning options~~ Using the new options for image-scanning options Aug 2, 2017

moolitayer reviewed Aug 2, 2017

View reviewed changes

miq-bot removed the wip label Aug 2, 2017

enoodle force-pushed the use_new_options_for_provider_settings branch from 408f9a8 to 71008d7 Compare August 3, 2017 13:15

enoodle force-pushed the use_new_options_for_provider_settings branch 2 times, most recently from 918eebe to 9152ba5 Compare August 3, 2017 13:40

moolitayer reviewed Aug 3, 2017

View reviewed changes

enoodle force-pushed the use_new_options_for_provider_settings branch from 9152ba5 to e9964da Compare August 3, 2017 15:17

moolitayer reviewed Aug 3, 2017

View reviewed changes

moolitayer approved these changes Aug 3, 2017

View reviewed changes

enoodle force-pushed the use_new_options_for_provider_settings branch 2 times, most recently from a8773d3 to 6a6c84c Compare August 6, 2017 12:48

cben reviewed Aug 6, 2017

View reviewed changes

using the new options column

9257f08

enoodle force-pushed the use_new_options_for_provider_settings branch from 6a6c84c to 9257f08 Compare August 6, 2017 14:31

cben requested changes Aug 6, 2017

View reviewed changes

cben approved these changes Aug 6, 2017

View reviewed changes

moolitayer reviewed Aug 6, 2017

View reviewed changes

moolitayer merged commit 544bd88 into ManageIQ:master Aug 6, 2017

enoodle mentioned this pull request Aug 8, 2017

require_nested for Kubernetes::ContainerManager::Options #83

Merged

moolitayer added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 9, 2017

This was referenced Aug 9, 2017

OPTIONS for /api/providers ManageIQ/manageiq-api#3

Closed

Adding options_description class method to base_manager ManageIQ/manageiq#15799

Merged

enoodle mentioned this pull request Nov 14, 2017

Use Prometheus timeouts from settings #167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using the new options for image-scanning options #45

Using the new options for image-scanning options #45

enoodle commented Jun 20, 2017 •

edited

Loading

simon3z commented Jun 20, 2017

moolitayer Aug 2, 2017 •

edited

Loading

enoodle commented Aug 3, 2017

moolitayer commented Aug 3, 2017

enoodle commented Aug 3, 2017

moolitayer Aug 3, 2017

moolitayer Aug 3, 2017

moolitayer Aug 3, 2017

moolitayer Aug 3, 2017

moolitayer Aug 3, 2017

moolitayer left a comment

moolitayer commented Aug 3, 2017

simon3z commented Aug 4, 2017

enoodle commented Aug 6, 2017

cben Aug 6, 2017

cben left a comment

cben Aug 6, 2017

cben Aug 6, 2017

enoodle Aug 6, 2017

cben Aug 6, 2017

enoodle Aug 6, 2017

miq-bot commented Aug 6, 2017

miq-bot commented Aug 6, 2017

cben left a comment

moolitayer Aug 6, 2017

moolitayer Aug 7, 2017

		@@ -0,0 +1,73 @@
		module ManageIQ::Providers::Kubernetes::ContainerManager::Options
		extend ActiveSupport::Concern

Using the new options for image-scanning options #45

Using the new options for image-scanning options #45

Conversation

enoodle commented Jun 20, 2017 • edited Loading

simon3z commented Jun 20, 2017

moolitayer Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

enoodle commented Aug 3, 2017

moolitayer commented Aug 3, 2017

enoodle commented Aug 3, 2017

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

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Aug 3, 2017

simon3z commented Aug 4, 2017

enoodle commented Aug 6, 2017

Choose a reason for hiding this comment

cben left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 6, 2017

miq-bot commented Aug 6, 2017

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Jun 20, 2017 •

edited

Loading

moolitayer Aug 2, 2017 •

edited

Loading