#1871 - Amazon EC2 VPC support #778

moshea · 2013-07-12T12:56:00Z

issue 1871

ability to select a security group for a amazon instance inside a vpc
this is a second pull request after the changes suggested from the first pull
main changes:
managed_ip moved to a VM
vpc and security groups display human names where possible (based on the standard tag "Name" )
moved code into helpers out of .erb files

this patch is rebased from 1.0 to 1.2. original patch created by rvrignaud

moved managed_ip to fog_extentions/aws/server security groups and VPC are displayed by name where possible instead of by amazon id moved code from _ec2 form into a helper

reverting provided_attributes to take the host out of it

updating the initial load of the security group list, so that names are displayed instead of the amazon ids on first load of virtual machine page

made static strings into translation ones using _(...) changed .map to .find_all in security_groups, in case there are any nil values

skottler · 2013-07-12T12:58:28Z

Gemfile

@@ -15,6 +15,7 @@ gem 'uuidtools'
 gem "apipie-rails", "~> 0.0.22"
 gem 'rabl', '>= 0.7.5'
 gem 'oauth'
+gem 'puppet'


This isn't in the gemfile because most people will use the system version. For development you should put it into bundler.d/Gemfile.local.rb.

OK, great, thanks for the tip!

On Fri, Jul 12, 2013 at 1:58 PM, Sam Kottler notifications@github.comwrote:

In Gemfile:

@@ -15,6 +15,7 @@ gem 'uuidtools'
gem "apipie-rails", "~> 0.0.22"
gem 'rabl', '>= 0.7.5'
gem 'oauth'
+gem 'puppet'

This isn't in the gemfile because most people will use the system version.
For development you should put it into bundler.d/Gemfile.local.rb.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/778/files#r5163939
.

This line is causing the tests to fail. Would you mind removing it so the initial tests can run?

skottler · 2013-07-12T13:18:41Z

[test]

made static strings into translation ones using _(...) changed .map to .find_all in security_groups, in case there are any nil values

skottler · 2013-07-12T14:36:31Z

[test]

ohadlevy · 2013-07-14T11:53:20Z

app/assets/javascripts/compute_resource.js

@@ -154,3 +154,16 @@ function enable_libvirt_dropdown(item){
  item.find(':input').attr('disabled',false)
  item.show();
 }
