[Ovirt-devel] [PATCH server] allow admin to setup iptables port forwarding on server for a vm's vnc port

[David Lutterkort]

On Tue, 2009-01-27 at 17:32 -0500, Mohammed Morsi wrote:
> diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
> index 523e81e..486798f 100644
> --- a/src/app/views/vm/_form.rhtml
> +++ b/src/app/views/vm/_form.rhtml
> @@ -51,6 +51,21 @@
>      <div class="clear_row"></div>
>      <div class="clear_row"></div>
>  
> +    <div style="width: 50%; float: left;">
> +    <%= check_box_tag_with_label "Forward vm's vnc <b>port</b> locally", "forward_vnc", 1, @vm.forward_vnc %>
> +    </div>
> +    <div style="width: 40%; float: left;">
> +    <%= text_field_with_label "", "vm", "forward_vnc_port", { :style=>"width: 80px;", :size => 7, :disabled => ! @vm.forward_vnc } %>
> +    </div>
> +    <div style="clear:both;"></div>
> +    <div class="clear_row"></div>
> +    <script type="text/javascript">
> +      $("#forward_vnc").click(function(){
> +        $("#vm_forward_vnc_port").attr("disabled", $("#forward_vnc").is(":checked") ? "" : "disabled");
> +      });
> +    </script>
> +
> +
>     <%= check_box_tag_with_label "Start VM Now? (pending current resource availability)", "start_now", nil if create or @vm.state == Vm::STATE_STOPPED %>
>     <%= check_box_tag_with_label "Restart VM Now? (pending current resource availability)", "restart_now", nil if @vm.state == Vm::STATE_RUNNING %>

Why does the user need to allocate the port manually ? Couldn't we do
that internally when the VM is started and release the allocation when
it's taken down ?

> diff --git a/src/task-omatic/taskomatic.rb b/src/task-omatic/taskomatic.rb
> index bcb9bd3..dca6fb7 100755
> --- a/src/task-omatic/taskomatic.rb
> +++ b/src/task-omatic/taskomatic.rb
> @@ -32,6 +32,7 @@ include Daemonize
>  
>  require 'task_vm'
>  require 'task_storage'
> +require 'vnc'
>  
>  class TaskOmatic
>  
> @@ -232,6 +233,8 @@ class TaskOmatic
>        raise "Error destroying VM: #{result.text}" unless result.status == 0
>      end
>  
> +    closeVmVncPort(vm)
> +
>      # undefine can fail, for instance, if we live migrated from A -> B, and
>      # then we are shutting down the VM on B (because it only has "transient"
>      # XML).  Therefore, just ignore undefine errors so we do the rest
> @@ -303,6 +306,7 @@ class TaskOmatic
>      # of places so you'll see a lot of .reloads.
>      db_vm.reload
>      set_vm_vnc_port(db_vm, result.description) unless result.status != 0
> +    forwardVmVncPort(db_vm)

These methods should be called close_vm_vnc_port and forward_vm_vnc_port
to stick with the standard Ruby style.

> diff --git a/src/task-omatic/vnc.rb b/src/task-omatic/vnc.rb
> new file mode 100644
> index 0000000..3eb0ca6
> --- /dev/null
> +++ b/src/task-omatic/vnc.rb
> @@ -0,0 +1,140 @@
> +# Copyright (C) 2008 Red Hat, Inc.
> +# Written by Mohammed Morsi <mmorsi at redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA  02110-1301, USA.  A copy of the GNU General Public License is
> +# also available at http://www.gnu.org/copyleft/gpl.html.
> +
> +# TODO no ruby/libiptc wrapper exists, when
> +# it does replace iptables command w/ calls to it
> + at iptables_cmd='/sbin/iptables '
> +
> +# TODO replace this w/ dnsruby inclusion / call
> + at dns_lookup_cmd='/usr/bin/dig'
> +
> + at ip_forward_command='echo 1 > /proc/sys/net/ipv4/ip_forward'

All this should at least become a module, and not just random things in
the toplevel. Also, please stay with the Ruby style of using '_' to
separate words in identifiers instead of StudLyCaps.

Also, use constants, not instance variables for those strings, i.e.

        IPTABLES_CMD = '/sbin/iptables'
        
> + at vnc_debug = false
> +
> +# TODO can this be retreived in any way
> +# since machine will have both external
> +# and internal network interface
> + at local_ip = '192.168.50.2'
> +
> +############################## 'private' methods

If you make this a module or a class, you can actually mark the methods
as private; for a module it requires a small amount of trickery:

        module Foo
          def self.pb; puts "public"; end
        
          def self.pr; puts "private"; end
        
          class << self
            private :pr
          end
        end
        
> +def _debug(msg)
> +  puts "\n" + msg + "\n" if @vnc_debug
> +end
> +
> +def _findVmHostIp(vm)
> +  cmdout='/tmp/ovirtvnc' + vm.forward_vnc_port.to_s
> +  cmd=@dns_lookup_cmd + ' ' + vm.host.hostname + 
> +      ' +noall +answer +short > ' + cmdout
> +
> +  system(cmd)
> +
> +  result = File.read(cmdout).rstrip
> +  _debug( "vm host hostname  resolved to " + result.to_s )
> +  return result
> +end

This should use Socket::getaddrinfo instead of shelling out to dig.

> +def _vncPortOpen?(port)
> +  cmdout='/tmp/ovirtvnc' + port.to_s
> +  cmd=@iptables_cmd + ' -t nat -nL | grep ' + port.to_s  +
> +       ' > ' + cmdout
> +  _debug("vncPortOpen? iptables command: " + cmd +
> +         " cmdout " + cmdout)
> +
> +  system(cmd)
> +  
> +   return File.size(cmdout) != 0
> +end

You could save yourself the temp file if you used IO::popen or
backticks:

        `#{IPTABLES_CMD} -t nat -nL`.each_line do |l|
          if l matches port
            return true
          end
        end
        return false
        
David