Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow greater flexibility in listen directive. #119

Merged
merged 1 commit into from
Sep 9, 2014

Conversation

Spredzy
Copy link

@Spredzy Spredzy commented Jul 28, 2014

Currently the module does not allow to specify specific bind options
per IP we are binding haproxy onto.
Hence, if a user has a service available across several network, but
with different haproxy bind option according to the network, s/he can't
configure haproxy with the current state of this module.

It aims to make it possible to have configuration as the following:

  bind 192.168.2.1:80 ssl crt public.puppetlabs.com.crt
  bind 10.0.0.1:80 ssl crt private.puppetlabs.com.crt
  bind 178.35.67.12:80

by doing

haproxy::listen {'apache00' : 
  ports => '80',
  ipaddresses_and_bind_options => {'192.168.2.1'  => ['ssl', 'crt', 'public.puppetlabs.com.crt'],
                                   '10.0.0.1'     => ['ssl', 'crt', 'private.puppetlabs.com.crt'],
                                   '178.35.67.12' => []},
}

@hunner
Copy link

hunner commented Aug 6, 2014

What do you think about using the existing parameters in a different way to accomplish the same thing? Perhaps if the ipaddress and bind_options are arrays, then it would assign the options to the ip addresses? Something like:

haproxy::listen {'apache00' :
  ipaddress    => [
    '192.168.2.1',
    '10.0.0.1',
    '178.35.67.12',
  ],
  ports        => '80',
  bind_options => [
    ['ssl', 'crt', 'public.puppetlabs.com.crt'],
    ['ssl', 'crt', 'private.puppetlabs.com.crt'],
    [],
  ],
}

Or even have the ipaddress parameter take a hash which includes bind options? Then the bind_options parameter wouldn't even be needed.

haproxy::listen {'apache00' :
  ports => '80',
  ipaddress => {
    '192.168.2.1'  => ['ssl', 'crt', 'public.puppetlabs.com.crt'],
    '10.0.0.1'     => ['ssl', 'crt', 'private.puppetlabs.com.crt'],
    '178.35.67.12' => [],
  },
}

@igalic
Copy link

igalic commented Aug 8, 2014

I like @hunner's second proposal, Alot. The first one is problematic because it needs specific ordering.
Not a big fan of ipaddresses_and_bind_options as a name.

@Spredzy
Copy link
Author

Spredzy commented Aug 8, 2014

I agree with @hunner second idea but that would break backward compatibility if that is an issue.

Should we add a warning if is_array is true and mutate the array to an hash with empty values?

@igalic
Copy link

igalic commented Aug 8, 2014

👍

@Spredzy
Copy link
Author

Spredzy commented Aug 18, 2014

Sorry for the delay was out for holidays, please let me know if this new version seems better

@antaflos
Copy link

@esbjerg raises a valid point in #122. Seems like we should be able to specify arbitrary bind options independently for IP addresses and ports, and valid combinations of IP addresses and ports.

Would something like this work?

haproxy::listen { 'apache00' :
  bind => {
    '192.168.2.1:80'   => [],
    '10.0.0.1:8080'    => [],
    '178.35.67.12:443' => [ 'ssl', 'crt', 'foo.example.com.crt.pem', ],
  },
}

This could be translated more or less easily and directly to a bind line in haproxy.cfg. Of course this breaks backwards compatibility without some thoughtful array-to-hash-conversion-and-validation.

@esbjerg
Copy link

esbjerg commented Aug 21, 2014

I like antaflos' solution. It provides the felixibility I need.

@KlavsKlavsen
Copy link

I agree with antaflos solution - it's actually even more flexible - and would mean I could go back to using upstream module (now that it supports haproxy 1.5 as well :)

@Spredzy Spredzy force-pushed the more_listen_flexibility branch from c168722 to e35c8c6 Compare August 21, 2014 13:32
@Spredzy
Copy link
Author

Spredzy commented Aug 21, 2014

Taking @antaflos remark in account this is how this new patch works :

To remain backward compatible, $ports will specify the defaults ports to bind to if port hasn't been specified in the ipaddress hash.

So taking the example :

haproxy::listen { 'apache00' :
  ipaddress => {
    '192.168.2.1'      => [],
    '10.0.0.1:8080'    => [],
    '178.35.67.12:443' => [ 'ssl', 'crt', 'foo.example.com.crt.pem', ],
  },
  ports => ['80','82'],
}

This would generate the following piece of haproxy configuration :

bind 192.168.2.1:80
bind 192.168.2.1:82 
bind 10.0.0.1:8080 
bind 178.35.67.12:443 ssl crl foo.example.com.crt.pem

Hope this meet everyone's needs. Any feedback is welcome

},
ipaddress => { '10.0.0.1' => ['ssl', 'crt', 'puppetlabs.com'],
'168.12.12.12:80' => [],
'192.168.122.42' => []},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the indentation of this look more like the rest? (Newline after {, two space indent, } on it's own line after, and trailing commas on all hash pairs.)

@Spredzy Spredzy force-pushed the more_listen_flexibility branch 2 times, most recently from 4b9ca3b to 404ee61 Compare August 21, 2014 23:32
@Spredzy
Copy link
Author

Spredzy commented Aug 29, 2014

Up

},
bind => {
'10.0.0.1:443' => ['ssl', 'crt', 'puppetlabs.com'],
'168.12.12.12:80' => [],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you line this up?

@Spredzy Spredzy force-pushed the more_listen_flexibility branch from 404ee61 to 5e2eab3 Compare September 8, 2014 17:07
}
end

it { should contain_concat__fragment('apache_listen_block').with(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the module does not allow to specify specific bind options
per IP we are binding haproxy onto.
Hence, if a user has a service available across several network, but
with different haproxy bind option according to the network, s/he can't
configure haproxy with the current state of this module.

It aims to make it possible to have configuration as the following:

  bind 192.168.2.1:80 ssl crt public.puppetlabs.com.crt
  bind 10.0.0.1:80 ssl crt private.puppetlabs.com.crt
  bind 178.35.67.12:80
@Spredzy Spredzy force-pushed the more_listen_flexibility branch from 5e2eab3 to 5f07d77 Compare September 8, 2014 18:16
hunner added a commit that referenced this pull request Sep 9, 2014
Allow greater flexibility in listen directive.
@hunner hunner merged commit 899891c into puppetlabs:master Sep 9, 2014
cegeka-jenkins pushed a commit to cegeka/puppet-haproxy that referenced this pull request Dec 19, 2017
Allow greater flexibility in listen directive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants