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

remove port from upstream member when service is defined #1283

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

Conversation

hbog
Copy link
Contributor

@hbog hbog commented Dec 10, 2018

The service parameter of the upstream member enables port discovery
via DNS SRV records. When it is used, a server port must not be
specified or nginx will fail with the following error:

nginx: [emerg] service upstream may not have port

Pull Request (PR) description

The PR removes the port from the server directive when the serviceparameter is set and updates the corresponding unit test.

This Pull Request (PR) fixes the following issues

Fixes #1282

The [*service*] parameter of the upstream member enables port discovery
via DNS SRV records. When it is used, a server port must not be
specified or nginx will fail with the following error:

   nginx: [emerg] service upstream may not have port
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks correct but I'll let Travis finish

@bastelfreak bastelfreak added bug Something isn't working needs-tests labels Dec 15, 2018
@bastelfreak
Copy link
Member

Hi @hbog, thanks for the change. Are you able to provide a tiny acceptance test for this? So we know for sure that the generated config works. You can find some examples in https://github.com/voxpupuli/puppet-nginx/tree/master/spec/acceptance. Let us know if you need some help. We're reachable in the #voxpupuli IRC channel on freenode and on https://slack.puppet.com

@ekohl
Copy link
Member

ekohl commented Dec 15, 2018

@bastelfreak I think acceptance tests are hard here because SRV records are hard without a real DNS server. I guess it'd be enough to verify that the server actually starts up though.

@hbog
Copy link
Contributor Author

hbog commented Dec 17, 2018

The service parameter for the server directive that I use is only available in the commercial offering NGINX-Plus.
Is there a license to add NGINX-plus to the acceptance tests ?
I think I could include dnsmasq in the test box to provide responses to the DNS SRV queries.

@bastelfreak
Copy link
Member

I would already be happy when we know that the generated config works and the daemon starts. IMO there is no need to test the actual SRV records. As mentioned in the other PR, I didn't know that this requires nginx-plus, so I assume simple acceptance testing won't be possible. Can you please update the docs with this new parameter? I think we can merge this without the acceptance tests.

@bastelfreak bastelfreak added needs-work not ready to merge just yet and removed needs-tests labels Dec 25, 2018
@bastelfreak
Copy link
Member

Ping @hbog :)

@vox-pupuli-tasks
Copy link

Dear @hbog, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

1 similar comment
@vox-pupuli-tasks
Copy link

Dear @hbog, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @hbog, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @hbog, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

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

Successfully merging this pull request may close these issues.

service upstream may not have port
4 participants