dont create frontend port binding for non tcp apps #533
Conversation
|
Can one of the admins verify this patch? |
5 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Hi @ajays20078, Sorry for the delay in getting around to reviewing this PR. I'm not sure I actually understand the use case. For now I'm going to close this, but if this is a feature you'd still like to see, please just reopen this PR, and drop a comment in here describing a bit more about why this is desired (preferably with example configs, but not required). Thanks for your work on making marathon-lb better. |
|
The use case is:
P.S. - I dont have permission to re-open a closed PR in this repo. |
|
Ah, I get it now. Thanks for clarifying. I'll test it out today, and add a commit updating the
Sorry, I probably should have looked at the repo setting first ;). |
There was a problem hiding this comment.
Unfortunately I was unable to test this since it accesses variables out of scope:
======================================================================
ERROR: test_config_simple_app_multiple_vhost (tests.test_marathon_lb_haproxy_options.transplant_class.<locals>.C)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/marathon-lb/tests/test_marathon_lb.py", line 387, in test_config_simple_app_multiple_vhost
ssl_certs, templater)
File "/marathon-lb/marathon_lb.py", line 524, in config
if args.remove_nontcp_binding:
NameError: name 'args' is not defined
-------------------- >> begin captured logging << --------------------
I've added a comment on how to go about fixing it since args isn't in scope of the config function. If you'd like to fix it (please also update the documentation Longhelp.md as well with the new option and description), I'd be happy to re-review as I agree that it would be nice to free up the port usage.
| sslCert=' ssl crt ' + app.sslCert if app.sslCert else '', | ||
| bindOptions=' ' + app.bindOptions if app.bindOptions else '' | ||
| ) | ||
| if args.remove_nontcp_binding: |
There was a problem hiding this comment.
args is not defined in this function, remove_nontcp_binding should be passed through the MarathonEventProcessor and regenerate_config much like how args.dont_bind_http_https is.
| mode=app.mode, | ||
| sslCert=' ssl crt ' + app.sslCert if app.sslCert else '', | ||
| bindOptions=' ' + app.bindOptions if app.bindOptions else '' | ||
| ) |
There was a problem hiding this comment.
This should be DRY'd up to something like
if app.mode == 'tcp' or not remove_nontcp_binding:
frontends += frontend_head.format(
bindAddr=app.bindAddr,
backend=backend,
servicePort=app.servicePort,
mode=app.mode,
sslCert=' ssl crt ' + app.sslCert if app.sslCert else '',
bindOptions=' ' + app.bindOptions if app.bindOptions else ''
)| @@ -507,7 +519,11 @@ def config(apps, groups, bind_http_https, ssl_certs, templater, | |||
| backends += templater.haproxy_backend_sticky_options(app) | |||
|
|
|||
| frontend_backend_glue = templater.haproxy_frontend_backend_glue(app) | |||
| frontends += frontend_backend_glue.format(backend=backend) | |||
| if args.remove_nontcp_binding: | |||
There was a problem hiding this comment.
The same logic from above can be used here to DRY it up.
Dont create frontend port binding for non-tcp apps, as http and https works on headers.