Fixes #18717 - Allows use of proxies on multiple interfaces #4346

sean797 · 2017-02-28T16:02:20Z

This moves Smart Proxies URL to its own model and during provision you select a URL instead of a proxy. All communication between foreman -> proxy is still done via the "Primary URL".

mention-bot · 2017-02-28T16:02:29Z

@sean797, thanks for your PR! By analyzing the history of the files in this pull request, we identified @isratrade, @dLobatog and @ohadlevy to be potential reviewers.

houndci-bot · 2017-02-28T16:02:51Z

app/models/smart_proxy_url.rb

+
+  validates :url, :length => {:maximum => 255}, :presence => true, :url_schema => ['http', 'https']
+  validates_uniqueness_of :smart_proxy_id, :scope => :url
+  validates_uniqueness_of :smart_proxy_id, :scope => :primary, if: :primary?


Prefer the new style validations validates :column, uniqueness: value over validates_uniqueness_of.

houndci-bot · 2017-02-28T16:02:51Z

app/models/smart_proxy_url.rb

+  has_many :puppet_ca_hostgroups, :class_name => 'Hostgroup', :foreign_key => 'puppet_ca_proxy_url_id'
+
+  validates :url, :length => {:maximum => 255}, :presence => true, :url_schema => ['http', 'https']
+  validates_uniqueness_of :smart_proxy_id, :scope => :url


Prefer the new style validations validates :column, uniqueness: value over validates_uniqueness_of.

houndci-bot · 2017-02-28T16:02:51Z

app/models/smart_proxy_url.rb

@@ -0,0 +1,37 @@
+class SmartProxyUrl < ActiveRecord::Base
+


Extra empty line detected at class body beginning.

houndci-bot · 2017-02-28T16:02:51Z

app/models/smart_proxy.rb

+  has_many_hosts :through => 'urls'
+  has_many :hostgroups, :through => 'urls'
+  has_many :puppet_ca_hosts, :class_name => 'Host::Managed', :through => 'urls'
+  has_many :puppet_ca_hostgroups, :class_name => 'Hostgroup', :through => 'urls' 


Trailing whitespace detected.

sean797 · 2017-03-02T10:10:41Z

Setting WIP, this needs a fair bit more work - only raised this really so I could run the whole test suite.

houndci-bot · 2017-03-02T19:46:17Z

app/helpers/form_helper.rb

@@ -320,6 +347,7 @@ def blank_or_inherit_f(f, attr)
    return true unless f.object.respond_to?(:parent_id) && f.object.parent_id
    inherited_value   = f.object.send(attr).try(:name_method)
    inherited_value ||= _("no value")
+


Trailing whitespace detected.

houndci-bot · 2017-03-03T12:45:31Z

test/controllers/smart_proxies_controller_test.rb

@@ -32,15 +32,17 @@ def test_edit

  def test_update_invalid
    SmartProxy.any_instance.stubs(:valid?).returns(false)
