add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable #367

matt-deboer · 2016-11-09T22:11:42Z

This allows for setting global default haproxy options via the HAPROXY_GLOBAL_DEFAULT_OPTIONS environment variable as a comma-separated list.

mesosphere-ci · 2016-11-09T22:11:43Z

Can one of the admins verify this patch?

robsonpeixoto · 2016-11-10T00:31:08Z

Add option tcplog would be great, too =D

robsonpeixoto · 2016-11-10T00:33:09Z

config.py

@@ -21,6 +21,11 @@ def add_template(self, template):
        self.t[template.name] = template

    def load(self):
+        httplog = os.getenv('HTTPLOG', default='')
+        option_httplog = ''
+        if httplog not in ['no', '0', 'false', '']:


if httplog.lower() not in ['no', '0', 'false', '']:

robsonpeixoto · 2016-11-10T00:33:59Z

tests/test_marathon_lb_httplog.py

+
+    def setUp(self):
+        self.maxDiff = None
+        os.environ['HTTPLOG'] = 'true'


Miss a test with values ['no', '0', 'false']

In UPPER case too

matt-deboer · 2016-11-10T05:04:46Z

Ok; added 'TCPLOG' option as well, and updated tests to cover various cases of 'disabled'

brndnmtthws · 2016-11-10T15:30:10Z

test this please

brndnmtthws

Can we make this a little more generic? How about we have an env var like HAPROXY_DEFAULT_OPTIONS which takes a comma separated list of default options that get appended to the default template?

Also, we need to document this and especially note that it will only apply if the default template has not been overridden.

matt-deboer · 2016-11-11T00:24:15Z

Ok, guys; take#3, now with HAPROXY_DEFAULT_OPTIONS and docs!

robsonpeixoto · 2016-11-11T05:14:47Z

config.py

@@ -20,6 +21,17 @@ class ConfigTemplater(object):
    def add_template(self, template):
        self.t[template.name] = template

+    def additional_default_options(self):


LGTM, but just a suggestion:

def additional_default_options(self): base_options = ['redispatch', 'http-server-close', 'dontlognull'] opts = set(s.strip() for s in os.getenv('HAPROXY_DEFAULT_OPTIONS', '').split(',')) template = ' option {opt}\n' lines = [template.format(opt=opt) for opt in opts if opt not in base_options] return ''.join(lines)

And remove the re import

Updating my suggestion after all @brndnmtthws suggestions:

def additional_default_options(self): default = 'redispatch,http-server-close,dontlognull' options = os.getenv('HAPROXY_GLOAL_DEFAULT_OPTIONS', default) template = ' option {opt}\n' lines = set(template.format(opt=opt.strip()) for opt in options.split(',')) return ''.join(lines)

brndnmtthws · 2016-11-11T15:10:37Z

config.py

+        if 'HAPROXY_DEFAULT_OPTIONS' in os.environ:
+            opts = set(re.split('\s*,\s',
+                                os.getenv('HAPROXY_DEFAULT_OPTIONS')))
+            for opt in [x for x in opts if x not in base_options]:


How about we remove the default options from the base template, and then set redispatch,http-server-close,dontlognull as the default option set. This way, if someone wants to remove some of the options, they can.

But we need to keep it very clear on documentation.

brndnmtthws · 2016-11-11T15:12:05Z

config.py

+    def additional_default_options(self):
+        output = ''
+        base_options = ['redispatch', 'http-server-close', 'dontlognull']
+        if 'HAPROXY_DEFAULT_OPTIONS' in os.environ:


Another nit: maybe instead of HAPROXY_DEFAULT_OPTIONS we could call it HAPROXY_GLOAL_DEFAULT_OPTIONS.

matt-deboer · 2016-11-11T21:46:56Z

Alright; we good now?

robsonpeixoto

LGTM. Just small fix

robsonpeixoto · 2016-11-12T01:50:45Z

README.md

+list of options may be specified via the `HAPROXY_GLOBAL_DEFAULT_OPTIONS` environment variable.
+The default value when not specified is `redispatch,http-server-close,dontlognull`; as an example, to add the
+`httplog` option (and keep the existing defaults), one should specify `HAPROXY_GLOBAL_DEFAULT_OPTIONS=redispatch,http-server-close,dontlognull,httplog`.
+ - _Note that this setting has no effect when the `HAPROXY_HEAD` tmeplate has been overridden._


tmeplate -> template

robsonpeixoto · 2016-11-12T01:51:28Z

config.py

