Add Security group functionality

[smcavallo]

Description

Adds the ability to manage security groups

Issues Resolved

#184

Check List

• [X ] All tests pass. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/TESTING.MD
• [X ] New functionality includes testing.
• [X ] New functionality has been documented in the README if applicable
• [X ] All commits have been signed for the Developer Certificate of Origin. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/CONTRIBUTING.MD

[majormoses]

@smcavallo I left some comments #184 (comment) I'd like to hear more about how this looks from a user story/experience. I am worried that this encourages poor security posture with aws as it would need to be overly permissive to be useful.

[majormoses]

I think this can be more clearly written. Here is an (untested) example inspired from rails source:

# Returns a true if an object is nil or empty
#   empty_array = []
#   empty_hash = {}
#   array = [0,1,2]
#   hash = { one: 1, two: 2, tree: 3}
#   blank?(empty_array) => true
#   blank?(empty_hash} => true
#   blank?(array) => false
#   blank?(hash) => false
#   hash.each_pair do |k,v|
#       blank?(k) || blank?(v) => false
#   end
#   empty_hash.each_pair do |k,v|
#       blank?(k) || blank?(v) => true
#   end
def blank?(obj)
    return true if obj.nil? || obj.respond_to(:'empty?') && obj.empty?
    false
end

# Replaces the hash without the empty key/values.
#   hash = { a: true, b: false, c: nil}
#   array = [0, '1', nil]
#   remove_blank!(hash) # => { a: true, b: false}
#   hash # => { a: true, b: false }
#   remove_blank!(array) # => [0, '1']
#   array # => [0, '1']
def remove_blanks!(obj)
    case obj.class
    when 'Hash'
        obj.each_pair do |k, v|
            delete(key) if blank?(k) || blank?(v)
        end
    when 'Array'
        obj.each do |elem|
            delete(elem) if blank?(elem)
        end
    end
    obj      
end

[smcavallo]

I tried those and several variations without any luck. what's the objection with what is in the PR? that it is patching the Hash class? lack of comments? It's 8 lines of code so very small and the usage is very limited - I wasn't too concerned with it however I can make changes if necessary.

[majormoses]

No real objection just thought it could be simpler for someone who is not very familiar with ruby development to read. Using &block and *splat operators are not intuitive to new ruby developers.

[majormoses]

should we default to have something that includes the security group name in it?

[smcavallo]

@majormoses - a better option might be to make this a required field with no default. what do you think? it's a required field in aws. I was thinking of adding some code to add the name into the description and it seemed like added code and complexity when we could just ask users to set a description.

[majormoses]

If aws requires it then I think we should as well. From what I recall these are immutable so its important to get it right the first time.

[majormoses]

can we add an alias for ip_permissions_ingress? I get this matches the aws sdk and we should follow that but we should probably make it so that implementers can opt for more explicit and readable code.

[smcavallo]

alias added, thanks

[majormoses]

@smcavallo a friendly reminder this is waiting on you to respond to my comments 😉

[smcavallo]

@majormoses - thanks for staying on top of this. I went in and made some changes based on your feedback.

[majormoses]

LGTM