-    put :update, {:id => SmartProxy.first.to_param, :smart_proxy => {:url => nil}}, set_session_user
+    put :update, {:id => SmartProxy.first.to_param, :smart_proxy => {:urls_attributes => {"0"=>{:url => nil, 


Trailing whitespace detected.

houndci-bot · 2017-03-07T17:39:15Z

app/helpers/hosts_helper.rb

-      SmartProxy.with_features(proxy_feature).map {|p| [p.name, p.id]},
+      :url_id,
+      options_for_select([[_("Select desired %s proxy URL") % _(proxy_feature), "disabled"],
+        [_("*Clear %s proxy URL*") % _(proxy_feature), "" ]]) +


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-03-08T19:28:05Z

app/models/smart_proxy_url.rb

+    end
+  end
+
+  def self.with_features_unscoped(*feature_names) 


Trailing whitespace detected.

sean797 · 2017-03-20T10:14:40Z

Thanks for taking a look @stbenjam

I'm a little concerned we're removing some API's instead of deprecating them.

Fair point, I've update it so if a proxy is given we use the primary URL. I've also on deprecated the API v2 params. I guess we should leave API v1 alone & stable ?

Do we need to explicit support for multiple URL's, or wouldn't just alternate host names be enough? The proxy is still going to be using the same protocol and port isn't it? Maybe there is a use case for different ports?

Host names are enough for our purposes. Though I'm not sure if a URL would be useful to any plugins?

What about letting the user specify a name as well as a URL? If a proxy exists in a lot of networks, it'd be easier to pick something like "Atlanta - Dev Network" than try to select a URL.

I'm not adverse to that, though I'm not sure its really that helpful. In my experience people tend to name things in foreman by another attribute, e.g Networks by CIDR, Proxies by hostname which a lot of the time creates unnecessary duplication.

This fixes the failing test on theforeman/foreman#4346

sean797 · 2017-03-22T16:42:50Z

Just a couple of screenshots in its current state

dmitri-d · 2017-03-22T16:52:41Z

Tangential to this PR, but to support https on multiple interfaces, smart-proxy will need a wildcard certificate. ATM there's no way to associate a certificate with a specific interface.

sean797 · 2017-03-24T12:14:32Z

Thanks @witlessbird, theforeman/puppet-puppet#490 fixes that using dns_alt_names 👍

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

+  end
+
+  test "can search smart proxy by feature" do
+    proxy = smart_proxies(:one) 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -92,9 +92,35 @@ class SmartProxyTest < ActiveSupport::TestCase
  end

  test "should not be saved if features are not array" do
-    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443')
+    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443') 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -83,7 +83,7 @@ class SmartProxyTest < ActiveSupport::TestCase
  end

  test "should not be saved if features do not exist" do
-    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443')
+    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443') 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -69,7 +69,7 @@ class SmartProxyTest < ActiveSupport::TestCase

  test "can count connected hosts" do
    proxy = FactoryGirl.create(:puppet_smart_proxy)
-    FactoryGirl.create(:host, :with_environment, :puppet_proxy => proxy)
+    FactoryGirl.create(:host, :with_environment, :puppet_proxy_hostname => proxy.hostnames.first) 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -49,7 +49,7 @@ class SmartProxyTest < ActiveSupport::TestCase

  # test taxonomix methods
  test "should get used location ids for host" do
-    FactoryGirl.create(:host, :with_environment, :puppet_proxy => smart_proxies(:puppetmaster),
+    FactoryGirl.create(:host, :with_environment, :puppet_proxy_hostname => hostnames(:puppetmaster), 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/controllers/smart_proxies_controller_test.rb

@@ -21,7 +21,7 @@ def test_create_valid
    ProxyAPI::Features.any_instance.stubs(:features => Feature.name_map.keys)
    SmartProxy.any_instance.stubs(:valid?).returns(true)
    SmartProxy.any_instance.stubs(:to_s).returns("puppet")
-    post :create, {:smart_proxy => {:name => "MySmartProxy", :url => "http://nowhere.net:8000"}}, set_session_user
+    post :create, {:smart_proxy => {:name => "MySmartProxy", :url => "http://nowhere.net:8000"}}, set_session_user 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/controllers/api/v2/smart_proxies_controller_test.rb

@@ -2,7 +2,7 @@
 require 'controllers/shared/smart_proxies_controller_shared_test'

 class Api::V2::SmartProxiesControllerTest < ActionController::TestCase
-  valid_attrs = { :name => 'master02', :url => 'http://server:8443' }
+  valid_attrs = { :name => 'master02', :url => 'http://server:8443' } 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

+  end
+
+  test "can search smart proxy by feature" do
+    proxy = smart_proxies(:one) 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -92,9 +92,35 @@ class SmartProxyTest < ActiveSupport::TestCase
  end

  test "should not be saved if features are not array" do
-    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443')
+    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443') 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -83,7 +83,7 @@ class SmartProxyTest < ActiveSupport::TestCase
  end

  test "should not be saved if features do not exist" do
-    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443')
+    proxy = SmartProxy.new(:name => 'Proxy', :url => 'https://some.where.net:8443') 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:32Z

test/models/smart_proxy_test.rb

@@ -69,7 +69,7 @@ class SmartProxyTest < ActiveSupport::TestCase

  test "can count connected hosts" do
    proxy = FactoryGirl.create(:puppet_smart_proxy)
-    FactoryGirl.create(:host, :with_environment, :puppet_proxy => proxy)
+    FactoryGirl.create(:host, :with_environment, :puppet_proxy_hostname => proxy.hostnames.first) 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:33Z

test/models/smart_proxy_test.rb

@@ -49,7 +49,7 @@ class SmartProxyTest < ActiveSupport::TestCase

  # test taxonomix methods
  test "should get used location ids for host" do
-    FactoryGirl.create(:host, :with_environment, :puppet_proxy => smart_proxies(:puppetmaster),
+    FactoryGirl.create(:host, :with_environment, :puppet_proxy_hostname => hostnames(:puppetmaster), 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:33Z

test/controllers/smart_proxies_controller_test.rb

@@ -21,7 +21,7 @@ def test_create_valid
    ProxyAPI::Features.any_instance.stubs(:features => Feature.name_map.keys)
    SmartProxy.any_instance.stubs(:valid?).returns(true)
    SmartProxy.any_instance.stubs(:to_s).returns("puppet")
-    post :create, {:smart_proxy => {:name => "MySmartProxy", :url => "http://nowhere.net:8000"}}, set_session_user
+    post :create, {:smart_proxy => {:name => "MySmartProxy", :url => "http://nowhere.net:8000"}}, set_session_user 


Trailing whitespace detected.

houndci-bot · 2017-05-26T10:15:33Z

test/controllers/api/v2/smart_proxies_controller_test.rb

@@ -2,7 +2,7 @@
 require 'controllers/shared/smart_proxies_controller_shared_test'

 class Api::V2::SmartProxiesControllerTest < ActionController::TestCase
-  valid_attrs = { :name => 'master02', :url => 'http://server:8443' }
+  valid_attrs = { :name => 'master02', :url => 'http://server:8443' } 


Trailing whitespace detected.

theforeman-bot added Needs testing Not yet reviewed labels Feb 28, 2017

houndci-bot reviewed Feb 28, 2017

View reviewed changes

sean797 force-pushed the proxy_int branch 3 times, most recently from 76f044d to e784d0c Compare March 1, 2017 14:48

sean797 changed the title ~~Fixes #18717 - Allows use of proxies on multiple interfaces~~ [WIP] Fixes #18717 - Allows use of proxies on multiple interfaces Mar 2, 2017

sean797 force-pushed the proxy_int branch from e784d0c to 9f70fd3 Compare March 2, 2017 19:45

houndci-bot reviewed Mar 2, 2017

View reviewed changes

sean797 force-pushed the proxy_int branch from 9f70fd3 to 210dcda Compare March 3, 2017 12:45

houndci-bot reviewed Mar 3, 2017

View reviewed changes

sean797 force-pushed the proxy_int branch 8 times, most recently from dd36c95 to 29a1490 Compare March 7, 2017 17:38

houndci-bot reviewed Mar 7, 2017

View reviewed changes

sean797 force-pushed the proxy_int branch from 29a1490 to 7473071 Compare March 8, 2017 19:27

houndci-bot reviewed Mar 8, 2017

View reviewed changes

app/models/smart_proxy_url.rb Outdated

end

end

def self.with_features_unscoped(*feature_names)

Copy link

houndci-bot Mar 8, 2017

Choose a reason for hiding this comment

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

Trailing whitespace detected.

sean797 force-pushed the proxy_int branch 6 times, most recently from 059060f to d517cbc Compare March 11, 2017 13:48

sean797 force-pushed the proxy_int branch from 547b776 to 20ae3a8 Compare March 20, 2017 14:23

sean797 added a commit to sean797/katello that referenced this pull request Mar 20, 2017

Refs #18907 - Create proxy with factory girl

65f14af

This fixes the failing test on theforeman/foreman#4346

sean797 mentioned this pull request Mar 20, 2017

Refs #18907 - Create proxy with factory girl Katello/katello#6689

Merged

jlsherrill pushed a commit to Katello/katello that referenced this pull request Mar 20, 2017

Refs #18907 - Create proxy with factory girl

aa4af5a

This fixes the failing test on theforeman/foreman#4346

sean797 force-pushed the proxy_int branch from 20ae3a8 to dd73bf4 Compare March 21, 2017 14:12

sean797 force-pushed the proxy_int branch from dd73bf4 to 756cf56 Compare March 22, 2017 17:12

sean797 force-pushed the proxy_int branch from 756cf56 to 9f192b6 Compare May 2, 2017 10:05

sean797 force-pushed the proxy_int branch from 9f192b6 to e99c115 Compare May 15, 2017 15:28

sean797 force-pushed the proxy_int branch 2 times, most recently from 5da98af to 52ffd33 Compare May 26, 2017 10:15

houndci-bot reviewed May 26, 2017

View reviewed changes

sean797 force-pushed the proxy_int branch 10 times, most recently from 1b15bee to 33c7a64 Compare May 30, 2017 10:03

Fixes #18717 - Allows use of proxies on multiple interfaces

d4a96ce

sean797 force-pushed the proxy_int branch from 33c7a64 to d4a96ce Compare May 30, 2017 11:27

sean797 closed this May 30, 2017

sean797 mentioned this pull request May 30, 2017

Fixes #18717 - Allow for loadbalanced and multi-homed proxies #4561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #18717 - Allows use of proxies on multiple interfaces #4346

Fixes #18717 - Allows use of proxies on multiple interfaces #4346

sean797 commented Feb 28, 2017

mention-bot commented Feb 28, 2017

houndci-bot Feb 28, 2017

houndci-bot Feb 28, 2017

houndci-bot Feb 28, 2017

houndci-bot Feb 28, 2017

sean797 commented Mar 2, 2017 •

edited

Loading

houndci-bot Mar 2, 2017

houndci-bot Mar 3, 2017

houndci-bot Mar 7, 2017

houndci-bot Mar 8, 2017

sean797 commented Mar 20, 2017 •

edited

Loading

sean797 commented Mar 22, 2017

dmitri-d commented Mar 22, 2017

sean797 commented Mar 24, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

houndci-bot May 26, 2017

Fixes #18717 - Allows use of proxies on multiple interfaces #4346

Fixes #18717 - Allows use of proxies on multiple interfaces #4346

Conversation

sean797 commented Feb 28, 2017

mention-bot commented Feb 28, 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

sean797 commented Mar 2, 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

sean797 commented Mar 20, 2017 • edited Loading

sean797 commented Mar 22, 2017

dmitri-d commented Mar 22, 2017

sean797 commented Mar 24, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean797 commented Mar 2, 2017 •

edited

Loading

sean797 commented Mar 20, 2017 •

edited

Loading