@@ -2,6 +2,7 @@

 import logging
 import os
+import re


Delete this line

robsonpeixoto · 2016-11-12T01:53:51Z

tests/test_marathon_lb_haproxy_options.py

+        os.environ['HAPROXY_GLOBAL_DEFAULT_OPTIONS'] = \
+            'httplog,tcplog,dontlognull,tcplog'
+        base_config = base_config_prefix
+        # base_config += template_option('redispatch')


Remove commented code

robsonpeixoto · 2016-11-12T01:54:12Z

tests/test_marathon_lb_haproxy_options.py

+        self.maxDiff = None
+        os.environ['HAPROXY_GLOBAL_DEFAULT_OPTIONS'] = 'httplog,tcplog'
+        base_config = base_config_prefix
+        # base_config += template_option('redispatch')


Remove commented code

added 'TCPLOG' option more tests, and 'is_option_enabled' update tests implemented HAPROXY_DEFAULT_OPTIONS update docs applied latest suggestions rename to global_default_options code & doc cleanup

brndnmtthws · 2016-11-12T16:32:54Z

Perfect 👌

matt-deboer · 2016-11-12T20:03:31Z

Hi guys, not sure if you saw in there was a bug introduced in one of the
last updates; I added a new PR to fix it...sorry about that :/

On Saturday, November 12, 2016, Brenden Matthews [email protected]
wrote:

Merged #367 #367.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#367 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABbBtaH6TZOFHwB2No9dfLZIr-4KG8xrks5q9epPgaJpZM4KuC0h
.

robsonpeixoto reviewed Nov 10, 2016

View reviewed changes

brndnmtthws suggested changes Nov 10, 2016

View reviewed changes

matt-deboer changed the title ~~add HTTPLOG default via environment variable~~ add HAPROXY_DEFAULT_OPTIONS via environment variable Nov 11, 2016

robsonpeixoto reviewed Nov 11, 2016

View reviewed changes

brndnmtthws suggested changes Nov 11, 2016

View reviewed changes

matt-deboer changed the title ~~add HAPROXY_DEFAULT_OPTIONS via environment variable~~ add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable Nov 11, 2016

robsonpeixoto reviewed Nov 12, 2016

View reviewed changes

add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable

fa5ec0f

added 'TCPLOG' option more tests, and 'is_option_enabled' update tests implemented HAPROXY_DEFAULT_OPTIONS update docs applied latest suggestions rename to global_default_options code & doc cleanup

brndnmtthws approved these changes Nov 12, 2016

View reviewed changes

brndnmtthws merged commit 1c400ff into d2iq-archive:master Nov 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable #367

add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable #367

matt-deboer commented Nov 9, 2016 •

edited

Loading

mesosphere-ci commented Nov 9, 2016

robsonpeixoto commented Nov 10, 2016

robsonpeixoto Nov 10, 2016

robsonpeixoto Nov 10, 2016

robsonpeixoto Nov 10, 2016

matt-deboer commented Nov 10, 2016

brndnmtthws commented Nov 10, 2016

brndnmtthws left a comment

matt-deboer commented Nov 11, 2016

robsonpeixoto Nov 11, 2016 •

edited

Loading

robsonpeixoto Nov 11, 2016

brndnmtthws Nov 11, 2016

robsonpeixoto Nov 11, 2016

brndnmtthws Nov 11, 2016

matt-deboer commented Nov 11, 2016

robsonpeixoto left a comment

robsonpeixoto Nov 12, 2016

robsonpeixoto Nov 12, 2016

robsonpeixoto Nov 12, 2016

robsonpeixoto Nov 12, 2016

brndnmtthws commented Nov 12, 2016

matt-deboer commented Nov 12, 2016

add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable #367

add HAPROXY_GLOBAL_DEFAULT_OPTIONS via environment variable #367

Conversation

matt-deboer commented Nov 9, 2016 • edited Loading

mesosphere-ci commented Nov 9, 2016

robsonpeixoto commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-deboer commented Nov 10, 2016

brndnmtthws commented Nov 10, 2016

brndnmtthws left a comment

Choose a reason for hiding this comment

matt-deboer commented Nov 11, 2016

robsonpeixoto Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-deboer commented Nov 11, 2016

robsonpeixoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brndnmtthws commented Nov 12, 2016

matt-deboer commented Nov 12, 2016

matt-deboer commented Nov 9, 2016 •

edited

Loading

robsonpeixoto Nov 11, 2016 •

edited

Loading