+
+function ec2_vpcSelected(form, security_groups, subnets){
+  $('#host_compute_attributes_security_group_ids').empty()


while not critical, the vm creation forms are also avail under /compute_resources/id/vms/new, hardcoding the host_id would not work no these conditions, maybe change to a regexp match that checks the ending of the id with security_groups_ids instead?

there isn't any reference from /compute_resources/id/vms/new to the function name yet.
maybe we could wait until that functionality has being added?

what is the difference between compute_resources and compute_resources_vms (presume its a difference between baremetal and could servers)?

compute_resource is the creation of the CR itself, CR_vms is a shortcut to create just the vm vs the full blown host object.

ah ok.
i see what you are getting at now.
this will need a little bit more work for another reason as when the multi
select list is created, it doesn't like to be changed by just altering the
options and calling multiSelectOnLoad();
but will also make the selection a regex so it picks up both multiselect
lists.

On Mon, Jul 15, 2013 at 2:42 PM, Ohad Levy notifications@github.com wrote:

In app/assets/javascripts/compute_resource.js:

@@ -154,3 +154,16 @@ function enable_libvirt_dropdown(item){
item.find(':input').attr('disabled',false)
item.show();
}
+
+function ec2_vpcSelected(form, security_groups, subnets){

$('#host_compute_attributes_security_group_ids').empty()

compute_resource is the creation of the CR itself, CR_vms is a shortcut to
create just the vm vs the full blown host object.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/778/files#r5189608
.

changing attributes to symbolic versions instead of direct method calls

moshea · 2013-07-15T13:39:40Z

lib/fog_extensions/aws/server.rb

@@ -13,6 +16,10 @@ def dns
         dns_name || private_dns_name
      end

+      def vm_ip_address
+        managed_ip == 'private' ? :private_ip_address : :public_ip_address


changed private_ip_address and public_ip_address to symbolic calls as suggested by @ohadlevy
both seem to work, but symbolic calls seem to comply more with convention

after some testing, this doesn't work. private_ip_address and public_ip_address need to be method names, otherwise the string private_ip_address is stored as the IP address of the vm.

This should be a method as vm_ip_address needs to the return the actual IP value.

Yes, after some extermination, that is the way I'll set it in the next
patch update.

One other thing, for the multi-select box, it seems the current library
that foreman uses is a little buggy around updating the multiselect box and
refreshing it.
After updating the library to v0.9.8 from v0.9.3, everything starting
working straight away.
Any objections to updating the library again to 0.9.8?

Mark

On Wed, Jul 17, 2013 at 10:47 AM, Dominic Cleal notifications@github.comwrote:

In lib/fog_extensions/aws/server.rb:

@@ -13,6 +16,10 @@ def dns
dns_name || private_dns_name
end

def vm_ip_address

managed_ip == 'private' ? :private_ip_address : :public_ip_address

This should be a method as vm_ip_address needs to the return the actual IP
value.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/778/files#r5238013
.

I asked @abenari about this and he said:

155901 < amos_> Dominic, replacing multi-select to 0.9.8 broke the headers (with the filter, select all, unselect all) I'll need to look into it probably a small issue.

Maybe he can suggest a fix that we can apply as part of this patch?

Tried upgrading the multi-select from v0.9.3 to v0.9.8 and found that on some of the multi-select boxes, in organization edit form the header that holds the filter and select/unselect all is missing. I was also able to unselect disabled items.

ohadlevy · 2013-07-18T05:19:03Z

[test]

skottler · 2013-07-18T14:10:46Z

@moshea can you please merge the develop branch into your branch so the patch will cleanly apply with git apply -v?

domcleal · 2013-07-18T14:35:31Z

app/views/compute_resources_vms/form/_ec2.html.erb

+<% security_groups = compute_resource.security_groups %>
+<% vpc_id = @host && @host.vpc_id %>
+
+<%= selectable_f f, :security_group_ids, security_groups.map{|sg| [sg.name, sg.group_id,] if sg.vpc_id == vpc_id}.compact!,


To avoid the .map.compact! expression, you can do security_groups.select { |sg| .. } instead

Sorry, re-reading this, it's actually restricting the options listed to the users if they're editing a host and have a security group selected.. seems that instead it should use multiple_selects instead of selectable_f + multiple: true, then you're able to pass in the currently selected value. Have a look at app/views/taxonomies/_form.html.erb for how they work.

If it's not possible (or sensible?) to change this on the fly, it should probably be disabled too for non-new hosts.

@domcleal I agree with everything you said. It's not possible to change the security groups a host is in after its launched so existing hosts need to have re-selection disabled when being edited.

The VMware form has a good example of this, borrow the new/disabled code from there.

patched jquery.multi-select so that 'refresh' method would now refresh the select box fixed multiselect form submission and initial loading issues moved vpc_id from generic host object to a helper function vpc_security_group_hash and subnet_vpc_hash added as 'data' attributes on select_f in _ec2.html.erb, they are now accessible without needing to export as JSON in this :onchange attribute rewrote security_groups method to make it cleaner cleaned up some test code

moshea · 2013-07-23T16:34:20Z

@domcleal - the multiselect library has being reverted back, however, I made a 4 line patch which fixes the refreshing of the select list. luckily it was so easy.

All of the other changes have being made except for the suggest about moving an f_selectable :multiple=> true to a multiple_select. possibly my lack of ruby front end, but multiple_select seems to want to work on ActiveRecord objects, and isn't so great for array's of hashes, as the .all method is used in multiple_selects

jmontleon · 2013-07-25T23:05:03Z

app/views/compute_resources_vms/form/_ec2.html.erb

+  {},
+  { :multiple => true, :label => _("Security groups"), :disabled => !@host.nil?, 
+  	:data => {:security_groups => vpc_security_group_hash(security_groups), :subnets => subnet_vpc_hash(subnets)} } %>
+


The above is causing an error for me when not using an account with a VPC, although it works as I'd expect on an account with a VPC configured.

edit: hmm, having trouble reproducing it now...

jmontleon · 2013-07-30T21:13:15Z

has DNS been tested by anyone else besides me? With a DNS proxy configured I get:

NoMethodError
undefined method []' for nil:NilClass app/helpers/compute_resources_vms_helper.rb:71:incompute_object_vpc_id'...

@jmontleon

patched jquery.multi-select so that 'refresh' method would now refresh the select box fixed multiselect form submission and initial loading issues moved vpc_id from generic host object to a helper function vpc_security_group_hash and subnet_vpc_hash added as 'data' attributes on select_f in _ec2.html.erb, they are now accessible without needing to export as JSON in this :onchange attribute rewrote security_groups method to make it cleaner cleaned up some test code FIXED: issue reported by @jmontleon, using an account without a VPC causes an error on _ec2.html.erb

@jmontleon

Conflicts: app/helpers/compute_resources_vms_helper.rb app/views/compute_resources_vms/form/_ec2.html.erb FIXED: issue reported by @jmontleon error when trying to create an instance without a VPC - error has being fixed, by checking for mil on compute_resource.security_groups. However, it turns out, once the default VPC has being removed, instances can no longer be created and need to contact amazon support to create a default VPC. Also cleaned up the code a little bit, and placed some logic inside a helper, getting security groups for VPC's

moshea · 2013-07-31T08:21:54Z

Hi @jmontleon
How do you setup a DNS proxy?(I haven't needed it before) I think the fix will be to check for nil on form.object.network_interfaces[0], however would like to test it here myself.

jmontleon · 2013-07-31T15:59:14Z

@moshea it's pretty straightforward. Install bind, create a forward zone, (could literally just be example.com if it's for testing) and configure as outlined here: http://www.theforeman.org/manuals/1.2/index.html#4.3.5BIND

One thing that might be nice, is adding ptr records for vpc hosts, but not critical for us, I don't think.

Also, fwiw I literally just added rescue nil to the end of that line and it works now.

jmontleon · 2013-08-01T12:13:02Z

@moshea another small point if it's possible. Can the subnet selection go above the security groups? As it is, because the security groups change based on the subnet selected it almost makes more sense to work from the bottom of the page up when making your selections.

jmontleon · 2013-08-01T12:16:32Z

@moshea also if I select one of the AWS Compute Resources when creating a new host and then select a hostgroup the Security Groups box gets grayed out. If I unselect and select the compute resource (or select it after selecting a hostgroup) the items listed in it are selectable (again).

moshea · 2013-08-23T14:05:03Z

@jmontleon moving the subnet selection above the security group makes perfect sense. so implemented that change. Also, stopped the securty group dialog box being disabled when a hostgroup is selected.
The problem there was, I disabled the security groups once a @host object exists (as you can't change security groups on an existing host). However, when you select a hostgroup, that generates a @host object. Just put in another check to see if the host has an ip address or not. seems to do the trick.
The rescue nil has gone in as well

moving the subnet selection above the security group stopped the securty group dialog box being disabled when a hostgroup is selected. The rescue nil for DNS proxy errors Conflicts: app/models/concerns/fog_extensions/aws/server.rb

moshea · 2013-08-23T14:13:30Z

sorry guys, seems I have messed up the pull request a little bit. i'll work on fixing it a little later today.

lzap · 2013-09-05T10:11:06Z

@lzap reminder - add this to Headline Feature for 1.3 if it gets merged

domcleal · 2013-09-07T12:35:37Z

Thank you for your efforts preparing and finalising this @moshea, and to @rvrignaud for the original implementation. I'm happy to say it's been merged as beed05d for Foreman 1.3.

Mark O'Shea added 9 commits July 12, 2013 12:00

fixes theforeman#1871

5e24610

this patch is rebased from 1.0 to 1.2. original patch created by rvrignaud

fixes theforeman#1871 typo in java script

5219ea4

fixes theforeman#1871 - removing duplicate dropdown box

cf068f5

fixes 1871 - removing empty params from security group multiselect list

e110ebd

removing .project file

fb66426

fixed theforeman#1871

5bb38c1

moved managed_ip to fog_extentions/aws/server security groups and VPC are displayed by name where possible instead of by amazon id moved code from _ec2 form into a helper

fixes theforeman#1871

c38e8fd

reverting provided_attributes to take the host out of it

fixes theforeman#1871

8752ed4

updating the initial load of the security group list, so that names are displayed instead of the amazon ids on first load of virtual machine page

fixes theforeman#1871

a4f6faa

made static strings into translation ones using _(...) changed .map to .find_all in security_groups, in case there are any nil values

skottler reviewed Jul 12, 2013
View reviewed changes

skottler mentioned this pull request Jul 12, 2013

1871 #743

Closed

fixes theforeman#1871

c6bd11a

made static strings into translation ones using _(...) changed .map to .find_all in security_groups, in case there are any nil values

ohadlevy reviewed Jul 14, 2013
View reviewed changes

Merge branch 'develop' of github.com:moshea/foreman into develop

d39ae9b

changing attributes to symbolic versions instead of direct method calls

moshea reviewed Jul 15, 2013
View reviewed changes

domcleal reviewed Jul 18, 2013
View reviewed changes

domcleal mentioned this pull request Jul 19, 2013

fixes #1871 Add support for VPC on EC2 compute #237

Closed

jmontleon reviewed Jul 25, 2013
View reviewed changes

Mark O'Shea added 2 commits July 31, 2013 08:57

moshea closed this Jul 31, 2013

moshea reopened this Jul 31, 2013

domcleal closed this Sep 7, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1871 - Amazon EC2 VPC support #778

#1871 - Amazon EC2 VPC support #778

moshea commented Jul 12, 2013

skottler Jul 12, 2013

moshea Jul 12, 2013

skottler Jul 12, 2013

skottler commented Jul 12, 2013

skottler commented Jul 12, 2013

ohadlevy Jul 14, 2013

moshea Jul 15, 2013

ohadlevy Jul 15, 2013

moshea Jul 15, 2013

moshea Jul 15, 2013

moshea Jul 16, 2013

domcleal Jul 17, 2013

moshea Jul 17, 2013

domcleal Jul 17, 2013

abenari Jul 18, 2013

ohadlevy commented Jul 18, 2013

skottler commented Jul 18, 2013

domcleal Jul 18, 2013

domcleal Jul 18, 2013

skottler Jul 18, 2013

domcleal Jul 18, 2013

moshea commented Jul 23, 2013

jmontleon Jul 25, 2013

jmontleon commented Jul 30, 2013

moshea commented Jul 31, 2013

jmontleon commented Jul 31, 2013

jmontleon commented Aug 1, 2013

jmontleon commented Aug 1, 2013

moshea commented Aug 23, 2013

moshea commented Aug 23, 2013

lzap commented Sep 5, 2013

domcleal commented Sep 7, 2013

#1871 - Amazon EC2 VPC support #778

#1871 - Amazon EC2 VPC support #778

Conversation

moshea commented Jul 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottler commented Jul 12, 2013

skottler commented Jul 12, 2013

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

ohadlevy commented Jul 18, 2013

skottler commented Jul 18, 2013

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

moshea commented Jul 23, 2013

Choose a reason for hiding this comment

jmontleon commented Jul 30, 2013

moshea commented Jul 31, 2013

jmontleon commented Jul 31, 2013

jmontleon commented Aug 1, 2013

jmontleon commented Aug 1, 2013

moshea commented Aug 23, 2013

moshea commented Aug 23, 2013

lzap commented Sep 5, 2013

domcleal commented Sep 7, 2013