Problem of route provider implementation


#1

The route provider always runs route command regardless of the current state of route´s table.

I have looked closely at the source code and it seems that the implementation contains several errors

a) in line 100 : “is_running” is actually a local variable not the attribute “is_running” of the object so it will get lost not being used in method action_add
this is fundamental error of programming, inside the instance method we must use @is_running to set the value of the attribute of the object
b) the method load_current_resource has wrong logic, checking destination/target of the current and new route , because while destination/target can be
the same, the gateway can be changed and the route should be considered as changed and must be updated
c) the method load_current_resource when pulling data from /proc/net/route shall take into consideration the header of the file and ignore it

[admlh@a0 chef-test]$ cat /proc/net/route
Iface Destination Gateway Flags RefCnt Use Metric Mask MTU Window IRTT
eth0 0064A8C0 00000000 0001 0 0 0 00FFFFFF 0 0 0
eth1 000A10AC 00000000 0001 0 0 0 00FEFFFF 0 0 0
eth1 0000FEA9 00000000 0001 0 0 0 0000FFFF 0 0 0
eth1 00000000 030A10AC 0003 0 0 0 00000000 0 0 0

65 def load_current_resource
66 is_running = nil
67
68 Chef::Log.debug(“Configuring Route #{@new_resource.name}”)
69
70 # cidr or quad dot mask
71 if @new_resource.netmask
72 new_ip = IPAddr.new("#{@new_resource.target}/#{@new_resource.netmask}")
73 else
74 new_ip = IPAddr.new(@new_resource.target)
75 end
76
77 # pull routes from proc
78 if node[:os] == “linux"
79 route_file = ::File.open(”/proc/net/route", “r”)
80 while (line = route_file.gets)
81 # proc layout
82 iface,destination,gateway,flags,refcnt,use,metric,mask,mtu,window,irtt = line.split
83
84 # need to convert packed adresses int quad dot
85 # the addrs are reversed hex packed decimal addrs. so this unwraps them. tho you could
86 # do this without ipaddr using unpack. ipaddr has no htoa method.
87 #
88 destination = IPAddr.new(destination.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
89 gateway = IPAddr.new(gateway.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
90 mask = IPAddr.new(mask.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
91 Chef::Log.debug( “System has route: dest=#{destination} mask=#{mask} gw=#{gateway}”)
92
93 # check if what were trying to configure is already there
94 # use an ipaddr object with ip/mask this way we can have
95 # a new resource be in cidr format (i don’t feel like
96 # expanding bitmask by hand.
97 #
98 running_ip = IPAddr.new("#{destination}/#{mask}")
99 Chef::Log.debug( "new ip: #{new_ip.inspect} running ip: #{running_ip.inspect} ")
100 is_running = true if running_ip == new_ip
101 end
102 route_file.close
103 end
104 end

106     def action_add
107         # check to see if load_current_resource found the route
108         if  is_running
109             Chef::Log.debug("Route #{@new_resource.name} already active ")
110         else
111             command =  "ip route replace #{@new_resource.target}"
112             command << "/#{MASK[@new_resource.netmask.to_s]}" if @new_resource.netmask
113             command << " via #{@new_resource.gateway} "
114             command << " dev #{@new_resource.device} " if @new_resource.device
115
116             Chef::Log.info("Adding route: #{command} ")
117             run_command( :command => command )
118             @new_resource.updated = true
119         end
120
121         #for now we always write the file (ugly but its what it is)
122         generate_config
123
124     end

ATTENTION:
The information in this electronic mail message is private and
confidential, and only intended for the addresses. Should you
receive this message by mistake, you are hereby notified that
any disclosure, reproduction, distribution or use of this
message is strictly prohibited. Please inform the sender by
reply transmission and delete the message without copying or
opening it.


#2

Hi there,

On Wed, Aug 18, 2010 at 8:41 AM, le.huy@ingdirect.es wrote:

The route provider always runs route command regardless of the current state
of route´s table.

I have looked closely at the source code and it seems that the
implementation contains several errors

a) in line 100 : “is_running” is actually a local variable not the
attribute “is_running” of the object so it will get lost not being
used in method action_add
this is fundamental error of programming, inside the instance method we must
use @is_running to set the value of the attribute of the object
b) the method load_current_resource has wrong logic, checking
destination/target of the current and new route , because while
destination/target can be
the same, the gateway can be changed and the route should be considered as
changed and must be updated
c) the method load_current_resource when pulling data from /proc/net/route
shall take into consideration the header of the file and ignore it

This is all fantastic feedback. Any chance you’d be willing to make
the changes you are suggesting inside a git checkout of the source
code and send along a patch?

  • seth

#3

did you happen to make any tickets in jira for these bugs ? Im happy to fix.

On Aug 18, 2010, at 8:41 AM, le.huy@ingdirect.es le.huy@ingdirect.es wrote:

The route provider always runs route command regardless of the current state of route´s table.

I have looked closely at the source code and it seems that the implementation contains several errors

a) in line 100 : “is_running” is actually a local variable not the attribute “is_running” of the object so it will get lost not being used in method action_add
this is fundamental error of programming, inside the instance method we must use @is_running to set the value of the attribute of the object
b) the method load_current_resource has wrong logic, checking destination/target of the current and new route , because while destination/target can be
the same, the gateway can be changed and the route should be considered as changed and must be updated
c) the method load_current_resource when pulling data from /proc/net/route shall take into consideration the header of the file and ignore it

