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.

Sign up for GitHub

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

Jump to bottom

[WIP] Lock down Prometheus Alertmanager and NATS ports by default #378

Merged
merged 3 commits into from Nov 30, 2017
Merged

[WIP] Lock down Prometheus Alertmanager and NATS ports by default #378

merged 3 commits into from Nov 30, 2017

Conversation

johnmccabe
Copy link
Contributor

@johnmccabe johnmccabe commented Nov 8, 2017 •
edited

Description

This PR makes the following changes:

  • Stop exposing the Alertmanager and NATS ports by default on Swarm.
  • Add instructions on locking down the Prometheus endpoint to the Nginx contrib docs

Motivation and Context

How Has This Been Tested?

Locking down Alertmanager and NATS

  • Tested auto-scaling still works on Swarm with the Alertmanager port no longer mapped outside of the func network.
  • Tested async functions still work on Swarm with the NATS ports no longer mapped outside of the func network.

TODO

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@johnmccabe
Stop exposing Alertmanager and NATS ports
d8154ab 
This commit stops exposing the Prometheus Alertmanager and NATS ports by
default on Swarm.

The respective sections are commented out with a note on re-enabling
them.

Signed-off-by: John McCabe <john@johnmccabe.net>
@alexellis
Copy link
Member

Great idea 👍 if people need these for testing they can re-enable again.

alexellis
alexellis reviewed Nov 9, 2017
View reviewed changes
docker-compose.extended.yml Outdated
@@ -50,7 +52,7 @@ services:
- 'node.platform.os == linux'

queue-worker:
image: functions/queue-worker:0.1.1
image: functions/queue-worker:latest-dev
Copy link
Member

Choose a reason for hiding this comment

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

oops.. ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you ain't seen nothing right, this didn't happen cough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in aba8850

@johnmccabe
Revert queue-worker dev tag in yaml
aba8850 
Signed-off-by: John McCabe <john@johnmccabe.net>
@alexellis
Copy link
Member

I can't merge due to rebasing error. Happy to take this change though.

alexellis
alexellis requested changes Nov 18, 2017
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Please rebase

@derek derek bot added the no-dco label Nov 30, 2017
@derek
Copy link

derek bot commented Nov 30, 2017

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

@alexellis alexellis merged commit bc16d12 into openfaas:master Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis requested changes

Assignees
No one assigned
Labels
no-dco
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@johnmccabe @alexellis