Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Support for additional groups and basic expectations in sockets #123

Open
wants to merge 3 commits into from

2 participants

@vihai

Hello, you may want to consider the two commits I made which implement initialization of additional group membership while changing privileges and basic expectation rules on what the SocketResponding condition should read from a socket to consider it alive.

@eric eric commented on the diff
lib/god/process.rb
@@ -292,7 +292,7 @@ def spawn(command)
::Dir.chroot(self.chroot) if self.chroot
::Process.setsid
- ::Process.groups = [gid_num] if self.gid
+ ::Process.initgroups(self.uid, gid_num) if self.uid
@eric Collaborator
eric added a note

Did this intend to say if self.uid but not check for self.gid?

@vihai
vihai added a note

Well, the ratio would be that you would init additional groups only if you're switching UID. It has to be checked that if the UID change clears the additional groups to avoid keeping the former additional groups when changing only the GID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@eric
Collaborator

Thanks for the pull request!

If you could add some tests for your new SocketResponding behavior, the additional options for the condition look great.

Just a side comment related to pull requests: it's easier to get things merged if unrelated changes are submitted as separate pull requests (by creating the changes in separate branches) to prevent problems or discussions from one change hang up an unrelated one.

lib/god/conditions/socket_responding.rb
@@ -118,6 +128,24 @@ def test
else
status = false
end
+
+ begin
+ timeout(expect_timeout) do
+ s.write(send_data) if send_data
+ if expect_data
+ line = s.readline.chop
+ if expect_data.is_a?(Regexp) && !expect_data.match(line) ||
+ line != expect_data
@eric Collaborator
eric added a note

For readability I think it would be good to put parens around the grouped conditions:

              if (expect_data.is_a?(Regexp) && !expect_data.match(line)) || line != expect_data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 29, 2013
  1. @vihai
  2. @vihai
Commits on Feb 7, 2013
  1. @vihai

    Fixed socket_responding

    vihai authored
This page is out of date. Refresh to see the latest.
Showing with 30 additions and 2 deletions.
  1. +29 −1 lib/god/conditions/socket_responding.rb
  2. +1 −1  lib/god/process.rb
View
30 lib/god/conditions/socket_responding.rb
@@ -1,4 +1,5 @@
require 'socket'
+require 'timeout'
include Socket::Constants
module God
@@ -14,6 +15,9 @@ module Conditions
# --one of port or path--
# +port+ is the port (required if +family+ is 'tcp')
# +path+ is the path (required if +family+ is 'unix')
+ # +send_data+ send this string upon connecting
+ # +expect_data+ String/Regexp to which the socket first read line should match
+ # +expect_timeout+ Time to wait for the response
#
# Examples
#
@@ -48,6 +52,8 @@ module Conditions
class SocketResponding < PollCondition
attr_accessor :family, :addr, :port, :path, :times
+ attr_accessor :send_data
+ attr_accessor :expect_data, :expect_timeout
def initialize
super
@@ -59,6 +65,10 @@ def initialize
self.path = nil
self.times = [1, 1]
+
+ self.send_data = nil
+ self.expect_data = nil
+ self.expect_timeout = 5
end
def prepare
@@ -116,8 +126,26 @@ def test
end
status = s.nil?
else
- status = false
+ begin
+ timeout(expect_timeout) do
+ s.write(send_data) if send_data
+ if expect_data
+ line = s.readline.chop
+ if expect_data.is_a?(Regexp) && !expect_data.match(line) ||
+ line != expect_data
+ self.info = "socket expected response does not match"
+ status = true
+ end
+ end
+ end
+ rescue Timeout::Error
+ self.info = "socket expect timeout"
+ status = true
+ else
+ status = false
+ end
end
+
@timeline.push(status)
history = "[" + @timeline.map {|t| t ? '*' : ''}.join(',') + "]"
if @timeline.select { |x| x }.size >= self.times.first
View
2  lib/god/process.rb
@@ -292,7 +292,7 @@ def spawn(command)
::Dir.chroot(self.chroot) if self.chroot
::Process.setsid
- ::Process.groups = [gid_num] if self.gid
+ ::Process.initgroups(self.uid, gid_num) if self.uid
@eric Collaborator
eric added a note

Did this intend to say if self.uid but not check for self.gid?

@vihai
vihai added a note

Well, the ratio would be that you would init additional groups only if you're switching UID. It has to be checked that if the UID change clears the additional groups to avoid keeping the former additional groups when changing only the GID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
::Process::Sys.setgid(gid_num) if self.gid
::Process::Sys.setuid(uid_num) if self.uid
self.dir ||= '/'
Something went wrong with that request. Please try again.