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

Expose API/stub_status on a custom port #344

Merged
merged 6 commits into from
Aug 20, 2018
Merged

Expose API/stub_status on a custom port #344

merged 6 commits into from
Aug 20, 2018

Conversation

isaachawley
Copy link
Contributor

@isaachawley isaachawley commented Aug 20, 2018

NGINX Plus will listen on a socket, /var/run/nginx-plus-api.sock, so the Ingress Controller can still connect to modify upstream servers when the API is disabled.

Adds command-line arguments:

  • -nginx-status to enable/disable status & stub_status
  • -nginx-status-port the port

Checklist:

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

isaac added 4 commits August 20, 2018 12:45
Also allow disabling the API. If disabled, nginx will listen on a
socket, /var/run/nginx.sock, so the IC can still connect to modify
upstream servers.
- Always enable Plus API over socket
- Update cli-arguments
- stub_status never listens on socket
- Validate status port
- Port range handling
- Assume API will be accessed by proxy
- No root directive in socket status block
- Remove bad socket listen in oss status
- Add test for status port
- Update cli args doc
@isaachawley isaachawley added the enhancement Pull requests for new features/feature enhancements label Aug 20, 2018
@isaachawley isaachawley self-assigned this Aug 20, 2018
@isaachawley isaachawley merged commit 8af77ef into master Aug 20, 2018
@isaachawley isaachawley deleted the stub-status branch August 20, 2018 16:15
@pleshakov pleshakov added the change Pull requests that introduce a change label Aug 21, 2018
@isaachawley
Copy link
Contributor Author

@r4j4h here's our internal stub-status PR. You can see it doesn't have the status-allow-ip feature that was in yours. I still think that's a good idea, maybe as an array of CIDRs? The nginx allow/deny directives take CIDR notation.

@r4j4h
Copy link
Contributor

r4j4h commented Sep 4, 2018

Thanks @isaachawley, this looks good. I think supporting multiple CIDRs is a great idea! I am thinking we can accept the array of CIDRs via comma separated list in the status-allow-ip argument. If you guys have better ways to implement in mind I am all ears, but in the meantime I will get started on a new pull request under that direction. 👍

@isaachawley
Copy link
Contributor Author

That sounds great @r4j4h, Thanks

@pleshakov
Copy link
Contributor

hi @r4j4h
we created an issue where we proposed an implementation for this feature. See #355

Please let us know what you think and if you're ok with following that implementation. We'd be happy to help with the implementation.

@r4j4h
Copy link
Contributor

r4j4h commented Oct 2, 2018

Hi @isaachawley and @pleshakov , sorry it took me a little while but I have a first stab at #387 if you would like to take a look when you have some time. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants