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

fix cookie name in http_upstream sticky directive #1286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hbog
Copy link
Contributor

@hbog hbog commented Dec 13, 2018

Pull Request (PR) description

The syntax for the cookie method of the sticky directive in
the ngx_http_upstream_module is:

sticky cookie name [expires=time] [domain=domain] ...

The cookie name parameter must be specified without 'name=' prefix.

This Pull Request (PR) fixes the following issues

Fixes #1285

The syntax for the cookie method of the sticky directive in
the ngx_http_upstream_module is:

  sticky cookie name [expires=time] [domain=domain] ...

The cookie name parameter must be specified without 'name=' prefix.
@bastelfreak
Copy link
Member

Hi @hbog. Could you add an acceptance test so we know for sure that the generated config works?

@bastelfreak bastelfreak added needs-tests tests-fail bug Something isn't working labels Dec 15, 2018
@hbog
Copy link
Contributor Author

hbog commented Dec 15, 2018

The sticky directive that I use is only available in the commercial offering NGINX-Plus.

While looking to setup an acceptance test, I noticed that there is also an open source third party module (nginx-sticky-module-ng) implementing the sticky directive with a slightly different syntax. It is this syntax which is implemented in the current master branch of puppet-nginx.
In other words, this PR is NGINX-Plus specific and will break setups using nginx-sticky-module-ng.

Shall I add logic to detect the nginx edition in use and generate the appropriate configuration?
Is there a license to add nginx-plus to the acceptance tests ?

@bastelfreak
Copy link
Member

Sadly we don't have a license available and I don't have a contact to nginx. Can you add support for the nginx-sticky-module-ng as well? Please update the README.md accordingly.

@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Dec 28, 2018
@dhoppe dhoppe removed the tests-fail label Jun 5, 2019
@bastelfreak
Copy link
Member

Ping @hbog :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-tests needs-work not ready to merge just yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect cookie name in http_upstream sticky directive
3 participants