[admlh@a0 chef-test]$ cat /proc/net/route
Iface Destination Gateway Flags RefCnt Use Metric Mask MTU Window IRTT
eth0 0064A8C0 00000000 0001 0 0 0 00FFFFFF 0 0 0
eth1 000A10AC 00000000 0001 0 0 0 00FEFFFF 0 0 0
eth1 0000FEA9 00000000 0001 0 0 0 0000FFFF 0 0 0
eth1 00000000 030A10AC 0003 0 0 0 00000000 0 0 0

65 def load_current_resource
66 is_running = nil
67
68 Chef::Log.debug(“Configuring Route #{@new_resource.name}”)
69
70 # cidr or quad dot mask
71 if @new_resource.netmask
72 new_ip = IPAddr.new("#{@new_resource.target}/#{@new_resource.netmask}")
73 else
74 new_ip = IPAddr.new(@new_resource.target)
75 end
76
77 # pull routes from proc
78 if node[:os] == “linux"
79 route_file = ::File.open(”/proc/net/route", “r”)
80 while (line = route_file.gets)
81 # proc layout
82 iface,destination,gateway,flags,refcnt,use,metric,mask,mtu,window,irtt = line.split
83
84 # need to convert packed adresses int quad dot
85 # the addrs are reversed hex packed decimal addrs. so this unwraps them. tho you could
86 # do this without ipaddr using unpack. ipaddr has no htoa method.
87 #
88 destination = IPAddr.new(destination.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
89 gateway = IPAddr.new(gateway.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
90 mask = IPAddr.new(mask.scan(/…/).reverse.to_s.hex, Socket::AF_INET).to_s
91 Chef::Log.debug( “System has route: dest=#{destination} mask=#{mask} gw=#{gateway}”)
92
93 # check if what were trying to configure is already there
94 # use an ipaddr object with ip/mask this way we can have
95 # a new resource be in cidr format (i don’t feel like
96 # expanding bitmask by hand.
97 #
98 running_ip = IPAddr.new("#{destination}/#{mask}")
99 Chef::Log.debug( "new ip: #{new_ip.inspect} running ip: #{running_ip.inspect} ")
100 is_running = true if running_ip == new_ip
101 end
102 route_file.close
103 end
104 end
106 def action_add
107 # check to see if load_current_resource found the route
108 if is_running
109 Chef::Log.debug("Route #{@new_resource.name} already active ")
110 else
111 command = "ip route replace #{@new_resource.target}"
112 command << “/#{MASK[@new_resource.netmask.to_s]}” if @new_resource.netmask
113 command << " via #{@new_resource.gateway} "
114 command << " dev #{@new_resource.device} " if @new_resource.device
115
116 Chef::Log.info("Adding route: #{command} ")
117 run_command( :command => command )
118 @new_resource.updated = true
119 end
120
121 #for now we always write the file (ugly but its what it is)
122 generate_config
123
124 end


ATTENTION:
The information in this electronic mail message is private and
confidential, and only intended for the addressee. Should you
receive this message by mistake, you are hereby notified that
any disclosure, reproduction, distribution or use of this
message is strictly prohibited. Please inform the sender by
reply transmission and delete the message without copying or
opening it.
Messages and attachments are scanned for all viruses known.
If this message contains password-protected attachments, the
files have NOT been scanned for viruses by the ING mail domain.
Always scan attachments before opening them.


#4

http://tickets.opscode.com/browse/CHEF-1598


De: Jesse Nelson [mailto:spheromak@gmail.com]
Enviado el: miércoles, 18 de agosto de 2010 22:46
Para: chef@lists.opscode.com
Asunto: [chef] Re: problem of route provider implementation

did you happen to make any tickets in jira for these bugs ? Im happy to fix.

On Aug 18, 2010, at 8:41 AM, le.huy@ingdirect.es le.huy@ingdirect.es wrote:

The route provider always runs route command regardless of the current state of route´s table.
 
I have looked closely at the source code and it seems that the implementation contains several  errors
 
 
a) in line 100 :  "is_running" is actually a local variable not the attribute "is_running" of the object so it will get lost not being used in method action_add
this is fundamental error of programming, inside the instance method we must use @is_running to set the value of the attribute of the object
b) the method load_current_resource has wrong logic, checking destination/target of the current and new route , because while destination/target can be
the same, the gateway can be changed  and the route should be considered as changed and must be updated
c) the method load_current_resource when pulling data from /proc/net/route shall take into consideration the header of the file and ignore it
 
[admlh@a0 chef-test]$ cat /proc/net/route
Iface   Destination     Gateway         Flags   RefCnt  Use     Metric  Mask            MTU     Window  IRTT                         
eth0    0064A8C0        00000000        0001    0       0       0       00FFFFFF        0       0       0                            
eth1    000A10AC        00000000        0001    0       0       0       00FEFFFF        0       0       0                            
eth1    0000FEA9        00000000        0001    0       0       0       0000FFFF        0       0       0                            
eth1    00000000        030A10AC        0003    0       0       0       00000000        0       0       0 
 
 
 65     def load_current_resource
 66         is_running = nil
 67
 68         Chef::Log.debug("Configuring Route #{@new_resource.name}")
 69
 70         # cidr or quad dot mask
 71         if @new_resource.netmask
 72             new_ip = IPAddr.new("#{@new_resource.target}/#{@new_resource.netmask <mailto:#{@new_resource.target}/%23{@new_resource.netmask> }")
 73         else
 74             new_ip = IPAddr.new(@new_resource.target)
 75         end
 76
 77         # pull routes from proc
 78         if node[:os] == "linux"
 79             route_file = ::File.open("/proc/net/route", "r")
 80             while (line = route_file.gets)
 81                 # proc layout
 82                 iface,destination,gateway,flags,refcnt,use,metric,mask,mtu,window,irtt = line.split
 83
 84                 # need to convert packed adresses int quad dot
 85                 #  the addrs are reversed hex packed decimal addrs. so this unwraps them. tho you could
 86                 #  do this without ipaddr using unpack. ipaddr has no htoa method.
 87                 #
 88                 destination = IPAddr.new(destination.scan(/../).reverse.to_s.hex, Socket::AF_INET).to_s
 89                 gateway = IPAddr.new(gateway.scan(/../).reverse.to_s.hex, Socket::AF_INET).to_s
 90                 mask = IPAddr.new(mask.scan(/../).reverse.to_s.hex, Socket::AF_INET).to_s
 91                 Chef::Log.debug( "System has route:  dest=#{destination} mask=#{mask} gw=#{gateway}")
 92
 93                 # check if what were trying to configure is already there
 94                 # use an ipaddr object with ip/mask this way we can have
 95                 # a new resource be in cidr format (i don't feel like
 96                 # expanding bitmask by hand.
 97                 #
 98                 running_ip = IPAddr.new("#{destination}/#{mask}")
 99                 Chef::Log.debug( "new ip: #{new_ip.inspect} running ip: #{running_ip.inspect} ")
100                 is_running = true if running_ip == new_ip
101             end
102             route_file.close
103         end
104     end

    106     def action_add
    107         # check to see if load_current_resource found the route
    108         if  is_running
    109             Chef::Log.debug("Route #{@new_resource.name} already active ")
    110         else
    111             command =  "ip route replace #{@new_resource.target}"
    112             command << "/#{MASK[@new_resource.netmask.to_s]}" if @new_resource.netmask
    113             command << " via #{@new_resource.gateway} "
    114             command << " dev #{@new_resource.device} " if @new_resource.device
    115
    116             Chef::Log.info("Adding route: #{command} ")
    117             run_command( :command => command )
    118             @new_resource.updated = true
    119         end
    120
    121         #for now we always write the file (ugly but its what it is)
    122         generate_config
    123
    124     end


 
 
 
 
-----------------------------------------------------------------
ATTENTION:
The information in this electronic mail message is private and
confidential, and only intended for the addressee. Should you
receive this message by mistake, you are hereby notified that
any disclosure, reproduction, distribution or use of this
message is strictly prohibited. Please inform the sender by
reply transmission and delete the message without copying or
opening it.
Messages and attachments are scanned for all viruses known.
If this message contains password-protected attachments, the
files have NOT been scanned for viruses by the ING mail domain.
Always scan attachments before opening them.
-----------------------------------------------------------------

ATTENTION:
The information in this electronic mail message is private and
confidential, and only intended for the addresses. Should you
receive this message by mistake, you are hereby notified that
any disclosure, reproduction, distribution or use of this
message is strictly prohibited. Please inform the sender by
reply transmission and delete the message without copying or
opening it.